autoscaling: add new crd for automatic storage scaling #3044

parth-gr · 2025-02-18T14:36:58Z

added a new crd for automatic storage scaling

parth-gr · 2025-02-18T14:37:54Z

Please review the new CRD
/assign @BlaineEXE @malayparida2000 @iamniting @nb-ohad @malayparida2000

parth-gr · 2025-02-18T15:14:13Z

/retest

iamniting

Can you pls add all generated changes? Pls look at the failed github action for more details.

openshift-ci · 2025-02-18T16:00:29Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: parth-gr
Once this PR has been reviewed and has the lgtm label, please ask for approval from iamniting. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

alfonsomthd · 2025-02-19T07:20:16Z

config/crd/bases/ocs.openshift.io_autostoragescalings.yaml

+                format: date-time
+                type: string
+              phase:
+                description: Phase describes the Phase of AutoStorageScaling


@parth-gr Shouldn't we describe the enum allowed values: waiting,inProgress,completed,failed ?

Its the controller that would be updating the status values, so I don't think we need enum here, wdyt?

Right, it's not strictly necessary. My suggestion intended to have a more descriptive explanation for users just like the StorageCluster CRD does for deviceType:

deviceType: description: |- DeviceType is the value of device type in this StorageDeviceSet. It can have one of the three values (SSD, HDD, NVMe) enum: - SSD - ssd - HDD - hdd - NVMe - NVME - nvme type: string

done, with constant
And validation using enum

PS: plus its a status spec so user should not be updating it, so its just to fine to keep the checks

Madhu-1

CRD need to be added to https://github.com/red-hat-storage/ocs-operator/blob/main/PROJECT as well.

Madhu-1 · 2025-02-19T08:11:13Z

api/v1/autostoragescale.go

+// +kubebuilder:printcolumn:name="LastRunTimeStamp",type=date,JSONPath=.status.lastRunTimeStamp,description="Last Run Time Stamp"
+// +operator-sdk:csv:customresourcedefinitions:displayName="Auto Storage Scaling"
+
+// AutoStorageScaling represents the automatic storage scaling for odf cluster.


how about calling it as StorageAutoScaling or StorageAutoScaler ? because keeping Storage in the front aligns with the idea that we are trying to scale storage.

Madhu-1 · 2025-02-19T08:12:06Z

api/v1/autostoragescale.go

+	StorageClusterName string `json:"storageClusterName,omitempty"`
+	// DeviceClassName is the name of the device class for which the storage scaling is to be done.
+	// +kubebuilder:validation:Required
+	DeviceClassName string `json:"deviceClassName,omitempty"`


why deviceClass is required? AFAIK its optional in the storagecluster creation

its optionally but ocs operator alwyas add ssd if not provided by storagecluster

I'm still confused though. The autoscaler should be autoscaling existing storageclassdevicesets, and the device class should be able to be read from the scDeviceSet being scaled, right?

I'm still confused though. The autoscaler should be autoscaling existing storageclassdevicesets, and the device class should be able to be read from the scDeviceSet being scaled, right?

AFAIK autoscaling will apply to the devicesets of a particular deviceclass, so we need to know the deviceclass in order to filter devicesets by deviceclass.

During creation its an optional it should be kept optional as well, if its not specified we should treat it the same way we do during creation.

During creation its an optional it should be kept optional as well, if its not specified we should treat it the same way we do during creation.

You mean that if not specified it should default to ssd like StorageCluster CR, right?

yes correct whatever ocs-operator chooses during storagecluster creation.

Madhu-1 · 2025-02-19T08:14:55Z

api/v1/autostoragescale.go

+	// The known size is the original size of the OSDs before the expansion in progress is completed.
+	// After the expansion is completed, this would be updated to the expected OSD size.
+	// Used for vertical scaling of OSDs.
+	KnownOsdSize resource.Quantity `json:"knownOsdSize,omitempty"`


instead of KnownOsdISize does it make sense to keep this as currentOsdSize?

Madhu-1 · 2025-02-19T08:15:55Z

api/v1/autostoragescale.go

+// AutoStorageScalingStatus defines the observed state of AutoStorageScaling
+type AutoStorageScalingStatus struct {
+	// Phase describes the Phase of AutoStorageScaling
+	Phase string `json:"phase,omitempty"`


can we please define a known Phase const of it?

Madhu-1 · 2025-02-19T08:16:54Z

api/v1/autostoragescale.go

+	// The known OSD count is the original count of the OSDs before the expansion in progress is completed.
+	// After the expansion is completed, this would be updated to the expected OSD count.
+	// Used for horizontal scaling of OSDs.
+	KnownOsdCount int32 `json:"knownOsdCount,omitempty"`


same here as well. and also do we need it to be int32? or just use uint16?

Madhu-1 · 2025-02-19T08:17:17Z

api/v1/autostoragescale.go

+	KnownOsdCount int32 `json:"knownOsdCount,omitempty"`
+	// The Expected OSD count is the count that the auto-expansion has decided to set.
+	// This will be set on the storageCLuster CR as the desired count of the OSDs.
+	// Used for horizontal scaling of OSDs.


storageCLuster to storageCluster

Madhu-1 · 2025-02-19T08:18:18Z

api/v1/autostoragescale.go

@@ -0,0 +1,75 @@
+/*
+Copyright 2020 Red Hat OpenShift Container Storage.


Madhu-1 · 2025-02-19T08:20:00Z

api/v1/autostoragescale.go

+// +kubebuilder:printcolumn:name="LastRunTimeStamp",type=date,JSONPath=.status.lastRunTimeStamp,description="Last Run Time Stamp"
+// +operator-sdk:csv:customresourcedefinitions:displayName="Auto Storage Scaling"
+
+// AutoStorageScaling represents the automatic storage scaling for odf cluster.


odf cluster. to storage cluster

added a new crd for automatic storage scaling Signed-off-by: parth-gr <[email protected]>

iamniting self-requested a review February 18, 2025 15:32

iamniting requested changes Feb 18, 2025

View reviewed changes

openshift-ci bot assigned iamniting Feb 18, 2025

parth-gr force-pushed the auto-storage-crd branch from 084a1ed to b4f8f5c Compare February 18, 2025 16:00

parth-gr force-pushed the auto-storage-crd branch 2 times, most recently from 81d56d9 to 2c6af46 Compare February 18, 2025 16:04

parth-gr requested a review from iamniting February 18, 2025 16:07

alfonsomthd reviewed Feb 19, 2025

View reviewed changes

parth-gr requested a review from alfonsomthd February 19, 2025 08:02

Madhu-1 suggested changes Feb 19, 2025

View reviewed changes

openshift-ci bot assigned Madhu-1 Feb 19, 2025

parth-gr force-pushed the auto-storage-crd branch from 2c6af46 to 3d3f124 Compare February 19, 2025 09:54

autoscaling: add new crd for automatic storagescaling

37a903a

added a new crd for automatic storage scaling Signed-off-by: parth-gr <[email protected]>

parth-gr force-pushed the auto-storage-crd branch from 3d3f124 to 37a903a Compare February 19, 2025 09:58

parth-gr requested a review from Madhu-1 February 19, 2025 09:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

autoscaling: add new crd for automatic storage scaling #3044

autoscaling: add new crd for automatic storage scaling #3044

parth-gr commented Feb 18, 2025

parth-gr commented Feb 18, 2025

parth-gr commented Feb 18, 2025

iamniting left a comment

openshift-ci bot commented Feb 18, 2025

alfonsomthd Feb 19, 2025

parth-gr Feb 19, 2025

alfonsomthd Feb 19, 2025

parth-gr Feb 19, 2025 •

edited

Loading

Madhu-1 left a comment

Madhu-1 Feb 19, 2025

alfonsomthd Feb 19, 2025

parth-gr Feb 19, 2025

Madhu-1 Feb 19, 2025

parth-gr Feb 19, 2025

BlaineEXE Feb 19, 2025

alfonsomthd Feb 19, 2025

Madhu-1 Feb 20, 2025

alfonsomthd Feb 20, 2025

Madhu-1 Feb 20, 2025

Madhu-1 Feb 19, 2025

Madhu-1 Feb 19, 2025

Madhu-1 Feb 19, 2025

Madhu-1 Feb 19, 2025

Madhu-1 Feb 19, 2025

Madhu-1 Feb 19, 2025

		@@ -0,0 +1,75 @@
		/*
		Copyright 2020 Red Hat OpenShift Container Storage.

autoscaling: add new crd for automatic storage scaling #3044

Are you sure you want to change the base?

autoscaling: add new crd for automatic storage scaling #3044

Conversation

parth-gr commented Feb 18, 2025

parth-gr commented Feb 18, 2025

parth-gr commented Feb 18, 2025

iamniting left a comment

Choose a reason for hiding this comment

openshift-ci bot commented Feb 18, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

parth-gr Feb 19, 2025 • edited Loading

Choose a reason for hiding this comment

Madhu-1 left a 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

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

parth-gr Feb 19, 2025 •

edited

Loading