Annotation to check for statically provisioned PVs when creating DataVolumes #2583

mhenriks · 2023-02-12T21:26:53Z

What this PR does / why we need it:

This PR basically does the following if a DataVolume that has the cdi.kubevirt.io/storage.checkStaticVolume annotation is created:

Check if there is PV explicitly provisioned (has claimRef name+namespace set) for the target PVC
If so, the "base" controller will create the target PVC in syncCommon()
The target PVC will initially have an annotation cdi.kubevirt.io/storage.persistentVolumeList that contains the names of the PVCs from step 1
When the target PVC gets bound to a PV, syncCommon() will check that the PVC is bound to one of the PVs in the persistentVolumeList annotation
If the bind works as expected, cdi.kubevirt.io/storage.persistentVolumeList is removed and cdi.kubevirt.io/storage.populatedFor is added to the PVC
If the PVC does not bind to one of the expected PVs, the PVC is deleted and we go back to step 1

The motivation for this PR is KubeVirt integration with a specific "Metro DR" solution. That solution bundles:

RamenDR
volume-replication-operator
OCM

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:

TODO

I've taken the liberty to do some minor refactoring along the way.

This has not been addressed and is bumming me out bigtime

Release note:

cdi.kubevirt.io/storage.checkStaticVolume annotation skips volume population step if a static PV is configured for target DataVOlume PVC

mhenriks · 2023-02-12T21:27:09Z

/hold

akalenyu · 2023-02-13T10:27:36Z

pkg/controller/datavolume/import-controller.go

@@ -101,12 +101,29 @@ func NewImportController(
 	return datavolumeController, nil
 }

+// TODO find a better place for this


There's

containerized-data-importer/pkg/controller/datavolume/smart-clone-controller.go

Line 362 in 11fc97e

key, err := cache.MetaNamespaceKeyFunc(snapshot)

But I might be missing a reason that it's not suitable for us

problem is that pv claimRef is not a metav1.Object so have to sonstruct from namespace/name

I would put a common func in pkg/controller/common and get rid of the following:

datasource-controller.go: getKey := func(namespace, name string) string { datavolume/clone-controller-base.go: getKey := func(namespace, name string) string { datavolume/pvc-clone-controller.go: getKey := func(namespace, name string) string {

akalenyu · 2023-02-13T10:42:26Z

pkg/controller/datavolume/controller-base.go

+	}
+	var pvNames []string
+	for _, pv := range pvList.Items {
+		// TODO - should we be more specific here, like do the actual matching in pv controller?


Maybe something from https://pkg.go.dev/k8s.io/component-helpers@v0.26.1/storage/volume
can help here?

unfortunately that package is not as fleshed out in k8s 23 but using it anyway

mhenriks · 2023-02-16T14:52:26Z

/retest-required

akalenyu · 2023-02-20T15:18:06Z

tests/static-volume_test.go

+
+			pvc, err := f.K8sClient.CoreV1().PersistentVolumeClaims(dv.Namespace).Get(context.TODO(), dv.Name, metav1.GetOptions{})
+			Expect(err).ToNot(HaveOccurred())
+			Expect(pvc.Spec.VolumeName).To(Equal(pvName))


This test is great, just one consideration, maybe we should make sure pv.CreationTimestamp < dv.CreationTimestamp

I am a broken record about this by now, but, maybe we would sleep better if there is an md5 check at the end of this test

not a bad idea

akalenyu · 2023-02-20T15:21:02Z

tests/utils/datavolume.go

@@ -317,6 +317,30 @@ func NewDataVolumeWithHTTPImportToBlockPV(dataVolumeName string, size string, ht

 // NewDataVolumeWithExternalPopulation initializes a DataVolume struct meant to be externally populated
 func NewDataVolumeWithExternalPopulation(dataVolumeName, size, storageClassName string, volumeMode corev1.PersistentVolumeMode, dataSource, dataSourceRef *corev1.TypedLocalObjectReference) *cdiv1.DataVolume {
+	dataVolume := &cdiv1.DataVolume{


Could you explain what happened here?
Usually, I thought we shift towards using the storage API mostly

The storage api obscures the matching a bit, just have to be consistent either way (all pvc or all storage). I took the PVC route to be explicit as possible. Basically when I originally wrote the test I was using pvc to create the source then didn't get a match I used storage for target.

mhenriks · 2023-02-20T22:58:58Z

@akalenyu @arnongilboa what do you think?

mhenriks · 2023-02-20T23:07:03Z

/unhold

arnongilboa · 2023-02-21T09:00:52Z

pkg/controller/datavolume/controller-base.go

+
+func shouldSetPending(pvc *corev1.PersistentVolumeClaim, dv *cdiv1.DataVolume) bool {
+	return checkStaticProvisionPending(pvc, dv) ||
+		(pvc == nil && dv.Status.Phase == cdiv1.PhaseUnset)


I would add && dv != nil, as checkStaticProvisionPending() returns false if pvc == nil || dv == nil.

dv will never be nil in either case and shouldSetPending kind of assumes the dv exists

arnongilboa · 2023-02-21T09:46:43Z

pkg/controller/datavolume/util.go

+// CheckVolumeModeMismatches is a convenience method that checks volumeMode for PersistentVolume
+// and PersistentVolumeClaims
+// TODO - later versions of k8s.io/component-helpers have this function
+func CheckVolumeModeMismatches(pvcSpec *v1.PersistentVolumeClaimSpec, pvSpec *v1.PersistentVolumeSpec) bool {


It's strange k8s (and you) choose Mismatches instead of being positive

I'd like to stick with k8s on this

arnongilboa · 2023-02-21T09:50:56Z

pkg/controller/datavolume/util.go

+		_, ok := pvModesMap[mode]
+		if !ok {


I think it's more clear to write it as:
if hasMode := pvModesMap[mode]; hasMode {

eh, would rather do exactly what the lib does

…lt as a parameter Signed-off-by: Michael Henriksen <mhenriks@redhat.com>

Signed-off-by: Michael Henriksen <mhenriks@redhat.com>

…cto common index creation Signed-off-by: Michael Henriksen <mhenriks@redhat.com>

…ions) even if source does not exist or user has no permission BUT no token is added so this is really just for the static/prepopulate cases Signed-off-by: Michael Henriksen <mhenriks@redhat.com>

arnongilboa · 2023-02-22T09:58:38Z

pkg/apiserver/webhooks/datavolume-mutate.go

+
+	cloneSourceHandler, err := newCloneSourceHandler(dataVolume, wh.cdiClient)
+	if err != nil {
+		if k8serrors.IsNotFound(err) && noTokenOkay {


Why do we continue with the following code when noTokenOkay and not just return?

Want to add the token if possible. Still have to handle initial provisioning case where DataVolume has static pv annotation but the pv does not exist

akalenyu · 2023-02-22T12:10:08Z

tests/static-volume_test.go

+			Expect(err).ToNot(HaveOccurred())
+			err = utils.DeleteVerifierPod(f.K8sClient, f.Namespace.Name)
+			Expect(err).ToNot(HaveOccurred())
+			sourceMD5 = md5


Really not big deal but I think you can get rid of this and use the md5 const

containerized-data-importer/tests/utils/upload.go

Lines 42 to 43 in 3788f70

// UploadFileMD5 is the expected MD5 of the uploaded file

UploadFileMD5 = "2a7a52285c846314d1dbd79e9818270d"

mhenriks · 2023-02-22T13:43:22Z

/retest-required

arnongilboa · 2023-02-22T13:58:44Z

tests/static-volume_test.go

+			By("Creating source DV")
+			// source here shouldn't matter
+			dvDef := importDef()
+			controller.AddAnnotation(dvDef, controller.AnnDeleteAfterCompletion, "false")


why GC needs to be disabled?

arnongilboa · 2023-02-22T14:27:47Z

/lgtm

arnongilboa · 2023-02-22T16:29:08Z

/retest

akalenyu · 2023-02-22T16:44:20Z

/lgtm

mhenriks · 2023-02-22T22:38:45Z

/approve

kubevirt-bot · 2023-02-22T22:38:55Z

[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

mhenriks · 2023-02-23T00:58:46Z

/cherrypick release-v1.56

kubevirt-bot · 2023-02-23T00:59:27Z

@mhenriks: new pull request created: #2605

In response to this:

/cherrypick release-v1.56

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 Feb 12, 2023

kubevirt-bot requested review from aglitke and maya-r February 12, 2023 21:26

kubevirt-bot added the size/L label Feb 12, 2023

kubevirt-bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 12, 2023

akalenyu reviewed Feb 13, 2023

View reviewed changes

mhenriks force-pushed the replicated-volume-annotation branch 4 times, most recently from dc54297 to 626abd1 Compare February 15, 2023 14:22

kubevirt-bot added size/XL and removed size/L labels Feb 16, 2023

mhenriks force-pushed the replicated-volume-annotation branch 3 times, most recently from 9f43eae to 3288da2 Compare February 18, 2023 00:59

akalenyu reviewed Feb 20, 2023

View reviewed changes

mhenriks force-pushed the replicated-volume-annotation branch from 3288da2 to 48b79e8 Compare February 20, 2023 22:56

kubevirt-bot added size/XXL and removed size/XL labels Feb 20, 2023

kubevirt-bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 20, 2023

arnongilboa reviewed Feb 21, 2023

View reviewed changes

mhenriks added 9 commits February 21, 2023 22:44

function should return dataVolumeSyncResult, take *dataVolumeSyncResu…

e2e132e

…lt as a parameter Signed-off-by: Michael Henriksen <mhenriks@redhat.com>

checkStaticVolume implemetation for import DataVolume

965c5e7

Signed-off-by: Michael Henriksen <mhenriks@redhat.com>

upload support for checkStaticVolume

f0166f8

Signed-off-by: Michael Henriksen <mhenriks@redhat.com>

checkStaticVolume for clone datavolumes

7787930

Signed-off-by: Michael Henriksen <mhenriks@redhat.com>

checkStaticVolume for snapshot clone

d079bf5

Signed-off-by: Michael Henriksen <mhenriks@redhat.com>

checkStaticVolume for external populator source

fb4470c

Signed-off-by: Michael Henriksen <mhenriks@redhat.com>

tignten up static volume check

9196adc

Signed-off-by: Michael Henriksen <mhenriks@redhat.com>

expand functional test to compare creation timestamps

868ccf9

Signed-off-by: Michael Henriksen <mhenriks@redhat.com>

updates from code review mostly add md5 verification to test and refa…

38fa097

…cto common index creation Signed-off-by: Michael Henriksen <mhenriks@redhat.com>

mhenriks force-pushed the replicated-volume-annotation branch from 84790a4 to a1f5684 Compare February 22, 2023 03:44

webhook changes, allow clone source DataVolumes (with special annotat…

3788f70

…ions) even if source does not exist or user has no permission BUT no token is added so this is really just for the static/prepopulate cases Signed-off-by: Michael Henriksen <mhenriks@redhat.com>

mhenriks force-pushed the replicated-volume-annotation branch from a1f5684 to 3788f70 Compare February 22, 2023 03:47

arnongilboa reviewed Feb 22, 2023

View reviewed changes

akalenyu reviewed Feb 22, 2023

View reviewed changes

arnongilboa reviewed Feb 22, 2023

View reviewed changes

kubevirt-bot assigned arnongilboa Feb 22, 2023

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

kubevirt-bot assigned akalenyu Feb 22, 2023

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

kubevirt-bot merged commit 496efbc into kubevirt:main Feb 22, 2023

kubevirt-bot mentioned this pull request Feb 23, 2023

[release-v1.56] Annotation to check for statically provisioned PVs when creating DataVolumes #2605

Merged

alromeros mentioned this pull request Jul 31, 2023

Feature Request: Support retained PV as source #2359

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Annotation to check for statically provisioned PVs when creating DataVolumes #2583

Annotation to check for statically provisioned PVs when creating DataVolumes #2583

mhenriks commented Feb 12, 2023 •

edited

Loading

mhenriks commented Feb 12, 2023

akalenyu Feb 13, 2023 •

edited

Loading

mhenriks Feb 20, 2023

arnongilboa Feb 21, 2023

akalenyu Feb 13, 2023

mhenriks Feb 20, 2023

mhenriks commented Feb 16, 2023

akalenyu Feb 20, 2023

mhenriks Feb 20, 2023

akalenyu Feb 21, 2023

mhenriks Feb 21, 2023

akalenyu Feb 20, 2023

mhenriks Feb 20, 2023

akalenyu Feb 21, 2023

mhenriks commented Feb 20, 2023

mhenriks commented Feb 20, 2023

arnongilboa Feb 21, 2023 •

edited

Loading

mhenriks Feb 21, 2023

arnongilboa Feb 21, 2023

mhenriks Feb 21, 2023

arnongilboa Feb 21, 2023 •

edited

Loading

mhenriks Feb 21, 2023

arnongilboa Feb 22, 2023

mhenriks Feb 22, 2023

akalenyu Feb 22, 2023

mhenriks commented Feb 22, 2023

arnongilboa Feb 22, 2023

arnongilboa commented Feb 22, 2023

arnongilboa commented Feb 22, 2023

akalenyu commented Feb 22, 2023

mhenriks commented Feb 22, 2023

kubevirt-bot commented Feb 22, 2023

mhenriks commented Feb 23, 2023

kubevirt-bot commented Feb 23, 2023

	// UploadFileMD5 is the expected MD5 of the uploaded file
	UploadFileMD5 = "2a7a52285c846314d1dbd79e9818270d"

Annotation to check for statically provisioned PVs when creating DataVolumes #2583

Annotation to check for statically provisioned PVs when creating DataVolumes #2583

Conversation

mhenriks commented Feb 12, 2023 • edited Loading

mhenriks commented Feb 12, 2023

akalenyu Feb 13, 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

mhenriks commented Feb 16, 2023

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

mhenriks commented Feb 20, 2023

mhenriks commented Feb 20, 2023

arnongilboa Feb 21, 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

arnongilboa Feb 21, 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

mhenriks commented Feb 22, 2023

Choose a reason for hiding this comment

arnongilboa commented Feb 22, 2023

arnongilboa commented Feb 22, 2023

akalenyu commented Feb 22, 2023

mhenriks commented Feb 22, 2023

kubevirt-bot commented Feb 22, 2023

mhenriks commented Feb 23, 2023

kubevirt-bot commented Feb 23, 2023

mhenriks commented Feb 12, 2023 •

edited

Loading

akalenyu Feb 13, 2023 •

edited

Loading

arnongilboa Feb 21, 2023 •

edited

Loading

arnongilboa Feb 21, 2023 •

edited

Loading