-
Notifications
You must be signed in to change notification settings - Fork 38.9k
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
Remove KubeSchedulerConfiguration.BindTimeoutSeconds #91580
Conversation
/sig scheduling |
/assign @alculquicondor |
Please link the KEP in the documentation section of the PR description https://github.com/kubernetes/enhancements/tree/master/keps/sig-scheduling/785-scheduler-component-config-api |
@@ -289,7 +287,6 @@ priorities: [] | |||
}, | |||
}, | |||
}), | |||
scheduler.WithBindTimeoutSeconds(defaultBindTimeout), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In theory, we should be replacing this with scheduler.WithProfiles
where you would pass a PluginConfig. In reality, these are integration tests. Do we have integration tests around volume binding that we have faked to run fast?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's strange to configure the timeout here because it verifies policy config map only
anyway, I've updated the PR to use scheduler.WithProfiles
here not to change the logic
Do we have integration tests around volume binding that we have faked to run fast?
pods not using PVCs are ignored by VolumeBinding plugin and always run fast
volume-related integration tests reside in test/integration/volumescheduling
pkg, but we didn't use customized volume binding timeout. maybe we can add some negative tests in the future
This PR may require API review. If so, when the changes are ready, complete the pre-review checklist and request an API review. Status of requested reviews is tracked in the API Review project. |
@@ -391,7 +391,6 @@ func InitTestSchedulerWithOptions( | |||
if policy != nil { | |||
opts = append(opts, scheduler.WithAlgorithmSource(CreateAlgorithmSourceFromPolicy(policy, testCtx.ClientSet))) | |||
} | |||
opts = append([]scheduler.Option{scheduler.WithBindTimeoutSeconds(600)}, opts...) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same as default, not necessary
updated |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
/hold
for the nit, but feel free to unhold
pkg/scheduler/scheduler_test.go
Outdated
} | ||
|
||
func TestRemoveNominatedNodeName(t *testing.T) { | ||
func TestSetNominatedNodeName(t *testing.T) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why change this line? or is this just coming from rebase?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
agree, keep this line unchanged
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh, it's a mistake made in resolving conflicts
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
I would suggest accumulating the docs in a WIP website PR, and point the release notes in the PR that introduced the v1beta1 config API to the location of that doc on http://kubernetes.io/ |
Done, the path should be kubernetes.io/docs/reference/scheduling/config#what-is-new kubernetes/website#21437 |
IIUC, we should point the release notes in the PR that introduced the v1beta1 config API to release notes in this PR removed |
/retest |
/hold |
@alculquicondor @cofyc : Thanks ! |
Thanks @puerco |
For now, let's leave a short release note here. |
Done. I guess |
/retest |
Good point. I've requested an update to the PR author. |
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: alculquicondor, 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:
Approvers can indicate their approval by writing |
What type of PR is this?
/kind api-change
What this PR does / why we need it:
This PR removes
KubeSchedulerConfiguration.BindTimeoutSeconds
from v1beta1 (as for k8s 1.19 release, it's gone with v1alpha2 version). Starting from v1beta1, users can configure it via VolumeBinding plugin args.Which issue(s) this PR fixes:
Fixes #90958
Special notes for your reviewer:
the first commit contains auto-generated files only
Does this PR introduce a user-facing change?:
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: