STOR 437- Create API for vsphere CSI topology configuration by gnufied · Pull Request #1271 · openshift/api

gnufied · 2022-08-26T19:43:21Z

xref https://issues.redhat.com/browse/STOR-437

openshift-ci · 2022-08-26T19:43:29Z

Hello @gnufied! Some important instructions when contributing to openshift/api:
API design plays an important part in the user experience of OpenShift and as such API PRs are subject to a high level of scrutiny to ensure they follow our best practices. If you haven't already done so, please review the OpenShift API Conventions and ensure that your proposed changes are compliant. Following these conventions will help expedite the api review process for your PR.

For merging purposes, this repository follows the no-Feature-Freeze process which means that in addition to the standard lgtm and approved labels this repository requires either:

bugzilla/valid-bug - applied if your PR references a valid bugzilla bug

OR

qe-approved, docs-approved, and px-approved - these labels can be applied by anyone in the openshift org via the /label <labelname> command.

Who should apply these qe/docs/px labels?

For a no-Feature-Freeze team who is merging a feature before code freeze, they need to get those labels applied to their api repo PR by the appropriate teams (i.e. qe, docs, px)
For a Feature Freeze (traditional) team who is merging a feature before FF, they can self-apply the labels (via /label commands), they are basically irrelevant for those teams
For a Feature Freeze team who is merging a feature after FF, the PR should be rejected barring an exception

gnufied · 2022-08-29T13:03:51Z

/retest

bertinatto · 2022-08-29T18:47:35Z

+	// +kubebuilder:validation:Required
+	// +unionDiscriminator
+	// +required
+	DriverName CSIDriverName `json:"driverName"`


CR.Name must be the CSI driver name, so field will duplicate that information?

Also, CR.Spec.DriverConfig.DriverName seems a bit verbose/redundant.

^ this is a good point IMO, I don't really understand the purpose of the new DriverName field.

yeah I am going to rename it. But we do need a field here as a union discriminator which will basically make it obvious that if driverName: vsphere.vmware.com then - only vsphere will be set.

https://github.com/openshift/enhancements/blob/master/dev-guide/api-conventions.md#discriminated-unions

I am torn about renaming driverName to Name. Elsewhere in OCP docs, union discriminators are called Type - if we do that, then we probably have to define a new enum.

Ah, I don't have a strong preference on renaming the field, I think DriverName is fine. I just didn't understand the purpose, but that doc on discriminated unions explains it.

gnufied · 2022-08-31T15:29:32Z

/assign @JoelSpeed @deads2k

JoelSpeed · 2022-08-31T15:55:46Z

+	// DriverConfig field can be used to optionally specify
+	// driver specific configuration.
+	// If this field is nil, platform chosen default configuration
+	// will be applied.
+	// +kubebuilder:validation:Optional
+	// +optional
+	DriverConfig *CSIDriverConfigSpec `json:"driverConfig,omitempty"`


Few nits on this:

The first word should match the json tag

We should use the standard wording that other APIs use for when this is optional

You only need one optional tag, we prefer +optional

You shouldn't use a pointer here unless you have a good reason to do so, do you need a pointer?

Suggested change

// DriverConfig field can be used to optionally specify

// driver specific configuration.

// If this field is nil, platform chosen default configuration

// will be applied.

// +kubebuilder:validation:Optional

// +optional

DriverConfig *CSIDriverConfigSpec `json:"driverConfig,omitempty"`

// driverConfig can be used to specify platform specific driver configuration.

// When omitted, this means no opinion and the platform is left to choose reasonable

// defaults. These defaults are subject to change over time.

// +optional

DriverConfig CSIDriverConfigSpec `json:"driverConfig,omitempty"`

I was reading about API conventions and although I have accepted this suggestion, one of the things which I find sometimes problematic is - CSIDriverConfigSpec says DriverName is a required field and hence by that definition - an empty CSIDriverConfigSpec isn't a valid object because DriverName is not set. I know on the wire, if field is empty - it is entirely omitted but I think that after deserializing we basically have a GO object that is essentially invalid and does not conform to schema it defines itself, which is weird.

So I think for union types which have a required union discriminator I think that a nil pointer is only sensible default if users don't want to specify one.

It's a trade off we have decide to make for the discoverability benefits. But yes, you need to teach your controller to handle the empty DriverConfig. It shouldn't be too difficult and isn't dissimilar to the nil check you'd have to otherwise do

emptyConfig := operatorv1.CSIDriverConfigSpec{} if spec.DriverConfig == emptyConfig { // No driver config was specified return nil }

JoelSpeed · 2022-08-31T16:00:10Z

+	// TopologyCategories indicates categories with which
+	// vcenter resources as hostcluster or datacenter were tagged with.


Can you clarify what this means? It doesn't make sense to me, do you mean resources such as?

Added more details. PTAL

JoelSpeed · 2022-08-31T16:00:26Z

+	// If unspecified CSI driver configuration will default to configuration
+	// specified in cluster configuration.


Where does this configuration live? On the Infrastructure object?

Yes - basically what @jcpowermac is proposing in his enhancement.

Ok, I'd expect the comment here to mention that it's on the Infrastructure object, gives user a hint as to where they can find the defaults

JoelSpeed · 2022-09-02T14:16:15Z

+	// driverName indicates which CSI driver should be installed.
+	// +kubebuilder:validation:Required
+	// +unionDiscriminator
+	DriverName CSIDriverName `json:"driverName"`


This should be an enum, do we have a list of valid values for this?

I have renamed this to CSIDriverType and defined a new enum. @bertinatto raised a concern that we were overloading CSIDriverName type which is used for defining driver name.

JoelSpeed · 2022-09-02T14:16:34Z

+
+	// vsphere is used to configure the vsphere CSI driver.
+	// +optional
+	VSphere *VSphereCSIDriverConfigSpec `json:"vsphere,omitempty"`


What value must DriverName have for this configuration to be observed?

Updated to reflect the values.

dobsonj · 2022-09-06T15:28:46Z

/lgtm

JoelSpeed

I think we are pretty close, a few more comments

JoelSpeed · 2022-09-08T09:46:43Z

+	//
+	// +kubebuilder:validation:Required
+	// +unionDiscriminator
+	DriverType CSIDriverType `json:"driverType"`


As this enum is expected over time, you need to add a developer note to explain what consumers should do when they observe an unknown value, there's typically two options there

Suggested change

DriverType CSIDriverType `json:"driverType"`

// ---

// + Consumers should treat unknown values as a no-op || Consumers should treat unknown values as an error and stop processing.

DriverType CSIDriverType `json:"driverType"`

JoelSpeed · 2022-09-08T09:48:12Z

+	// specified in Infrastructure object. If both Infrastructure object and
+	// ClusterCSIDriver specify topology - then values in Infrastructure object
+	// will take precedence.


Typically the closer object to the action (this resource) would take precedence, it seems odd to say the Infrastructure object will take precedence here, I would expect to reverse that.

Having the closer object allows whoever owns that specific component to override the global configuration should they need to.

Why are you suggesting it to be this way around?

Currently its bit like you would expect ebs provisioner as default in a storageclass deployed on AWS cluster not ceph provisioner. One can deploy ceph on OCP AWS cluster sure but it requires bunch of manual configuration.

Similarly topology from Infrastructure object is guaranteed to be right and does not require any further manual configuration in vcenter, so we want to start with somewhat stricter definition of topologyCategories if Infra object already defines one.

Currently we are thinking - once Infra changes are merged, we will not allow setting these values at all by end users and any changes to ClusterCSIDriver topologies will be rejected by webhook, if Infra object already defines a topology(via failure-domains). I will update the wording here to be more clear on this.

But we also realize that, compute and storage topologies can have two different values - so we don't want to close the door completely on that option, so we may choose to relax this in a future version of OCP.

jsafrane · 2022-09-08T11:53:56Z

+
+	// vsphere is used to configure the vsphere CSI driver.
+	// +optional
+	VSphere *VSphereCSIDriverConfigSpec `json:"vsphere,omitempty"`


If the right CSIDriverType is "VSphere", then this must be json:"vSphere

Check the discriminated unions in the config v1 types and follow their patterns

gnufied · 2022-09-09T14:52:46Z

@JoelSpeed PTAL. Dropped omitempty from optional but non-pointer fields and addressed other review comments.

JoelSpeed

API looks good, have you tested it against a real cluster to make sure it behaves how you expect it should?

gnufied · 2022-09-09T19:13:47Z

@JoelSpeed yep. The entire PR has been verified using linked changes into vmware-vpshere-csi-driver openshift/vmware-vsphere-csi-driver-operator#107 and it works as expected.

JoelSpeed · 2022-09-12T09:54:21Z

/lgtm

Update API to use latest guidelines, drop omitempty

openshift-ci · 2022-09-12T14:37:56Z

@gnufied: all tests passed!

Full PR test history. Your PR dashboard.

Details

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.

jsafrane · 2022-09-12T14:52:54Z

/lgtm

deads2k · 2022-09-12T20:08:02Z

/approve

openshift-ci · 2022-09-12T20:10:04Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: deads2k, dobsonj, gnufied, JoelSpeed, jsafrane

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

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

gnufied · 2022-09-12T20:34:47Z

/docs approved
/px approved

gnufied · 2022-09-13T11:23:36Z

Since this is just an API change.

/label docs-approved
/label px-approved

duanwei33 · 2022-09-13T13:24:39Z

/label qe-approved

openshift-ci Bot requested review from adambkaplan and sjenning August 26, 2022 19:44

gnufied force-pushed the api-for-vsphere-topology-config branch 4 times, most recently from 6ee97f8 to c275e8f Compare August 29, 2022 19:02

bertinatto reviewed Aug 29, 2022

View reviewed changes

openshift-ci Bot assigned deads2k and JoelSpeed Aug 31, 2022

JoelSpeed reviewed Aug 31, 2022

View reviewed changes

JoelSpeed reviewed Sep 2, 2022

View reviewed changes

openshift-ci Bot assigned dobsonj Sep 6, 2022

openshift-ci Bot added the lgtm Indicates that a PR is ready to be merged. label Sep 6, 2022

gnufied force-pushed the api-for-vsphere-topology-config branch from 1d1f84c to 5a99faf Compare September 7, 2022 15:39

openshift-ci Bot removed the lgtm Indicates that a PR is ready to be merged. label Sep 7, 2022

gnufied mentioned this pull request Sep 7, 2022

STOR-1050: Enable topology aware provisioning openshift/vmware-vsphere-csi-driver-operator#107

Merged

JoelSpeed reviewed Sep 8, 2022

View reviewed changes

jsafrane reviewed Sep 8, 2022

View reviewed changes

JoelSpeed reviewed Sep 9, 2022

View reviewed changes

openshift-ci Bot added the lgtm Indicates that a PR is ready to be merged. label Sep 12, 2022

Create API for vsphere CSI topology configuration

269199d

Update API to use latest guidelines, drop omitempty

gnufied force-pushed the api-for-vsphere-topology-config branch from 2d7e29c to 269199d Compare September 12, 2022 14:19

openshift-ci Bot removed the lgtm Indicates that a PR is ready to be merged. label Sep 12, 2022

openshift-ci Bot assigned jsafrane Sep 12, 2022

openshift-ci Bot added the lgtm Indicates that a PR is ready to be merged. label Sep 12, 2022

openshift-ci Bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 12, 2022

openshift-ci Bot added docs-approved Signifies that Docs has signed off on this PR px-approved Signifies that Product Support has signed off on this PR labels Sep 13, 2022

openshift-ci Bot added the qe-approved Signifies that QE has signed off on this PR label Sep 13, 2022

openshift-merge-robot merged commit 67162ff into openshift:master Sep 13, 2022

gnufied mentioned this pull request Sep 20, 2022

[CNF-5646] bump api version to add new capability openshift/cluster-version-operator#835

Merged

		// TopologyCategories indicates categories with which
		// vcenter resources as hostcluster or datacenter were tagged with.

		// If unspecified CSI driver configuration will default to configuration
		// specified in cluster configuration.

Conversation

gnufied commented Aug 26, 2022

Uh oh!

openshift-ci Bot commented Aug 26, 2022

Uh oh!

gnufied commented Aug 29, 2022

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!

Uh oh!

gnufied commented Aug 31, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gnufied Aug 31, 2022 • 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!

Uh oh!

Uh oh!

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!

gnufied Aug 31, 2022 • 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!

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!

dobsonj commented Sep 6, 2022

Uh oh!

JoelSpeed left a comment

Choose a reason for hiding this comment

Uh oh!

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!

gnufied commented Sep 9, 2022

Uh oh!

JoelSpeed left a comment

Choose a reason for hiding this comment

Uh oh!

gnufied commented Sep 9, 2022

Uh oh!

JoelSpeed commented Sep 12, 2022

gnufied Aug 31, 2022 •

edited

Loading

gnufied Aug 31, 2022 •

edited

Loading