Add a persistent volume label controller to the cloud-controller-manager #44680

rrati · 2017-04-19T19:23:33Z

Part of kubernetes/enhancements#88

Outstanding concerns needing input:

Why 5 threads for controller processing?
Remove direct linkage to aws/gce cloud providers [Remove linkage to cloud providers in persistent volume label controller #51629]
Modify shared informers to allow added event handlers ability to include uninitialized objects/using unshared informer Controllers keeps creating extra controllees if initialization is slow #48893
Use cache.MetaNamespaceKeyFunc in event handler?

I'm willing to work on addressing the removal of the direct linkage to aws/gce after this PR gets in.

k8s-reviewable · 2017-04-19T19:23:39Z

This change is

rrati · 2017-04-19T19:23:45Z

@wlan0

justinsb · 2017-04-19T20:27:42Z

So AFAICT the magic here is to set "Phase" to Pending.

Do we avoid doing anything with the volume while the Phase is Pending? I'm not familiar with it, and want to be sure there's not going to be a race.
It looks like the volume will be stuck in Pending if no labels are applied with AWS/GCE, or for all non AWS/GCE volumes?
If we're removing from apiserver, this makes running cloud-controller-manager mandatory in 1.7. Has that been decided?

wlan0 · 2017-04-19T21:23:39Z

So AFAICT the magic here is to set "Phase" to Pending.

What's the latest on the generic admission controller that you were working on. Honestly, that would satisfy this use-case better.

It looks like the volume will be stuck in Pending if no labels are applied with AWS/GCE, or for all non AWS/GCE volumes?

Yeah, that's not ok. The previous behavior was -

If the admission controller was activated, and the labels couldn't be obtained for EBS/PD, then PV creation failed.
In case of other PV types, the admission controller simply let the creation happen.

If we're removing from apiserver, this makes running cloud-controller-manager mandatory in 1.7. Has that been decided?

We are NOT making ccm mandatory in 1.7.

rrati · 2017-04-20T13:54:58Z

   It looks like the volume will be stuck in Pending if no labels are applied with AWS/GCE, or for all non AWS/GCE volumes?

Yeah, that's not ok. The previous behavior was -

If the admission controller was activated, and the labels couldn't be obtained for EBS/PD, then PV creation failed.
In case of other PV types, the admission controller simply let the creation happen.

I looked at conditionalizing the setting of the status, but it looks like the pv-controller sets the status to Available when performing a sync or when the pv is unbound. That should handle setting the pv to Available for non-cloud pv shouldn't it? If not I could set the status only for GCE or AWS volumes.

Taking this approach would allow us to completely deprecate the admission plugin in the future. When the ccm becomes mandatory there will really be no need for the admission plugin at all.

   If we're removing from apiserver, this makes running cloud-controller-manager mandatory in 1.7. Has that been decided?

We are NOT making ccm mandatory in 1.7.

Are you wanting to keep the admission plugin as is? That could cause complications.

ncdc · 2017-04-20T14:05:07Z

cc @sttts @liggitt @deads2k @derekwaynecarr @kubernetes/rh-cluster-infra @eparis

sttts · 2017-04-20T14:18:32Z

cmd/cloud-controller-manager/app/controllermanager.go

@@ -216,6 +217,11 @@ func StartControllers(s *options.CloudControllerManagerServer, kubeconfig *restc
 	nodeController.Run()
 	time.Sleep(wait.Jitter(s.ControllerStartInterval.Duration, ControllerStartJitter))

+	// Start thePersistentVolumeLabelController


the<space>

sttts · 2017-04-20T14:19:45Z

pkg/controller/persistentvolumelabel/pvlcontroller.go

+	"k8s.io/client-go/tools/cache"
+	"k8s.io/client-go/util/workqueue"
+
+	"k8s.io/kube-aggregator/pkg/controllers"


Do we allow this import from pkg/ to k8s.io/kube-aggregator? IMO, we shouldn't. /cc @deads2k

Moved to the controller import

sttts · 2017-04-20T14:20:22Z

pkg/controller/persistentvolumelabel/pvlcontroller.go

+	"k8s.io/kubernetes/pkg/cloudprovider"
+	"k8s.io/kubernetes/pkg/cloudprovider/providers/aws"
+	"k8s.io/kubernetes/pkg/cloudprovider/providers/gce"
+	vol "k8s.io/kubernetes/pkg/volume"


why the alias? volume is nice and short.

sttts · 2017-04-20T14:21:41Z

plugin/pkg/admission/persistentvolume/label/admission_test.go

@@ -15,154 +15,3 @@ limitations under the License.
 */

 package label


is this empty now? Then just delete it.

sttts · 2017-04-20T14:22:24Z

pkg/controller/persistentvolumelabel/pvlcontroller_test.go

+
+import (
+	"testing"
+


no empty line

sttts · 2017-04-20T14:23:31Z

pkg/controller/persistentvolumelabel/pvlcontroller.go

+limitations under the License.
+*/
+
+package cloud


are here any changes or is it a plain move? I think the only way to make this easier to review is to have a pure move commit and one to fix-up everything.

pvlcontroller is new. The meat of the work is the logic from the admission controller, but there have been some minor changes to work within a controller vs the admission plugin. The logic related to creating/starting the controller is all new

sttts · 2017-04-20T14:24:26Z

cmd/cloud-controller-manager/app/controllermanager.go

@@ -216,6 +217,11 @@ func StartControllers(s *options.CloudControllerManagerServer, kubeconfig *restc
 	nodeController.Run()
 	time.Sleep(wait.Jitter(s.ControllerStartInterval.Duration, ControllerStartJitter))

+	// Start thePersistentVolumeLabelController
+	pvlController := pvlcontroller.NewPersistentVolumeLabelController(sharedInformers.Core().V1().PersistentVolumes(), cloud)
+	go pvlController.Run(5, stop)


threadiness := 5 would make this clearer.

thockin · 2017-04-21T17:02:07Z

I will take a look ASAP

rrati · 2017-04-24T18:29:26Z

@thockin atm, this PR is incomplete because of the race condition with the pv-controller and setting the status of cloud pv. We need to resolve that before you spend time reviewing.

justinsb · 2017-06-01T05:00:44Z

pkg/controller/persistentvolumelabel/pvlcontroller.go

+		curVol.Labels = volume.Labels
+		curVol.Status.Phase = volume.Status.Phase
+
+		_, err = pvlc.kubeClient.Core().PersistentVolumes().Update(curVol)


Can we use a patch here? Avoids the version skew problems, and should be clearer.

Switched to using a patch

justinsb · 2017-06-01T05:03:20Z

pkg/controller/persistentvolumelabel/pvlcontroller.go

+		}
+		//	glog.Info("Pausing before setting volume available")
+		//	time.Sleep(30 * time.Second)
+		volume.Status.Phase = v1.VolumeAvailable


(I feel like I've done this review before, so forgive me if you've already explained this somewhere and just point me there!)

So it is possible that there are no labels, and I don't understand why we wouldn't then mark the volume as Available in that case also. But in general, what is the lifecycle of the volume phase here? It looks like we aren't solely responsible for updating the phase. And in that case, are we guaranteed that the volume won't be used before we label it?

justinsb · 2017-06-01T05:04:50Z

Never mind, looks like I probably shouldn't have reviewed this one. Sorry for noise.

rrati · 2017-07-06T18:23:27Z

@smarterclayton PTAL at the initializer use

rrati · 2017-08-30T20:11:49Z

/test pull-kubernetes-e2e-gce-gpu

caesarxuchao · 2017-08-30T20:27:58Z

pkg/controller/cloud/pvlcontroller.go

+	// Only add labels if in the list of initializers
+	if needsInitialization(vol.Initializers, initializerName) {
+		if vol.Spec.AWSElasticBlockStore != nil {
+			labels, err := pvlc.findAWSEBSLabels(vol)


@rrati How long is this call going to take? We might enforce a global timeout on all initializers.

@rrati I put the do-not-merge label on the PR for this question because it might affect the evolving of initializers.

This is the same logic that is in the admission plugin. It will be going away in a future revision of this controller. The call is very fast in my testing, but I haven't hit any instances where the API won't respond.

@caesarxuchao what happens if this initializer times out? Is the object considered uninitializable?

We perhaps will let the apiserver or garbage collector delete the object.

The call is very fast in my testing

How fast is the API call? Less than 1s? Could you do caching?

The API call itself is <<1s in my testing. However, that number could go up if the cloud provider is having issues, and I don't have any numbers for that. I never ran into a case where that occurred.

<<1s sounds good. Thanks.

deads2k · 2017-08-31T13:52:36Z

@rrati I put the do-not-merge label on the PR for this question because it might affect the evolving of initializers.

This is the same logic that is in the admission plugin. It will be going away in a future revision of this controller. The call is very fast in my testing, but I haven't hit any instances where the API won't respond.

@caesarxuchao I think the question has been answered. Looks to be fine. Even if it did take a while, this still appears to the structurally correct way of dealing with the problem.

smarterclayton · 2017-08-31T14:23:03Z

Looks ok as long as the "alpha initializers are disabled ootb" doesn't cause issues for this.

/approve

k8s-github-robot · 2017-08-31T14:23:51Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: deads2k, ironcladlou, rrati, smarterclayton

Associated issue: 88

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

Needs approval from an approver in each of these OWNERS Files:

~~cmd/cloud-controller-manager/OWNERS~~ [smarterclayton]
~~examples/OWNERS~~ [smarterclayton]
~~pkg/controller/cloud/OWNERS~~ [deads2k,smarterclayton]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

smarterclayton · 2017-08-31T14:24:09Z

re: timeout this is an excellent example of an initializer that should block progress on PVCs until it completes - i don't realistically think failure is an option for correct labelling.

rrati · 2017-08-31T15:32:17Z

@smarterclayton Can you approve the changes you requested?

wlan0 · 2017-08-31T15:48:12Z

Great to see this going through @rrati! Thanks for seeing this through.

@smarterclayton - the review is on an outdated commit, and the concerns have been addressed since. If everything looks good to you, please approve it.

deads2k · 2017-08-31T15:51:44Z

Great to see this going through @rrati! Thanks for seeing this through.

@smarterclayton - the review is on an outdated commit, and the concerns have been addressed since. If everything looks good to you, please approve it.

@smarterclayton Can you approve the changes you requested?

The PR is lgtm'ed, approved, and in queue.

paperwork

caesarxuchao · 2017-08-31T17:19:16Z

@smarterclayton @deads2k @rrati I was in a discussion on whether it's realistic to set a global initilaization timeout at 15s, and let the apiserver or a garbage collector delete objects that exceed the limit. I think timing out this initializer is ok, user should retry creating the PV, as long as the timeout only happens occasionally.

cheftako

Looks pretty good but I we need a plan to better break out the provider specific pieces.

cheftako · 2017-08-31T17:32:42Z

cmd/cloud-controller-manager/app/controllermanager.go

@@ -220,6 +220,12 @@ func StartControllers(s *options.CloudControllerManagerServer, kubeconfig *restc
 	nodeController.Run()
 	time.Sleep(wait.Jitter(s.ControllerStartInterval.Duration, ControllerStartJitter))

+	// Start the PersistentVolumeLabelController
+	pvlController := cloudcontrollers.NewPersistentVolumeLabelController(client("pvl-controller"), cloud)
+	threads := 5


Why 5? Would it be worth making this configurable?

cheftako · 2017-08-31T17:58:01Z

pkg/controller/cloud/pvlcontroller.go

+
+	// Only add labels if in the list of initializers
+	if needsInitialization(vol.Initializers, initializerName) {
+		if vol.Spec.AWSElasticBlockStore != nil {


Do we have an idea of how to get this block out of here and in to the AWS specific cloud provider section?
It seems as though we should add a GetVolumeLabel method to the cloud provider interface.

cheftako · 2017-08-31T17:59:38Z

pkg/controller/cloud/pvlcontroller.go

+			}
+			volumeLabels = labels
+		}
+		if vol.Spec.GCEPersistentDisk != nil {


Ditto can we work out how to move this into the gce provider section?

rrati · 2017-08-31T20:25:40Z

/test pull-kubernetes-bazel-test

cheftako

Looks good and thank you for doing this. We need to track moving the AWS/GCE specific code into the relevant provider section.

luxas · 2017-08-31T21:26:30Z

@cheftako can you please open an issue for tracking that?

…

On 01 Sep 2017, at 00:11, Walter Fender ***@***.***> wrote: @cheftako commented on this pull request. Looks good and thank you for doing this. We need to track moving the AWS/GCE specific code into the relevant provider section. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

rrati · 2017-09-01T02:13:24Z

There already is an issue for removing the cloud provider linkage #51629

k8s-github-robot · 2017-09-01T06:13:21Z

Automatic merge from submit-queue (batch tested with PRs 51574, 51534, 49257, 44680, 48836)

dims · 2017-09-08T11:11:14Z

related to #49402

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Apr 19, 2017

k8s-github-robot assigned derekwaynecarr and justinsb Apr 19, 2017

k8s-github-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. release-note-label-needed labels Apr 19, 2017

rrati force-pushed the pvl-controller branch from 8e4ba65 to 6097789 Compare April 19, 2017 19:33

sttts reviewed Apr 20, 2017

View reviewed changes

thockin self-assigned this Apr 21, 2017

rrati force-pushed the pvl-controller branch 2 times, most recently from 01a7a39 to 2d6195f Compare April 24, 2017 17:00

derekwaynecarr removed their assignment May 26, 2017

justinsb reviewed Jun 1, 2017

View reviewed changes

rrati force-pushed the pvl-controller branch from 2d6195f to 7a404ed Compare July 6, 2017 18:01

rrati force-pushed the pvl-controller branch from 7a404ed to 8fe0858 Compare July 6, 2017 19:29

caesarxuchao reviewed Aug 30, 2017

View reviewed changes

caesarxuchao added the do-not-merge DEPRECATED. Indicates that a PR should not merge. Label can only be manually applied/removed. label Aug 30, 2017

k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 31, 2017

smarterclayton removed the do-not-merge DEPRECATED. Indicates that a PR should not merge. Label can only be manually applied/removed. label Aug 31, 2017

cheftako reviewed Aug 31, 2017

View reviewed changes

cheftako mentioned this pull request Aug 31, 2017

Move AWS & GCE specific logic out of pkg/controller/cloud/pvlcontroller.go #51743

Closed

k8s-github-robot merged commit 43a40f7 into kubernetes:master Sep 1, 2017

wlan0 mentioned this pull request Sep 3, 2017

Support out-of-process and out-of-tree cloud providers kubernetes/enhancements#88

Closed

luxas mentioned this pull request Sep 15, 2017

Stop using the PersistentVolumeLabel admission controller in v1.9 kubernetes/kubeadm#444

Closed

dims mentioned this pull request Sep 17, 2017

Remove the PersistentVolumeLabel Admission Controller #52617

Closed

2 tasks

dims mentioned this pull request Oct 16, 2017

Deprecate cloud-provider options in kube-apiserver #53912

Closed

superseb mentioned this pull request Nov 18, 2017

Check PersistentVolumeLabel admission controller status for kube-apiserver rancher/rancher#10367

Closed

andrewsykim mentioned this pull request Feb 25, 2019

Investigate alternatives for out-of-tree PersistentVolume labelling kubernetes/cloud-provider#4

Closed

rrati mentioned this pull request Jul 10, 2020

REQUEST: New membership for rrati kubernetes/org#2016

Closed

6 tasks

		@@ -15,154 +15,3 @@ limitations under the License.
		*/

		package label

Add a persistent volume label controller to the cloud-controller-manager #44680

Add a persistent volume label controller to the cloud-controller-manager #44680

Conversation

rrati commented Apr 19, 2017 • edited Loading

k8s-reviewable commented Apr 19, 2017

rrati commented Apr 19, 2017

justinsb commented Apr 19, 2017

wlan0 commented Apr 19, 2017 • edited Loading

rrati commented Apr 20, 2017

ncdc commented Apr 20, 2017

sttts Apr 20, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thockin commented Apr 21, 2017

rrati commented Apr 24, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

justinsb commented Jun 1, 2017

rrati commented Jul 6, 2017

rrati commented Aug 30, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

deads2k commented Aug 31, 2017

smarterclayton commented Aug 31, 2017

k8s-github-robot commented Aug 31, 2017

smarterclayton commented Aug 31, 2017

rrati commented Aug 31, 2017

wlan0 commented Aug 31, 2017

deads2k commented Aug 31, 2017

caesarxuchao commented Aug 31, 2017

cheftako left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rrati commented Aug 31, 2017

cheftako left a comment

Choose a reason for hiding this comment

luxas commented Aug 31, 2017 via email

rrati commented Sep 1, 2017

k8s-github-robot commented Sep 1, 2017

dims commented Sep 8, 2017

rrati commented Apr 19, 2017 •

edited

Loading

wlan0 commented Apr 19, 2017 •

edited

Loading

sttts Apr 20, 2017 •

edited

Loading