Add support to only run selected CSI services #2316

NotTheEvilOne · 2023-08-05T02:43:41Z

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 · 2023-08-05T02:43:45Z

The committers listed above are authorized under a signed CLA.

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

k8s-ci-robot · 2023-08-05T02:43:49Z

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 · 2023-08-05T02:43:50Z

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 · 2023-08-07T14:54:10Z

/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 · 2023-08-21T18:02:12Z

/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 · 2023-08-22T08:44:08Z

/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 · 2023-08-22T17:38:02Z

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 · 2023-08-24T14:54:37Z

/lgtm

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

NotTheEvilOne · 2024-01-17T08:42:42Z

Hi,

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

dulek

Looks pretty good to me, question inline.

dulek · 2024-01-17T17:36:05Z

cmd/manila-csi-plugin/main.go

-	if err := cmd.MarkPersistentFlagRequired("nodeid"); err != nil {
-		klog.Fatalf("Unable to mark flag nodeid to be required: %v", err)
-	}


Should we still do it when provideNodeService is true?

There's a new validation in Driver.SetupNodeService() ensuring that the nodeid is not empty if required.

Marking it as required sounds wrong if provideNodeService is false, marking it only required if provideNodeService is true seems to be weird. The description of the PersistentFlag may be extended to note that it is required for the node service?

Yeah, true, we could update the description, otherwise looks good. Please update that, I'll do /approve anyway.

dulek · 2024-01-17T17:36:49Z

pkg/csi/cinder/controllerserver_test.go

Looks like changes in this (and other) file are caused by your IDE formatting the code. It's fine, but it makes reviewing more difficult.

Sorry for that. I think the last commit got a bit mixed up as well when I rebased to HEAD. Let me know if I should restructure the commits :)

dulek · 2024-01-18T16:22:28Z

/approve

k8s-ci-robot · 2024-01-18T16:22:38Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dulek

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

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

dulek · 2024-01-19T10:01:25Z

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

jichenjc · 2024-01-22T08:53:39Z

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

NotTheEvilOne · 2024-01-22T16:44:12Z

@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 · 2024-01-23T16:34:36Z

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

This commit updates the documentation for the CSI controller and node service providing parameters.

dulek · 2024-01-23T17:01:12Z

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

dulek · 2024-01-23T17:27:56Z

These CI errors look unrelated.

/retest

dulek · 2024-01-24T08:50:18Z

/retest

NotTheEvilOne · 2024-01-26T12:29:16Z

/retest

NotTheEvilOne · 2024-01-28T15:55:15Z

/retest

dulek · 2024-01-29T17:11:52Z

#2529 should fix the CIs.

dulek · 2024-01-30T09:01:15Z

/retest

NotTheEvilOne · 2024-02-01T13:01:01Z

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

dulek · 2024-02-02T14:00:59Z

/lgtm

k8s-ci-robot added the do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. label Aug 5, 2023

k8s-ci-robot added the cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. label Aug 5, 2023

k8s-ci-robot requested review from Fedosin and tombarron August 5, 2023 02:43

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Aug 5, 2023

k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Aug 5, 2023

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Aug 7, 2023

k8s-ci-robot assigned dulek Aug 24, 2023

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 23, 2023

NotTheEvilOne added 3 commits January 17, 2024 09:40

Add support to only run selected CSI services of the cinder CSI driver

2565000

Add support to only run selected CSI services of the manila CSI driver

10f7b00

Clean up source files to successfully complete linting

a95a7bf

NotTheEvilOne force-pushed the pr/csi-split-controller-and-node-server branch from fb21909 to a95a7bf Compare January 17, 2024 08:41

k8s-ci-robot removed lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Jan 17, 2024

dulek reviewed Jan 17, 2024

View reviewed changes

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 18, 2024

Update description of the nodeid command line parameter

f54bee3

Update documentation for the CSI service parameters

608369d

This commit updates the documentation for the CSI controller and node service providing parameters.

dulek mentioned this pull request Jan 23, 2024

fix: add missing labels to resources #2519

Merged

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 2, 2024

k8s-ci-robot merged commit 7f1daa8 into kubernetes:master Feb 2, 2024
8 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support to only run selected CSI services #2316

Add support to only run selected CSI services #2316

NotTheEvilOne commented Aug 5, 2023 •

edited

linux-foundation-easycla bot commented Aug 5, 2023 •

edited

k8s-ci-robot commented Aug 5, 2023

k8s-ci-robot commented Aug 5, 2023

dulek commented Aug 7, 2023

NotTheEvilOne commented Aug 21, 2023

dulek commented Aug 22, 2023

NotTheEvilOne commented Aug 22, 2023

dulek commented Aug 24, 2023

NotTheEvilOne commented Jan 17, 2024

dulek left a comment

dulek Jan 17, 2024

NotTheEvilOne Jan 18, 2024

dulek Jan 18, 2024

dulek Jan 17, 2024

NotTheEvilOne Jan 18, 2024

dulek commented Jan 18, 2024

k8s-ci-robot commented Jan 18, 2024

dulek commented Jan 19, 2024

jichenjc commented Jan 22, 2024

NotTheEvilOne commented Jan 22, 2024

dulek commented Jan 23, 2024 •

edited

dulek commented Jan 23, 2024

dulek commented Jan 23, 2024

dulek commented Jan 24, 2024

NotTheEvilOne commented Jan 26, 2024

NotTheEvilOne commented Jan 28, 2024

dulek commented Jan 29, 2024

dulek commented Jan 30, 2024

NotTheEvilOne commented Feb 1, 2024

dulek commented Feb 2, 2024

Add support to only run selected CSI services #2316

Add support to only run selected CSI services #2316

Conversation

NotTheEvilOne commented Aug 5, 2023 • edited

linux-foundation-easycla bot commented Aug 5, 2023 • edited

k8s-ci-robot commented Aug 5, 2023

k8s-ci-robot commented Aug 5, 2023

dulek commented Aug 7, 2023

NotTheEvilOne commented Aug 21, 2023

dulek commented Aug 22, 2023

NotTheEvilOne commented Aug 22, 2023

dulek commented Aug 24, 2023

NotTheEvilOne commented Jan 17, 2024

dulek left a comment

Choose a reason for hiding this comment

dulek Jan 17, 2024

Choose a reason for hiding this comment

NotTheEvilOne Jan 18, 2024

Choose a reason for hiding this comment

dulek Jan 18, 2024

Choose a reason for hiding this comment

dulek Jan 17, 2024

Choose a reason for hiding this comment

NotTheEvilOne Jan 18, 2024

Choose a reason for hiding this comment

dulek commented Jan 18, 2024

k8s-ci-robot commented Jan 18, 2024

dulek commented Jan 19, 2024

jichenjc commented Jan 22, 2024

NotTheEvilOne commented Jan 22, 2024

dulek commented Jan 23, 2024 • edited

dulek commented Jan 23, 2024

dulek commented Jan 23, 2024

dulek commented Jan 24, 2024

NotTheEvilOne commented Jan 26, 2024

NotTheEvilOne commented Jan 28, 2024

dulek commented Jan 29, 2024

dulek commented Jan 30, 2024

NotTheEvilOne commented Feb 1, 2024

dulek commented Feb 2, 2024

NotTheEvilOne commented Aug 5, 2023 •

edited

linux-foundation-easycla bot commented Aug 5, 2023 •

edited

dulek commented Jan 23, 2024 •

edited