Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 getNone
. https://go.dev/play/p/xKyIFkmrIKlThere 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
rook/pkg/daemon/ceph/client/osd.go
Line 313 in 2519018
But still, see that
osdInfo.DeviceClass
is set asNone
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. variablerook/pkg/operator/ceph/cluster/osd/osd.go
Lines 495 to 496 in bd962e3
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:
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 problemThere 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 outputceph-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