Create PVC if possible even if the StorageClass is missing #2683

arnongilboa · 2023-04-04T23:07:05Z

What this PR does / why we need it:
When PVC is created with storageClassName and the SC is not found, k8s looks for PV with the storageClassName for satisfying this claim. In this case k8s supports also a blank (“”, not the nil default one) storageClassName. To support this behavior we added:

DV controller support for this flow (for both “” and non-existing SC)
Condition update and event when StorageSpec PVC rendering errors and PVC is not created (e.g. missing both AccessModes and SC/PV)
PVC is created even if a satisfying SC/PV doesn't exist if spec.pvc/storage AccessModes is set (otherwise k8s PVC validation fails). PVC/DV phase will be Pending until a satisfying SC/PV is found
PV watch to reconcile DVs waiting for the PV storageClassName
PV storageClassName indexer, so we can list the relevant PVs

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 #2186

Special notes for your reviewer:

Release note:

Create PVC if possible even if the StorageClass is missing; Update DV condition fire and event on PVC rendering error

mhenriks

storageClass is optional on a PV

user may ask for storageClass "" on a pvc and they will get a PV with no storageclass (assuming other param match of course)

mhenriks · 2023-04-07T14:47:57Z

pkg/apiserver/webhooks/dataimportcron-validate.go

-	causes = validateNameLength(spec.ManagedDataSource, validation.DNS1035LabelMaxLength)
-	if len(causes) > 0 {
+	if cause := validateNameLength(spec.ManagedDataSource, validation.DNS1035LabelMaxLength); cause != nil {
+		causes = append(causes, *cause)


Is there any functional difference in the diff of this file or just style?

Since we moved from returning a slice of causes to a single cause in validateNameLength. This just changes the way that is handled, but functionally does look the same.

Right, the previous one was a bit clumsy.

mhenriks · 2023-04-07T19:21:18Z

pkg/apiserver/webhooks/datavolume-validate.go

+	if *sc == "" {
+		return &metav1.StatusCause{
+			Type:    metav1.CauseTypeFieldValueInvalid,
+			Message: fmt.Sprintf("storageClassName is empty"),


"" storage class name is valid

apiVersion: v1 kind: PersistentVolume metadata: name: host-volume spec: capacity: storage: 10Gi accessModes: - ReadWriteOnce hostPath: path: "/mnt/data" --- apiVersion: v1 kind: PersistentVolumeClaim metadata: name: my-pvc spec: volumeMode: Filesystem storageClassName: "" accessModes: - ReadWriteOnce resources: requests: storage: 5Gi

✗ k get pvc selecting docker as container runtime NAME STATUS VOLUME CAPACITY ACCESS MODES STORAGECLASS AGE my-pvc Bound host-volume 10Gi RWO 3s

So had this discussion with Arnon, if you read this, it does say "" is equivalent to missing, due to historical reasons, but having a blank string is sort of gray at this point. This is fixing a reported issue where someone is passing a blank storage class by accident. I think we should disallow blank, and if someone wants to use the default, they can omit the field.

I dunno, this pr provides no way to explicitly bind to "" storageclass . That combined with the fact that in 1.26 "" on PVC will bind to default storage class if exists, makes me believe that we should leave things as-is OR allow updating the storageclass on the DV as you can do with PVCs in 1.26

Finally tried this out on a 1.26 cluster. Apparently, I misread the doc. "" will not bind to the default storageclass. And pvc with "" storageClassName can't be updated.

Nevertheless because "" is a legit value I don't think we should disallow it

I agree that "" is legit value. Fixing accordingly. Keeping the style changes, as the previous code (my lines) is quit ugly.

mhenriks

This is all a little awkward and racy. Remind me why we can't go straight to "create the pvc even if the storage class doesn't exist"?

mhenriks · 2023-04-20T20:46:06Z

pkg/controller/common/util.go

@@ -338,7 +338,7 @@ func GetStorageClassByName(client client.Client, name *string) (*storagev1.Stora
 		storageClass := &storagev1.StorageClass{}
 		if err := client.Get(context.TODO(), types.NamespacedName{Name: *name}, storageClass); err != nil {
 			klog.V(3).Info("Unable to retrieve storage class", "storage class name", *name)
-			return nil, errors.New("unable to retrieve storage class")
+			return nil, errors.New(fmt.Sprintf("unable to retrieve storage class %s", *name))


not a big deal, no need to change, but errors.Errorf can be used when string formatting errors

mhenriks · 2023-04-20T20:50:20Z

pkg/controller/datavolume/conditions.go

@@ -122,7 +122,7 @@ func UpdateReadyCondition(conditions []cdiv1.DataVolumeCondition, status corev1.
 	return updateCondition(conditions, cdiv1.DataVolumeReady, status, message, reason)
 }

-func updateBoundCondition(conditions []cdiv1.DataVolumeCondition, pvc *corev1.PersistentVolumeClaim, reason string) []cdiv1.DataVolumeCondition {
+func updateBoundCondition(conditions []cdiv1.DataVolumeCondition, pvc *corev1.PersistentVolumeClaim, message, reason string) []cdiv1.DataVolumeCondition {


is there a unit test for this change where message != ""?

mhenriks · 2023-04-20T20:55:11Z

pkg/controller/datavolume/controller-base.go

@@ -76,6 +76,8 @@ const (
 	dvPhaseField = "status.phase"

 	claimRefField = "spec.claimRef"
+
+	claimStorageClassField = "spec.claimStorageClass"


spec.storageClassName is the json property and the standard naming scheme we use

mhenriks · 2023-04-20T20:56:26Z

pkg/controller/datavolume/controller-base.go

+				return
+			}
+			for _, dv := range dvList.Items {
+				storage := dv.Spec.Storage


what about dv.spec.pvc?

It's not needed as we create the pvc even if the storage class doesn't exist if AccessModes is set (otherwise k8s PVC validation fails). PVC/DV phase will be Pending until a satisfying SC/PV is found.

arnongilboa · 2023-04-23T12:32:29Z

This is all a little awkward and racy. Remind me why we can't go straight to "create the pvc even if the storage class doesn't exist"?

We will create the pvc even if the storage class doesn't exist if spec.pvc/storage AccessModes is set (otherwise k8s PVC validation fails). PVC/DV phase will be Pending until a satisfying SC/PV is found.

mhenriks · 2023-04-24T20:01:49Z

This is all a little awkward and racy. Remind me why we can't go straight to "create the pvc even if the storage class doesn't exist"?

We will create the pvc even if the storage class doesn't exist if spec.pvc/storage AccessModes is set (otherwise k8s PVC validation fails). PVC/DV phase will be Pending until a satisfying SC/PV is found.

Okay, that makes sense, so what about this in the description, what is the follow up?

TODO in a follow-up PR: use k8s PVC SC/PV pending mechanism and get rid of it in CDI

mhenriks · 2023-04-24T21:11:56Z

pkg/controller/datavolume/util.go

+// Gets either the bound PV or an available satisfying PV
+func getPV(c client.Client, pvcSpec *v1.PersistentVolumeClaimSpec, pvcName, pvcNamespace string) (*corev1.PersistentVolume, error) {
+	if pvcSpec.StorageClassName == nil {
+		return nil, fmt.Errorf("PVC spec is missing StorageClassName")


Is this really an error? Can't storageclass name be nil for either spec.pvc or spec.storage?

Is that tested?

It's not an error. Fixed. Covered in [test_id:8383]Import fails when no default storage class, and recovers when default is set.

arnongilboa · 2023-04-25T07:55:13Z

Okay, that makes sense, so what about this in the description, what is the follow up?

TODO in a follow-up PR: use k8s PVC SC/PV pending mechanism and get rid of it in CDI

just forgot to remove it :)

duyanyan · 2023-04-25T08:15:48Z

tests/datavolume_test.go

+		},
+			table.Entry("[test_id:XXXX]the storage class is created", testScName, createStorageClass),
+			table.Entry("[test_id:XXXX]PV with the SC name is created", testScName, createPV),
+			table.Entry("[test_id:XXXX]PV with the SC name (\"\" blank) is created", "", createPV),


test_ids: CNV-9922
test_ids: CNV-9924
test_ids: CNV-9925

tests/datavolume_test.go

mhenriks · 2023-04-26T00:04:55Z

/test pull-containerized-data-importer-fossa

mhenriks · 2023-04-26T00:37:24Z

pkg/controller/datavolume/util.go

+		if pv.Status.Phase == corev1.VolumeBound &&
+			pv.Spec.ClaimRef != nil &&
+			pv.Spec.ClaimRef.Name == pvcName &&
+			pv.Spec.ClaimRef.Namespace == pvcNamespace {


Not sure about this loop. If a PV exists that meets this criteria, I'm pretty sure it would mean that the target PVC already exists.

I think just the following loop is sufficient

mhenriks · 2023-04-28T00:01:34Z

/approve

kubevirt-bot · 2023-04-28T00:01:44Z

[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 a few minor nitpicky things.

awels · 2023-04-28T14:05:10Z

pkg/apiserver/webhooks/dataimportcron-validate.go

-	causes = validateNameLength(spec.ManagedDataSource, validation.DNS1035LabelMaxLength)
-	if len(causes) > 0 {
+	if cause := validateNameLength(spec.ManagedDataSource, validation.DNS1035LabelMaxLength); cause != nil {
+		causes = append(causes, *cause)


Since we moved from returning a slice of causes to a single cause in validateNameLength. This just changes the way that is handled, but functionally does look the same.

awels · 2023-04-28T14:33:30Z

tests/datavolume_test.go

@@ -1702,7 +1705,7 @@ var _ = Describe("[vendor:cnv-qe@redhat.com][level:component]DataVolume tests",

 		createDataVolumeForImport := func(f *framework.Framework, storageSpec cdiv1.StorageSpec) *cdiv1.DataVolume {
 			dataVolume := utils.NewDataVolumeWithHTTPImportAndStorageSpec(
-				dataVolumeName, "1Gi", fmt.Sprintf(utils.TinyCoreQcow2URLRateLimit, f.CdiInstallNs))
+				dataVolumeName, "1Gi", fmt.Sprintf(utils.TinyCoreQcow2URL, f.CdiInstallNs))


Good catch, there seems to be no reason this was the rate limited url.

tests/datavolume_test.go

alromeros

Nice job! I have some comments and questions about spec rendering, which I think is getting confusing. Other than that looks good to me.

alromeros · 2023-04-28T15:35:28Z

pkg/controller/datavolume/util.go

@@ -151,6 +152,41 @@ func copyStorageAsPvc(log logr.Logger, storage *cdiv1.StorageSpec) *v1.Persisten
 	return pvcSpec
 }

+// Update the PVC spec from either the created PVC or an available satisfying PV
+func updatePvcSpecFromPvcOrPv(c client.Client, pvcSpec *v1.PersistentVolumeClaimSpec, pvc *v1.PersistentVolumeClaim) error {


I see we update the pvcSpec here but I'm struggling to see the cases where this is relevant if the PVC already exists. Why do we need to set VolumeMode and AccessMode if the PVC already has them?

Good catch! fixed.

alromeros · 2023-04-28T15:45:14Z

pkg/controller/datavolume/util.go

 	if storageClass == nil {
+		if err := updatePvcSpecFromPvcOrPv(client, pvcSpec, pvc); err != nil {


I think this flow is getting confusing, it'd be good to have some comments explaining why are we doing this. I'm also not a fan of the updatePvcSpecFromPvcOrPv name, it's not a must but I think this whole storageClass handling would benefit from being in a separate function.

Simplified and renamed.

When PVC is created with storageClassName and the SC is not found, k8s looks for PV with the storageClassName for satisfying this claim. In this case k8s supports also a blank (“”, not the nil default one) storageClassName. To support this behavior we added: -DV controller support for this flow (for both “” and non-existing SC) -Condition update and event when StorageSpec PVC rendering errors and PVC is not created (e.g. missing both AccessModes and SC/PV) -PVC is created even if a satisfying SC/PV doesn't exist if pvc/storage AccessModes is set (otherwise k8s PVC validation fails). PVC/DV phase will be Pending until a satisfying SC/PV is found -PV watch to reconcile DVs waiting for the PV storageClassName -PV storageClassName indexer, so we can list the relevant PVs Signed-off-by: Arnon Gilboa <agilboa@redhat.com>

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

arnongilboa · 2023-05-01T06:07:48Z

/test pull-cdi-linter

alromeros

Looks good! just a couple of nitpicks that I think could improve the spec rendering code

alromeros · 2023-05-02T11:35:35Z

pkg/controller/datavolume/util.go

@@ -44,85 +44,105 @@ const (
 )

 // renderPvcSpec creates a new PVC Spec based on either the dv.spec.pvc or dv.spec.storage section
-func renderPvcSpec(client client.Client, recorder record.EventRecorder, log logr.Logger, dv *cdiv1.DataVolume) (*v1.PersistentVolumeClaimSpec, error) {
+func renderPvcSpec(client client.Client, recorder record.EventRecorder, log logr.Logger, dv *cdiv1.DataVolume, pvc *v1.PersistentVolumeClaim) (*v1.PersistentVolumeClaimSpec, error) {


I think this would look cleaner if we do something like:

// If the PVC already exists, just return its spec if pvc != nil { return pvc.Spec.DeepCopy(), nil } if dv.Spec.PVC != nil { return dv.Spec.PVC.DeepCopy(), nil } if dv.Spec.Storage != nil { return pvcFromStorage(client, recorder, log, dv) }

pvcFromStorage would require fewer arguments too, wdyt?

It won't work, see the following comment about renderPvcSpecVolumeSize.

alromeros · 2023-05-02T11:38:04Z

pkg/controller/datavolume/util.go

+		pvcSpec = pvc.Spec.DeepCopy()
+	}
+
+	if err := renderPvcSpecVolumeSize(client, dv.Spec, pvcSpec); err != nil {


Related to my previous comment, we are still rendering the volume size for already existing PVCs, which I believe is not necessary. I think this function would be better without any PVC handling.

Or do we really want to update the volume size? In that case, are we updating the PVC later to reflect the new pvcSpec?

It is necessary to render the volume size for smart clones, as the DV may require volume size greater than the source PVC, so the traget PVC should be expanded.

Oh nice, wasn't sure about that. Thanks for explaining!

alromeros · 2023-05-02T11:45:10Z

pkg/controller/datavolume/util.go


+func renderPvcSpecVolumeModeAndAccessModes(client client.Client, recorder record.EventRecorder, log logr.Logger, dv *cdiv1.DataVolume, pvcSpec *v1.PersistentVolumeClaimSpec) error {


Maybe renderVolumeAndAccessModes? I think we could remove the pvcSpec from the name, but renderPvcSpecVolumeAndAccessModes would also work

alromeros · 2023-05-02T14:03:44Z

/lgtm

kubevirt-bot added dco-signoff: yes Indicates the PR's author has DCO signed all their commits. do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. size/L labels Apr 4, 2023

kubevirt-bot requested review from aglitke and maya-r April 4, 2023 23:07

kubevirt-bot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Apr 4, 2023

arnongilboa force-pushed the dv_sc_validation branch 2 times, most recently from 86241ca to d88d20f Compare April 7, 2023 14:56

mhenriks reviewed Apr 7, 2023

View reviewed changes

arnongilboa force-pushed the dv_sc_validation branch from d88d20f to e2203a3 Compare April 19, 2023 09:39

arnongilboa changed the title ~~Validate DV SC; update conditions if SC is missing~~ Allow creating PVC if there is a satisfying PV but SC is missing Apr 19, 2023

arnongilboa force-pushed the dv_sc_validation branch from e2203a3 to 2e23d09 Compare April 19, 2023 14:43

mhenriks reviewed Apr 20, 2023

View reviewed changes

arnongilboa force-pushed the dv_sc_validation branch from 2e23d09 to c4b3453 Compare April 24, 2023 13:01

arnongilboa changed the title ~~Allow creating PVC if there is a satisfying PV but SC is missing~~ Create PVC if possible even if the StorageClass is missing Apr 24, 2023

arnongilboa force-pushed the dv_sc_validation branch 3 times, most recently from 0967366 to 58c8536 Compare April 24, 2023 13:34

mhenriks reviewed Apr 24, 2023

View reviewed changes

arnongilboa force-pushed the dv_sc_validation branch from 58c8536 to 343f977 Compare April 25, 2023 07:57

duyanyan reviewed Apr 25, 2023

View reviewed changes

tests/datavolume_test.go Show resolved Hide resolved

arnongilboa force-pushed the dv_sc_validation branch from 343f977 to 047dd6f Compare April 25, 2023 08:22

mhenriks reviewed Apr 26, 2023

View reviewed changes

arnongilboa force-pushed the dv_sc_validation branch from 047dd6f to a202278 Compare April 27, 2023 09:51

arnongilboa force-pushed the dv_sc_validation branch 2 times, most recently from 64d1109 to 57ef88a Compare April 27, 2023 14:44

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

awels reviewed Apr 28, 2023

View reviewed changes

alromeros reviewed Apr 28, 2023

View reviewed changes

arnongilboa force-pushed the dv_sc_validation branch from 37f9791 to a549247 Compare April 30, 2023 10:57

arnongilboa force-pushed the dv_sc_validation branch from a549247 to 3829441 Compare April 30, 2023 11:12

CR fixes

137ec17

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

arnongilboa force-pushed the dv_sc_validation branch from 3829441 to 137ec17 Compare April 30, 2023 17:58

kubevirt-bot added size/XL and removed size/L labels Apr 30, 2023

alromeros reviewed May 2, 2023

View reviewed changes

kubevirt-bot assigned alromeros May 2, 2023

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

kubevirt-bot merged commit 5d78da3 into kubevirt:main May 2, 2023
1 check passed

akalenyu mentioned this pull request May 7, 2023

Allow snapshots as format for DataImportCron created sources #2700

Merged

akalenyu mentioned this pull request Jun 19, 2023

Feature Request: Support retained PV as source #2359

Closed

		if storageClass == nil {
		if err := updatePvcSpecFromPvcOrPv(client, pvcSpec, pvc); err != nil {


		func renderPvcSpecVolumeModeAndAccessModes(client client.Client, recorder record.EventRecorder, log logr.Logger, dv cdiv1.DataVolume, pvcSpec v1.PersistentVolumeClaimSpec) error {

Create PVC if possible even if the StorageClass is missing #2683

Create PVC if possible even if the StorageClass is missing #2683

Conversation

arnongilboa commented Apr 4, 2023 • edited Loading

mhenriks 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

mhenriks 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

arnongilboa Apr 23, 2023 • edited Loading

Choose a reason for hiding this comment

arnongilboa commented Apr 23, 2023

mhenriks commented Apr 24, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arnongilboa Apr 25, 2023 • edited Loading

Choose a reason for hiding this comment

arnongilboa commented Apr 25, 2023

Choose a reason for hiding this comment

mhenriks commented Apr 26, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mhenriks commented Apr 28, 2023

kubevirt-bot commented Apr 28, 2023

awels left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alromeros 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

arnongilboa commented May 1, 2023

alromeros left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alromeros May 2, 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

alromeros commented May 2, 2023

arnongilboa commented Apr 4, 2023 •

edited

Loading

arnongilboa Apr 23, 2023 •

edited

Loading

arnongilboa Apr 25, 2023 •

edited

Loading

alromeros May 2, 2023 •

edited

Loading