Prioritizing nodes based on volume capacity: API changes #99594

cofyc · 2021-03-01T15:07:04Z

What type of PR is this?

/kind api-change

What this PR does / why we need it:

Based on #96347. This adds api changes.

#96347 must be merged first.

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?

NONE

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

[KEP]: https://github.com/kubernetes/enhancements/tree/master/keps/sig-storage/1845-prioritization-on-volume-capacity

k8s-ci-robot · 2021-03-01T15:07:12Z

@cofyc: This issue is currently awaiting triage.

If a SIG or subproject determines this is a relevant issue, they will accept it by applying the triage/accepted label and provide further guidance.

The triage/accepted label can be added by org members by writing /triage accepted in a comment.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

cofyc · 2021-03-03T03:12:30Z

/retest

ahg-g · 2021-06-24T13:04:25Z

you need to update the VolumeBinding defaults in the following testing file for some tests to pass:

kubernetes/pkg/scheduler/apis/config/testing/defaults/defaults.go

Line 222 in 48b7492

Name: "VolumeBinding",

I guess you didn't need this because the feature is still disabled.

ahg-g · 2021-06-24T13:05:40Z

/approve

this is good from the scheduler's POV.

msau42 · 2021-06-24T22:52:52Z

pkg/scheduler/apis/config/types_pluginargs.go

@@ -211,6 +211,11 @@ type VolumeBindingArgs struct {
 	// Value must be non-negative integer. The value zero indicates no waiting.
 	// If this value is nil, the default value will be used.
 	BindTimeoutSeconds int64
+
+	// Shape specifies the points defining the score function shape.


Can you add more description to this field? What does each point represent and how does it impact scoring?

It would also be good to indicate that this field requires the alpha VolumeCapacityPriority feature gate to be enabled.

It would also be good to indicate that this field requires the alpha VolumeCapacityPriority feature gate to be enabled.

Is the +featureGate=VolumeCapacityPriority for this purpose?

Can you add more description to this field? What does each point represent and how does it impact scoring?

tried my best, but I'm not good at this, suggestions are welcome!

Thanks, this is helpful.

Can you also describe how volume capacity maps to utilization? Is it based on #pvs, total capacity available to the node?

Can you also describe how volume capacity maps to utilization? Is it based on #pvs, total capacity available to the node?

done in 5d9baa9

the existing bound PVs on the node are excluded as they cannot be shared by other pods like cpu/memory.

msau42 · 2021-06-24T22:54:44Z

pkg/scheduler/apis/config/v1beta1/defaults.go

@@ -273,6 +273,18 @@ func SetDefaults_VolumeBindingArgs(obj *v1beta1.VolumeBindingArgs) {
 	if obj.BindTimeoutSeconds == nil {
 		obj.BindTimeoutSeconds = pointer.Int64Ptr(600)
 	}
+	if len(obj.Shape) == 0 && feature.DefaultFeatureGate.Enabled(features.VolumeCapacityPriority) {


Can we add unit tests for this?

msau42 · 2021-06-24T22:57:03Z

pkg/scheduler/apis/config/validation/validation_pluginargs.go

 	}

-	return err
+	if utilfeature.DefaultFeatureGate.Enabled(features.VolumeCapacityPriority) {
+		allErrs = append(allErrs, validateFunctionShape(args.Shape, path.Child("shape"))...)


Should we validate the argument even if the feature gate is off? Otherwise someone could pass an invalid argument and we ignore it, and then in the future when we turn on the feature, their config no longer works.

@liggitt we don't have "field dropping" for config APIs right? In lieu of that, I think validating the field in all cases and ignoring it at runtime is the best approach.

Another approach of returning an error if the feature is disabled will break rollback/disable scenarios.

In lieu of that, I think validating the field in all cases and ignoring it at runtime is the best approach.

+1 and implemented

I'd go a step further and forbid values in this config field if the feature is off... otherwise someone can specify a shape that is syntactically valid but does not behave at all like they want, successfully run the scheduler with this feature off, and then break themselves with the same config when the feature turns on in the future.

If we return an error, then if someone has the feature on and sets the config, but then rolls back/disables the feature, then they would have to update their config at that time too otherwise the config will fail validation. Is that the expected approach for config apis?

hmm... I don't love either direction, but in the rollback case, since they're already modifying the invocation, I think making them comment out the config for the disabled feature is better than letting people put timebomb inert configuration in the file and then activating it on upgrade when the feature becomes active

it seems a safer choice, done in 3e788a3

msau42 · 2021-06-24T23:07:24Z

pkg/scheduler/apis/config/v1beta1/defaults.go

@@ -273,6 +273,18 @@ func SetDefaults_VolumeBindingArgs(obj *v1beta1.VolumeBindingArgs) {
 	if obj.BindTimeoutSeconds == nil {
 		obj.BindTimeoutSeconds = pointer.Int64Ptr(600)
 	}


I see there is a v1 type. Do we need to add defaults there? I notice there are no defaults at all for v1. cc @ahg-g

the v1 is not for component config, it is for the old policy api, which we will be deprecating in the next release.

msau42 · 2021-06-29T16:18:36Z

api review
/lgtm

liggitt · 2021-06-29T18:53:48Z

one comment about forbidding this config value in validation if the feature is off, then lgtm

liggitt · 2021-06-30T14:37:23Z

/approve
for API changes

/hold for final scheduler lgtm and approval

ahg-g · 2021-06-30T18:57:50Z

can you please squash the commits.

cofyc · 2021-07-01T02:01:45Z

can you please squash the commits.

done

ahg-g · 2021-07-01T02:48:17Z

/lgtm
/approve

k8s-ci-robot · 2021-07-01T02:48:38Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ahg-g, cofyc, 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:

~~pkg/scheduler/OWNERS~~ [ahg-g,liggitt]
~~staging/src/k8s.io/kube-scheduler/config/OWNERS~~ [liggitt]

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

cofyc · 2021-07-01T06:10:03Z

/retest

ahg-g · 2021-07-01T14:47:09Z

/hold cancel

k8s-ci-robot · 2021-07-01T17:23:48Z

@cofyc: The following test failed, say /retest to rerun all failed tests:

Test name	Commit	Details	Rerun command
pull-kubernetes-bazel-test	61592a7c8fd48dd3675240d87f59bc6a86ff759c	link	`/test pull-kubernetes-bazel-test`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

fejta-bot · 2021-07-01T20:07:28Z

/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.

cofyc changed the title ~~Prioritizing nodes based on volume capacity: API changes~~ WIP: Prioritizing nodes based on volume capacity: API changes Mar 1, 2021

k8s-ci-robot added do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Mar 1, 2021

k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Mar 1, 2021

k8s-ci-robot requested review from ahg-g and damemi March 1, 2021 15:07

cofyc mentioned this pull request Mar 2, 2021

Prioritizing nodes based on volume capacity kubernetes/enhancements#1845

Open

reylejano mentioned this pull request Mar 2, 2021

KEP-1845: Prioritizing nodes based on volume capacity kubernetes/website#26712

Merged

cofyc mentioned this pull request Mar 2, 2021

Prioritizing nodes based on volume capacity #96347

Merged

cofyc force-pushed the kep1845-api branch from 61592a7 to 5feb2d5 Compare March 2, 2021 06:22

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 2, 2021

cofyc force-pushed the kep1845-api branch 4 times, most recently from 3df62be to bedf08a Compare March 2, 2021 10:49

cofyc force-pushed the kep1845-api branch from bedf08a to 383db9f Compare March 3, 2021 08:51

msau42 reviewed Jun 24, 2021

View reviewed changes

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

cofyc force-pushed the kep1845-api branch from 5d9baa9 to bfe1e72 Compare June 30, 2021 02:52

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

k8s-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Jun 30, 2021

Prioritizing nodes based on volume capacity: API changes

b522e95

cofyc force-pushed the kep1845-api branch from 03b4f36 to b522e95 Compare July 1, 2021 02:01

k8s-ci-robot assigned ahg-g Jul 1, 2021

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 1, 2021

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 1, 2021

k8s-ci-robot merged commit 25bbe2e into kubernetes:master Jul 1, 2021

k8s-ci-robot added this to the v1.22 milestone Jul 1, 2021

cofyc deleted the kep1845-api branch July 2, 2021 08:51

BenTheElder mentioned this pull request Jul 2, 2021

[Failing Test][sig-release] ci-kubernetes-e2e-gci-gce-alpha-features #103440

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Prioritizing nodes based on volume capacity: API changes #99594

Prioritizing nodes based on volume capacity: API changes #99594

cofyc commented Mar 1, 2021

k8s-ci-robot commented Mar 1, 2021

cofyc commented Mar 3, 2021

ahg-g commented Jun 24, 2021

ahg-g commented Jun 24, 2021

msau42 Jun 24, 2021

cofyc Jun 25, 2021 •

edited

Loading

cofyc Jun 25, 2021

msau42 Jun 25, 2021

cofyc Jun 29, 2021

msau42 Jun 24, 2021

cofyc Jun 25, 2021

msau42 Jun 24, 2021

msau42 Jun 24, 2021

cofyc Jun 25, 2021 •

edited

Loading

liggitt Jun 29, 2021

msau42 Jun 30, 2021

liggitt Jun 30, 2021

cofyc Jun 30, 2021

msau42 Jun 24, 2021

ahg-g Jun 24, 2021

msau42 commented Jun 29, 2021

liggitt commented Jun 29, 2021

liggitt commented Jun 30, 2021

ahg-g commented Jun 30, 2021

cofyc commented Jul 1, 2021

ahg-g commented Jul 1, 2021

k8s-ci-robot commented Jul 1, 2021

cofyc commented Jul 1, 2021

ahg-g commented Jul 1, 2021

k8s-ci-robot commented Jul 1, 2021 •

edited

Loading

fejta-bot commented Jul 1, 2021

Prioritizing nodes based on volume capacity: API changes #99594

Prioritizing nodes based on volume capacity: API changes #99594

Conversation

cofyc commented Mar 1, 2021

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

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

k8s-ci-robot commented Mar 1, 2021

cofyc commented Mar 3, 2021

ahg-g commented Jun 24, 2021

ahg-g commented Jun 24, 2021

Choose a reason for hiding this comment

cofyc Jun 25, 2021 • 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

cofyc Jun 25, 2021 • 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

msau42 commented Jun 29, 2021

liggitt commented Jun 29, 2021

liggitt commented Jun 30, 2021

ahg-g commented Jun 30, 2021

cofyc commented Jul 1, 2021

ahg-g commented Jul 1, 2021

k8s-ci-robot commented Jul 1, 2021

cofyc commented Jul 1, 2021

ahg-g commented Jul 1, 2021

k8s-ci-robot commented Jul 1, 2021 • edited Loading

fejta-bot commented Jul 1, 2021

cofyc Jun 25, 2021 •

edited

Loading

cofyc Jun 25, 2021 •

edited

Loading

k8s-ci-robot commented Jul 1, 2021 •

edited

Loading