osd: add deviceType label to an OSD #11159

parth-gr · 2022-10-17T14:57:02Z

Closes:#11059
Signed-off-by: parth-gr paarora@redhat.com

Description of your changes:

Which issue is resolved by this Pull Request:
Resolves #

Checklist:

Commit Message Formatting: Commit titles and messages follow guidelines in the developer guide).
Skip Tests for Docs: If this is only a documentation change, add the label skip-ci on the PR.
Reviewed the developer guide on Submitting a Pull Request
Pending release notes updated with breaking and/or notable changes for the next minor release.
Documentation has been updated, if necessary.
Unit tests have been added, if necessary.
Integration tests have been added, if necessary.

parth-gr · 2022-10-18T15:29:00Z

Testing results:

metadata:
  creationTimestamp: "2022-10-18T15:20:05Z"
  generateName: rook-ceph-osd-0-6c8df8569c-
  labels:
    app: rook-ceph-osd
    app.kubernetes.io/component: cephclusters.ceph.rook.io
    app.kubernetes.io/created-by: rook-ceph-operator
    app.kubernetes.io/instance: "0"
    app.kubernetes.io/managed-by: rook-ceph-operator
    app.kubernetes.io/name: ceph-osd
    app.kubernetes.io/part-of: my-cluster
    ceph-osd-id: "0"
    ceph-version: 17.2.5-0
    ceph_daemon_id: "0"
    ceph_daemon_type: osd
    device-type: rotational
    ```

subhamkrai · 2022-10-18T15:39:08Z

pkg/operator/ceph/cluster/osd/labels.go

@@ -52,6 +52,7 @@ func (c *Cluster) getOSDLabels(osd OSDInfo, failureDomainValue string, portable
 	labels[OsdIdLabelKey] = stringID
 	labels[FailureDomainKey] = failureDomainValue
 	labels[portableKey] = strconv.FormatBool(portable)
+	labels["device-type"] = osd.DeviceType


Suggested change

labels["device-type"] = osd.DeviceType

labels["device-type"] = osd.DeviceType

can we not use hard-coded and declare this somewhere similar to other labels above?

travisn · 2022-10-18T16:37:48Z

.github/workflows/canary-integration-test.yml

@@ -67,6 +67,10 @@ jobs:
          # print existing client auth
          kubectl -n rook-ceph exec $toolbox -- ceph auth ls

+      - name: test OSD label
+        run: |
+          kubectl get pod -l ceph-osd-id=0 -nrook-ceph -o yaml


Seems like we should either remove this, or add a check for the label. I think unit tests will be sufficient though, not sure we need the CI test.

Just for testing is the OSD has the label or not

So you'll revert this before merging?

travisn · 2022-10-18T16:45:29Z

pkg/daemon/ceph/osd/daemon.go

@@ -229,6 +229,14 @@ func Provision(context *clusterd.Context, agent *OsdAgent, crushLocation, topolo
 		return errors.Wrap(err, "failed to configure devices")
 	}

+	for i := range deviceOSDs {
+		if deviceOSDs[i].DeviceClass == "hdd" {


A few thoughts/questions about the device class:

Is this DeviceClass always set for an OSD? I assume it's returned by ceph-volume and it would be set.

The user can override the device class with a setting in the cluster CR, so it might still be rotational even though the device class is not hdd

If we're using the device class property, we should just name the OSD property as deviceClass instead of calling it rotational, since we don't really know if it's rotational.

@umangachapagain Would the device class property be sufficient for your scenario? As long as the user doesn't override the device class and it's a rotational device, we would expect it to be set to hdd. And if that is sufficient, we already have an env var for ROOK_OSD_DEVICE_CLASS on the OSDs. Perhaps that would already work for your scenario? If not, we could add this same device class as a label. Or if the device class is not sufficient, we need to detect the rotational property differently.

If users can override it, I'd want to avoid using it. How common is it for users to override it?

It totally depends on the user, if they want to override it or not, so defer from user to user on the basis of their env.
https://github.com/rook/rook/blame/master/Documentation/CRDs/Cluster/ceph-cluster-crd.md#L391

Here is the place in the Cluster CR where user can specify deviceClass

rook/deploy/examples/cluster.yaml

Line 236 in 57d6020

# Individual nodes and their config can be specified as well, but 'useAllNodes' above must be set to false. Then, only the named

So we need a DeviceType label on the OSDs, which would be only set if the DeviceType is governed by the prepare jobs?

From the documentation

By default, if a device's class has not already been set, OSDs will automatically set a device's class to either `hdd`, `ssd`, or `nvme` based on the hardware properties exposed by the Linux kernel.

I think this info as label would be good enough even if device's class is already set by user.

travisn · 2022-10-18T16:46:46Z

pkg/operator/ceph/cluster/osd/osd.go

@@ -96,6 +96,7 @@ type OSDInfo struct {
 	UUID           string `json:"uuid"`
 	DevicePartUUID string `json:"device-part-uuid"`
 	DeviceClass    string `json:"device-class"`
+	DeviceType     string `json:"device-type"`


I was going to suggest to change this to DeviceClass, but I see that property already exists. So maybe we just need to add the label with that deviceClass property that already exists.

Suggested change

DeviceType string `json:"device-type"`

Yes, right but it should be device type

travisn · 2022-10-18T16:47:54Z

pkg/operator/ceph/cluster/osd/envs.go

@@ -32,6 +32,7 @@ const (
 	osdWalSizeEnvVarName      = "ROOK_OSD_WAL_SIZE"
 	osdsPerDeviceEnvVarName   = "ROOK_OSDS_PER_DEVICE"
 	osdDeviceClassEnvVarName  = "ROOK_OSD_DEVICE_CLASS"
+	osdDeviceTypeEnvVarName   = "ROOK_OSD_DEVICE_TYPE"


Instead of an env var, let's add a label. But I see that we already have an env var for the device class so I wonder if that will be sufficient and we don't need a separate label.

I guess adding a label would be fine which directly says it's rotational or not fetched from the device class info,
@umangachapagain thoughts??

travisn · 2022-10-19T14:55:27Z

.github/workflows/canary-integration-test.yml

@@ -67,6 +67,10 @@ jobs:
          # print existing client auth
          kubectl -n rook-ceph exec $toolbox -- ceph auth ls

+      - name: test OSD label
+        run: |
+          kubectl get pod -l ceph-osd-id=0 -nrook-ceph -o yaml


So you'll revert this before merging?

travisn · 2022-10-19T14:58:45Z

pkg/operator/ceph/cluster/osd/labels.go

@@ -52,6 +52,11 @@ func (c *Cluster) getOSDLabels(osd OSDInfo, failureDomainValue string, portable
 	labels[OsdIdLabelKey] = stringID
 	labels[FailureDomainKey] = failureDomainValue
 	labels[portableKey] = strconv.FormatBool(portable)
+	if osd.DeviceClass == "hdd" {


If we're basing this on the device class, I really think we should just directly add the device-class label. Then whoever looks at the label can interpret it themselves as rotational or not.

Suggested change

if osd.DeviceClass == "hdd" {

labels["device-class"] = osd.DeviceClass

Closes: rook#11059 Signed-off-by: parth-gr <paarora@redhat.com>

parth-gr · 2022-10-20T10:52:16Z

@umangachapagain please review it once.
Does it solve the actual issue?

parth-gr · 2022-10-27T13:49:20Z

@umangachapagain @travisn ^^

sp98 · 2022-10-31T08:49:02Z

Testing results:

metadata:
  creationTimestamp: "2022-10-18T15:20:05Z"
  generateName: rook-ceph-osd-0-6c8df8569c-
  labels:
    app: rook-ceph-osd
    app.kubernetes.io/component: cephclusters.ceph.rook.io
    app.kubernetes.io/created-by: rook-ceph-operator
    app.kubernetes.io/instance: "0"
    app.kubernetes.io/managed-by: rook-ceph-operator
    app.kubernetes.io/name: ceph-osd
    app.kubernetes.io/part-of: my-cluster
    ceph-osd-id: "0"
    ceph-version: 17.2.5-0
    ceph_daemon_id: "0"
    ceph_daemon_type: osd
    device-type: rotational
    ```

is the result still valid? Looks like PR was updated to use deviceClass

pkg/operator/ceph/cluster/osd/labels.go

parth-gr · 2022-10-31T14:11:25Z

Testing results:

metadata:
  creationTimestamp: "2022-10-18T15:20:05Z"
  generateName: rook-ceph-osd-0-6c8df8569c-
  labels:
    app: rook-ceph-osd
    app.kubernetes.io/component: cephclusters.ceph.rook.io
    app.kubernetes.io/created-by: rook-ceph-operator
    app.kubernetes.io/instance: "0"
    app.kubernetes.io/managed-by: rook-ceph-operator
    app.kubernetes.io/name: ceph-osd
    app.kubernetes.io/part-of: my-cluster
    ceph-osd-id: "0"
    ceph-version: 17.2.5-0
    ceph_daemon_id: "0"
    ceph_daemon_type: osd
    device-type: rotational
    ```

is the result still valid? Looks like PR was updated to use deviceClass

yaa these were the old testing result

osd: add deviceType label to an OSD (backport #11159)

parth-gr marked this pull request as draft October 17, 2022 14:57

parth-gr force-pushed the rotational-disk branch 2 times, most recently from 64d1094 to 919423c Compare October 18, 2022 12:52

parth-gr marked this pull request as ready for review October 18, 2022 12:52

parth-gr force-pushed the rotational-disk branch from 919423c to c862dc4 Compare October 18, 2022 13:08

parth-gr marked this pull request as draft October 18, 2022 13:38

parth-gr force-pushed the rotational-disk branch 5 times, most recently from 03356cd to edc0b5c Compare October 18, 2022 15:28

parth-gr marked this pull request as ready for review October 18, 2022 15:30

parth-gr requested a review from travisn October 18, 2022 15:30

subhamkrai requested changes Oct 18, 2022

View reviewed changes

travisn requested changes Oct 18, 2022

View reviewed changes

parth-gr requested a review from umangachapagain October 19, 2022 07:13

parth-gr force-pushed the rotational-disk branch from edc0b5c to 35fc4bc Compare October 19, 2022 14:41

parth-gr requested review from travisn and subhamkrai October 19, 2022 14:42

travisn requested changes Oct 19, 2022

View reviewed changes

osd: add deviceType label to an OSD

3bcc29e

Closes: rook#11059 Signed-off-by: parth-gr <paarora@redhat.com>

parth-gr force-pushed the rotational-disk branch from 35fc4bc to 3bcc29e Compare October 19, 2022 16:11

parth-gr requested a review from travisn October 19, 2022 16:11

parth-gr added the backport-release-1.10 label Oct 27, 2022

travisn approved these changes Oct 27, 2022

View reviewed changes

sp98 reviewed Oct 31, 2022

View reviewed changes

pkg/operator/ceph/cluster/osd/labels.go Show resolved Hide resolved

umangachapagain approved these changes Nov 3, 2022

View reviewed changes

sp98 approved these changes Nov 3, 2022

View reviewed changes

subhamkrai approved these changes Nov 3, 2022

View reviewed changes

travisn merged commit d39258e into rook:master Nov 3, 2022

mergify bot mentioned this pull request Nov 3, 2022

osd: add deviceType label to an OSD (backport #11159) #11261

Merged

travisn added a commit that referenced this pull request Nov 3, 2022

Merge pull request #11261 from rook/mergify/bp/release-1.10/pr-11159

91b55d6

osd: add deviceType label to an OSD (backport #11159)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

osd: add deviceType label to an OSD #11159

osd: add deviceType label to an OSD #11159

parth-gr commented Oct 17, 2022

parth-gr commented Oct 18, 2022

subhamkrai Oct 18, 2022

parth-gr Oct 19, 2022

travisn Oct 18, 2022

parth-gr Oct 19, 2022

travisn Oct 19, 2022

travisn Oct 18, 2022

travisn Oct 18, 2022

umangachapagain Oct 19, 2022

parth-gr Oct 19, 2022

umangachapagain Oct 19, 2022 •

edited

parth-gr Oct 19, 2022 •

edited

travisn Oct 18, 2022

parth-gr Oct 19, 2022 •

edited

travisn Oct 18, 2022

parth-gr Oct 19, 2022

travisn Oct 19, 2022

travisn Oct 19, 2022

parth-gr Oct 19, 2022

parth-gr commented Oct 20, 2022

parth-gr commented Oct 27, 2022

sp98 commented Oct 31, 2022

parth-gr commented Oct 31, 2022

	labels["device-type"] = osd.DeviceType
	labels["device-type"] = osd.DeviceType

	if osd.DeviceClass == "hdd" {
	labels["device-class"] = osd.DeviceClass

osd: add deviceType label to an OSD #11159

osd: add deviceType label to an OSD #11159

Conversation

parth-gr commented Oct 17, 2022

parth-gr commented Oct 18, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

umangachapagain Oct 19, 2022 • edited

Choose a reason for hiding this comment

parth-gr Oct 19, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

parth-gr Oct 19, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

parth-gr commented Oct 20, 2022

parth-gr commented Oct 27, 2022

sp98 commented Oct 31, 2022

parth-gr commented Oct 31, 2022

umangachapagain Oct 19, 2022 •

edited

parth-gr Oct 19, 2022 •

edited

parth-gr Oct 19, 2022 •

edited