BUILD-159: add projected share resource csi driver operator to the list managed by the CSO #952

gabemontero · 2021-06-23T14:23:44Z

make update etc. is behaving weird for me today ... I think my upgrading controller-gen for v0.5 for tekton has introduced formatting differences.

initially submitting with WIP and with any make update changes to see exactly how the verify job fails, and then see if I can split the difference

or I may have to maintain 2 versions of controller-gen

gabemontero · 2021-06-23T14:39:45Z

OK everything is green ... interesting. Moving forward.

/assign @jsafrane
/assign @adambkaplan

PTAL ... my early testing in trying to construct a CSO managed operator for the projected shared resource CSI driver is that I need these updates vendored into openshift/apiserver in order to mirror the build and run locally instructions articulated at https://github.com/openshift/gcp-pd-csi-driver-operator#quick-start

without this, even with CVO and CSO disabled, I get

The ClusterCSIDriver "csi-driver-projected-resource.openshift.io" is invalid: metadata.name: Unsupported valucsi.projected.shared.resources.openshift.ioe: "csi.projected.shared.resources.openshift.io": supported values: "ebs.csi.aws.com", "disk.csi.azure.com", "pd.csi.storage.gke.io", "cinder.csi.openstack.org", "csi.vsphere.vmware.com", "manila.csi.openstack.org", "csi.ovirt.org", "csi.kubevirt.io"

gabemontero · 2021-06-24T17:14:09Z

operator/v1/types_csi_cluster_driver.go

-	ManilaCSIDriver    CSIDriverName = "manila.csi.openstack.org"
-	OvirtCSIDriver     CSIDriverName = "csi.ovirt.org"
-	KubevirtCSIDriver  CSIDriverName = "csi.kubevirt.io"
+	AWSEBSCSIDriver                  CSIDriverName = "ebs.csi.aws.com"


@jsafrane - per the comment on line 41, I see you removed use of the kubebuilder:validation:Enum directive for the CSIDriverName type with #718

Hence, the manual update of both 000_90... yaml files

Now, I did try in my local copy of this branch adding that directive back, and after undoing the change to the crd yam (but not the patch yaml) a make update did add my new driver to the crd yaml

I take your #718 (comment) as meaning the removal of the kubebuilder comment for CSIDriverName was intentional.

Anyway, it seems to me if we leave things as is, the comment saying update kubebuilder:validation:Enum should be changed, since that directive is no longer specified.

Or would you like me to add it back?

Yes, the comment is wrong, there is no kubebuilder:validation:Enum now. Can you please remove the comment?

If I remember correctly, a patch was the only way to add validation of metadata.name.

yep I'll update the comment ... fighting a few fires this AM, but hopefully will still circle back to this today

mention of kubebuilder Enum in the comment removed @jsafrane

gabemontero · 2021-06-28T12:32:39Z

/assign @bparees

need an OWNERS approve to sign off on this next step in installing via the cluster storage operator the projected resource csi driver we are going to use to mount entitlement secrets in build without having to copy the secret to the same namespace as the build.

bparees · 2021-06-28T14:04:32Z

/approve

adambkaplan

Some necessary bike-shedding on the name

adambkaplan · 2021-06-28T19:06:58Z

operator/v1/0000_90_cluster_csi_driver_01_config.crd.yaml-patch

@@ -13,3 +13,4 @@
      - manila.csi.openstack.org
      - csi.ovirt.org
      - csi.kubevirt.io
+      - csi.projected.shared.resources.openshift.io


Is this the name we want? I'd think one of the following would be better:

csi.projected-resources.openshift.io

csi.shared-resources.openshift.io

So none of the other names I hyphens, so I was not comfortable introducing that pattern, though I thought of it. So unless @jsafrane says it is OK, I disagree there.

Of the two reduced versions you have, and losing hyphens, I like csi.shared.resources.openshift.io better.

You good with that @adambkaplan or are do you want to hold out for the hyphens being blessed?

These are ultimately used as the name for a k8s CSIDriver object, which at worst requires us to meet the DNS Label requirement [1]. Hyphens in the middle are OK.

Let's go with csi.shared-resources.openshift.io.

https://kubernetes.io/docs/concepts/overview/working-with-objects/names/#dns-label-names

since this is a product-facing name, does @smarterclayton need to sign off?

updated @adambkaplan

adambkaplan · 2021-06-28T19:07:22Z

operator/v1/0000_90_cluster_csi_driver_01_config.crd.yaml

@@ -46,6 +46,7 @@ spec:
                - manila.csi.openstack.org
                - csi.ovirt.org
                - csi.kubevirt.io
+                - csi.projected.shared.resources.openshift.io


adambkaplan · 2021-06-28T19:07:34Z

operator/v1/types_csi_cluster_driver.go

+	ManilaCSIDriver                  CSIDriverName = "manila.csi.openstack.org"
+	OvirtCSIDriver                   CSIDriverName = "csi.ovirt.org"
+	KubevirtCSIDriver                CSIDriverName = "csi.kubevirt.io"
+	ProjectedSharedResourceCSIDriver CSIDriverName = "csi.projected.shared.resources.openshift.io"


operator/v1/types_csi_cluster_driver.go

gabemontero · 2021-06-29T12:35:03Z

your suggested commit has been pulled in @adambkaplan and commits have been squashed

jsafrane · 2021-06-29T13:08:39Z

Just to be sure, is csi.shared-resources.openshift.io really name of the driver? The name must match CSIDriver instance name and also driver name in StorageClasses and PVs and it's basically impossible to change it later.

gabemontero · 2021-06-29T13:12:29Z

Just to be sure, is csi.shared-resources.openshift.io really name of the driver? The name must match CSIDriver instance name and also driver name in StorageClasses and PVs and it's basically impossible to change it later.

Thanks for the pointer @jsafrane

Yep I'll need to update our new shared resources CSO operator repo and the existing csi driver repo to make sure things line up with the name @adambkaplan has directed me to before we connect the dots and install via the CSO.

jsafrane · 2021-07-02T09:31:38Z

/lgtm
From storage perspective.

openshift-ci · 2021-07-02T09:31:51Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bparees, gabemontero, jsafrane

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

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

openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 23, 2021

openshift-ci bot requested review from abhinavdahiya and deads2k June 23, 2021 14:23

gabemontero changed the title ~~WIP: BUILD-159: add projected share resource csi driver operator to the list managed by the CSO~~ BUILD-159: add projected share resource csi driver operator to the list managed by the CSO Jun 23, 2021

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 Jun 23, 2021

openshift-ci bot assigned adambkaplan and jsafrane Jun 23, 2021

gabemontero mentioned this pull request Jun 24, 2021

WIP: BUILD-159:fake openshift/api bump openshift/openshift-apiserver#223

Closed

gabemontero commented Jun 24, 2021

View reviewed changes

openshift-ci bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 25, 2021

gabemontero force-pushed the add-proj-share-csi-driver-for-cso branch from 6b0a5c6 to 592fb86 Compare June 25, 2021 23:12

openshift-ci bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 25, 2021

openshift-ci bot assigned bparees Jun 28, 2021

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 28, 2021

adambkaplan reviewed Jun 28, 2021

View reviewed changes

gabemontero force-pushed the add-proj-share-csi-driver-for-cso branch from 592fb86 to 5bf387a Compare June 28, 2021 20:55

adambkaplan reviewed Jun 29, 2021

View reviewed changes

operator/v1/types_csi_cluster_driver.go Outdated Show resolved Hide resolved

gabemontero force-pushed the add-proj-share-csi-driver-for-cso branch from 08107d9 to 32d62d0 Compare June 29, 2021 12:34

add shared resource csi driver operator to the list managed by the CSO

1048cce

gabemontero force-pushed the add-proj-share-csi-driver-for-cso branch from 32d62d0 to 1048cce Compare June 29, 2021 12:50

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jul 2, 2021

openshift-merge-robot merged commit edc45a4 into openshift:master Jul 2, 2021

gabemontero deleted the add-proj-share-csi-driver-for-cso branch July 2, 2021 16:49

gabemontero mentioned this pull request Jul 2, 2021

BUILD-159: openshift/api bump; in repo updates for new CSIDriver name and openshift/api constant name openshift/csi-driver-shared-resource-operator#3

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

BUILD-159: add projected share resource csi driver operator to the list managed by the CSO #952

BUILD-159: add projected share resource csi driver operator to the list managed by the CSO #952

gabemontero commented Jun 23, 2021

gabemontero commented Jun 23, 2021

gabemontero Jun 24, 2021

jsafrane Jun 25, 2021

gabemontero Jun 25, 2021

gabemontero Jun 25, 2021

gabemontero commented Jun 28, 2021

bparees commented Jun 28, 2021

adambkaplan left a comment

adambkaplan Jun 28, 2021

gabemontero Jun 28, 2021

adambkaplan Jun 28, 2021

adambkaplan Jun 28, 2021

gabemontero Jun 28, 2021

adambkaplan Jun 28, 2021

gabemontero Jun 28, 2021

adambkaplan Jun 28, 2021

gabemontero Jun 28, 2021

gabemontero commented Jun 29, 2021

jsafrane commented Jun 29, 2021 •

edited

gabemontero commented Jun 29, 2021

jsafrane commented Jul 2, 2021

openshift-ci bot commented Jul 2, 2021

BUILD-159: add projected share resource csi driver operator to the list managed by the CSO #952

BUILD-159: add projected share resource csi driver operator to the list managed by the CSO #952

Conversation

gabemontero commented Jun 23, 2021

gabemontero commented Jun 23, 2021

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

gabemontero commented Jun 28, 2021

bparees commented Jun 28, 2021

adambkaplan 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

gabemontero commented Jun 29, 2021

jsafrane commented Jun 29, 2021 • edited

gabemontero commented Jun 29, 2021

jsafrane commented Jul 2, 2021

openshift-ci bot commented Jul 2, 2021

jsafrane commented Jun 29, 2021 •

edited