API change for volume topology aware dynamic provisioning #63233

lichuqiang · 2018-04-27T06:10:49Z

What this PR does / why we need it:

Split PR #63193 for better review
part 2: API change

Which issue(s) this PR fixes
Feature: kubernetes/enhancements#561
Design: kubernetes/community#2168

Special notes for your reviewer:
/sig storage
/sig scheduling
/assign @msau42 @jsafrane @thockin

Release note:

API change for volume topology aware dynamic provisioning

dims · 2018-04-27T23:41:11Z

pkg/apis/storage/util/util.go

@@ -27,4 +27,10 @@ func DropDisabledAlphaFields(class *storage.StorageClass) {
 	if !utilfeature.DefaultFeatureGate.Enabled(features.VolumeScheduling) {
 		class.VolumeBindingMode = nil
 	}
+	if !utilfeature.DefaultFeatureGate.Enabled(features.DynamicProvisioningScheduling) {
+		if class.Status != nil {


why are we not setting class.Status to nil?

dims · 2018-04-27T23:42:04Z

pkg/apis/storage/util/util_test.go

+	if class.AllowedTopologies != nil {
+		t.Errorf("AllowedTopology field didn't get dropped: %+v", class.AllowedTopologies)
+	}
+	if class.Status.Capacity != nil {


Shouldn't we check if class.Status is nil?

dims · 2018-04-27T23:51:13Z

pkg/printers/internalversion/describe.go

+}
+
+func printCapacityInStorageClass(w PrefixWriter, capacity *storage.StorageClassCapacity) {
+	w.Write(LEVEL_0, "Capacity:\t")


Is this the best practice? Capacity: <none> even when capacity is nil? (asking since this is just a alpha feature, it should not even show up if it is not set right?)

dims · 2018-04-27T23:54:33Z

pkg/registry/storage/storageclass/strategy.go

+	newClass.AllowVolumeExpansion = oldClass.AllowVolumeExpansion
+	newClass.VolumeBindingMode = oldClass.VolumeBindingMode
+	newClass.AllowedTopologies = oldClass.AllowedTopologies
+


We don't set Status right?

msau42 · 2018-04-27T23:58:08Z

@lichuqiang can you add WIP to the title since the api design is still being discussed?

lichuqiang · 2018-04-28T01:11:29Z

Sorry, it's my fault. @dims, as @msau42 implied, the API design is still under discussion, really appreciate if you can move to kubernetes/community#1857 for API design discussion together :)

dims · 2018-04-28T01:14:12Z

no worries. i just had some free time :)

lichuqiang · 2018-05-17T12:20:42Z

Updated according to latest update in proposal.
Remove the capacity reporting for now, and will implement the part in individual PRs

msau42 · 2018-06-04T16:53:57Z

/lgtm

lichuqiang · 2018-06-04T17:31:40Z

@thockin Can you take a look at the change?

thockin · 2018-06-04T21:06:17Z

pkg/apis/storage/types.go

+	// Restrict the node topologies where volumes can be dynamically provisioned.
+	// Each volume plugin defines its own supported topology specifications.
+	// This field is alpha-level and is only honored by servers that enable
+	// the DynamicProvisioningScheduling feature.


Document default behavior if empty

thockin · 2018-06-04T21:08:00Z

API LGTM. Small comment change requested. Will approve now, please fix before or after merge

thockin · 2018-06-04T21:08:13Z

/approve

lichuqiang · 2018-06-05T01:51:02Z

@thockin @msau42 Can you help add milestone and related labels to get it merged?

msau42 · 2018-06-05T02:28:04Z

/lgtm

k8s-ci-robot · 2018-06-05T02:28:11Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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:

~~api/OWNERS~~ [thockin]
~~docs/api-reference/OWNERS~~ [thockin]
~~pkg/apis/OWNERS~~ [thockin]
~~staging/src/k8s.io/api/OWNERS~~ [thockin]

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

msau42 · 2018-06-05T05:31:52Z

/priority important-soon

msau42 · 2018-06-05T05:32:47Z

/kind feature

k8s-github-robot · 2018-06-05T05:33:24Z

[MILESTONENOTIFIER] Milestone Pull Request: Up-to-date for process

@jsafrane @lichuqiang @msau42 @thockin

Pull Request Labels

sig/scheduling sig/storage: Pull Request will be escalated to these SIGs if needed.
priority/important-soon: Escalate to the pull request owners and SIG owner; move out of milestone after several unsuccessful escalation attempts.
kind/feature: New functionality.

Help

k8s-github-robot · 2018-06-05T07:02:57Z

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

k8s-github-robot · 2018-06-05T07:56:16Z

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

@msau42

Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>. Volume topology aware dynamic provisioning: work based on new API **What this PR does / why we need it**: The PR has been split to 3 parts: Part1: kubernetes#63232 for basic scheduler and PV controller plumbing Part2: kubernetes#63233 for API change and the PR itself includes work based on the API change: - Dynamic provisioning allowed topologies scheduler work - Update provisioning interface to be aware of selected node and topology **Which issue(s) this PR fixes** Feature: kubernetes/enhancements#561 Design: kubernetes/community#2168 **Special notes for your reviewer**: /sig storage /sig scheduling /assign @msau42 @jsafrane @saad-ali @bsalamat @kubernetes/sig-storage-pr-reviews @kubernetes/sig-scheduling-pr-reviews **Release note**: ```release-note Volume topology aware dynamic provisioning ```

k8s-ci-robot assigned jsafrane, msau42 and thockin Apr 27, 2018

k8s-ci-robot requested review from caesarxuchao, deads2k, dims, mbohlool, vishh, yifan-gu and zmerlynn April 27, 2018 06:11

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

lichuqiang mentioned this pull request Apr 27, 2018

Volume topology aware dynamic provisioning: basic plumbing #63232

Merged

k8s-github-robot added the kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API label Apr 27, 2018

lichuqiang mentioned this pull request Apr 27, 2018

Volume topology aware dynamic provisioning: work based on new API #63193

Merged

dims reviewed Apr 27, 2018

View reviewed changes

lichuqiang changed the title ~~API change for volume topology aware dynamic provisioning~~ WIP: API change for volume topology aware dynamic provisioning Apr 28, 2018

k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 28, 2018

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels May 17, 2018

k8s-ci-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Jun 2, 2018

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 4, 2018

thockin reviewed Jun 4, 2018

View reviewed changes

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 4, 2018

lichuqiang added 2 commits June 5, 2018 09:27

API changes for Topology aware dynamic provisioning

9a77899

generated files

2065439

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 5, 2018

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 5, 2018

saad-ali added the status/approved-for-milestone label Jun 5, 2018

saad-ali added this to the v1.11 milestone Jun 5, 2018

k8s-github-robot added the milestone/incomplete-labels label Jun 5, 2018

k8s-ci-robot added the priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. label Jun 5, 2018

k8s-ci-robot added the kind/feature Categorizes issue or PR as related to a new feature. label Jun 5, 2018

k8s-github-robot removed the milestone/incomplete-labels label Jun 5, 2018

k8s-github-robot merged commit 77d996b into kubernetes:master Jun 5, 2018

lichuqiang mentioned this pull request Jun 21, 2018

Should add volume topology-aware dynamic provisioning related integration case #65295

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

API change for volume topology aware dynamic provisioning #63233

API change for volume topology aware dynamic provisioning #63233

lichuqiang commented Apr 27, 2018 •

edited

dims Apr 27, 2018

dims Apr 27, 2018

dims Apr 27, 2018

dims Apr 27, 2018

msau42 commented Apr 27, 2018

lichuqiang commented Apr 28, 2018

dims commented Apr 28, 2018

lichuqiang commented May 17, 2018

msau42 commented Jun 4, 2018

lichuqiang commented Jun 4, 2018

thockin Jun 4, 2018

lichuqiang Jun 5, 2018

thockin commented Jun 4, 2018

thockin commented Jun 4, 2018

lichuqiang commented Jun 5, 2018

msau42 commented Jun 5, 2018

k8s-ci-robot commented Jun 5, 2018

msau42 commented Jun 5, 2018

msau42 commented Jun 5, 2018

k8s-github-robot commented Jun 5, 2018

k8s-github-robot commented Jun 5, 2018

k8s-github-robot commented Jun 5, 2018

API change for volume topology aware dynamic provisioning #63233

API change for volume topology aware dynamic provisioning #63233

Conversation

lichuqiang commented Apr 27, 2018 • edited

dims Apr 27, 2018

Choose a reason for hiding this comment

dims Apr 27, 2018

Choose a reason for hiding this comment

dims Apr 27, 2018

Choose a reason for hiding this comment

dims Apr 27, 2018

Choose a reason for hiding this comment

msau42 commented Apr 27, 2018

lichuqiang commented Apr 28, 2018

dims commented Apr 28, 2018

lichuqiang commented May 17, 2018

msau42 commented Jun 4, 2018

lichuqiang commented Jun 4, 2018

thockin Jun 4, 2018

Choose a reason for hiding this comment

lichuqiang Jun 5, 2018

Choose a reason for hiding this comment

thockin commented Jun 4, 2018

thockin commented Jun 4, 2018

lichuqiang commented Jun 5, 2018

msau42 commented Jun 5, 2018

k8s-ci-robot commented Jun 5, 2018

msau42 commented Jun 5, 2018

msau42 commented Jun 5, 2018

k8s-github-robot commented Jun 5, 2018

k8s-github-robot commented Jun 5, 2018

k8s-github-robot commented Jun 5, 2018

lichuqiang commented Apr 27, 2018 •

edited