From 7413b49a868bfc62078bb59108266f47fbcb508d Mon Sep 17 00:00:00 2001 From: parth-gr Date: Tue, 26 Nov 2024 19:34:19 +0530 Subject: [PATCH 1/3] osd: fix device class label on osd pod we were adding the device class as the disk rotational type, but device class can be user defined, so if user has added the device class, use that value Signed-off-by: parth-gr --- pkg/daemon/ceph/osd/volume.go | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/pkg/daemon/ceph/osd/volume.go b/pkg/daemon/ceph/osd/volume.go index 75029082b..f5510c096 100644 --- a/pkg/daemon/ceph/osd/volume.go +++ b/pkg/daemon/ceph/osd/volume.go @@ -1158,10 +1158,17 @@ func GetCephVolumeRawOSDs(context *clusterd.Context, clusterInfo *client.Cluster if !skipDeviceClass { diskInfo, err := clusterd.PopulateDeviceInfo(blockPath, context.Executor) - if err != nil { - return nil, errors.Wrapf(err, "failed to get device info for %q", blockPath) + crushDeviceClass := os.Getenv(oposd.CrushDeviceClassVarName) + if crushDeviceClass != "" { + osd.DeviceClass = crushDeviceClass + } + + if osd.DeviceClass == "" { + if err != nil { + return nil, errors.Wrapf(err, "failed to get device info for %q", blockPath) + } + osd.DeviceClass = sys.GetDiskDeviceClass(diskInfo) } - osd.DeviceClass = sys.GetDiskDeviceClass(diskInfo) logger.Infof("setting device class %q for device %q", osd.DeviceClass, diskInfo.Name) } From 01456e8683e67b8769964917035fa135e39f69c8 Mon Sep 17 00:00:00 2001 From: parth-gr Date: Tue, 26 Nov 2024 19:45:57 +0530 Subject: [PATCH 2/3] osd: add a new devicetype label for osds currently device class and device type labels were clubbed with eachother create a seperate label, as these two labels can be seperate Signed-off-by: parth-gr --- pkg/daemon/ceph/osd/device.go | 2 +- pkg/daemon/ceph/osd/volume.go | 16 +++++++--------- pkg/operator/ceph/cluster/osd/labels.go | 3 +++ pkg/operator/ceph/cluster/osd/osd.go | 2 ++ pkg/util/sys/device.go | 12 +++++++++++- pkg/util/sys/device_test.go | 17 +++++++++++++---- 6 files changed, 37 insertions(+), 15 deletions(-) diff --git a/pkg/daemon/ceph/osd/device.go b/pkg/daemon/ceph/osd/device.go index 3e4bd57f6..0d2385748 100644 --- a/pkg/daemon/ceph/osd/device.go +++ b/pkg/daemon/ceph/osd/device.go @@ -94,5 +94,5 @@ func (d *DesiredDevice) UpdateDeviceClass(agent *OsdAgent, device *sys.LocalDisk } } - d.DeviceClass = sys.GetDiskDeviceClass(device) + d.DeviceClass = sys.GetDiskDeviceType(device) } diff --git a/pkg/daemon/ceph/osd/volume.go b/pkg/daemon/ceph/osd/volume.go index f5510c096..83e53597c 100644 --- a/pkg/daemon/ceph/osd/volume.go +++ b/pkg/daemon/ceph/osd/volume.go @@ -1158,17 +1158,15 @@ func GetCephVolumeRawOSDs(context *clusterd.Context, clusterInfo *client.Cluster if !skipDeviceClass { diskInfo, err := clusterd.PopulateDeviceInfo(blockPath, context.Executor) - crushDeviceClass := os.Getenv(oposd.CrushDeviceClassVarName) - if crushDeviceClass != "" { - osd.DeviceClass = crushDeviceClass + if err != nil { + return nil, errors.Wrapf(err, "failed to get device info for %q", blockPath) } + deviceType := sys.GetDiskDeviceType(diskInfo) + osd.DeviceType = deviceType + logger.Infof("setting device type %q for device %q", osd.DeviceType, diskInfo.Name) - if osd.DeviceClass == "" { - if err != nil { - return nil, errors.Wrapf(err, "failed to get device info for %q", blockPath) - } - osd.DeviceClass = sys.GetDiskDeviceClass(diskInfo) - } + crushDeviceClass := sys.GetDiskDeviceClass(oposd.CrushDeviceClassVarName, deviceType) + osd.DeviceClass = crushDeviceClass logger.Infof("setting device class %q for device %q", osd.DeviceClass, diskInfo.Name) } diff --git a/pkg/operator/ceph/cluster/osd/labels.go b/pkg/operator/ceph/cluster/osd/labels.go index f663311f0..aea609806 100644 --- a/pkg/operator/ceph/cluster/osd/labels.go +++ b/pkg/operator/ceph/cluster/osd/labels.go @@ -60,6 +60,9 @@ func (c *Cluster) getOSDLabels(osd OSDInfo, failureDomainValue string, portable labels[portableKey] = strconv.FormatBool(portable) labels[deviceClass] = osd.DeviceClass labels[osdStore] = osd.Store + if osd.DeviceType != "" { + labels[deviceType] = osd.DeviceType + } for k, v := range getOSDTopologyLocationLabels(osd.Location) { labels[k] = v diff --git a/pkg/operator/ceph/cluster/osd/osd.go b/pkg/operator/ceph/cluster/osd/osd.go index eb2886e19..4eeb1bb29 100644 --- a/pkg/operator/ceph/cluster/osd/osd.go +++ b/pkg/operator/ceph/cluster/osd/osd.go @@ -73,6 +73,7 @@ const ( bluestorePVCData = "data" deviceClass = "device-class" osdStore = "osd-store" + deviceType = "device-type" ) // Cluster keeps track of the OSDs @@ -121,6 +122,7 @@ type OSDInfo struct { ExportService bool `json:"exportService"` NodeName string `json:"nodeName"` PVCName string `json:"pvcName"` + DeviceType string `json:"device-type"` } // OrchestrationStatus represents the status of an OSD orchestration diff --git a/pkg/util/sys/device.go b/pkg/util/sys/device.go index 4a7f93f1b..fc9d2f64a 100644 --- a/pkg/util/sys/device.go +++ b/pkg/util/sys/device.go @@ -19,6 +19,7 @@ package sys import ( "encoding/json" "fmt" + "os" osexec "os/exec" "strconv" "strings" @@ -279,7 +280,7 @@ func GetDiskUUID(device string, executor exec.Executor) (string, error) { return parseUUID(device, output) } -func GetDiskDeviceClass(disk *LocalDisk) string { +func GetDiskDeviceType(disk *LocalDisk) string { if disk.Rotational { return "hdd" } @@ -289,6 +290,15 @@ func GetDiskDeviceClass(disk *LocalDisk) string { return "ssd" } +func GetDiskDeviceClass(crushDeviceClassVarName, deviceType string) string { + crushDeviceClass := os.Getenv(crushDeviceClassVarName) + if crushDeviceClass != "" { + return crushDeviceClass + } else { + return deviceType + } +} + // CheckIfDeviceAvailable checks if a device is available for consumption. The caller // needs to decide based on the return values whether it is available. func CheckIfDeviceAvailable(executor exec.Executor, devicePath string, pvcBacked bool) (bool, string, error) { diff --git a/pkg/util/sys/device_test.go b/pkg/util/sys/device_test.go index 01023c0ff..03e085902 100644 --- a/pkg/util/sys/device_test.go +++ b/pkg/util/sys/device_test.go @@ -197,12 +197,21 @@ func TestListDevicesChildListDevicesChild(t *testing.T) { assert.Equal(t, 3, len(child)) } -func TestGetDiskDeviceClass(t *testing.T) { +func TestGetDiskDeviceType(t *testing.T) { d := &LocalDisk{} - assert.Equal(t, "ssd", GetDiskDeviceClass(d)) + assert.Equal(t, "ssd", GetDiskDeviceType(d)) d.Rotational = true - assert.Equal(t, "hdd", GetDiskDeviceClass(d)) + assert.Equal(t, "hdd", GetDiskDeviceType(d)) d.Rotational = false d.RealPath = "nvme" - assert.Equal(t, "nvme", GetDiskDeviceClass(d)) + assert.Equal(t, "nvme", GetDiskDeviceType(d)) +} + +func TestGetDiskDeviceClass(t *testing.T) { + t.Setenv("ROOK_OSD_CRUSH_DEVICE_CLASS", "test") + assert.Equal(t, "test", GetDiskDeviceClass("ROOK_OSD_CRUSH_DEVICE_CLASS", "hdd")) + t.Setenv("ROOK_OSD_CRUSH_DEVICE_CLASS", "test1") + assert.Equal(t, "test1", GetDiskDeviceClass("ROOK_OSD_CRUSH_DEVICE_CLASS", "hdd")) + t.Setenv("ROOK_OSD_CRUSH_DEVICE_CLASS", "") + assert.Equal(t, "nvme", GetDiskDeviceClass("ROOK_OSD_CRUSH_DEVICE_CLASS", "nvme")) } From 50d6d6ab5fa5e8f1bc3828bb97314a4bb85bd819 Mon Sep 17 00:00:00 2001 From: parth-gr Date: Wed, 4 Dec 2024 18:12:06 +0530 Subject: [PATCH 3/3] osd: fix osd resources per device class GetOSDResources method was populating wrong device class to get the osd resources This fix will fix the deviceClass label for the GetOSDResources function Signed-off-by: parth-gr --- pkg/operator/ceph/cluster/osd/key_rotation.go | 2 +- pkg/operator/ceph/cluster/osd/osd.go | 6 +++--- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/pkg/operator/ceph/cluster/osd/key_rotation.go b/pkg/operator/ceph/cluster/osd/key_rotation.go index e69973310..db0aca375 100644 --- a/pkg/operator/ceph/cluster/osd/key_rotation.go +++ b/pkg/operator/ceph/cluster/osd/key_rotation.go @@ -271,7 +271,7 @@ func (c *Cluster) reconcileKeyRotationCronJob() error { return errors.Errorf("pvc name label %q for osd %q is empty", OSDOverPVCLabelKey, osdDep.Name) } - osdProps, err := c.getOSDPropsForPVC(pvcName, osd.DeviceClass) + osdProps, err := c.getOSDPropsForPVC(pvcName) if err != nil { return errors.Wrapf(err, "failed to generate config for osd %q", osdDep.Name) } diff --git a/pkg/operator/ceph/cluster/osd/osd.go b/pkg/operator/ceph/cluster/osd/osd.go index 4eeb1bb29..0189446c7 100644 --- a/pkg/operator/ceph/cluster/osd/osd.go +++ b/pkg/operator/ceph/cluster/osd/osd.go @@ -408,7 +408,7 @@ func deploymentOnNode(c *Cluster, osd *OSDInfo, nodeName string, config *provisi func deploymentOnPVC(c *Cluster, osd *OSDInfo, pvcName string, config *provisionConfig) (*appsv1.Deployment, error) { osdLongName := fmt.Sprintf("OSD %d on PVC %q", osd.ID, pvcName) - osdProps, err := c.getOSDPropsForPVC(pvcName, osd.DeviceClass) + osdProps, err := c.getOSDPropsForPVC(pvcName) if err != nil { return nil, errors.Wrapf(err, "failed to generate config for %s", osdLongName) } @@ -468,7 +468,7 @@ func (c *Cluster) getOSDPropsForNode(nodeName, deviceClass string) (osdPropertie return osdProps, nil } -func (c *Cluster) getOSDPropsForPVC(pvcName, osdDeviceClass string) (osdProperties, error) { +func (c *Cluster) getOSDPropsForPVC(pvcName string) (osdProperties, error) { for _, deviceSet := range c.deviceSets { // The data PVC template is required. dataSource, dataOK := deviceSet.PVCSources[bluestorePVCData] @@ -491,7 +491,7 @@ func (c *Cluster) getOSDPropsForPVC(pvcName, osdDeviceClass string) (osdProperti } if deviceSet.Resources.Limits == nil && deviceSet.Resources.Requests == nil { - deviceSet.Resources = cephv1.GetOSDResources(c.spec.Resources, osdDeviceClass) + deviceSet.Resources = cephv1.GetOSDResources(c.spec.Resources, deviceSet.CrushDeviceClass) } osdProps := osdProperties{