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

Move volume scheduling and local storage to beta #59391

Merged
merged 5 commits into from Feb 20, 2018

Conversation

@msau42
Copy link
Member

msau42 commented Feb 6, 2018

What this PR does / why we need it:

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #59390

Special notes for your reviewer:

Release note:

ACTION REQUIRED: VolumeScheduling and LocalPersistentVolume features are beta and enabled by default.  The PersistentVolume NodeAffinity alpha annotation is deprecated and will be removed in a future release.
@lichuqiang

This comment has been minimized.

Copy link
Member

lichuqiang commented Feb 6, 2018

/cc

@k8s-ci-robot k8s-ci-robot requested a review from lichuqiang Feb 6, 2018

@msau42 msau42 changed the title WIP: Move volume scheduling and local storage to beta Move volume scheduling and local storage to beta Feb 6, 2018

@k8s-merge-robot k8s-merge-robot removed the lgtm label Feb 17, 2018

@msau42

This comment has been minimized.

Copy link
Member Author

msau42 commented Feb 17, 2018

I rebased and also added a commit to update volume predicate invalidation for the new NodeAffinity field.

/assign @resouer @bsalamat
for scheduler review

@msau42 msau42 force-pushed the msau42:topology-beta branch from f3aecc7 to f5325be Feb 17, 2018

@msau42

This comment has been minimized.

Copy link
Member Author

msau42 commented Feb 17, 2018

Nevermind, ignore my last comment. There is validation that the PV.NodeAffinity field cannot be changed. So there is no need to update predicate invalidation for PV update. I removed the last commit.

@jsafrane

This comment has been minimized.

Copy link
Member

jsafrane commented Feb 19, 2018

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm label Feb 19, 2018

@thockin
Copy link
Member

thockin left a comment

Please clarify and handle the transitional case where both are set. The alpha should win.

// VolumeNodeAffinity defines constraints that limit what nodes this volume can be accessed from.
type VolumeNodeAffinity struct {
// Required specifies hard node constraints that must be met.
Required *NodeSelector

This comment has been minimized.

@thockin

thockin Feb 20, 2018

Member

In general, disallow field updates until/unless you know exactly what they are for.

@@ -1497,6 +1500,10 @@ func ValidatePersistentVolume(pv *core.PersistentVolume) field.ErrorList {
nodeAffinitySpecified, errs := validateStorageNodeAffinityAnnotation(pv.ObjectMeta.Annotations, metaPath.Child("annotations"))
allErrs = append(allErrs, errs...)

volumeNodeAffinitySpecified, errs := validateVolumeNodeAffinity(pv.Spec.NodeAffinity, specPath.Child("nodeAffinity"))

This comment has been minimized.

@thockin

thockin Feb 20, 2018

Member

What if both this field and the older annotation are set?

This comment has been minimized.

@msau42

msau42 Feb 20, 2018

Author Member

The current code will evaluate both if both are set, but it can be changed to have beta be ignored if alpha is set.

@@ -4952,6 +4963,27 @@ func validateStorageNodeAffinityAnnotation(annotations map[string]string, fldPat
return policySpecified, allErrs
}

// validateVolumeNodeAffinity tests that the PersistentVolume.NodeAffinity has valid data

This comment has been minimized.

@thockin

thockin Feb 20, 2018

Member

document the bool return

Disallow setting both alpha and beta PV nodeAffinity
Allow setting PV nodeAffinity if previously unset

@k8s-merge-robot k8s-merge-robot removed the lgtm label Feb 20, 2018

@thockin

This comment has been minimized.

Copy link
Member

thockin commented Feb 20, 2018

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm label Feb 20, 2018

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Feb 20, 2018

[APPROVALNOTIFIER] This PR is APPROVED

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

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

@k8s-merge-robot

This comment has been minimized.

Copy link
Contributor

k8s-merge-robot commented Feb 20, 2018

/test all [submit-queue is verifying that this PR is safe to merge]

@k8s-merge-robot

This comment has been minimized.

Copy link
Contributor

k8s-merge-robot commented Feb 20, 2018

Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions here.

@k8s-merge-robot k8s-merge-robot merged commit 6ba4696 into kubernetes:master Feb 20, 2018

12 of 13 checks passed

Submit Queue Required Github CI test is not green: pull-kubernetes-verify
Details
cla/linuxfoundation msau42 authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-cross Skipped
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-gke Skipped
pull-kubernetes-e2e-kops-aws Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce Job succeeded.
Details
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-unit Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details

@shyamjvs shyamjvs referenced this pull request Feb 28, 2018

Closed

[test flakes] master-scalability suites #60589

3 of 3 tasks complete

@NickrenREN NickrenREN referenced this pull request Mar 3, 2018

Closed

Using beta api: promote NodeAffinity #638

2 of 3 tasks complete
@ryanwalls

This comment has been minimized.

Copy link

ryanwalls commented Apr 13, 2018

Is there documentation for how to use persistent local storage? Can't seem to find anything.

@ryanwalls

This comment has been minimized.

Copy link

ryanwalls commented Apr 17, 2018

@mightwork FYI, get a 404 on that link.

@msau42

This comment has been minimized.

Copy link
Member Author

msau42 commented Apr 17, 2018

The official kubernetes documentation has a link to the external-storage walkthrough as well

@mightwork

This comment has been minimized.

Copy link

mightwork commented Apr 17, 2018

@ryanwalls my bad. Updated with the correct link.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment