Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Delete the out-of-tree PV labeler controller #74615

Merged
merged 1 commit into from Mar 5, 2019

Conversation

@andrewsykim
Copy link
Member

andrewsykim commented Feb 26, 2019

What type of PR is this?
/kind cleanup

What this PR does / why we need it:
Removes the PersistentVolume labeler controller. This is a controller that runs as part of the CCM (i.e. meant for out-of-tree providers). With Initializer support removed for v1.14, this controller is now effectively useless because it would only check for PVs with initializers enabled.

Some justifications for this change:

  • we could not find any provider that relied on this controller (because they never created the InitializerConfiguration resource for PVs). For the few out-of-tree providers that have reported usage of this feature, we added corresponding volumes to the in-tree admission controller (vSphere #72687 & Cinder #73102)
  • This controller also has some serious bugs and lacked feature parity with the in-tree PersistentVolumeLabel admission controller. The most notable is the fact that the out-of-tree PVL controller would apply failure domain labels to all PVs as long as the cloud provider implemented GetLabelsForVolume. This is a wrong assumption (at least in comparison to the in-tree admission controller) and could result in a segfault.
  • Longer term, we want to only be using PV.NodeAffinity rather than labels for zone topology scheduling. SIG Cloud Provider & SIG Storage will be working on a longer-term out-of-tree solution for this in v1.15.

Which issue(s) this PR fixes:
Part of kubernetes/cloud-provider#4

Special notes for your reviewer:
More discussions around the intentions of this PR in kubernetes/cloud-provider#4

SIG cloud provider mailing list thread: https://groups.google.com/forum/#!topic/kubernetes-sig-cloud-provider/jZUB-Qk4S-M

Does this PR introduce a user-facing change?:

Remove the out-of-tree PersistentVolumeLabel controller because it cannot run without Initializers (removed in v1.14). If you are using AWS EBS, GCE PD, Azure Disk, Cinder Disk or vSphere volumes and rely on zone labels, then enable the `PersistentVolumeLabel` admission controller in the `kube-apiserver` in the `--enable-admission-plugins` flag. 
@andrewsykim

This comment has been minimized.

Copy link
Member Author

andrewsykim commented Feb 26, 2019

/sig storage
/sig cloudprovider

@andrewsykim

This comment has been minimized.

Copy link
Member Author

andrewsykim commented Feb 26, 2019

/area cloudprovider
/priorty important-soon

@andrewsykim

This comment has been minimized.

Copy link
Member Author

andrewsykim commented Feb 26, 2019

/priority important-soon

@andrewsykim andrewsykim force-pushed the andrewsykim:delete-pvl-controller branch from c7e843c to 2901def Feb 26, 2019
@andrewsykim

This comment has been minimized.

Copy link
Member Author

andrewsykim commented Feb 26, 2019

@dims

This comment has been minimized.

Copy link
Member

dims commented Feb 26, 2019

/approve
/lgtm

(cross my fingers about the CI jobs!)

@k8s-ci-robot k8s-ci-robot added the lgtm label Feb 26, 2019
@andrewsykim

This comment has been minimized.

Copy link
Member Author

andrewsykim commented Feb 27, 2019

/milestone v1.14

@k8s-ci-robot k8s-ci-robot added this to the v1.14 milestone Feb 27, 2019
@roycaihw

This comment has been minimized.

Copy link
Member

roycaihw commented Feb 28, 2019

/assign @cheftako

@liggitt

This comment has been minimized.

Copy link
Member

liggitt commented Mar 4, 2019

/lgtm
/approve

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Mar 4, 2019

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: andrewsykim, dims, liggitt

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:

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

@fejta-bot

This comment has been minimized.

Copy link

fejta-bot commented Mar 5, 2019

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

1 similar comment
@fejta-bot

This comment has been minimized.

Copy link

fejta-bot commented Mar 5, 2019

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

@k8s-ci-robot k8s-ci-robot merged commit dccb8ab into kubernetes:master Mar 5, 2019
17 checks passed
17 checks passed
cla/linuxfoundation andrewsykim authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-conformance-image-test Skipped.
pull-kubernetes-cross Skipped.
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-100-performance Job succeeded.
Details
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-godeps Skipped.
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce-big Job succeeded.
Details
pull-kubernetes-local-e2e Skipped.
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details
pull-publishing-bot-validate Skipped.
tide In merge pool.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.