Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Default storage class for virtualization purposes #2913

Merged
merged 3 commits into from
Oct 27, 2023
Merged

Default storage class for virtualization purposes #2913

merged 3 commits into from
Oct 27, 2023

Conversation

akalenyu
Copy link
Collaborator

@akalenyu akalenyu commented Oct 1, 2023 •
edited

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
Copy link
Contributor

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

@kubevirt-bot kubevirt-bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. release-note Denotes a PR that will be considered when it comes time to generate release notes. dco-signoff: yes Indicates the PR's author has DCO signed all their commits. size/L labels Oct 1, 2023
@akalenyu
Copy link
Collaborator Author

akalenyu commented Oct 1, 2023

/test all

@akalenyu
Copy link
Collaborator Author

akalenyu commented Oct 1, 2023

/test pull-cdi-unit-test

@akalenyu
Copy link
Collaborator Author

akalenyu commented Oct 1, 2023

/retest

@akalenyu akalenyu changed the title [WIP] Default virt storage class Default storage class for virtualization purposes Oct 2, 2023
@akalenyu akalenyu marked this pull request as ready for review October 2, 2023 17:17
@kubevirt-bot 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
@akalenyu
Copy link
Collaborator Author

akalenyu commented Oct 2, 2023

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

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

akalenyu commented Oct 4, 2023

/retest

aglitke
aglitke reviewed Oct 5, 2023
View reviewed changes
pkg/controller/common/util.go Outdated
@@ -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"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@akalenyu akalenyu mentioned this pull request Oct 10, 2023
Merged
mhenriks
mhenriks reviewed Oct 11, 2023
View reviewed changes
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)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

pkg/controller/common/util.go Outdated
// 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) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Collaborator Author

@akalenyu akalenyu Oct 15, 2023 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 akalenyu mentioned this pull request Oct 15, 2023
Merged
@akalenyu
Copy link
Collaborator Author

/retest

@mhenriks
Copy link
Member

/retest-required

@mhenriks
Copy link
Member

/approve

@kubevirt-bot
Copy link
Contributor

[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:

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

@kubevirt-bot kubevirt-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 17, 2023
awels
awels reviewed Oct 20, 2023
View reviewed changes
Copy link
Member

@awels awels left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good, just one minor code duplication question

pkg/controller/common/util.go Show resolved Hide resolved
@akalenyu
Default virt storage class
0017048 
Signed-off-by: Alex Kalenyuk <akalenyu@redhat.com>
@akalenyu
Add alert for multiple default virt storage classes
c0e3492 
Signed-off-by: Alex Kalenyuk <akalenyu@redhat.com>
@akalenyu
Refactor content type funcs to not return strings
462f400 
Signed-off-by: Alex Kalenyuk <akalenyu@redhat.com>
@kubevirt-bot kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label Oct 26, 2023
@akalenyu
Copy link
Collaborator Author

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

@kubevirt-bot kubevirt-bot merged commit 934a0be into kubevirt:main Oct 27, 2023
18 checks passed
@GingerGeek GingerGeek mentioned this pull request Jan 11, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aglitke aglitke aglitke left review comments

@mhenriks mhenriks mhenriks left review comments

@awels awels awels approved these changes

@ShellyKa13 ShellyKa13 Awaiting requested review from ShellyKa13

@arnongilboa arnongilboa Awaiting requested review from arnongilboa

Assignees

@awels awels

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. dco-signoff: yes Indicates the PR's author has DCO signed all their commits. lgtm Indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/XL
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@akalenyu @kubevirt-bot @mhenriks @aglitke @awels