Add support to only run selected CSI services

[NotTheEvilOne]

What this PR does / why we need it:
This PR adds support to both cinder-csi-plugin and manila-csi-plugin to execute the controller and node services independently.

This enables use cases where the CSI driver controller deployment is not running on an OpenStack cloud e.g. in a multi-cloud environment. Furthermore it decouples the node ID discovery (either given or metadata server) from the controller service making workaround like --nodeid=fake-id unnecessary. The node ID parameter is no longer required therefore.

Which issue this PR fixes(if applicable):
Fixes #2523

Release note:

Add support to cinder-csi-plugin and manila-csi-plugin to only run selected services

[linux-foundation-easycla]

The committers listed above are authorized under a signed CLA.

• ✅ login: NotTheEvilOne / name: Tobias Wolf (2565000, 10f7b00, a95a7bf, f54bee3, 608369d)

[k8s-ci-robot]

Welcome @NotTheEvilOne!

It looks like this is your first PR to kubernetes/cloud-provider-openstack 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes/cloud-provider-openstack has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

[k8s-ci-robot]

Hi @NotTheEvilOne. 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.

[dulek]

/ok-to-test

I understand the intent here, but it feels smelly to me. Shouldn't we just provide 2 binaries instead? What's the point of running 2 services in a single container anyway?

[NotTheEvilOne]

/ok-to-test

I understand the intent here, but it feels smelly to me. Shouldn't we just provide 2 binaries instead? What's the point of running 2 services in a single container anyway?

Thanks for your valuable questions. I'm sorry to come back to the topic late as I've been on holiday. I both agree and disagree with your idea of two separate binaries.

Looking at the Go code:

• of both CSI driver implementations breaking changes (including but not limited to of course the different binary names) would be required and still a larger amount of boiler plate code (OpenStack client initializiation) would be shared (copy&paste) by both main entry points for controller and node binaries. Furthermore it's common to run both the controller and node on the same k8s node so UNIX socket's may become another issue.
• implementations for GCP [1] and AWS [2] implement similar command line parameters to just select what the CSI driver should offer.

For that reason it seems to me that the one-binary approach is the least invasive implementation for the cinder and manila CSI drivers.

[1] https://github.com/kubernetes-sigs/gcp-compute-persistent-disk-csi-driver/blob/96226fcfe521d0f295c2d258e0c37954e93cfcc9/cmd/gce-pd-csi-driver/main.go#L41
[2] https://github.com/kubernetes-sigs/aws-ebs-csi-driver/blob/a85fb6358eae7b83a083eb8003cf929b3f31d413/charts/aws-ebs-csi-driver/templates/controller.yaml#L72

[dulek]

/ok-to-test
I understand the intent here, but it feels smelly to me. Shouldn't we just provide 2 binaries instead? What's the point of running 2 services in a single container anyway?

Thanks for your valuable questions. I'm sorry to come back to the topic late as I've been on holiday. I both agree and disagree with your idea of two separate binaries.

Looking at the Go code:

• of both CSI driver implementations breaking changes (including but not limited to of course the different binary names) would be required and still a larger amount of boiler plate code (OpenStack client initializiation) would be shared (copy&paste) by both main entry points for controller and node binaries. Furthermore it's common to run both the controller and node on the same k8s node so UNIX socket's may become another issue.
• implementations for GCP [1] and AWS [2] implement similar command line parameters to just select what the CSI driver should offer.

Alright, this is a solid argument that other CSI drivers do a similar thing. Could we try to maintain some kind of consistency and rename --provide-*-service to --run-*-service then?

[NotTheEvilOne]

Alright, this is a solid argument that other CSI drivers do a similar thing. Could we try to maintain some kind of consistency and rename --provide-*-service to --run-*-service then?

This consistency is not yet found. In fact even another Google CSI driver (gcp-filestore-csi-driver) names it differently (only controller and node as bool) and the Alibaba Cloud CSI driver implements it as "run-as-controller".

I've the feeling that "run" is the wrong word as it implies that another process or service is spawned which is not the case. Only registered services changes based on the bool parameters, that's why I named it "provide". I'll change it on demand of course :). Just let me know.

Some tests fail currently, I'll need to check if proper testing can be implemented for the new methods introduced.

[dulek]

/lgtm

Please take care of formal requirements - add a release note and fill CLA.

[NotTheEvilOne]

Hi,

I rebased the changes to be compatible with the current state again. I would highly appreciate your support to get this merged :)

[dulek]

@jichenjc, @kayrus, @zetaab: Hi! Can you give your outlook here?

[jichenjc]

@NotTheEvilOne can you help create an issue to track this change and
maybe we need update to the doc to reflect the change

[NotTheEvilOne]

@NotTheEvilOne can you help create an issue to track this change and maybe we need update to the doc to reflect the change

I added issue #2523 for that PR. Should I add the documentation to this PR or another one so that this one can be merged if no further issues exist?

[dulek]

We would probably prefer updating the docs in this PR. It can be done in a new commit.

[dulek]

Docs look good for me, leaving the final approval to others.

[dulek]

These CI errors look unrelated.

/retest

[dulek]

/retest

[NotTheEvilOne]

/retest

[NotTheEvilOne]

/retest

[dulek]

#2529 should fix the CIs.

[dulek]

/retest

[NotTheEvilOne]

Looks like we finally made it :) Do you think we can merge it then? :)

[dulek]

/lgtm

[kayrus]

/cherry-pick release-1.29
required for #2656

[k8s-infra-cherrypick-robot]

@kayrus: new pull request created: #2663

In response to this:

/cherry-pick release-1.29
required for #2656

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-sigs/prow repository.

[kayrus]

/cherry-pick release-1.28
required for #2656

[k8s-infra-cherrypick-robot]

@kayrus: #2316 failed to apply on top of branch "release-1.28":

Applying: Add support to only run selected CSI services of the cinder CSI driver
Using index info to reconstruct a base tree...
M	cmd/cinder-csi-plugin/main.go
M	pkg/csi/cinder/controllerserver_test.go
M	pkg/csi/manila/driver.go
Falling back to patching base and 3-way merge...
Auto-merging pkg/csi/manila/driver.go
Auto-merging pkg/csi/cinder/controllerserver_test.go
Auto-merging cmd/cinder-csi-plugin/main.go
CONFLICT (content): Merge conflict in cmd/cinder-csi-plugin/main.go
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 Add support to only run selected CSI services of the cinder CSI driver
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

In response to this:

/cherry-pick release-1.28
required for #2656

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-sigs/prow repository.

[kayrus]

/cherry-pick release-1.28

[k8s-infra-cherrypick-robot]

@kayrus: new pull request created: #2670

In response to this:

/cherry-pick release-1.28

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-sigs/prow repository.