autoscaling: add new crd for automatic storage scaling

[parth-gr]

added a new crd for automatic storage scaling

[parth-gr]

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

[parth-gr]

/retest

[iamniting]

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

[Madhu-1]

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

[parth-gr]

/assign @travisn @nb-ohad

[travisn]

/lgtm

[openshift-ci]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: parth-gr, travisn

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

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [travisn]

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

[openshift-ci]

New changes are detected. LGTM label has been removed.

[BlaineEXE]

I'd suggest asking @nb-ohad and @iamniting whether they think these time fields should be pointers/nullable so that the output doesn't include the epoch date-time when time is equal to 0.

Suggested change

	// LastExpansionStartTime is the time stamp of the last run start of the storage scaling
	// +optional
	LastExpansionStartTime metav1.Time `json:"lastExpansionStartTime,omitempty"`
	// LastExpansionCompletionTime is the time stamp of the last run completion of the storage scaling
	// +optional
	LastExpansionCompletionTime metav1.Time `json:"lastExpansionCompletionTime,omitempty"`
	// LastExpansionStartTime is the time stamp of the last run start of the storage scaling
	// +optional
	// +nullable
	LastExpansionStartTime *metav1.Time `json:"lastExpansionStartTime,omitempty"`
	// LastExpansionCompletionTime is the time stamp of the last run completion of the storage scaling
	// +optional
	// +nullable
	LastExpansionCompletionTime *metav1.Time `json:"lastExpansionCompletionTime,omitempty"`

[parth-gr]

Its already see we used without pointer in lot of places where we report heartbeat, so not a concern for now, can discuss this async.

[BlaineEXE]

I think a previous note I added as a reply got missed. It'd be best to not enum the phase. This can only introduce errors creating the CRD, and I don't know of any resources that enum status items like this.

For comparison the StorageCluster status phase isn't enum-ed

ocs-operator/api/v1/storagecluster_types.go

Lines 545 to 548 in be77c56

	// Phase describes the Phase of StorageCluster
	// This is used by OLM UI to provide status information
	// to the user
	Phase string `json:"phase,omitempty"`

[parth-gr]

why cant we use enum with +optional or + nullable?

[parth-gr]

I had a better way,
+kubebuilder:default="NotStarted"

[BlaineEXE]

I'm not sure if we can use enum with nullable, since nullable implies a type that is a pointer. It might be valid or might not. But in general, I don't think enums in status items are very helpful. In my experience, they are unlikely to help and likely to cause a problem, so don't bother. They won't help users, since users don't set the status themselves. Also, they can result in the operator being unable to set otherwise valid statuses, resulting in unexpected runtime errors that we need to bug fix.

[parth-gr]

Actually I removed +nullable, and used a default value,

The main reason here is to use enum is that coming from alert team, as alert are based on the phase they had to make sure operator is setting up correctly

[BlaineEXE]

I'm curious what benefit this gives us over StartOsdSize and ExpectedOsdSize. I think those are the things that actually control whether expansion will continue or not right?

Doesn't this have room for calculation errors between Ceph's reported total capacity and the PV/PVC capacity?

[parth-gr]

we have to use these values with the alerts, #3045 (comment)

[parth-gr]

/retest