Allow StorageProfile to use a specific VolumeSnapshotClass #2898

arnongilboa · 2023-09-14T16:34:42Z

What this PR does / why we need it:
When there are several VolumeSnapshotClasses of the same provisioner but with different parameters, we would like to allow StorageProfile choose VolumeSnapshotClass for its StorageClass, so we won't choose the wrong one.

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 bz #2219774

Special notes for your reviewer:

Release note:

Allow StorageProfile to use a specific VolumeSnapshotClass

akalenyu

PR looks good and makes sense according to the sig storage call,
just some questions

/cc @aglitke
for some more eyes on the approach

@arnongilboa BTW, is the plan to backport this? or could we live with this just in main?

akalenyu · 2023-09-18T16:32:10Z

pkg/controller/common/util.go

@@ -1803,7 +1803,7 @@ func ValidateSnapshotCloneProvisioners(ctx context.Context, c client.Client, sna
 }

 // GetSnapshotClassForSmartClone looks up the snapshot class based on the storage class
-func GetSnapshotClassForSmartClone(dvName string, targetPvcStorageClassName *string, log logr.Logger, client client.Client) (string, error) {
+func GetSnapshotClassForSmartClone(dvName string, targetPvcStorageClassName, snapshotClassName *string, log logr.Logger, client client.Client) (string, error) {


Maybe we should define something more deterministic for the else { case?
Right now we're just picking the first match, but maybe we can define something like

StorageProfile

Default cluster snap class

first match in sorted slice

Sure, in the else we can return the default cluster snap class (if there is one) instead of the first match.

Agree but you can only pick the default SnapClass if the provisioner matches. So the order is something like:

StorageProfile

If there is a single snap class matching the provisioner (easy case): Use single matching snap class

If there are multiple matches and there is a default snap class with matching provisioner: Default cluster snap class

If there are multiple matches and no matching default snap class: choose first match in sorted slice

@aglitke that's exactly what implemented.

akalenyu · 2023-09-18T16:39:57Z

pkg/controller/common/util.go

@@ -1803,7 +1803,7 @@ func ValidateSnapshotCloneProvisioners(ctx context.Context, c client.Client, sna
 }

 // GetSnapshotClassForSmartClone looks up the snapshot class based on the storage class
-func GetSnapshotClassForSmartClone(dvName string, targetPvcStorageClassName *string, log logr.Logger, client client.Client) (string, error) {
+func GetSnapshotClassForSmartClone(dvName string, targetPvcStorageClassName, snapshotClassName *string, log logr.Logger, client client.Client) (string, error) {


Am I missing something, or does this need to change too?

containerized-data-importer/pkg/controller/clone/common.go

Lines 214 to 215 in 581dfb9

// GetCompatibleVolumeSnapshotClass returns a VolumeSnapshotClass name that works for all PVCs

func GetCompatibleVolumeSnapshotClass(ctx context.Context, c client.Client, pvcs ...*corev1.PersistentVolumeClaim) (*string, error) {

Yes it should, and I guess they should be refactored to reuse the same code.

I can see how its more tricky in the populator case, since you probably don't want to start looking up storage profiles here

Look at the updated code. Iterating the pvcs related StorageProfiles in the populator case doesn't sound that tricky, but not sure it's a must when we already added the default support. wdyt?

I am more concerned about the coupling of storage profiles and our populators
@alromeros Did we ever mean for our set of populators to be used standalone?
Or is this fine to look up storage profiles?

@alromeros Did we ever mean for our set of populators to be used standalone?

Yeah... so it depends on what we mean by standalone usage. We first merged the populators without DV support and ensured it was well-tested and reliable as a standalone feature. However they'll always be dependent on CDI controllers, so don't know if relying on storage profiles would be against this design decision.

However they'll always be dependent on CDI

Yeah, that is what I thought as well, you still have to have things like the clone-controller for host-assisted right? @mhenriks wdyt?

Yeah, we plan to keep depending on the import, upload, and clone controllers. So I guess that if the main rationale behind populators was always to stop depending on DVs (and we are close to achieving that), there's no problem with resorting to storage profiles since they were never an issue to begin with.

pkg/controller/storageprofile-controller.go

kubevirt-bot · 2023-09-18T16:43:37Z

@akalenyu: GitHub didn't allow me to request PR reviews from the following users: the, approach, for, some, more, eyes, on.

Note that only kubevirt members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

PR looks good and makes sense according to the sig storage call,
just some questions

/cc @aglitke for some more eyes on the approach

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

aglitke · 2023-09-19T17:19:28Z

pkg/controller/common/util.go

 	}

 	logger.Info("Could not match snapshotter with storage class, falling back to host assisted clone")
 	return "", nil
 }

+// GetVolumeSnapshotClass looks up the snapshot class based on the driver and an optional specific name
+func GetVolumeSnapshotClass(ctx context.Context, c client.Client, driver string, snapshotClassName *string) (*string, error) {


Let's make sure we log why we chose a particular snapshot class. This reminds me of the work we are doing to help people understand why we needed to fall back to host-assisted clone.

aglitke

Approach looks good. I made some comments.

awels · 2023-09-22T12:29:33Z

pkg/controller/clone/common.go

@@ -148,6 +145,23 @@ func GetStorageClassForClaim(ctx context.Context, c client.Client, pvc *corev1.P
 	return nil, nil
 }

+func getSnapshotClassForClaim(ctx context.Context, c client.Client, pvc *corev1.PersistentVolumeClaim) (*string, error) {
+	if pvc.Spec.StorageClassName == nil || *pvc.Spec.StorageClassName == "" {
+		return nil, nil


What about default storage classes? We have a function that looks up the storage class name of a PVC including default storage class if set.

awels · 2023-09-22T12:41:05Z

pkg/controller/common/util.go

+			return nil, err
+		}
+		if vsc.Driver == driver {
+			logger.Info("VolumeSnapshotClass selected according to StorageProfile", "name", vsc.Name)


Besides logging this in the cdi deployment log, can we emit an event on the target PVC (and maybe even put an annotation on it, so we can report it in a datavolume condition) on which snapshot class we picked and why?

Choosing a specific VolumeSnapshotClass is a simple good-path logic and a rare use case, so I think logging it is more than enough (unlike the fallback to host-assisted clone where we added pvc event and anotation). @aglitke wdyt?

Since this decision was made based on something other than the PVC/DV spec, I think we should err on the side of over-communicating this decision. I agree with @awels .

I guess the problem here is that we don't have the PVC so it is difficult to communicate this information.

But we could pass in the name of the PVC, and use that to then look up the PVC and emit the event.

awels · 2023-09-22T12:42:29Z

pkg/controller/common/util.go

+	for _, vsc := range vscList.Items {
+		if vsc.Driver == driver {
+			if vsc.Annotations[AnnDefaultSnapshotClass] == "true" {
+				logger.Info("Default VolumeSnapshotClass selected", "name", vsc.Name)


Same on emitting event

awels · 2023-09-22T12:42:34Z

pkg/controller/common/util.go

+
+	if len(candidates) > 0 {
+		sort.Strings(candidates)
+		logger.Info("First VolumeSnapshotClass selected", "name", candidates[0], "candidates", len(candidates))


Same on emitting event

akalenyu

So apparently I lied and default-snapshot-class is a per-provisioner setting!
I don't think it's clear from k8s docs but I stumbled across the code

https://github.com/kubernetes-csi/external-snapshotter/blob/ff71329d8cc08dca53194f8d4568ed53e69ecb82/pkg/common-controller/snapshot_controller.go#L1362-L1374

I guess this makes the situation solvable without any additions?
@arnongilboa @aglitke

arnongilboa · 2023-09-26T14:41:01Z

So apparently I lied and default-snapshot-class is a per-provisioner setting! I don't think it's clear from k8s docs but I stumbled across the code

https://github.com/kubernetes-csi/external-snapshotter/blob/ff71329d8cc08dca53194f8d4568ed53e69ecb82/pkg/common-controller/snapshot_controller.go#L1362-L1374

I guess this makes the situation solvable without any additions? @arnongilboa @aglitke

@akalenyu but the whole idea (see the bz) was allowing to use different VolumeSnapshotClasses with the same provisioner, so how does default-per-provisioner help here?

akalenyu · 2023-09-26T14:50:57Z

So apparently I lied and default-snapshot-class is a per-provisioner setting! I don't think it's clear from k8s docs but I stumbled across the code
https://github.com/kubernetes-csi/external-snapshotter/blob/ff71329d8cc08dca53194f8d4568ed53e69ecb82/pkg/common-controller/snapshot_controller.go#L1362-L1374
I guess this makes the situation solvable without any additions? @arnongilboa @aglitke

@akalenyu but the whole idea (see the bz) was allowing to use different VolumeSnapshotClasses with the same provisioner, so how does default-per-provisioner help here?

Right we would still need changes. But we could avoid the storageprofile field and just add respecting for the default snapclass, which they can always change as needed

arnongilboa · 2023-09-26T15:00:51Z

So apparently I lied and default-snapshot-class is a per-provisioner setting! I don't think it's clear from k8s docs but I stumbled across the code
https://github.com/kubernetes-csi/external-snapshotter/blob/ff71329d8cc08dca53194f8d4568ed53e69ecb82/pkg/common-controller/snapshot_controller.go#L1362-L1374
I guess this makes the situation solvable without any additions? @arnongilboa @aglitke

@akalenyu but the whole idea (see the bz) was allowing to use different VolumeSnapshotClasses with the same provisioner, so how does default-per-provisioner help here?

Right we would still need changes. But we could avoid the storageprofile field and just add respecting for the default snapclass, which they can always change as needed

but the user wants to use two different VolumeSnapshotClasses with the same provisioner in parallel, so patching the default with the desired value per PVC creation seems like a an ugly hack.

akalenyu · 2023-09-26T18:44:59Z

So apparently I lied and default-snapshot-class is a per-provisioner setting! I don't think it's clear from k8s docs but I stumbled across the code
https://github.com/kubernetes-csi/external-snapshotter/blob/ff71329d8cc08dca53194f8d4568ed53e69ecb82/pkg/common-controller/snapshot_controller.go#L1362-L1374
I guess this makes the situation solvable without any additions? @arnongilboa @aglitke

@akalenyu but the whole idea (see the bz) was allowing to use different VolumeSnapshotClasses with the same provisioner, so how does default-per-provisioner help here?

Right we would still need changes. But we could avoid the storageprofile field and just add respecting for the default snapclass, which they can always change as needed

but the user wants to use two different VolumeSnapshotClasses with the same provisioner in parallel, so patching the default with the desired value per PVC creation seems like a an ugly hack.

Yep, in that case we still need a storage profile field, it's too ugly to keep patching them

awels

looks good minus the issue about emitting an event.

Signed-off-by: Arnon Gilboa <agilboa@redhat.com>

in GetCompatibleVolumeSnapshotClass Signed-off-by: Arnon Gilboa <agilboa@redhat.com>

Signed-off-by: Arnon Gilboa <agilboa@redhat.com>

arnongilboa · 2023-11-06T13:11:48Z

/retest

arnongilboa · 2023-11-06T18:48:24Z

/retest

awels · 2023-11-06T21:39:15Z

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

arnongilboa · 2023-11-07T07:00:30Z

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

akalenyu

Looks good, just mainly concerned about the loop var pointer being a recipe for trouble

akalenyu · 2023-11-07T13:29:23Z

pkg/controller/storageprofile-controller.go

 		installerLabels: installerLabels,
 	}

 	storageProfileController, err := controller.New(
-		"storageprofile-controller",
+		dataImportControllerName,


dataImportControllerName?

good catch:)

akalenyu · 2023-11-07T13:37:31Z

pkg/controller/common/util.go

+		if vsc.Driver == driver {
+			if vsc.Annotations[AnnDefaultSnapshotClass] == "true" {
+				logEvent(MessageDefaultVolumeSnapshotClassSelected, vsc.Name)
+				return &vsc.Name, nil


Usually pointers to loop vars are an endless path of pain and sadness https://go.dev/blog/loopvar-preview

pkg/controller/common/util.go

Signed-off-by: Arnon Gilboa <agilboa@redhat.com>

akalenyu · 2023-11-07T16:08:26Z

/approve
/test pull-cdi-apidocs
/cc @awels

kubevirt-bot · 2023-11-07T16:08:29Z

@akalenyu: GitHub didn't allow me to request PR reviews from the following users: for, lgtm.

Note that only kubevirt members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

/approve
/test pull-cdi-apidocs
/cc @awels for lgtm

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

kubevirt-bot · 2023-11-07T16:08:44Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: akalenyu

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

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

awels · 2023-11-07T20:18:50Z

/lgtm

akalenyu · 2023-11-07T21:06:19Z

/test pull-containerized-data-importer-e2e-hpp-previous

arnongilboa · 2023-11-08T06:57:09Z

/retest

arnongilboa · 2023-11-08T10:00:33Z

/test pull-containerized-data-importer-e2e-hpp-latest

arnongilboa · 2023-11-08T13:24:37Z

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

awels · 2023-11-08T15:23:53Z

/cherrypick release-v1.58

kubevirt-bot · 2023-11-08T15:23:55Z

@awels: once the present PR merges, I will cherry-pick it on top of release-v1.58 in a new PR and assign it to you.

In response to this:

/cherrypick release-v1.58

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

akalenyu · 2023-11-09T09:17:02Z

/retest

akalenyu · 2023-11-09T09:50:54Z

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

kubevirt-bot · 2023-11-09T12:24:30Z

@arnongilboa: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
pull-containerized-data-importer-e2e-ceph	`5386a44`	link	unknown	`/test pull-containerized-data-importer-e2e-ceph`
pull-containerized-data-importer-e2e-hpp-latest	`5386a44`	link	true	`/test pull-containerized-data-importer-e2e-hpp-latest`

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

akalenyu · 2023-11-09T12:33:23Z

/test pull-containerized-data-importer-e2e-hpp-latest

kubevirt-bot · 2023-11-09T12:38:14Z

@awels: new pull request created: #2974

In response to this:

/cherrypick release-v1.58

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

kubevirt-bot added 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. labels Sep 14, 2023

kubevirt-bot requested review from awels and ShellyKa13 September 14, 2023 16:34

kubevirt-bot added the size/L label Sep 14, 2023

akalenyu reviewed Sep 18, 2023

View reviewed changes

kubevirt-bot requested a review from aglitke September 18, 2023 16:43

arnongilboa force-pushed the storageprofile_snapshotclass branch from 581dfb9 to 1fa4590 Compare September 19, 2023 15:39

aglitke reviewed Sep 19, 2023

View reviewed changes

awels reviewed Sep 22, 2023

View reviewed changes

akalenyu reviewed Sep 26, 2023

View reviewed changes

arnongilboa force-pushed the storageprofile_snapshotclass branch from 312a0b8 to 1f6fa85 Compare October 9, 2023 11:12

awels reviewed Oct 9, 2023

View reviewed changes

kubevirt-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 27, 2023

arnongilboa added 3 commits November 5, 2023 17:24

Allow StorageProfile to use a specific VolumeSnapshotClass

d9a2892

Signed-off-by: Arnon Gilboa <agilboa@redhat.com>

Use PVCs common snapshot class

4bd3046

in GetCompatibleVolumeSnapshotClass Signed-off-by: Arnon Gilboa <agilboa@redhat.com>

Add VolumeSnapshotClass selection logs

2d3b684

Signed-off-by: Arnon Gilboa <agilboa@redhat.com>

arnongilboa force-pushed the storageprofile_snapshotclass branch from 1f6fa85 to c826a7f Compare November 5, 2023 15:51

kubevirt-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 5, 2023

CR fixes

5392808

Signed-off-by: Arnon Gilboa <agilboa@redhat.com>

arnongilboa force-pushed the storageprofile_snapshotclass branch from c826a7f to 5392808 Compare November 6, 2023 11:54

awels approved these changes Nov 6, 2023

View reviewed changes

kubevirt-bot assigned awels Nov 6, 2023

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

akalenyu reviewed Nov 7, 2023

View reviewed changes

More CR fixes

5386a44

Signed-off-by: Arnon Gilboa <agilboa@redhat.com>

kubevirt-bot removed the lgtm Indicates that a PR is ready to be merged. label Nov 7, 2023

kubevirt-bot requested a review from awels November 7, 2023 16:08

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

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

kubevirt-bot merged commit 1a04ba9 into kubevirt:main Nov 9, 2023
19 of 21 checks passed

kubevirt-bot mentioned this pull request Nov 9, 2023

[release-v1.58] Allow StorageProfile to use a specific VolumeSnapshotClass #2974

Merged

	// GetCompatibleVolumeSnapshotClass returns a VolumeSnapshotClass name that works for all PVCs
	func GetCompatibleVolumeSnapshotClass(ctx context.Context, c client.Client, pvcs ...corev1.PersistentVolumeClaim) (string, error) {

Allow StorageProfile to use a specific VolumeSnapshotClass #2898

Allow StorageProfile to use a specific VolumeSnapshotClass #2898

Conversation

arnongilboa commented Sep 14, 2023 • edited Loading

akalenyu left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arnongilboa Sep 19, 2023 • edited Loading

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

akalenyu Sep 21, 2023 • edited Loading

Choose a reason for hiding this comment

alromeros Sep 21, 2023 • edited Loading

Choose a reason for hiding this comment

kubevirt-bot commented Sep 18, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aglitke 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

akalenyu left a comment • edited Loading

Choose a reason for hiding this comment

arnongilboa commented Sep 26, 2023

akalenyu commented Sep 26, 2023 • edited Loading

arnongilboa commented Sep 26, 2023

akalenyu commented Sep 26, 2023

awels left a comment

Choose a reason for hiding this comment

arnongilboa commented Nov 6, 2023

arnongilboa commented Nov 6, 2023

awels commented Nov 6, 2023

arnongilboa commented Nov 7, 2023

akalenyu 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

akalenyu commented Nov 7, 2023 • edited Loading

kubevirt-bot commented Nov 7, 2023

kubevirt-bot commented Nov 7, 2023

awels commented Nov 7, 2023

akalenyu commented Nov 7, 2023

arnongilboa commented Nov 8, 2023

arnongilboa commented Nov 8, 2023

arnongilboa commented Nov 8, 2023

awels commented Nov 8, 2023

kubevirt-bot commented Nov 8, 2023

akalenyu commented Nov 9, 2023

akalenyu commented Nov 9, 2023

kubevirt-bot commented Nov 9, 2023

akalenyu commented Nov 9, 2023

kubevirt-bot commented Nov 9, 2023

arnongilboa commented Sep 14, 2023 •

edited

Loading

akalenyu left a comment •

edited

Loading

arnongilboa Sep 19, 2023 •

edited

Loading

akalenyu Sep 21, 2023 •

edited

Loading

alromeros Sep 21, 2023 •

edited

Loading

akalenyu left a comment •

edited

Loading

akalenyu commented Sep 26, 2023 •

edited

Loading

akalenyu commented Nov 7, 2023 •

edited

Loading