[cinder-csi-plugin] Allow disabling particular driver services

[rfranzke]

What this PR does / why we need it:
This PR adds two new command line flags --run-controller-service and --run-node-service (both default to true) which allow to particularly disable individual services if not required.

This enables the use-case of running the CSI controllers (csi-provisioner, csi-attacher, etc., + the driver controller) separately outside of the cluster they are serving, and not necessarily on an OpenStack nova VM. The node service part of the CSI driver will still inside the cluster for mounting/unmounting the volume, etc.

Release note:

For the cinder CSI plugin, it is now possible to disable the controller service by setting `--run-controller-service=false`. Similarly, it is possible to disable the node service by setting `--run-node-service=false`. The latter enables running the controller server of the Cinder CSI driver separately/outside of the cluster it is serving.

[k8s-ci-robot]

Welcome @rfranzke!

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 @rfranzke. 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.

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
To complete the pull request process, please assign chrigl
You can assign the PR to them by writing /assign @chrigl in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

• OWNERS

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

[rfranzke]

/assign @chrigl

[theopenlab-ci]

Build succeeded.

• cloud-provider-openstack-format : SUCCESS in 2m 49s

[theopenlab-ci]

Build succeeded.

• cloud-provider-openstack-unittest : SUCCESS in 3m 54s

[theopenlab-ci]

Build succeeded.

• cloud-provider-openstack-acceptance-test-csi-cinder : SUCCESS in 15m 40s

[theopenlab-ci]

Build succeeded.

• cloud-provider-openstack-e2e-test-csi-cinder : SUCCESS in 34m 38s

[jichenjc]

/ok-to-test

[lingxiankong]

This enables the use-case of running the CSI controllers (csi-provisioner, csi-attacher, etc., + the driver controller) separately outside of the cluster they are serving, and not necessarily on an OpenStack nova VM.

cinder-csi-plugin already supports this without this PR, it's running a gPRC server with all the interfaces implemented, we don't need to add two extra parameters explicitly.

/hold

[rfranzke]

cinder-csi-plugin already supports this without this PR, it's running a gPRC server with all the interfaces implemented

@jianglingxia Can you explain more? As of today, when starting the cinder CSI driver it unconditionally sets up both the controller and the node server, see https://github.com/kubernetes/cloud-provider-openstack/blob/master/pkg/csi/cinder/driver.go#L138-L144. I don't see how is it supported already today?

[lingxiankong]

Can you explain more? As of today, when starting the cinder CSI driver it unconditionally sets up both the controller and the node server, see https://github.com/kubernetes/cloud-provider-openstack/blob/master/pkg/csi/cinder/driver.go#L138-L144. I don't see how is it supported already today?

The code you mentioned is just registering services and its implementation to the gRPC server, they are not the 'real' server running behind, after registering, you still have only one server running, but can serve different types of gRPC calling.

What problem do you want to solve? What problems are you facing currently? If you do see an issue caused by the current implementation, it would be great if you could submit an issue before actually submitting a PR.

[rfranzke]

The code you mentioned is just registering services and its implementation to the gRPC server, they are not the 'real' server running behind, after registering, you still have only one server running, but can serve different types of gRPC calling.

This is understood. However, I want to optionally enable that either the controller or the node service can be disabled from being registered to the gRPC server.

What problem do you want to solve?

I added docs and a description for the problem in the PR. Concretely, I want to enable the use-case where the CSI controllers are NOT running inside the cluster they are serving but outside in a different cluster. Imagine cluster A that runs the CSI provisioner+attacher + Cinder CSI driver (with only controller service enabled) for cluster B. In cluster B the CSI driver (with only node service enabled) is ran (+driver-registrar and liveness probe).

Also, it should be made possible that cluster A is not necessarily backed by OpenStack VMs. The today's implementation assumes that the controller service can talk to the OpenStack metadata service which is not possible in this case.

What problems are you facing currently?

The above mentioned scenarios are not possible to run today because it's not possible to cleanly separate the driver services.

If you do see an issue caused by the current implementation, it would be great if you could submit an issue before actually submitting a PR.

Well, if you prefer I can open an issue now and describe the above problems + reference this PR as solution for it. Shall I really do it now that we already have this discussion thread + a proposal for a fix?

FYI: We have done similar enhancements to the AWS EBS CSI driver (kubernetes-sigs/aws-ebs-csi-driver#438) and to the GCE PD CSI driver (kubernetes-sigs/gcp-compute-persistent-disk-csi-driver#449) already.

[lingxiankong]

Hi @rfranzke I'm still confused, I totally understand want you want do do with this PR, but I don't understand why you can't run controller services and node services separately? If you look at https://github.com/kubernetes/cloud-provider-openstack/tree/master/manifests/cinder-csi-plugin, as example, we are actually running controller and node plugin at different places using different yaml files.

Maybe I'm wrong or I didn't fully understand you use case, but can you paste some error logs with your preferable deployment methods?

If it's possible, please create an issue with the steps and error log, we could move discussion there. Thanks for your patience.

@ramineni @jichenjc @chrigl Please also join us for this discussion if you could.

[ramineni]

Also, it should be made possible that cluster A is not necessarily backed by OpenStack VMs. The today's implementation assumes that the controller service can talk to the OpenStack metadata service which is not possible in this case.

Controller service doesnt depend on Openstack metatdata service, it never queries it as per https://github.com/kubernetes/cloud-provider-openstack/blob/master/pkg/csi/cinder/controllerserver.go
https://github.com/kubernetes/cloud-provider-openstack/blob/master/cmd/cinder-csi-plugin/main.go#L111 , this only creates an object , doesnt actually query, so if only controller plugin manifest is deployed on node which is not backed by Openstack VMs, this should ideally still work? Are you facing issue with that?

Imagine cluster A that runs the CSI provisioner+attacher + Cinder CSI driver (with only controller service enabled) for cluster B. In cluster B the CSI driver (with only node service enabled) is ran (+driver-registrar and liveness probe).

As per current implementation, even if we have both node service and controller service enabled in plugin, unless we run node-driver-registrar sidecar, NodeGetInfo, NodeStageVolume, NodePublishVolume are not possible , so no effect of node service if only controller plugin manifest is deployed on the node ?

If the above doesnt solve your usecase, could you elaborate on issues you are facing with both enabled and having different manifests doesnt solve your usecase

[rfranzke]

@lingxiankong @ramineni Thanks for your views! I was trying it out again and thoroughly followed the example manifests, and indeed, it works as you expected. Probably I was working with outdated manifests earlier or did something else wrong. Anyways, I guess the PR can be closed then as the scenario is already supported. Thanks again for your help and sorry for taking your time with this.

/close

[lingxiankong]

@rfranzke thanks for the followup