Bug 1936871: allow setting a custom config for the driver #29

Fedosin · 2021-03-09T11:56:23Z

This patch allows users to set custom config for the driver. To do so, they need to create a config map called "openstack-cinder-custom-config" in "openshift-cluster-csi-drivers" namespace, with a key "cloud.conf".

If this config map exists, then the driver will use the custom config.

openshift-ci-robot · 2021-03-09T11:56:27Z

@Fedosin: This pull request references Bugzilla bug 1936871, which is invalid:

expected the bug to target the "4.8.0" release, but it targets "---" instead

Comment /bugzilla refresh to re-evaluate validity if changes to the Bugzilla bug are made, or edit the title of this pull request to link to a different bug.

In response to this:

Bug 1936871: set ignore-volume-az to true

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-robot · 2021-03-09T11:56:36Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Fedosin

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

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

Fedosin · 2021-03-09T11:57:29Z

/bugzilla refresh

openshift-ci-robot · 2021-03-09T11:57:38Z

@Fedosin: This pull request references Bugzilla bug 1936871, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target release (4.8.0) matches configured target release for branch (4.8.0)
bug is in the state NEW, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

No GitHub users were found matching the public email listed for the QA contact in Bugzilla (rlobillo@redhat.com), skipping review request.

In response to this:

/bugzilla refresh

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.

Fedosin · 2021-03-09T12:08:45Z

/assign @mandre

mandre

I'm not very keen on changing the default config, but according to the ignore-volume-az documentation the volume node affinity effectively prevents attaching to the node if the compute zone doesn't match the volume one when Topology feature is turned on (that's the case for openshift).

@jsafrane is there a way to update the csi driver config as day2 similarly to what we're doing for the in-tree cinder provisioner? I'd prefer that if that was an option.

jsafrane · 2021-03-09T14:32:07Z

In-tree volume plugin seems to be controller by ConfigMap cloud-provider-config in openshift-config. https://github.com/openshift/installer/blob/master/docs/user/openstack/known-issues.md#cinder-availability-zones

The Cinder operator should watch this ConfigMap and update Deployment/DaemonSet accordingly via DeploymentHook / DaemonSetHook, see e.g. how AWS EBS operator modifies Deployment to inject optional CA bundle into Deployment )

Fedosin · 2021-03-09T16:30:02Z

/test e2e-openstack

This patch allows users to set custom config for the driver. To do so, they need to create a config map called "openstack-cinder-custom-config" in "openshift-cluster-csi-drivers" namespace, with a key "cloud.conf". If this config map exists, then the driver will use the custom config.

openshift-ci-robot · 2021-03-25T15:09:51Z

@Fedosin: This pull request references Bugzilla bug 1936871, which is valid.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target release (4.8.0) matches configured target release for branch (4.8.0)
bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

No GitHub users were found matching the public email listed for the QA contact in Bugzilla (rlobillo@redhat.com), skipping review request.

In response to this:

Bug 1936871: set ignore-volume-az to true

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.

mdbooth

I'm uncomfortable with this change. As currently designed it could be improved as noted in review comments by:

changing the configmap which we pass to the deployment rather than passing a second one
refactoring the 2 podspec mutating methods to use common code

However, I'm more generally uncomfortable with the design of the feature.

It requires the user to pass in our default configuration in addition to any new configuration they want to add. This presents us with an incredibly messy upgrade problem if we ever need to change this.

It requires the user to pass in raw configuration at all. Can we not add an operator parameter which would set this, for eg?

If we decide to go with the user passing in config, please can we have them pass in an ini fragment which is merged with and overrides the default configuration? We should note in the documentation that they should not include any configuration directives that they don't want to override. In this way they could pass in simply:

[BlockStorage]
ignore-volume-az = true

rather than

[Global]
use-clouds = true
clouds-file = /etc/kubernetes/secret/clouds.yaml
cloud = openstack

[BlockStorage]
ignore-volume-az = true

/hold

mdbooth · 2021-04-08T16:54:04Z

pkg/operator/starter.go

+			return nil
+		}
+		deployment.Spec.Template.Spec.Volumes = append(deployment.Spec.Template.Spec.Volumes, corev1.Volume{
+			Name: "custom-config",


If we're just doing a simple replace, why mount both config maps? Why not mount the custom one in place of the default?

mdbooth · 2021-04-08T16:54:43Z

pkg/operator/starter.go

+				continue
+			}
+			container.Env = append(container.Env, corev1.EnvVar{
+				Name:  "CLOUD_CONFIG",


Looks like this will define CLOUD_CONFIG twice? Looks kinda weird.

mdbooth · 2021-04-08T16:56:21Z

pkg/operator/starter.go

+		case !used:
+			return nil
+		}
+		daemonset.Spec.Template.Spec.Volumes = append(daemonset.Spec.Template.Spec.Volumes, corev1.Volume{


These 2 methods should be refactored to a use a common function which modifies, e.g. a PodSpec.

mdbooth · 2021-04-09T13:00:55Z

I discussed this with @mandre earlier and we both have the same concern about the upgrade issue if we force the user to fork our default configuration.

We discussed the possibility of having some operator config which would write a new config itself containing ignore-volume-az = true. Neither of us were a huge fan of that, myself for many of the reasons I don't like TripleO. However, we did discuss it.

The 2 options we discussed for merging a default configuration with a user-supplied additional config were:

Adding a tool to do this to the operator image, and then using this in an init container in the CSI controller pod
Adding support to the CPO controller to take multiple configfile command line arguments, and merge them

Either of these would work for me as they both avoid the upgrade problem which is the real issue here. I personally prefer the latter as it would be cleaner, so I'm going to see if I can write that and propose it against upstream CPO on Monday. If we had that we'd just need some tweaks to this PR to add the second configfile to the command line in addition to the changes it already makes.

mdbooth · 2021-04-09T17:00:58Z

kubernetes/cloud-provider-openstack#1476 would allow specifying multiple config files, which would make this much cleaner and remove the upgrade problem.

openshift-bot · 2021-07-08T20:12:42Z

Issues go stale after 90d of inactivity.

Mark the issue as fresh by commenting /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.
Exclude this issue from closing by commenting /lifecycle frozen.

If this issue is safe to close now please do so with /close.

/lifecycle stale

openshift-ci · 2021-07-08T20:12:48Z

@Fedosin: PR needs rebase.

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-robot added bugzilla/severity-high Referenced Bugzilla bug's severity is high for the branch this PR is targeting. bugzilla/invalid-bug Indicates that a referenced Bugzilla bug is invalid for the branch this PR is targeting. labels Mar 9, 2021

openshift-ci-robot requested review from gnufied and huffmanca March 9, 2021 11:56

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 9, 2021

openshift-ci-robot added bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. and removed bugzilla/invalid-bug Indicates that a referenced Bugzilla bug is invalid for the branch this PR is targeting. labels Mar 9, 2021

openshift-ci-robot assigned mandre Mar 9, 2021

mandre reviewed Mar 9, 2021

View reviewed changes

Fedosin changed the title ~~Bug 1936871: set ignore-volume-az to true~~ Bug 1936871: allow setting a custom config for the driver Mar 25, 2021

mdbooth suggested changes Apr 8, 2021

View reviewed changes

openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 8, 2021

openshift-ci-robot assigned mdbooth Apr 8, 2021

mdbooth mentioned this pull request Apr 12, 2021

[cinder-csi-plugin] Allow passing multiple config files kubernetes/cloud-provider-openstack#1481

Closed

openshift-ci bot mentioned this pull request May 18, 2021

Bug 1936871: Add volumes with credentials to Cinder operator deployment openshift/cluster-storage-operator#141

Merged

openshift-ci bot mentioned this pull request Jun 1, 2021

Bug 1936871: support clouds with multiple availability zones #43

Merged

openshift-ci bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jul 8, 2021

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

Fedosin closed this Jul 23, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bug 1936871: allow setting a custom config for the driver #29

Bug 1936871: allow setting a custom config for the driver #29

Fedosin commented Mar 9, 2021 •

edited

openshift-ci-robot commented Mar 9, 2021

openshift-ci-robot commented Mar 9, 2021

Fedosin commented Mar 9, 2021

openshift-ci-robot commented Mar 9, 2021

Fedosin commented Mar 9, 2021

mandre left a comment

jsafrane commented Mar 9, 2021

Fedosin commented Mar 9, 2021

openshift-ci-robot commented Mar 25, 2021

mdbooth left a comment

mdbooth Apr 8, 2021

mdbooth Apr 8, 2021

mdbooth Apr 8, 2021

mdbooth commented Apr 9, 2021

mdbooth commented Apr 9, 2021

openshift-bot commented Jul 8, 2021

openshift-ci bot commented Jul 8, 2021

Bug 1936871: allow setting a custom config for the driver #29

Bug 1936871: allow setting a custom config for the driver #29

Conversation

Fedosin commented Mar 9, 2021 • edited

openshift-ci-robot commented Mar 9, 2021

openshift-ci-robot commented Mar 9, 2021

Fedosin commented Mar 9, 2021

openshift-ci-robot commented Mar 9, 2021

Fedosin commented Mar 9, 2021

mandre left a comment

Choose a reason for hiding this comment

jsafrane commented Mar 9, 2021

Fedosin commented Mar 9, 2021

openshift-ci-robot commented Mar 25, 2021

mdbooth left a comment

Choose a reason for hiding this comment

mdbooth Apr 8, 2021

Choose a reason for hiding this comment

mdbooth Apr 8, 2021

Choose a reason for hiding this comment

mdbooth Apr 8, 2021

Choose a reason for hiding this comment

mdbooth commented Apr 9, 2021

mdbooth commented Apr 9, 2021

openshift-bot commented Jul 8, 2021

openshift-ci bot commented Jul 8, 2021

Fedosin commented Mar 9, 2021 •

edited