Pass actual cluster name to cinder-csi-plugin

[ederst]

This passes the acutal cluster name to the cinder-csi-plugin, so that the plugin will add the name as metadata to the backing volume in OpenStack.

Effectively, the change will help to better identify which volume in OpenStack belongs to which cluster, which is especially helpful when running multiple clusters in one OpenStack tenant/project.

Setting the cluster name in both - the controller and the nodeplugin - pods will ensure that dynamic and ephemeral volumes will receive the correct metadata.

[k8s-ci-robot]

Hi @ederst. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

[hakman]

/ok-to-test
/cc @zetaab

[zetaab]

hmm, what will happen to existing installations? It might work correctly to newer one, but what about old ones?

[ederst]

tried to answer this in one place, here: #15095 (comment)

[zetaab]

also I think we need to add this also to occm? Seems that loadbalancer names have the cluster name, so if you create two similar named LBs to two different clusters it is facing an issue. Anyway this change is not backwards compatible, so I would like to see some boolean flag for that feature. It should be turned on for new clusters, but not to old ones.

if I read code correctly, modifying existing CSI cinder cluster name should not matter(?). The cluster name is used only when creating snapshots or new volumes, but it will not affect anything else (old volumes should work?). Anyway this might be risky move, if later on the behaviour will change in csi cinder driver (the labels should be modified at least).

IMO the best what we can do here, is to add that new boolean value which will define the behaviour for both: occm and csi. After that it can be used for new clusters but not the old ones

[ederst]

also I think we need to add this also to occm? Seems that loadbalancer names have the cluster name, so if you create two similar named LBs to two different clusters it is facing an issue. Anyway this change is not backwards compatible, so I would like to see some boolean flag for that feature. It should be turned on for new clusters, but not to old ones.

Don't know about that, I know there was an issue resolved in OCCM (kubernetes/cloud-provider-openstack#1809), but AFAIK the --cluster-name is already configurable for the (O)CCMs in kOps:

https://github.com/kubernetes/kops/blob/master/pkg/apis/kops/componentconfig.go#L675-L676

So in case someone experiences this issue, they can do that already.

if I read code correctly, modifying existing CSI cinder cluster name should not matter(?). The cluster name is used only when creating snapshots or new volumes, but it will not affect anything else (old volumes should work?). Anyway this might be risky move, if later on the behaviour will change in csi cinder driver (the labels should be modified at least).
IMO the best what we can do here, is to add that new boolean value which will define the behaviour for both: occm and csi. After that it can be used for new clusters but not the old ones

Yes, right now the label seems to be just informative, that's what I read from code and doc as well. And I tried it out on a few of our test clusters, does not seem to impact the old volumes, and new volumes just receive the cluster name instead of "kubernetes" in the property.

But you got a point with "when the behavior changes later on". So this might be better off as a config flag. But maybe not a boolean but also a clusterName property here:

kops/pkg/apis/kops/componentconfig.go

Lines 784 to 793 in ffdffb3

	type OpenstackBlockStorageConfig struct {
	Version *string `json:"bs-version,omitempty"`
	IgnoreAZ *bool `json:"ignore-volume-az,omitempty"`
	OverrideAZ *string `json:"override-volume-az,omitempty"`
	IgnoreVolumeMicroVersion *bool `json:"ignore-volume-microversion,omitempty"`
	// CreateStorageClass provisions a default class for the Cinder plugin
	CreateStorageClass *bool `json:"createStorageClass,omitempty"`
	CSIPluginImage string `json:"csiPluginImage,omitempty"`
	CSITopologySupport *bool `json:"csiTopologySupport,omitempty"`
	}

For example like this:

type OpenstackBlockStorageConfig struct {
	Version                  *string `json:"bs-version,omitempty"`
	IgnoreAZ                 *bool   `json:"ignore-volume-az,omitempty"`
	OverrideAZ               *string `json:"override-volume-az,omitempty"`
	IgnoreVolumeMicroVersion *bool   `json:"ignore-volume-microversion,omitempty"`
	// CreateStorageClass provisions a default class for the Cinder plugin
	CreateStorageClass *bool  `json:"createStorageClass,omitempty"`
	CSIPluginImage     string `json:"csiPluginImage,omitempty"`
	CSITopologySupport *bool  `json:"csiTopologySupport,omitempty"`
        // ClusterName sets the --cluster flag of the cinder-csi-pluing to the provided name instead of "kubernetes"
        ClusterName  string `json:"clusterName,omitempty"`  // default value: "kubernetes"
}

Edit: did the changes i mentioned above, marked PR as WIP intmt

[ederst]

/restest

[zetaab]

/retest

@ederst IMO this looks almost correct in case of CSI cinder. The only missing item is that we should populate this clusterName in case of new clusters. If we do that, we make it better for users hopefully in future. Perhaps good place for that is in https://github.com/kubernetes/kops/blob/master/upup/pkg/fi/cloudup/new_cluster.go#L326

However, I checked our kops occm installations and those does not have cluster-name flag at all for some reason(?).. anyways that can be handled as separate issue/pr if needed.

[ederst]

/retest

@ederst IMO this looks almost correct in case of CSI cinder. The only missing item is that we should populate this clusterName in case of new clusters. If we do that, we make it better for users hopefully in future. Perhaps good place for that is in https://github.com/kubernetes/kops/blob/master/upup/pkg/fi/cloudup/new_cluster.go#L326

Ah, OK now I think I know what you mean. Sounds good, will check out the issue (on Monday probably).

However, I checked our kops occm installations and those does not have cluster-name flag at all for some reason(?).. anyways that can be handled as separate issue/pr if needed.

Strange, but maybe it is only available in OCCM >=1.26 b/c it was not cherry-picked to older versions? Had this issue with some features.

Nope, this should be coming from the controller lib occm has as dependency, so not sure what's the problem with your occm installations.

[hakman]

Please squash the commits first.
/hold

[zetaab]

/approve cancel

also we need that default value to https://github.com/kubernetes/kops/blob/master/upup/pkg/fi/cloudup/new_cluster.go#L326

sorry I was too fast, did not remember that missing piece :)

[ederst]

@hakman @zetaab good to go now, or should the --cluster flag be set to the value of the cloud controller manager?

https://github.com/kubernetes/kops/pull/15139/files#diff-cf0e850db1e4461eab5d4dac860d20f2176b940f236e50825ae538ee23688a1e

[hakman]

Thanks @ederst. Leaving the final review to @zetaab.
/hold cancel

[zetaab]

@ederst if you could add the clustername by default here https://github.com/kubernetes/kops/blob/master/upup/pkg/fi/cloudup/new_cluster.go#L326

something like

			BlockStorage: &api.OpenstackBlockStorageConfig{
				Version:     fi.PtrTo("v3"),
				IgnoreAZ:    fi.PtrTo(opt.OpenstackStorageIgnoreAZ),
				ClusterName: opt.ClusterName,
			},

then I think you need to execute ./hack/update-expected.sh. Please make code changes to first commit and then update-expected.sh to second commit

[ederst]

@zetaab done like suggested, updated commits again so that code changes are separate from the generated stuff

[zetaab]

thank you

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: zetaab

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 [zetaab]

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