STOR-1759: add enhancement for vSphere driver configuration by RomanBednar · Pull Request #1563 · openshift/enhancements

RomanBednar · 2024-02-09T13:02:56Z

Initial design discussion: https://docs.google.com/document/d/1U1032SG_pTK5FyF8vOB-vGtZGCZ0Mq9FnQdRkd2k998/edit#heading=h.9rnb7c5q1zhy

cc @openshift/storage

openshift-ci-robot · 2024-02-09T13:03:00Z

@RomanBednar: This pull request references STOR-1759 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.16.0" version, but no target version was set.

Details

In response to this:

Initial design discussion: https://docs.google.com/document/d/1U1032SG_pTK5FyF8vOB-vGtZGCZ0Mq9FnQdRkd2k998/edit#heading=h.9rnb7c5q1zhy

cc @openshift/storage

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 openshift-eng/jira-lifecycle-plugin repository.

jsafrane · 2024-02-12T11:39:20Z

enhancements/storage/vsphere-driver-configuration.md

+For this solution we would have a new field in the ClusterCSIDriver CRD where each field would represent a single
+configuration option. This would be similar to Option 1 but would allow us to add more configuration options at once.
+We would basically mirror the configuration options from the driver into the CRD.


Please clearly state that this is option 1, but with all options configurable in CSI drivers proactively exposed into ClusterCSIDriver.

Added a highlighted note.

jsafrane · 2024-02-12T11:46:35Z

enhancements/storage/vsphere-driver-configuration.md

+  - Testing would get too complex
+
+
+### Workflow Description


All these options are the same, except for the first 2-3 steps. IMO it does not add anything new to what was already written above.

Agreed, I'll remove this section since we'll have the examples above in Proposal.

jsafrane · 2024-02-12T11:47:25Z

enhancements/storage/vsphere-driver-configuration.md

+For this solution we would have a new field in the ClusterCSIDriver CRD where each field would represent a single
+configuration option. This would be similar to Option 1 but would allow us to add more configuration options at once.
+We would basically mirror the configuration options from the driver into the CRD.
+


I think it would be useful to add an example with snapshot count.

jsafrane · 2024-02-12T11:47:46Z

enhancements/storage/vsphere-driver-configuration.md

+configured in 3 different ways: `GlobalMaxSnapshotsPerBlockVolume`, `GranularMaxSnapshotsPerBlockVolumeInVSAN`,
+`GranularMaxSnapshotsPerBlockVolumeInVVOL`. In this case we would pick only the first one which is a global setting and
+overrides the other two.
+


I think it would be useful to add an example with snapshot count.

jsafrane · 2024-02-12T11:47:59Z

enhancements/storage/vsphere-driver-configuration.md

+as a map (`map[string]string`), something like clustercsidriver.spec.vSphere.configuration. Then we would merge it into
+any other valid destination for the driver - command line arguments, environment variables, ini file, driver
+feature gate ConfigMap, etc.
+


I think it would be useful to add an example with snapshot count.

jsafrane · 2024-02-12T12:01:08Z

enhancements/storage/vsphere-driver-configuration.md

+On the other hand if we take the safe route and implement each option separately as new fields in the CRD, we're risking
+a scenario where an option or feature could be suddenly dropped upstream, and we can not simply remove fields from release
+to release.


We have this problem in each of the options described above, as it is somewhat orthogonal to how we expose configuration options. Any CSI driver could remove any feature / config option at any time

It would be nice to also mention that CSI drivers have much less rigorous feature development and removal process than k/k.

Good point, I rewrote this section to emphasize this.

gnufied · 2024-02-13T14:42:38Z

enhancements/storage/vsphere-driver-configuration.md

+Example:
+
+1. Cluster administrator creates a ClusterCSIDriver object with the configuration map set to `{"maxSnapshots": "10"}`
+under a generic `map[string]string` field (`clustercsidriver.spec.vSphere.configuration`)


Here is what I am thinking:

We break down the singular configuration field into two:

clusterCSIDriver.Spec.vSphere.configParameters and clusterCSIDriver.Spec.vSphere.featureFlags . These are still a map[string]string - but internally in vSphere operator we keep track of configParameters/featureFlags we support (like a whitelist) and only whitelisted fields can be applied and have an effect. We can ignore unknown config/featureFlags and log a message or something.

This is still very similar to option#1 but only benefit it provides is, say after a rebase we want to support a new config option, then we don't necessarily have to bump openshift/api. We just update the whitelist inside vmware-vsphere-csi-driver.

We are not in a position to support any random config parameter, because we don't even know how that will be applied, so we can only support a sanctioned list of configs and that is where whitelist might be helpful.

Having a separate field for featureFlags sounds good, I'm adding it.

Whitelist is a type of validation, at first I was not too specific about this - adding it.

The benefit of this approach (not having to bump openshift/api) is already mentioned under Pros:.

dhellmann · 2024-02-13T16:27:46Z

#1555 is changing the enhancement template in a way that will cause the header check in the linter job to fail for existing PRs. If this PR is merged within the development period for 4.16 you may override the linter if the only failures are caused by issues with the headers (please make sure the markdown formatting is correct). If this PR is not merged before 4.16 development closes, please update the enhancement to conform to the new template.

RomanBednar · 2024-02-22T09:31:55Z

All linter errors are caused by the recent template change and should be merged in 4.16 development period:

enhancements/storage/vsphere-driver-configuration.md missing "### Topology Considerations"
enhancements/storage/vsphere-driver-configuration.md missing "#### Hypershift / Hosted Control Planes"
enhancements/storage/vsphere-driver-configuration.md missing "#### Standalone Clusters"
enhancements/storage/vsphere-driver-configuration.md missing "#### Single-node Deployments or MicroShift"
enhancements/storage/vsphere-driver-configuration.md missing "## Test Plan"
enhancements/storage/vsphere-driver-configuration.md missing "## Graduation Criteria"
enhancements/storage/vsphere-driver-configuration.md missing "### Dev Preview -> Tech Preview"
enhancements/storage/vsphere-driver-configuration.md missing "### Tech Preview -> GA"
enhancements/storage/vsphere-driver-configuration.md missing "### Removing a deprecated feature"
enhancements/storage/vsphere-driver-configuration.md missing "## Upgrade / Downgrade Strategy"
enhancements/storage/vsphere-driver-configuration.md missing "## Version Skew Strategy"
enhancements/storage/vsphere-driver-configuration.md missing "## Operational Aspects of API Extensions"
enhancements/storage/vsphere-driver-configuration.md missing "## Support Procedures"

/override ci/prow/markdownlint

openshift-ci · 2024-02-22T09:32:33Z

@RomanBednar: RomanBednar unauthorized: /override is restricted to Repo administrators, approvers in top level OWNERS file, and the following github teams:openshift: openshift-release-oversight openshift-staff-engineers.

Details

In response to this:

All linter errors are caused by the recent template change and should be merged in 4.16 development period - overriding:

enhancements/storage/vsphere-driver-configuration.md missing "### Topology Considerations"
enhancements/storage/vsphere-driver-configuration.md missing "#### Hypershift / Hosted Control Planes"
enhancements/storage/vsphere-driver-configuration.md missing "#### Standalone Clusters"
enhancements/storage/vsphere-driver-configuration.md missing "#### Single-node Deployments or MicroShift"
enhancements/storage/vsphere-driver-configuration.md missing "## Test Plan"
enhancements/storage/vsphere-driver-configuration.md missing "## Graduation Criteria"
enhancements/storage/vsphere-driver-configuration.md missing "### Dev Preview -> Tech Preview"
enhancements/storage/vsphere-driver-configuration.md missing "### Tech Preview -> GA"
enhancements/storage/vsphere-driver-configuration.md missing "### Removing a deprecated feature"
enhancements/storage/vsphere-driver-configuration.md missing "## Upgrade / Downgrade Strategy"
enhancements/storage/vsphere-driver-configuration.md missing "## Version Skew Strategy"
enhancements/storage/vsphere-driver-configuration.md missing "## Operational Aspects of API Extensions"
enhancements/storage/vsphere-driver-configuration.md missing "## Support Procedures"

/override ci/prow/markdownlint

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.

openshift-ci · 2024-02-22T09:32:43Z

@RomanBednar: RomanBednar unauthorized: /override is restricted to Repo administrators, approvers in top level OWNERS file, and the following github teams:openshift: openshift-release-oversight openshift-staff-engineers.

Details

In response to this:

/override markdownlint

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.

RomanBednar · 2024-02-22T09:35:21Z

@jsafrane You should be able to override the linter test.

RomanBednar · 2024-03-01T13:59:57Z

Updating enhancement to reflect latest discussion, other proposed options moved to alternatives section.

jsafrane · 2024-03-06T16:36:44Z

enhancements/storage/vsphere-driver-configuration.md

+2. Additionally, they can configure any other option supported by the driver in ClusterCSIDriver - each as a separate
+field that matches driver config ini file values. Administrator does not have to deal with configuring the driver directly.


I thought we wanted to allow just nr. of snapshots and nothing else.

This could be written better to make it clear we intend to allow only 3 options in total for now. I've changed this to make it more clear.

jsafrane · 2024-03-07T12:06:46Z

/lgtm
/approve

/override ci/prow/markdownlint

If this PR is merged within the development period for 4.16 you may override the linter if the only failures are caused by issues with the headers

openshift-ci · 2024-03-07T12:08:11Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: 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~~ [jsafrane]

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

openshift-ci · 2024-03-07T12:08:43Z

@jsafrane: Overrode contexts on behalf of jsafrane: ci/prow/markdownlint

Details

In response to this:

/lgtm
/approve

/override ci/prow/markdownlint

If this PR is merged within the development period for 4.16 you may override the linter if the only failures are caused by issues with the headers

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.

openshift-ci · 2024-03-07T12:08:56Z

@RomanBednar: 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.

openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Feb 9, 2024

openshift-ci bot requested review from coreydaley and flavianmissi February 9, 2024 13:03

jsafrane reviewed Feb 12, 2024

View reviewed changes

RomanBednar force-pushed the vsphere-config branch from 33f0fa3 to a73249b Compare February 13, 2024 13:08

gnufied reviewed Feb 13, 2024

View reviewed changes

RomanBednar force-pushed the vsphere-config branch 2 times, most recently from c4ae7e1 to 79fb217 Compare February 21, 2024 11:20

RomanBednar force-pushed the vsphere-config branch from 79fb217 to 61e01db Compare March 1, 2024 13:59

jsafrane reviewed Mar 6, 2024

View reviewed changes

add enhancement for vSphere driver configuration

dae9a4c

RomanBednar force-pushed the vsphere-config branch from 61e01db to dae9a4c Compare March 6, 2024 16:44

RomanBednar mentioned this pull request Mar 6, 2024

STOR-1803: add vsphere snapshot configuration fields to ClusterCSIDriver openshift/api#1783

Merged

openshift-ci bot assigned jsafrane Mar 7, 2024

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Mar 7, 2024

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 7, 2024

openshift-merge-bot bot merged commit a0f1167 into openshift:master Mar 7, 2024

		2. Additionally, they can configure any other option supported by the driver in ClusterCSIDriver - each as a separate
		field that matches driver config ini file values. Administrator does not have to deal with configuring the driver directly.

Conversation

RomanBednar commented Feb 9, 2024

Uh oh!

openshift-ci-robot commented Feb 9, 2024 • edited by openshift-ci bot Loading Uh oh! There was an error while loading. Please reload this page.

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!

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!

dhellmann commented Feb 13, 2024

Uh oh!

RomanBednar commented Feb 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openshift-ci bot commented Feb 22, 2024

Uh oh!

openshift-ci bot commented Feb 22, 2024

Uh oh!

RomanBednar commented Feb 22, 2024

Uh oh!

RomanBednar commented Mar 1, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jsafrane commented Mar 7, 2024

Uh oh!

openshift-ci bot commented Mar 7, 2024

Uh oh!

openshift-ci bot commented Mar 7, 2024

Uh oh!

openshift-ci bot commented Mar 7, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

openshift-ci-robot commented Feb 9, 2024 •

edited by openshift-ci bot

Loading

RomanBednar commented Feb 22, 2024 •

edited

Loading