New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
osd: update existing OSDs with deviceClass #9259
Conversation
@@ -126,7 +126,7 @@ func (c *updateConfig) updateExistingOSDs(errs *provisionErrors) { | |||
} | |||
|
|||
// backward compatibility for old deployments | |||
if osdInfo.DeviceClass == "" { | |||
if osdInfo.DeviceClass == "" || osdInfo.DeviceClass == "None" { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As mentioned in the huddle I think using ,omitempty"
might better solve this instead of checking for None. Also I cannot get a repro on why we would get None
. https://go.dev/play/p/xKyIFkmrIKl
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added the ,omitempty"
at these two places,
rook/pkg/operator/ceph/cluster/osd/osd.go
Line 99 in 2519018
DeviceClass string `json:"device-class"` |
rook/pkg/daemon/ceph/client/osd.go
Line 313 in 2519018
DeviceClass string `json:"device_class"` |
But still, see that osdInfo.DeviceClass
is set as None
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hum I'd be curious to trace back and really understand why the command will return None. That shouldn't be the case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@leseb, I traced it out None
value is returned from the env. variable
rook/pkg/operator/ceph/cluster/osd/osd.go
Lines 495 to 496 in bd962e3
if envVar.Name == osdDeviceClassEnvVarName { | |
osd.DeviceClass = envVar.Value |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, weird it is not supposed to since it defaults to "".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if this https://github.com/rook/rook/blob/master/pkg/operator/ceph/cluster/osd/osd.go#L99 is not missing ,omitempty"
too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, this is the culprit https://github.com/rook/rook/blob/master/pkg/daemon/ceph/osd/volume.go#L85,
see:
[block] /dev/ceph-a8ae9f01-a440-4a0e-8e4d-592d3bce3a9d/osd-block-058f4926-6ee7-4165-9a55-db5a1fd22d2f
block device /dev/ceph-a8ae9f01-a440-4a0e-8e4d-592d3bce3a9d/osd-block-058f4926-6ee7-4165-9a55-db5a1fd22d2f
block uuid cr4xnI-h1sx-4DT3-ZC2N-0kI9-RtZF-Nkr3q1
cephx lockbox secret
cluster fsid f20931eb-9336-4234-a5c7-b0b44ab8c07a
cluster name ceph
crush device class None
encrypted 0
osd fsid 058f4926-6ee7-4165-9a55-db5a1fd22d2f
osd id 2
osdspec affinity
type block
vdo 0
devices /dev/vdd
With crush device class None
.
In the end, it's valid to check for None
since we don't control this behavior!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have checked it while testing it doesn't make any change.
I see the ROOK_OSD_DEVICE_CLASS
environment variable is returned as {ROOK_OSD_DEVICE_CLASS None nil}
this might be the actual problem
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, got it.
This is because of how ceph
returns the output ceph-volume
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please leave a comment in the code to explain why we need to check against None
and add a link to this tracker: https://tracker.ceph.com/issues/53425
dc47f56
to
5c6bf89
Compare
@@ -126,7 +126,7 @@ func (c *updateConfig) updateExistingOSDs(errs *provisionErrors) { | |||
} | |||
|
|||
// backward compatibility for old deployments | |||
if osdInfo.DeviceClass == "" { | |||
if osdInfo.DeviceClass == "" || osdInfo.DeviceClass == "None" { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please leave a comment in the code to explain why we need to check against None
and add a link to this tracker: https://tracker.ceph.com/issues/53425
If we apply useAllNodes to false for the current deployment, the OSDs should get updated with the individual nodes values and config, The deviceClass was not updating to the existing OSDs because there was bug in the check. The check osdInfo.DeviceClass == "" which should be checked like this osdInfo.DeviceClass == "None" Updated the code so OSDs can make use of the devices present Signed-off-by: parth-gr <paarora@redhat.com>
5c6bf89
to
cbe505d
Compare
osd: update existing OSDs with deviceClass (backport #9259)
If we apply useAllNodes to false for the current deployment,
the OSDs should get updated with the individual nodes values and config,
The deviceClass was not updating to the existing OSDs because there was
bug in the check.
The check osdInfo.DeviceClass == "" which should be
checked like this osdInfo.DeviceClass == "None"
Updated the code so OSDs can make use of the devices present
Signed-off-by: parth-gr paarora@redhat.com
Description of your changes:
Which issue is resolved by this Pull Request:
Resolves #
Checklist:
make codegen
) has been run to update object specifications, if necessary.