Kubevirt CSI StorageClass mapping API #2528

davidvossel · 2023-05-05T20:02:34Z

The KubeVirt CSI driver allows passthrough of storageclasses within the mgmt/infra cluster (the cluster hosting the VMs) into the guest cluster.

Today, by default people automatically get a kubevirt csi driver storage class in their guest cluster that maps to the default storageclass of the infra cluster.

This PR aims to give users more flexibility over this functionality by allowing users to passthrough multiple infra storageclasses into their guest cluster as well as disabling kubevirt csi entirely.

Examples

Default behavior of passing through default infra storageclass into guest cluster. This works today and will continue to work after this new API is introduced.

apiVersion: hypershift.openshift.io/v1beta1
kind: HostedCluster
.
.
.
  platform:
    type: KubeVirt

Disable kubevirt csi entirely

apiVersion: hypershift.openshift.io/v1beta1
kind: HostedCluster
.
.
.
  platform:
    type: KubeVirt
    kubevirt:
        storageDriver:
            type: None

Mapping infra storageclasses into guest

apiVersion: hypershift.openshift.io/v1beta1
kind: HostedCluster
.
.
.
  platform:
    type: KubeVirt
    kubevirt:
        storageDriver:
            type: Manual
            manual:
                storageClassMapping:
                    - infraStorageClassName: ceph-rdb
                      guestStorageClassName: kubevirt-rdb
                    - infraStorageClassName: ceph-fs
                      guestStorageClassName: kubevirt-fs

openshift-ci · 2023-05-05T20:03:19Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: davidvossel

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

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

netlify · 2023-05-05T20:03:27Z

✅ Deploy Preview for hypershift-docs ready!

Name	Link
🔨 Latest commit	`2e46670`
🔍 Latest deploy log	https://app.netlify.com/sites/hypershift-docs/deploys/646bb9848a0ed60008981cea
😎 Deploy Preview	https://deploy-preview-2528--hypershift-docs.netlify.app/reference/api
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

mhenriks

Other than the json naming I think it's probably fine as is but some minor suggestions mostly about naming.

mhenriks · 2023-05-08T12:53:13Z

api/v1alpha1/hostedcluster_types.go

+	// Defaults to false
+	// +optional
+	// +kubebuilder:validation:Optional
+	Disable bool `json:"Disable,omitempty"`


json disable not Disable

Why is this needed? Why not just delete all mappings?

I was hoping we could get away with some default behavior if no mappings are set. So, a zero length mapping slice would result in a default storageclass in the guest that maps to the default infra storageclass.

The disable bool would opt out of all kubevirt csi behavior.

I think it would be best for this map (or something in status) to mirror what mappings actually exist. Also, instead of just a bool maybe an enum like None Default Explicit. Default could be Default and it would map the default storage class automatically. Explicit would be just what's in the map

Related question when mapping over the default storage class, what will it be named?

Related question when mapping over the default storage class, what will it be named?

right now, the guest magically gets a storageclass called kubevirt-csi-infra-default by default

think it would be best for this map (or something in status) to mirror what mappings actually exist. Also, instead of just a bool maybe an enum like None Default Explicit. Default could be Default and it would map the default storage class automatically. Explicit would be just what's in the map

I think an enum is a good idea here and makes this less confusing. I'll work with that a bit

mhenriks · 2023-05-08T12:53:52Z

api/v1alpha1/hostedcluster_types.go

+	//
+	// +optional
+	// +kubebuilder:validation:Optional
+	StorageClassMapping []KubevirtCSIMapping `json:"StorageClassMapping,omitempty"`


json storageClassMapping

mhenriks · 2023-05-08T13:03:03Z

api/v1alpha1/hostedcluster_types.go

+	// +kubebuilder:validation:Optional
+	// +optional
+	// +immutable
+	CSIDriver *KubevirtCSIDriverSpec `json:"csiDriver,omitempty"`


Finally, StorageDriver instead of CSIDriver

mhenriks · 2023-05-08T13:05:41Z

api/v1alpha1/hostedcluster_types.go

+	StorageClassMapping []KubevirtCSIMapping `json:"StorageClassMapping,omitempty"`
+}
+
+type KubevirtCSIMapping struct {


I like KubevirtStorageClassMapping or StorageClassMapping better but again whatever

mhenriks · 2023-05-08T13:06:08Z

api/v1alpha1/hostedcluster_types.go

@@ -697,6 +697,39 @@ type KubevirtPlatformCredentials struct {
 	InfraNamespace string `json:"infraNamespace"`
 }

+type KubevirtCSIDriverSpec struct {


I like KubevirtStorageDriverSpec better but whatever.

awels · 2023-05-08T13:20:09Z

Agreed with Michael, and any reason we are also putting this in the alpha CR? if you just put it in the beta, it gives people motivation to use the beta version.

davidvossel · 2023-05-08T14:21:02Z

Agreed with Michael, and any reason we are also putting this in the alpha CR? if you just put it in the beta, it gives people motivation to use the beta version.

alpha and beta are mirrors of one another right now.

awels · 2023-05-08T14:26:10Z

Agreed with Michael, and any reason we are also putting this in the alpha CR? if you just put it in the beta, it gives people motivation to use the beta version.

alpha and beta are mirrors of one another right now.

Okay, so what is the point of having a beta if it is the same as alpha. I understand what is happening, we did the same thing for the longest time in CDI as well. I am just suggesting that is okay for beta to diverge from alpha, and it gives people motivation to use the beta version.

davidvossel · 2023-05-08T17:25:26Z

@mhenriks @awels updated.

mhenriks · 2023-05-08T22:54:02Z

api/v1alpha1/hostedcluster_types.go

+	Manual *KubevirtManualStorageClassMapping `json:"manual,omitempty"`
+}
+
+type KubevirtManualStorageClassMapping struct {


Maybe KubevirtStorageDriverConfig? Especially if there may eventually be other stuff in here.

yep, fixed.

mhenriks · 2023-05-19T17:50:01Z

control-plane-operator/controllers/hostedcontrolplane/csi/kubevirt/kubevirt.go


 	cm.Data = map[string]string{
 		"infraClusterNamespace":        cm.Namespace,
-		"infraClusterLabels":           fmt.Sprintf("%s=%s", hyperv1.InfraIDLabel, infraID),
+		"infraClusterLabels":           fmt.Sprintf("%s=%s", hyperv1.InfraIDLabel, hcp.Spec.InfraID),
 		"infraStorageClassEnforcement": storageClassEnforcement,


Correct me if I'm wrong, but it appears that the value of storageClassEnforcement becomes the value of the INFRA_STORAGE_CLASS_ENFORCEMENT environment variable for the CSI driver via the driver-config config map?

So changing the type/mapping will update the configmap. But that will have no effect on the driver once it is running. Is that "good enough?" Should the driver be changed read this configmap key as a file instead?

Yep, you understand it correctly.

Right now we're locked in to this enforcement config once the driver starts, and we can't change it. We weren't really thinking about how these values could potentially mutate when the kubevirt-csi functionality was added. It's likely I was under the impression it wouldn't be practical to change the StorageClass mappings after the driver starts, so I didn't even consider mutability then.

mhenriks · 2023-05-22T14:33:43Z

api/v1beta1/hostedcluster_types.go

+	// storageclass will be present for the corresponding guest clusters storageclass.
+	//
+	// +optional
+	// +immutable


For my info, do you mind sharing how immutability is actually enforced here? I don't anything at the code/schema level in this pr that makes it possible. But I'm sure I'm missing something

fixed... Yeah i think something changed in the way the crd schema is built recently. I added a CEL validation to enforce all this immutability stuff, and it seems to work.

Signed-off-by: David Vossel <davidvossel@gmail.com>

mhenriks

/lgtm

mhenriks · 2023-05-23T19:33:59Z

api/v1beta1/hostedcluster_types.go

+	// +kubebuilder:validation:Optional
+	// +optional
+	// +immutable
+	// +kubebuilder:validation:XValidation:rule="self == oldSelf", message="storageDriver is immutable"


I wonder if only this validation is necessary? But I guess the others probably don't hurt

openshift-ci-robot · 2023-05-24T00:21:25Z

/retest-required

Remaining retests: 0 against base HEAD a3ebcaf and 2 for PR HEAD 2e46670 in total

openshift-ci-robot · 2023-05-24T02:08:03Z

/retest-required

Remaining retests: 0 against base HEAD f03113a and 1 for PR HEAD 2e46670 in total

openshift-ci-robot · 2023-05-24T14:33:56Z

/retest-required

Remaining retests: 0 against base HEAD 8861506 and 0 for PR HEAD 2e46670 in total

openshift-ci-robot · 2023-05-24T16:13:01Z

/hold

Revision 2e46670 was retested 3 times: holding

davidvossel · 2023-05-24T21:02:22Z

/test e2e-aws

openshift-ci · 2023-05-24T21:14:38Z

@davidvossel: 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
ci/prow/e2e-ibmcloud-iks	`2e46670`	link	false	`/test e2e-ibmcloud-iks`
ci/prow/e2e-ibmcloud-roks	`2e46670`	link	false	`/test e2e-ibmcloud-roks`

Full PR test history. Your PR dashboard.

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.

davidvossel · 2023-05-25T14:20:29Z

/test e2e-aws
/hold cancel

davidvossel marked this pull request as draft May 5, 2023 20:02

openshift-ci bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. do-not-merge/needs-area labels May 5, 2023

openshift-ci bot requested review from csrwng and enxebre May 5, 2023 20:03

openshift-ci bot added area/cli Indicates the PR includes changes for CLI area/documentation Indicates the PR includes changes for documentation labels May 5, 2023

openshift-ci bot added area/hypershift-operator Indicates the PR includes changes for the hypershift operator and API - outside an OCP release approved Indicates a PR has been approved by an approver from all required OWNERS files. and removed do-not-merge/needs-area labels May 5, 2023

mhenriks reviewed May 8, 2023

View reviewed changes

davidvossel force-pushed the csi-mappings-v1 branch 4 times, most recently from 1392c1a to 43eda0b Compare May 8, 2023 17:24

davidvossel mentioned this pull request May 8, 2023

Lock down kubevirt csi storageclass mappings #2534

Merged

mhenriks reviewed May 8, 2023

View reviewed changes

davidvossel force-pushed the csi-mappings-v1 branch from 43eda0b to c65af78 Compare May 9, 2023 17:12

davidvossel force-pushed the csi-mappings-v1 branch from c65af78 to 6f5ef9b Compare May 16, 2023 20:49

openshift-ci bot added the area/control-plane-operator Indicates the PR includes changes for the control plane operator - in an OCP release label May 16, 2023

davidvossel force-pushed the csi-mappings-v1 branch 3 times, most recently from bc79b53 to 842c536 Compare May 18, 2023 19:35

davidvossel marked this pull request as ready for review May 18, 2023 19:38

openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 18, 2023

openshift-ci bot requested review from isco-rodriguez and nirarg May 18, 2023 19:39

davidvossel changed the title ~~[Draft] Kubevirt CSI StorageClass mapping API~~ Kubevirt CSI StorageClass mapping API May 18, 2023

davidvossel force-pushed the csi-mappings-v1 branch from 842c536 to d5928ec Compare May 19, 2023 14:24

mhenriks reviewed May 19, 2023

View reviewed changes

mhenriks reviewed May 22, 2023

View reviewed changes

Kubevirt CSI StorageClass mapping API

2e46670

Signed-off-by: David Vossel <davidvossel@gmail.com>

davidvossel force-pushed the csi-mappings-v1 branch from d5928ec to 2e46670 Compare May 22, 2023 18:50

mhenriks reviewed May 23, 2023

View reviewed changes

openshift-ci bot assigned mhenriks May 23, 2023

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label May 23, 2023

openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 24, 2023

openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 25, 2023

openshift-merge-robot merged commit b5323e4 into openshift:main May 25, 2023
12 of 14 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Kubevirt CSI StorageClass mapping API #2528

Kubevirt CSI StorageClass mapping API #2528

davidvossel commented May 5, 2023 •

edited

openshift-ci bot commented May 5, 2023

netlify bot commented May 5, 2023 •

edited

mhenriks left a comment

mhenriks May 8, 2023

mhenriks May 8, 2023

davidvossel May 8, 2023

mhenriks May 8, 2023

davidvossel May 8, 2023

davidvossel May 8, 2023

mhenriks May 8, 2023

mhenriks May 8, 2023

mhenriks May 8, 2023

mhenriks May 8, 2023

davidvossel May 8, 2023

awels commented May 8, 2023

davidvossel commented May 8, 2023

awels commented May 8, 2023

davidvossel commented May 8, 2023

mhenriks May 8, 2023

davidvossel May 9, 2023

mhenriks May 19, 2023

davidvossel May 22, 2023

mhenriks May 22, 2023

davidvossel May 22, 2023

mhenriks left a comment

mhenriks May 23, 2023

openshift-ci-robot commented May 24, 2023

openshift-ci-robot commented May 24, 2023

openshift-ci-robot commented May 24, 2023

openshift-ci-robot commented May 24, 2023

davidvossel commented May 24, 2023

openshift-ci bot commented May 24, 2023 •

edited

davidvossel commented May 25, 2023

Kubevirt CSI StorageClass mapping API #2528

Kubevirt CSI StorageClass mapping API #2528

Conversation

davidvossel commented May 5, 2023 • edited

Examples

Default behavior of passing through default infra storageclass into guest cluster. This works today and will continue to work after this new API is introduced.

Disable kubevirt csi entirely

Mapping infra storageclasses into guest

openshift-ci bot commented May 5, 2023

netlify bot commented May 5, 2023 • edited

✅ Deploy Preview for hypershift-docs ready!

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

awels commented May 8, 2023

davidvossel commented May 8, 2023

awels commented May 8, 2023

davidvossel commented May 8, 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

mhenriks left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

openshift-ci-robot commented May 24, 2023

openshift-ci-robot commented May 24, 2023

openshift-ci-robot commented May 24, 2023

openshift-ci-robot commented May 24, 2023

davidvossel commented May 24, 2023

openshift-ci bot commented May 24, 2023 • edited

davidvossel commented May 25, 2023

davidvossel commented May 5, 2023 •

edited

netlify bot commented May 5, 2023 •

edited

openshift-ci bot commented May 24, 2023 •

edited