Default storage class for virtualization purposes #2913

akalenyu · 2023-10-01T14:19:29Z

What this PR does / why we need it:
Storage providers in the field may supply many ways of utilizing a back end storage solution;
They do so by providing different Kubernetes StorageClass parameters.

With the growth of kubevirt adoptions, we may sometimes see a certain combination of storage class parameters is preferrable for VM workloads.
This PR presents a possible way of steering kubevirt users
towards using such identified storage class.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #

Special notes for your reviewer:
Was considering not doing functional tests but this feels too important.
There is little to no penalty because the entire block of tests I'm adding takes less than 30 seconds.
(No wait for dv success)

Release note:

Default virtualization storage class that takes precedence over k8s default storage class

kubevirt-bot · 2023-10-01T14:19:30Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

akalenyu · 2023-10-01T14:20:35Z

/test all

akalenyu · 2023-10-01T14:40:05Z

/test pull-cdi-unit-test

akalenyu · 2023-10-01T14:50:53Z

/retest

akalenyu · 2023-10-02T17:31:50Z

/test pull-containerized-data-importer-e2e-ceph

akalenyu · 2023-10-04T12:18:41Z

/retest

aglitke · 2023-10-05T18:29:16Z

pkg/controller/common/util.go

@@ -211,6 +212,8 @@ const (

 	//AnnDefaultStorageClass is the annotation indicating that a storage class is the default one.
 	AnnDefaultStorageClass = "storageclass.kubernetes.io/is-default-class"
+	// AnnDefaultVirtStorageClass is the annotation indicating that a storage class is the default one for virtualization purposes
+	AnnDefaultVirtStorageClass = "storageclass.kubevirt.io/is-default-class"


Maybe this should be storageclass.kubevirt.io/is-default-virt-class to avoid confusion with the standard k8s annotation. I know we have a different URI but still.

Was just following the pattern
snapshot.storage.kubernetes.io/is-default-class
ingressclass.kubernetes.io/is-default-class
storageclass.kubernetes.io/is-default-class

I am up for extra clarity though so just let me know if this still makes sense

mhenriks · 2023-10-11T15:52:02Z

cmd/cdi-controller/controller.go

@@ -369,6 +369,7 @@ func getTokenPrivateKey() *rsa.PrivateKey {
 func registerMetrics() {
 	metrics.Registry.MustRegister(controller.IncompleteProfileGauge)
 	controller.IncompleteProfileGauge.Set(-1)
+	metrics.Registry.MustRegister(controller.DefaultVirtStorageClassesGauge)


Should StorageProfile status report if a StorageClsss is a virt default?

Good point, not sure if anyone would look up storage profiles though

mhenriks · 2023-10-11T16:03:07Z

pkg/controller/common/util.go

+// default kubevirt storage class (if the caller is interested) storageclass.kubevirt.io/is-default-class
+// default k8s storage class storageclass.kubernetes.io/is-default-class
+// If no storage class is found, returns nil
+func GetStorageClassByNameWithVirtFallback(ctx context.Context, client client.Client, name *string, considerVirtSc bool) (*storagev1.StorageClass, error) {


Why both GetStorageClassByName and GetStorageClassByNameWithVirtFallback? I'm sure there's a good reason. Prob worth adding a comment why

Also, bool params should be avoided when possible. Maybe it would be better to have function take a DataVolume as a param instead?

Why both GetStorageClassByName and GetStorageClassByNameWithVirtFallback? I'm sure there's a good reason. Prob worth adding a comment why

Renamed to better communicate the reason.
Basically, there will still be flows where you don't want to take the virt sc into account, quick example is when using the PVC API

Also, bool params should be avoided when possible. Maybe it would be better to have function take a DataVolume as a param instead?

Yes definitely. Hopefully this is better now, whole DV was a bit overkill but content type should be fine

akalenyu · 2023-10-15T21:42:51Z

/retest

mhenriks · 2023-10-17T20:26:47Z

/retest-required

mhenriks · 2023-10-17T20:26:55Z

/approve

kubevirt-bot · 2023-10-17T20:27:05Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mhenriks

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~~ [mhenriks]

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

awels

looks good, just one minor code duplication question

pkg/controller/common/util.go

Signed-off-by: Alex Kalenyuk <akalenyu@redhat.com>

akalenyu · 2023-10-26T20:24:19Z

/test pull-containerized-data-importer-e2e-istio

kubevirt-bot requested review from aglitke and ShellyKa13 October 1, 2023 14:19

akalenyu force-pushed the default-virt-storage-class branch from a727630 to 68be7f8 Compare October 2, 2023 17:14

akalenyu changed the title ~~[WIP] Default virt storage class~~ Default storage class for virtualization purposes Oct 2, 2023

akalenyu marked this pull request as ready for review October 2, 2023 17:17

kubevirt-bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 2, 2023

kubevirt-bot requested review from arnongilboa and mhenriks October 2, 2023 17:17

akalenyu force-pushed the default-virt-storage-class branch from 68be7f8 to d0a352f Compare October 2, 2023 17:22

akalenyu force-pushed the default-virt-storage-class branch 2 times, most recently from 44f4a6c to a6cdc92 Compare October 3, 2023 15:03

aglitke reviewed Oct 5, 2023

View reviewed changes

akalenyu mentioned this pull request Oct 10, 2023

Add alert CDIMultipleDefaultVirtStorageClasses kubevirt/monitoring#201

Merged

mhenriks reviewed Oct 11, 2023

View reviewed changes

akalenyu mentioned this pull request Oct 15, 2023

Add default virtualization storage class annotation red-hat-storage/ocs-operator#2213

Merged

akalenyu force-pushed the default-virt-storage-class branch from a6cdc92 to 25f35c0 Compare October 15, 2023 21:24

kubevirt-bot added size/XL and removed size/L labels Oct 15, 2023

kubevirt-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 17, 2023

akalenyu force-pushed the default-virt-storage-class branch from 25f35c0 to f0c7ab7 Compare October 19, 2023 17:39

awels reviewed Oct 20, 2023

View reviewed changes

pkg/controller/common/util.go Show resolved Hide resolved

akalenyu added 3 commits October 22, 2023 14:19

Default virt storage class

0017048

Signed-off-by: Alex Kalenyuk <akalenyu@redhat.com>

Add alert for multiple default virt storage classes

c0e3492

Signed-off-by: Alex Kalenyuk <akalenyu@redhat.com>

Refactor content type funcs to not return strings

462f400

Signed-off-by: Alex Kalenyuk <akalenyu@redhat.com>

akalenyu force-pushed the default-virt-storage-class branch from f0c7ab7 to 462f400 Compare October 22, 2023 12:08

awels approved these changes Oct 26, 2023

View reviewed changes

kubevirt-bot assigned awels Oct 26, 2023

kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label Oct 26, 2023

kubevirt-bot merged commit 934a0be into kubevirt:main Oct 27, 2023

GingerGeek mentioned this pull request Jan 11, 2024

[release-1.10] Backport CDI 1.58.0 into 1.10 operator release kubevirt/hyperconverged-cluster-operator#2727

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Default storage class for virtualization purposes #2913

Default storage class for virtualization purposes #2913

akalenyu commented Oct 1, 2023 •

edited

Loading

kubevirt-bot commented Oct 1, 2023

akalenyu commented Oct 1, 2023

akalenyu commented Oct 1, 2023

akalenyu commented Oct 1, 2023

akalenyu commented Oct 2, 2023

akalenyu commented Oct 4, 2023

aglitke Oct 5, 2023

akalenyu Oct 8, 2023

akalenyu Oct 8, 2023

akalenyu Oct 15, 2023

mhenriks Oct 11, 2023

akalenyu Oct 15, 2023

mhenriks Oct 11, 2023

akalenyu Oct 15, 2023 •

edited

Loading

akalenyu commented Oct 15, 2023

mhenriks commented Oct 17, 2023

mhenriks commented Oct 17, 2023

kubevirt-bot commented Oct 17, 2023

awels left a comment

akalenyu commented Oct 26, 2023

Default storage class for virtualization purposes #2913

Default storage class for virtualization purposes #2913

Conversation

akalenyu commented Oct 1, 2023 • edited Loading

kubevirt-bot commented Oct 1, 2023

akalenyu commented Oct 1, 2023

akalenyu commented Oct 1, 2023

akalenyu commented Oct 1, 2023

akalenyu commented Oct 2, 2023

akalenyu commented Oct 4, 2023

aglitke Oct 5, 2023

Choose a reason for hiding this comment

akalenyu Oct 8, 2023

Choose a reason for hiding this comment

akalenyu Oct 8, 2023

Choose a reason for hiding this comment

akalenyu Oct 15, 2023

Choose a reason for hiding this comment

mhenriks Oct 11, 2023

Choose a reason for hiding this comment

akalenyu Oct 15, 2023

Choose a reason for hiding this comment

mhenriks Oct 11, 2023

Choose a reason for hiding this comment

akalenyu Oct 15, 2023 • edited Loading

Choose a reason for hiding this comment

akalenyu commented Oct 15, 2023

mhenriks commented Oct 17, 2023

mhenriks commented Oct 17, 2023

kubevirt-bot commented Oct 17, 2023

awels left a comment

Choose a reason for hiding this comment

akalenyu commented Oct 26, 2023

akalenyu commented Oct 1, 2023 •

edited

Loading

akalenyu Oct 15, 2023 •

edited

Loading