Allow disabling particular driver services #449

rfranzke · 2020-01-10T11:09:23Z

What type of PR is this?
/kind feature

What this PR does / why we need it:
This PR adds the new zone field to the GCE cloud config and makes the controller server independent of the GCE metadata service if both project-id and zone are present in the config.
Also, it 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, and not necessarily on an GCE compute instance.

Does this PR introduce a user-facing change?:

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 GCE PD driver separately/outside of the cluster it is serving. Also, if both `project-id` and `zone` are specified in the GCE cloud config then the controller server does no longer try to contact the GCE metadata service.

k8s-ci-robot · 2020-01-10T11:09:31Z

Welcome @rfranzke!

It looks like this is your first PR to kubernetes-sigs/gcp-compute-persistent-disk-csi-driver 🎉. 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-sigs/gcp-compute-persistent-disk-csi-driver 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 · 2020-01-10T11:09:32Z

Hi @rfranzke. Thanks for your PR.

I'm waiting for a kubernetes-sigs 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.

rfranzke · 2020-01-10T11:10:33Z

/assign @msau42

msau42 · 2020-01-10T21:05:32Z

/ok-to-test
/assign @davidz627

rfranzke · 2020-01-11T14:05:21Z

/retest

rfranzke · 2020-01-19T12:10:46Z

@davidz627 @msau42 @jingxu97 any feedback for this PR?

hantaowang · 2020-01-21T10:34:07Z

why have SetupGCEDriver1 take runAsController as a *bool instead of just the bool? Seems weird because then in all tests you declare a bool and then take the address of it, despite the bool being a built in type.

Another nit is that (correct me if im wrong) but I don't think runAsController can ever be a nil pointer since it defaults to false.

Its been a while since I've looked at this code but it looks like your changes to pkg/gce-cloud-provider/compute/gce.go and renaming gceConfigFilePath are separate from the goal of this PR.

rfranzke · 2020-01-21T11:31:49Z

why have SetupGCEDriver1 take runAsController as a *bool instead of just the bool? Seems weird because then in all tests you declare a bool and then take the address of it, despite the bool being a built in type.
Another nit is that (correct me if im wrong) but I don't think runAsController can ever be a nil pointer since it defaults to false.

Thanks, valid feedback, I improved it, PTAL.

Its been a while since I've looked at this code but it looks like your changes to pkg/gce-cloud-provider/compute/gce.go

Yes, they are related to the goal. Without this change you can't run the driver in the controller mode on a non-GCE instance.

and renaming gceConfigFilePath are separate from the goal of this PR.

This is not related to the goal. If it matters that much I can also keep it as it was before.

cmd/main.go

davidz627

mostly looks good, this is something we've been thinking about for a while so thank you for picking it up :)

docs/kubernetes/user-guides/driver-install.md

pkg/gce-cloud-provider/compute/gce.go

rfranzke · 2020-01-26T10:52:39Z

Thanks for your feedback @davidz627, can you now check again? I updated the PR based on your suggestions. Please double-check the docs section.

rfranzke · 2020-01-26T11:22:05Z

/retest

davidz627

@rfranzke I think I understand what you're trying to do now - please let me know if I'm mistaken. I've added comments for both "using local or client configuration" (what I think you're trying to do). As well as for "disabling certain driver services" (what this PR description and title led me to believe as the goal)

cmd/main.go

docs/kubernetes/user-guides/driver-install.md

rfranzke · 2020-02-11T16:48:21Z

Hi again, meanwhile I had time to update the PR.

I was first trying to implement @davidz627's suggestion (#449 (comment)), however, I reached a point where it became clear that this isn't enough for my use-case.

Just to reiterate: I want to allow running the controller server of the GCE PD CSI driver separately/outside of the cluster that it is serving - potentially not even on a GCE instance.
With only adding zone to the GCE cloud config and making the controller server independent of the GCE metadata service this would not be possible because the node server would still be activated - and it is relying on the metadata service.

I reached out to @davidz627 via Slack and we agreed on introducing the earlier discussed run-controller-service and run-node-service flags that allow disabling the particular services individually. This enables use-cases like mine, but probably even more.

TL;DR - The PR now comprises two commits:

Making the controller service independent of the GCE metadata service if both project-id and zone are specified in its config
Allow disabling the controller and/or node service via command line flags

The tests are now clearly instantiating only the objects that are really needed for the various servers (e.g., the identity server test does no longer create a fake metadata service).

Please take a look, I'm happy to get your feedback on this.

davidz627

2 small comments, LGTM besides those two!!
Thanks for the iterations. This looks brilliant :)

pkg/gce-pd-csi-driver/gce-pd-driver.go

pkg/gce-cloud-provider/compute/gce-compute.go

rfranzke · 2020-02-11T19:57:32Z

/retest

davidz627

/lgtm

davidz627 · 2020-02-11T23:11:02Z

cmd/main.go

+		}
+		controllerServer = driver.NewControllerServer(gceDriver, cloudProvider)
+	} else if *cloudConfigFilePath != "" {
+		klog.Warningf("controller service is disabled but cloud config given - it has no effect")


davidz627 · 2020-02-11T23:11:57Z

/approve

k8s-ci-robot · 2020-02-11T23:12:09Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: davidz627, rfranzke

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

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

k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. kind/feature Categorizes issue or PR as related to a new feature. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Jan 10, 2020

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 Jan 10, 2020

k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Jan 10, 2020

k8s-ci-robot requested review from davidz627 and jingxu97 January 10, 2020 11:09

k8s-ci-robot assigned msau42 Jan 10, 2020

k8s-ci-robot added the ok-to-test Indicates a non-member PR verified by an org member that is safe to test. label Jan 10, 2020

k8s-ci-robot assigned davidz627 Jan 10, 2020

k8s-ci-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Jan 10, 2020

rfranzke force-pushed the feature/run-as-controller branch from 55e14b8 to 94cdf36 Compare January 11, 2020 14:04

rfranzke force-pushed the feature/run-as-controller branch from 94cdf36 to 0660d8a Compare January 21, 2020 11:29

davidz627 reviewed Jan 21, 2020

View reviewed changes

cmd/main.go Outdated Show resolved Hide resolved

davidz627 reviewed Jan 21, 2020

View reviewed changes

docs/kubernetes/user-guides/driver-install.md Outdated Show resolved Hide resolved

pkg/gce-cloud-provider/compute/gce.go Outdated Show resolved Hide resolved

rfranzke force-pushed the feature/run-as-controller branch from 0660d8a to 8526aad Compare January 26, 2020 10:51

rfranzke changed the title ~~Allow running the controller service separately~~ Allow disabling particular driver services Jan 26, 2020

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jan 26, 2020

rfranzke requested a review from davidz627 January 26, 2020 10:52

davidz627 reviewed Jan 27, 2020

View reviewed changes

rfranzke force-pushed the feature/run-as-controller branch from 8526aad to 2c903e1 Compare February 11, 2020 16:46

davidz627 reviewed Feb 11, 2020

View reviewed changes

pkg/gce-pd-csi-driver/gce-pd-driver.go Outdated Show resolved Hide resolved

pkg/gce-cloud-provider/compute/gce-compute.go Outdated Show resolved Hide resolved

rfranzke added 2 commits February 11, 2020 20:32

Make controller server independent of GCE metadata service

4fcbde3

Allow disabling controller or node service

905c1d4

rfranzke force-pushed the feature/run-as-controller branch from 2c903e1 to 905c1d4 Compare February 11, 2020 19:49

davidz627 reviewed Feb 11, 2020

View reviewed changes

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

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 11, 2020

k8s-ci-robot merged commit 7552602 into kubernetes-sigs:master Feb 11, 2020

rfranzke deleted the feature/run-as-controller branch April 8, 2020 12:21

This was referenced Apr 9, 2020

Allow disabling controller/node service via command line flags kubernetes-sigs/azuredisk-csi-driver#354

Closed

[cinder-csi-plugin] Allow disabling particular driver services kubernetes/cloud-provider-openstack#1021

Closed

Jiawei0227 mentioned this pull request Feb 5, 2021

Add Flag to Driver for Controller or Node Mode #34

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow disabling particular driver services #449

Allow disabling particular driver services #449

rfranzke commented Jan 10, 2020 •

edited

Loading

k8s-ci-robot commented Jan 10, 2020

k8s-ci-robot commented Jan 10, 2020

rfranzke commented Jan 10, 2020

msau42 commented Jan 10, 2020

rfranzke commented Jan 11, 2020

rfranzke commented Jan 19, 2020

hantaowang commented Jan 21, 2020 •

edited

Loading

rfranzke commented Jan 21, 2020

davidz627 left a comment

rfranzke commented Jan 26, 2020

rfranzke commented Jan 26, 2020

davidz627 left a comment

rfranzke commented Feb 11, 2020 •

edited

Loading

davidz627 left a comment

rfranzke commented Feb 11, 2020

davidz627 left a comment

davidz627 Feb 11, 2020

davidz627 commented Feb 11, 2020

k8s-ci-robot commented Feb 11, 2020

Allow disabling particular driver services #449

Allow disabling particular driver services #449

Conversation

rfranzke commented Jan 10, 2020 • edited Loading

k8s-ci-robot commented Jan 10, 2020

k8s-ci-robot commented Jan 10, 2020

rfranzke commented Jan 10, 2020

msau42 commented Jan 10, 2020

rfranzke commented Jan 11, 2020

rfranzke commented Jan 19, 2020

hantaowang commented Jan 21, 2020 • edited Loading

rfranzke commented Jan 21, 2020

davidz627 left a comment

Choose a reason for hiding this comment

rfranzke commented Jan 26, 2020

rfranzke commented Jan 26, 2020

davidz627 left a comment

Choose a reason for hiding this comment

rfranzke commented Feb 11, 2020 • edited Loading

davidz627 left a comment

Choose a reason for hiding this comment

rfranzke commented Feb 11, 2020

davidz627 left a comment

Choose a reason for hiding this comment

davidz627 Feb 11, 2020

Choose a reason for hiding this comment

davidz627 commented Feb 11, 2020

k8s-ci-robot commented Feb 11, 2020

rfranzke commented Jan 10, 2020 •

edited

Loading

hantaowang commented Jan 21, 2020 •

edited

Loading

rfranzke commented Feb 11, 2020 •

edited

Loading