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

Remove scheduler flags that were marked as deprecated 2+ releases ago. #34471

Merged

Conversation

timothysc
Copy link
Member

@timothysc timothysc commented Oct 10, 2016

Starting to spin back into scheduler testing and performance evaluation, and I noticed some leftover cleanup work from 2+ releases ago.

Release note: Removed unused bind-pods-qps, and bind-pods-burst flags from the scheduler.


This change is Reviewable

@timothysc timothysc added sig/scheduling Categorizes an issue or PR as relevant to SIG Scheduling. 0 - Triage labels Oct 10, 2016
@timothysc timothysc added this to the next-candidate milestone Oct 10, 2016
fs.MarkDeprecated("bind-pods-qps", "flag is unused and will be removed. Use kube-api-qps instead.")
var unusedBindPodsBurst int32
fs.Int32Var(&unusedBindPodsBurst, "bind-pods-burst", 0, "unused, use --kube-api-burst")
fs.MarkDeprecated("bind-pods-burst", "flag is unused and will be removed. Use kube-api-burst instead.")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@davidopp - what is our policy about deprecated flags? Do we promise anything about them?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FWIW they we're completely unused, but any-who I think we should probably have some formal deprecation policy.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah - especially that there's a number of deprecated flags throughout our binaries.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ping @davidopp @bgrant0607 - do we have/want to have formal deprecation policy? Tomorrow I'm going to assume that waiting two minor releases is good enough.

@k8s-github-robot k8s-github-robot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. release-note-label-needed labels Oct 10, 2016
@k8s-ci-robot
Copy link
Contributor

Jenkins GKE smoke e2e failed for commit b65af38. Full PR test history.

The magic incantation to run this job again is @k8s-bot gke e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

@gmarek gmarek added release-note-action-required Denotes a PR that introduces potentially breaking changes that require user action. and removed release-note-label-needed labels Oct 10, 2016
@gmarek
Copy link
Contributor

gmarek commented Oct 10, 2016

@timothysc please add release note.

@timothysc
Copy link
Member Author

please add release note.

Done.

@gmarek gmarek added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-action-required Denotes a PR that introduces potentially breaking changes that require user action. labels Oct 11, 2016
@bgrant0607 bgrant0607 added release-note-action-required Denotes a PR that introduces potentially breaking changes that require user action. and removed release-note Denotes a PR that will be considered when it comes time to generate release notes. labels Oct 11, 2016
@bgrant0607
Copy link
Member

@gmarek @timothysc This needs to go into the "action required" section. I changed the label appropriately.

We don't have an official deprecation policy for flags, but we do need one. Deprecated in 2 minor releases prior to removal seems reasonable.

We also need an official process. Marking flags as deprecated isn't sufficient. The initial deprecation also needs an action-required release note, though we might be able to get away without that in this case if we're fairly sure nobody is using them.

Once we move from flags to config, we'll use API versioning to manage component options.

FWIW, a Google search finds some mentions of these outside Kubernetes. Did anyone look at those?

Also, does this imply that #10377 is obsolete?

cc @roberthbailey @mikedanese re. binary flag/config deprecation policy

@timothysc
Copy link
Member Author

FWIW, a Google search finds some mentions of these outside Kubernetes. Did anyone look at those?

Looked, and not concerned.

Also, does this imply that #10377 is obsolete?

I think the separate throttles are pretty suspect, and am inclined to say yes, but I didn't write the original code. I'm re-digging on the scheduler this cycle and saw a number of tech-debt issues that need addressing.

@timothysc
Copy link
Member Author

@gmarek ?

@wojtek-t
Copy link
Member

/lgtm

@k8s-github-robot k8s-github-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 13, 2016
@gmarek
Copy link
Contributor

gmarek commented Oct 13, 2016

Sorry - it went off my radar.

@k8s-github-robot
Copy link

Automatic merge from submit-queue

@k8s-github-robot k8s-github-robot merged commit e0fc43b into kubernetes:master Oct 14, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note-action-required Denotes a PR that introduces potentially breaking changes that require user action. sig/scheduling Categorizes an issue or PR as relevant to SIG Scheduling. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants