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

Do not query the cloud if dynamic PV has all the labels #82830

Merged
merged 1 commit into from Sep 20, 2019

Conversation

@jsafrane
Copy link
Member

jsafrane commented Sep 18, 2019

This saves one cloud API call in admission plugin. As result, API server latency can go down significantly when creating many PVs and cloud API starts throttling requests.

/kind bug
Fixes #82826

Special notes for your reviewer:
See the release note, it slightly changes behavior of the admission plugin,

PersistentVolumeLabel admission plugin, responsible for labeling `PersistentVolumes` with topology labels, now does not overwrite existing labels on PVs that were dynamically provisioned. It trusts the  dynamic provisioning that it provided the correct labels to the `PersistentVolume`, saving one potentially expensive cloud API call. `PersistentVolumes` created manually by users are labelled by the admission plugin in the same way as before.

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:


This saves one cloud API call.
@gnufied

This comment has been minimized.

Copy link
Member

gnufied commented Sep 18, 2019

/kind bug
/priority important-soon

@gnufied

This comment has been minimized.

Copy link
Member

gnufied commented Sep 18, 2019

/lgtm

@gnufied

This comment has been minimized.

Copy link
Member

gnufied commented Sep 18, 2019

@k8s-ci-robot k8s-ci-robot added sig/storage and removed needs-sig labels Sep 18, 2019
@jsafrane

This comment has been minimized.

Copy link
Member Author

jsafrane commented Sep 19, 2019

/assign @msau42
for approval

// All cloud providers set only these two labels.
domain, domainOK := existingLabels[v1.LabelZoneFailureDomain]
region, regionOK := existingLabels[v1.LabelZoneRegion]
if domainOK && regionOK {

This comment has been minimized.

Copy link
@andrewsykim

andrewsykim Sep 19, 2019

Member

This is a somewhat significant behavior change. Currently we overwrote the PV labels, and that was ok because it's a kubernetes.io namespaced label (i.e. we own it). Now we are respecting what users set previously which leaves room for errors (e.g. setting the wrong zone) but maybe that's worth the trade-off?

@dims

This comment has been minimized.

Copy link
Member

dims commented Sep 20, 2019

/uncc

@k8s-ci-robot k8s-ci-robot removed the request for review from dims Sep 20, 2019
@deads2k

This comment has been minimized.

Copy link
Contributor

deads2k commented Sep 20, 2019

This is a somewhat significant behavior change. Currently we overwrote the PV labels, and that was ok because it's a kubernetes.io namespaced label (i.e. we own it). Now we are respecting what users set previously which leaves room for errors (e.g. setting the wrong zone) but maybe that's worth the trade-off?

I think this is a good tradeoff to make. If the user has chosen to take ownership of these fields, it is their responsibility to set them correctly and they may actually be surprised that we force changed these. If a user isn't certain, then they shouldn't set them. I think we have some flexibility in finding the correct balance between making a user-friendly API and one that is performant.

@jsafrane jsafrane force-pushed the jsafrane:pv-admission-fix branch from d854a7b to f08dcdc Sep 20, 2019
@k8s-ci-robot k8s-ci-robot removed the lgtm label Sep 20, 2019
@jsafrane

This comment has been minimized.

Copy link
Member Author

jsafrane commented Sep 20, 2019

Added extra heuristics for dynamically provisioned PV vs user provisioned PV: the dynamically provisioned ones have annotation pv.kubernetes.io/provisioned-by.

So now it breaks only PVs created by user that have the annotation dedicated for dynamic provisioning.

v1.LabelZoneRegion: "existingRegion",
},
Annotations: map[string]string{
persistentvolume.AnnDynamicallyProvisioned: "true",

This comment has been minimized.

Copy link
@andrewsykim

andrewsykim Sep 20, 2019

Member

In the admission controller we only check existence, is that true for other consumers of this annotation? Asking cause the value is "true" and was wondering if that was arbitrary or intentional.

This comment has been minimized.

Copy link
@jsafrane

jsafrane Sep 20, 2019

Author Member
  • It is used by PV controller
  • And any nozero value is valid here (technically, it's a name of provisioner). Maybe I should have chosen a better value

This comment has been minimized.

Copy link
@jsafrane

jsafrane Sep 20, 2019

Author Member

changed to kubernetes.io/aws-ebs, thanks @msau42 for holding the PR

@msau42

This comment has been minimized.

Copy link
Member

msau42 commented Sep 20, 2019

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm label Sep 20, 2019
@msau42

This comment has been minimized.

Copy link
Member

msau42 commented Sep 20, 2019

/hold
to address comments

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Sep 20, 2019

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jsafrane, msau42

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

@jsafrane jsafrane force-pushed the jsafrane:pv-admission-fix branch from 3eb6900 to a160bf8 Sep 20, 2019
@k8s-ci-robot k8s-ci-robot removed the lgtm label Sep 20, 2019
@andrewsykim

This comment has been minimized.

Copy link
Member

andrewsykim commented Sep 20, 2019

/lgtm

Copy link
Member

bertinatto left a comment

/lgtm
as well

@andrewsykim

This comment has been minimized.

Copy link
Member

andrewsykim commented Sep 20, 2019

/retitle Do not query the cloud if dynamic PV has all the labels

@k8s-ci-robot k8s-ci-robot changed the title Do not query the cloud if PV has all the labels Do not query the cloud if dynamic PV has all the labels Sep 20, 2019
@msau42

This comment has been minimized.

Copy link
Member

msau42 commented Sep 20, 2019

/hold cancel

@k8s-ci-robot k8s-ci-robot merged commit ac8ac0f into kubernetes:master Sep 20, 2019
25 checks passed
25 checks passed
cla/linuxfoundation jsafrane authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-conformance-image-test Skipped.
pull-kubernetes-conformance-kind-ipv6 Skipped.
pull-kubernetes-cross Skipped.
pull-kubernetes-dependencies Job succeeded.
Details
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-100-performance Job succeeded.
Details
pull-kubernetes-e2e-gce-alpha-features Skipped.
pull-kubernetes-e2e-gce-csi-serial Skipped.
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-gce-iscsi Skipped.
pull-kubernetes-e2e-gce-iscsi-serial Skipped.
pull-kubernetes-e2e-gce-storage-slow Skipped.
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-node-e2e-containerd 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
@k8s-ci-robot k8s-ci-robot added this to the v1.17 milestone Sep 20, 2019
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.