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.

Sign up for GitHub

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

Jump to bottom

Remove flags low-diskspace-threshold-mb and outofdisk-transition-frequency #48846

Merged
merged 2 commits into from
Jul 25, 2017
Merged

Remove flags low-diskspace-threshold-mb and outofdisk-transition-frequency #48846

merged 2 commits into from
Jul 25, 2017

Conversation

dashpole
Copy link
Contributor

issue: #48843

This removes two flags replaced by the eviction manager. These have been depreciated for two releases, which I believe correctly follows the kubernetes depreciation guidelines.

Remove depreciated flags: --low-diskspace-threshold-mb and --outofdisk-transition-frequency, which are replaced by --eviction-hard

cc @mtaufen since I am changing kubelet flags
cc @vishh @derekwaynecarr
/sig node

@k8s-ci-robot k8s-ci-robot added sig/node Categorizes an issue or PR as relevant to SIG Node. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Jul 12, 2017
@k8s-github-robot k8s-github-robot added kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. release-note Denotes a PR that will be considered when it comes time to generate release notes. labels Jul 12, 2017
@mtaufen
Copy link
Contributor

mtaufen commented Jul 13, 2017

Yay kill the flags! 😈
You need to run hack/update-bazel.sh.

mtaufen
mtaufen reviewed Jul 13, 2017
View reviewed changes
Copy link
Contributor

@mtaufen mtaufen left a comment

Choose a reason for hiding this comment

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

A couple small comments.

pkg/kubelet/kubelet_node_status.go Outdated
@@ -56,6 +56,9 @@ const (
// maxNamesPerImageInNodeStatus is max number of names per image stored in
// the node status.
maxNamesPerImageInNodeStatus = 5

// mb is used to easily convert an int to an mb
mb = 1024 * 1024
Copy link
Contributor

Choose a reason for hiding this comment

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

There must be a helper for this somewhere, right? We do these sorts of calculations for memory limits, don't we?
Consider risk of overflow if you ever use ints for GB-size values with this, they can be 32 bits (unless you only ever divide by mb?).
It's a nit but differentiating between MB and Mb is also sometimes important.

Copy link
Contributor

Choose a reason for hiding this comment

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

Is this constant still used? I don't see it show up anywhere in this PR except for removed code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was used in a test file. But the numbers didnt actually matter, so I removed this.

Copy link
Contributor

Choose a reason for hiding this comment

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

SG

@dashpole
Copy link
Contributor Author

/retest

@mtaufen
Copy link
Contributor

mtaufen commented Jul 13, 2017

Still need to remove mb from pkg/kubelet/runonce_test.go

@dashpole dashpole force-pushed the remove_ood branch 2 times, most recently from 9cda7b5 to 04459f0 Compare July 13, 2017 19:17
@dashpole
Copy link
Contributor Author

/test pull-kubernetes-federation-e2e-gce

@pwittrock
Copy link
Member

/unassign

@mtaufen
Copy link
Contributor

mtaufen commented Jul 18, 2017

/lgtm

looks like you need to rebase but the bot didn't add the label :\

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 18, 2017
@k8s-github-robot k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 18, 2017
@k8s-github-robot k8s-github-robot removed lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Jul 20, 2017
@dashpole
Copy link
Contributor Author

/unassign @yifan-gu
/assign @vishh

@k8s-ci-robot k8s-ci-robot assigned vishh and unassigned yifan-gu Jul 20, 2017
@dashpole dashpole force-pushed the remove_ood branch 2 times, most recently from f8b62c8 to aced667 Compare July 20, 2017 17:51
@dashpole dashpole force-pushed the remove_ood branch 2 times, most recently from c640b7d to 5e65014 Compare July 20, 2017 22:24
@dashpole
Copy link
Contributor Author

/test pull-kubernetes-kubemark-e2e-gce

@mtaufen
Copy link
Contributor

mtaufen commented Jul 24, 2017 •
edited
Loading

/lgtm
post-rebase

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 24, 2017
@mtaufen
Copy link
Contributor

mtaufen commented Jul 24, 2017

/assign @smarterclayton
for approval

@smarterclayton
Copy link
Contributor

/approve

Deprecation was announced and communicated over several releases, consistent with other flags removed from the Kubelet.

@k8s-github-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dashpole, mtaufen, smarterclayton

Associated issue: 48843

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these OWNERS Files:

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

@k8s-github-robot k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 25, 2017
@k8s-github-robot
Copy link

Automatic merge from submit-queue

@k8s-github-robot k8s-github-robot merged commit 7f1d938 into kubernetes:master Jul 25, 2017
@rohitagarwal003 rohitagarwal003 mentioned this pull request Jul 29, 2017
Closed
This was referenced Jul 29, 2017
Merged
Closed
Closed
k8s-github-robot pushed a commit that referenced this pull request Aug 3, 2017
Merge pull request #49841 from dashpole/fix_gpu
f6e2eab 
Automatic merge from submit-queue (batch tested with PRs 49237, 49656, 49980, 49841, 49899)

[Bug Fix] Set NodeOODCondition to false

fixes #49839, which was introduced by #48846

This PR makes the kubelet set NodeOODCondition to false, so that the scheduler and other controllers do not consider the node to be unschedulable.

/assign @vishh 
/sig node
/release-note-none
@MrHohn MrHohn mentioned this pull request Aug 3, 2017
Closed
@dashpole dashpole mentioned this pull request Aug 3, 2017
Merged
@dashpole dashpole deleted the remove_ood branch January 15, 2019 20:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mtaufen mtaufen mtaufen left review comments

Assignees

@mtaufen mtaufen

@smarterclayton smarterclayton

@vishh vishh

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/node Categorizes an issue or PR as relevant to SIG Node. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@dashpole @mtaufen @pwittrock @smarterclayton @k8s-github-robot @yifan-gu @vishh @k8s-ci-robot