Add PVC spec mutating webhook rendering based on StorageProfiles by arnongilboa · Pull Request #2813 · kubevirt/containerized-data-importer

arnongilboa · 2023-07-19T10:16:09Z

What this PR does / why we need it:

The webhook mutates the PVC Spec based on the available StorageProfiles, so for example you can create PVC without accessModes and it will be auto-completed.

To use this feature, enable the WebhookPvcRendering feature gate.

For any PVC you want to use StorageProfile, label it with cdi.kubevirt.io/useStorageProfile: "true"

If you want to use volumeMode preferred by CDI according to StorageProfiles, set it to FromStorageProfile. Otherwise if not explicitly set to Block, it will be Filesystem by k8s default.

E.g.:

apiVersion: v1
kind: PersistentVolumeClaim
metadata:
  name: pvc-test
  labels:
    cdi.kubevirt.io/useStorageProfile: "true"
spec:
  storageClassName: rook-ceph-block
  volumeMode: FromStorageProfile
  resources:
    requests:
      storage: 1Mi

Special notes for your reviewer:

Release note:

Add PVC spec mutating webhook rendering based on StorageProfiles

awels · 2023-07-19T12:55:30Z

So is there any particular reason, we can't fill out partially completed PVC specs, so if I want to override the storage profile (which may be block) to filesystem, I just fill out the volumeMode in the pvc spec, and anything missing is completed by the webhook? This would mirror what we are doing with DataVolumes right now.

I like that it only applies if you annotate the PVC, so we won't be doing it for all PVCs.

arnongilboa · 2023-07-19T13:10:16Z

So is there any particular reason, we can't fill out partially completed PVC specs, so if I want to override the storage profile (which may be block) to filesystem, I just fill out the volumeMode in the pvc spec, and anything missing is completed by the webhook? This would mirror what we are doing with DataVolumes right now.

That's exactly what we are doing in this POC. Copied the DV StorageProfiles logic to the webhook and patched it, just to get it quickly working.

I like that it only applies if you annotate the PVC, so we won't be doing it for all PVCs.

Right

awels · 2023-07-19T13:47:00Z

So what is the point of cdi.kubervirt.io/useStorageProfile: "Filesystem" then? You can just set the volumeMode to filesystem.

arnongilboa · 2023-07-19T14:05:29Z

So what is the point of cdi.kubervirt.io/useStorageProfile: "Filesystem" then? You can just set the volumeMode to filesystem.

The k8s PVC mutating webhook puts the default Filesystem (if spec.volumeMode is nil) before it gets to CDI PVC mutating webhook, so we can't know if it's the user or k8s choice when we get it.

akalenyu · 2023-07-20T12:27:30Z

So what is the point of cdi.kubervirt.io/useStorageProfile: "Filesystem" then? You can just set the volumeMode to filesystem.

The k8s PVC mutating webhook puts the default Filesystem (if spec.volumeMode is nil) before it gets to CDI PVC mutating webhook, so we can't know if it's the user or k8s choice when we get it.

This may help:
https://kubernetes.io/docs/reference/access-authn-authz/extensible-admission-controllers/#reinvocation-policy

Also pasting the object selector link from our offline discussion:
https://github.com/kubevirt/kubevirt/blob/main/tests/storage/snapshot.go#L183

arnongilboa · 2023-07-20T15:52:53Z

/retest

akalenyu · 2023-08-30T15:48:41Z

+}
+
+func (wh *pvcMutatingWebhook) ListPersistentVolumes(ctx context.Context) (*v1.PersistentVolumeList, error) {
+	return wh.k8sClient.CoreV1().PersistentVolumes().List(ctx, metav1.ListOptions{})


Note these are not cached clients so those list calls will be costly at scale ("real" clusters)

Rendering is done once per labeled-pvc creation, so not sure it's that expansive in comparison to its population...

Do we have a way to use the cached client from the webhook?

arnongilboa · 2024-01-18T13:46:14Z

/retest

arnongilboa · 2024-01-19T07:42:01Z

/retest

The webhook mutates the PVC Spec based on the available StorageProfiles, so for example you can create PVC without accessModes and it will be auto-completed. To use this feature, enable the `WebhookPvcRendering` feature gate. For any PVC you want to use StorageProfile, label it with: cdi.kubevirt.io/useStorageProfile: "true" If you want to use volumeMode preferred by CDI according to StorageProfiles, set it to FromStorageProfile. Otherwise if not explicitly set to Block, it will be Filesystem by k8s default. E.g.: apiVersion: v1 kind: PersistentVolumeClaim metadata: name: pvc-test labels: cdi.kubevirt.io/useStorageProfile: "true" spec: storageClassName: rook-ceph-block volumeMode: FromStorageProfile resources: requests: storage: 1Mi Signed-off-by: Arnon Gilboa <[email protected]>

mhenriks · 2024-01-29T19:39:19Z

+		}
+		return nil
+	}
+	if err := c.Delete(context.TODO(), whc); client.IgnoreNotFound(err) != nil {


pkg/operator/resources is really just for static configuration. Check out callbacks.go. You can write a callback that creates/deletes the webhook based on the feature gate somewhat similar to what we do when creating Routes/SCCs on openshift

mhenriks · 2024-01-29T19:43:02Z

 )

+// RenderPvc renders the PVC according to StorageProfiles
+func RenderPvc(ctx context.Context, client client.Client, pvc *v1.PersistentVolumeClaim) error {


What about setting virt default storage class if storageclass is unset?

We get it for free from the call to renderPvcSpecVolumeModeAndAccessModes() which uses GetStorageClassByNameWithVirtFallback().

Okay do you think it would make sense to extract that bit and create a renderPvcSpecStorageClass function? Or change renderPvcSpecVolumeModeAndAccessModes to renderPvcSpecVolumeModeAndAccessModesAndStorageClass?

mhenriks · 2024-01-30T20:54:53Z

+		return nil
+	}
+
+	crd := args.CurrentObject.(*extv1.CustomResourceDefinition)


I think it would make more sense to link this with the "cdi-apiserver" deployment. If for some reason cdi-apiserver is not created we would not want to create a webhook

But then how would you delete the webhook if "cdi-apiserver" somehow disappears? not sure these cases are relevant as you cannot do anything without "cdi-apiserver" (can you?), and we reconcile for its eventual consistency.

Yeah interesting point, it is doubtful that we will ever not deploy cdi-apiserver but I still think it is better to time the creation of the webhook close to the creation of cdi-apiserver ideally right before it

mhenriks · 2024-01-31T14:31:54Z

+			return err
+		}
+		if enabled {
+			whc = cluster.CreatePvcMutatingWebhook(args.Namespace, args.Client, args.Logger)


I think to be consistent with the other resources created by callbacks, maybe put CreatePvcMutatingWebhook in this package.

And don't forget to make sure it is owned by CDI

Move it to this package although all other similar webhook CR creation code is in the cluster package? much harder to maintain

And don't forget to make sure it is owned by CDI

fixed

Move it to this package although all other similar webhook CR creation code is in the cluster package? much harder to maintain

Can you help me understand that? To me, being in the same file or package is more maintainable for non-shared code

Moved. I buy it when you explain it this way.

awels

Looks pretty good to me, a few nits.

awels · 2024-01-31T17:31:30Z

+
+PVC Mutating Webhook Rendering is an optional CDI feature, allowing users to get CDI PVC rendering functionality without using a `DataVolume`. Traditionally, when the CDI DV controller creates a PVC, it renders the PVC spec (`volumeMode`, `accessMode`, `storage`) according to the DV storage spec (or default) `storageClass`, CDI `StorageProfiles`, `CDIConfig` `filesystemOverhead` etc.
+
+The PVC mutating webhook eliminats the need for DVs for `StorageProfile` based rendering, providing auto-completion of PVC missing spec fields based on optimal values per `StorageClass`. The webhook intercepts only explicitly CDI-labeled PVCs, so it won't affect cluster stability if the CDI api server is down. For labeled PVCs, `objectSelectors` decide when to call out over HTTP to the webhook, so if the CDI api server is down the request and PVC creation will fail. Unlabeled PVC will not be affected at all.


Suggested change

The PVC mutating webhook eliminats the need for DVs for `StorageProfile` based rendering, providing auto-completion of PVC missing spec fields based on optimal values per `StorageClass`. The webhook intercepts only explicitly CDI-labeled PVCs, so it won't affect cluster stability if the CDI api server is down. For labeled PVCs, `objectSelectors` decide when to call out over HTTP to the webhook, so if the CDI api server is down the request and PVC creation will fail. Unlabeled PVC will not be affected at all.

The PVC mutating webhook eliminates the need for DVs for `StorageProfile` based rendering, providing auto-completion of PVC missing spec fields, based on optimal values per `StorageClass`. The webhook intercepts only explicitly CDI-labeled PVCs, so it won't affect cluster stability if the CDI api server is down. For labeled PVCs, `objectSelectors` decide when to call out over HTTP to the webhook, so if the CDI api server is down the request and PVC creation will fail. Unlabeled PVC will not be affected at all.

awels · 2024-01-31T19:35:23Z

 		}
-		volumeMode := v1.PersistentVolumeFilesystem
-		pvcSpec.VolumeMode = &volumeMode
+		pvcSpec.VolumeMode = &cc.FilesystemMode


Can we use ptr.To[type](volumeMode) here? To match its usage everywhere else?

plus some CR fixes and cleanups Signed-off-by: Arnon Gilboa <[email protected]>

mhenriks · 2024-02-01T01:43:18Z

+	}
+
+	deployment, ok := args.DesiredObject.(*appsv1.Deployment)
+	if !ok || deployment.Name != "cdi-apiserver" {


check out recently added CDIApiServerResourceName const

arnongilboa · 2024-02-01T14:46:34Z

/retest

awels · 2024-02-01T19:06:08Z

/lgtm

mhenriks · 2024-02-02T21:20:25Z

+		},
+	}
+
+	if c == nil {


This should never be the case in this context

Signed-off-by: Arnon Gilboa <[email protected]>

mhenriks · 2024-02-05T21:03:44Z

+	err = args.Client.Get(context.TODO(), key, whc)
+	if err == nil {
+		if enabled {
+			return nil


Can't exit here. May have to update the webhook configuration when:

user changes any "critical" value

but perhaps most importantly, whenever the cacert gets rotated the cabundle will change

Signed-off-by: Arnon Gilboa <[email protected]>

mhenriks · 2024-02-08T23:05:40Z

+		return err
+	}
+	if !reflect.DeepEqual(whc, whcCopy) {
+		return args.Client.Update(context.TODO(), whc)


I was curious how frequently update would get called so I pulled your branch added a log here and ran the code. Turns out it was being called every reconcile even if nothing changed. Turned out it was because you are setting:

whc.APIVersion = "admissionregistration.k8s.io/v1" whc.Kind = "MutatingWebhookConfiguration"

Controller runtime strips that out. So I'd suggest leaving it out here too.

FYI: it is added in operator/resources because the objects may be serialized to yaml

Great catch & great lesson learned. Fixed.

Signed-off-by: Arnon Gilboa <[email protected]>

mhenriks · 2024-02-09T19:08:41Z

/approve

kubevirt-bot · 2024-02-09T19:09:03Z

[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

Details

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 · 2024-02-12T14:25:55Z

/lgtm

kubevirt-bot requested review from alromeros and awels July 19, 2023 10:16

arnongilboa force-pushed the pvc_mutate_experiment branch from a78691e to 9453e09 Compare July 19, 2023 10:23

arnongilboa changed the title ~~[wip] Add PVC Spec mutating webhook rendering based on the available StorageProfiles~~ [wip] Add PVC spec mutating webhook rendering based on the available StorageProfiles Jul 19, 2023

arnongilboa changed the title ~~[wip] Add PVC spec mutating webhook rendering based on the available StorageProfiles~~ [wip] Add PVC spec mutating webhook rendering based on StorageProfiles Jul 19, 2023

arnongilboa force-pushed the pvc_mutate_experiment branch from 9453e09 to b9d81c5 Compare July 20, 2023 15:42

arnongilboa force-pushed the pvc_mutate_experiment branch from b9d81c5 to 071f54b Compare July 20, 2023 16:09

arnongilboa force-pushed the pvc_mutate_experiment branch from 071f54b to e8a3988 Compare July 30, 2023 08:24

arnongilboa force-pushed the pvc_mutate_experiment branch 5 times, most recently from e2e64d2 to 7ae2147 Compare August 30, 2023 15:44

akalenyu reviewed Aug 30, 2023

View reviewed changes

arnongilboa force-pushed the pvc_mutate_experiment branch 3 times, most recently from ebdd48f to e475ecf Compare August 31, 2023 07:42

arnongilboa changed the title ~~[wip] Add PVC spec mutating webhook rendering based on StorageProfiles~~ [WIP] Add PVC spec mutating webhook rendering based on StorageProfiles Aug 31, 2023

arnongilboa force-pushed the pvc_mutate_experiment branch 2 times, most recently from 98d7c18 to ebdd48f Compare August 31, 2023 11:11

kubevirt-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 23, 2024

arnongilboa force-pushed the pvc_mutate_experiment branch from b04b51f to 10f670a Compare January 25, 2024 13:24

kubevirt-bot added size/XXL and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XL labels Jan 25, 2024

arnongilboa force-pushed the pvc_mutate_experiment branch 3 times, most recently from 7147ef1 to 08c9e4a Compare January 25, 2024 14:36

mhenriks reviewed Jan 29, 2024

View reviewed changes

mhenriks reviewed Jan 30, 2024

View reviewed changes

mhenriks reviewed Jan 31, 2024

View reviewed changes

awels reviewed Jan 31, 2024

View reviewed changes

Move webhook create/delete to callback

a3ada3e

plus some CR fixes and cleanups Signed-off-by: Arnon Gilboa <[email protected]>

mhenriks reviewed Feb 1, 2024

View reviewed changes

mhenriks reviewed Feb 2, 2024

View reviewed changes

Move webhook CR creation to sit with callbacks

028c7c6

Signed-off-by: Arnon Gilboa <[email protected]>

mhenriks reviewed Feb 5, 2024

View reviewed changes

Update existing webhook if modified

32f26f1

Signed-off-by: Arnon Gilboa <[email protected]>

mhenriks reviewed Feb 8, 2024

View reviewed changes

Eliminate unnecessary CR update

dfeeec8

Signed-off-by: Arnon Gilboa <[email protected]>

coderabbitai Bot mentioned this pull request Aug 20, 2026

fix(cdi): update CDI to v1.66.1 cozystack/cozystack#3920

Merged

1 of 11 tasks


		PVC Mutating Webhook Rendering is an optional CDI feature, allowing users to get CDI PVC rendering functionality without using a `DataVolume`. Traditionally, when the CDI DV controller creates a PVC, it renders the PVC spec (`volumeMode`, `accessMode`, `storage`) according to the DV storage spec (or default) `storageClass`, CDI `StorageProfiles`, `CDIConfig` `filesystemOverhead` etc.

		The PVC mutating webhook eliminats the need for DVs for `StorageProfile` based rendering, providing auto-completion of PVC missing spec fields based on optimal values per `StorageClass`. The webhook intercepts only explicitly CDI-labeled PVCs, so it won't affect cluster stability if the CDI api server is down. For labeled PVCs, `objectSelectors` decide when to call out over HTTP to the webhook, so if the CDI api server is down the request and PVC creation will fail. Unlabeled PVC will not be affected at all.

Conversation

arnongilboa commented Jul 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

awels commented Jul 19, 2023

Uh oh!

arnongilboa commented Jul 19, 2023

Uh oh!

awels commented Jul 19, 2023

Uh oh!

arnongilboa commented Jul 19, 2023

Uh oh!

akalenyu commented Jul 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

arnongilboa commented Jul 20, 2023

Uh oh!

akalenyu Aug 30, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

arnongilboa Aug 30, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

arnongilboa commented Jan 18, 2024

Uh oh!

arnongilboa commented Jan 19, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

arnongilboa Jan 31, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

arnongilboa Jan 31, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

awels left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

arnongilboa commented Feb 1, 2024

Uh oh!

awels commented Feb 1, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

arnongilboa commented Jul 19, 2023 •

edited

Loading

akalenyu commented Jul 20, 2023 •

edited

Loading

akalenyu Aug 30, 2023 •

edited

Loading

arnongilboa Aug 30, 2023 •

edited

Loading

arnongilboa Jan 31, 2024 •

edited

Loading

arnongilboa Jan 31, 2024 •

edited

Loading