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

kubelet: change image-gc-high-threshold below docker dm.min_free_space #40432

Merged
merged 1 commit into from
Apr 3, 2017

Conversation

sjenning
Copy link
Contributor

docker dm.min_free_space defaults to 10%, which "specifies the min free space percent in a thin pool require for new device creation to succeed....Whenever a new a thin pool device is created (during docker pull or during container creation), the Engine checks if the minimum free space is available. If sufficient space is unavailable, then device creation fails and any relevant docker operation fails." [1]

This setting is preventing the storage usage to cross the 90% limit. However, image GC is expected to kick in only beyond image-gc-high-threshold. The image-gc-high-threshold has a default value of 90%, and hence GC never triggers. If image-gc-high-threshold is set to a value lower than (100 - dm.min_free_space)%, GC triggers.

xref https://bugzilla.redhat.com/show_bug.cgi?id=1408309

changed kubelet default image-gc-high-threshold to 85% to resolve a conflict with default settings in docker that prevented image garbage collection from resolving low disk space situations when using devicemapper storage.

@derekwaynecarr @sdodson @rhvgoyal

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jan 25, 2017
@k8s-reviewable
Copy link

This change is Reviewable

@k8s-github-robot k8s-github-robot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. release-note Denotes a PR that will be considered when it comes time to generate release notes. labels Jan 25, 2017
@dchen1107
Copy link
Member

The change makes sense to me, but @dashpole, could you please take another look at this?

@dashpole
Copy link
Contributor

@sjenning do you think this is the cause of this issue?. I noticed many people were using devicemapper, and some were wondering why image garbage collection hadn't kicked in.

The change makes sense to me as well.

Would it also make sense to decrease the ImageGCLowThresholdPercent?

I am not actually sure what kubemark is, but I found a default for ImageGCHighThresholdPercent here as well. Should we change that value to match?

@dchen1107
Copy link
Member

@dashpole Yes, that is what I suspected and looped you here since you looked at #32542 lately.

@sjenning
Copy link
Contributor Author

@dashpole could be. The issues seems to have a number of different cases in it. The case this PR handles is when Docker refuses to pull/start a container due to low dm thin pool space, yet the node doesn't report disk pressure and doesn't do image GC, effectively wedging the node.

@dchen1107
Copy link
Member

/approve

I approve this change, but expect @dashpole review the code more. Thanks!

@derekwaynecarr derekwaynecarr self-assigned this Feb 1, 2017
@k8s-github-robot k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 1, 2017
@derekwaynecarr
Copy link
Member

This is /lgtm

@derekwaynecarr derekwaynecarr added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 1, 2017
@dashpole
Copy link
Contributor

dashpole commented Feb 1, 2017

@dchen1107 this lgtm as well

@derekwaynecarr
Copy link
Member

if and when we look to define a default for imagefs based eviction thresholds, we need to keep this in mind as well.

@dashpole
Copy link
Contributor

dashpole commented Feb 1, 2017

@derekwaynecarr, why is that? The image-gc-high-threshold modified in this PR only affects periodic garbage collection. Eviction passes this check and directly triggers deletion of all unused images IIRC.

@vishh
Copy link
Contributor

vishh commented Feb 1, 2017

I wonder if it is not possible to change the docker setting to 8% for example? Given that disk reclamation is slow, I suspect that even with this PR, we will see image pull failures.

@vishh
Copy link
Contributor

vishh commented Feb 1, 2017

I don't see why we have to change kube defaults based on the behavior of a single docker storage driver.

@vishh vishh removed the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 1, 2017
@k8s-github-robot k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 1, 2017
@vishh
Copy link
Contributor

vishh commented Feb 1, 2017

/approve cancel

@k8s-github-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

The following people have approved this PR: dchen1107, sjenning

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

@vishh vishh added the do-not-merge DEPRECATED. Indicates that a PR should not merge. Label can only be manually applied/removed. label Feb 1, 2017
@derekwaynecarr
Copy link
Member

@vishh -- should not the default value kube selects work reasonably well across the popular set of container runtime storage drivers out of the box? if operators wanted to be more aggressive for a particular storage driver, then they can modify the default higher? it seems anti-user to have a default that doesnt work well on popular options by default?

@derekwaynecarr
Copy link
Member

to phrase it another way, we should select a default for OSS version of k8s that works for the broadest number of users, and causes the least amount of noise in the form of related issue creation when things just do not work.

@vishh
Copy link
Contributor

vishh commented Feb 1, 2017 via email

@derekwaynecarr
Copy link
Member

@vishh -- prior to sending this PR, @sjenning and I spoke with @mrunalp and @rhvgoyal to understand why the 10% value for dm.min_free_space value was chosen, and it was chosen because they felt it was a sensible default, and reducing it below 10% felt too low in their opinion. They can weigh in w/ more details if they feel I am misrepresenting our discussion, but it seemed fair.

My prior point remains which is that I think OSS k8s defaults should work with the broadest set of container run-time storage driver options out of the box just to reduce the amount of issues/noise/support in the broader community for when something does not work. Are we more likely to get issues that we are not utlizing an extra 5% of disk, or that something just doesn't work on Centos/Fedora/etc when using POSIX compliant storage drivers? We are not going to get the existing versions of docker out in the wild to all change.

@vishh
Copy link
Contributor

vishh commented Feb 2, 2017 via email

@vishh
Copy link
Contributor

vishh commented Feb 18, 2017

Ping. This PR is still not merged.

@derekwaynecarr
Copy link
Member

@vishh - for the moment, we are carrying a reduced default in openshift. for k8s, i am still not sure how to proceed. as a default, i think 85% in OSS is not the worst. ideally, we would have a way to ask a container runtime if it was doing any behind the scenes image management configuration behavior like this and dynamically reduce or warn in the kubelet our settings in response if the one-size fits all default is not portable.

@vishh
Copy link
Contributor

vishh commented Feb 24, 2017 via email

@vishh vishh removed the do-not-merge DEPRECATED. Indicates that a PR should not merge. Label can only be manually applied/removed. label Feb 27, 2017
@sjenning
Copy link
Contributor Author

sjenning commented Apr 3, 2017

@k8s-bot cri e2e test this
@k8s-bot kops aws e2e test this

@k8s-ci-robot
Copy link
Contributor

k8s-ci-robot commented Apr 3, 2017

@sjenning: The following test(s) failed:

Test name Commit Details Rerun command
Jenkins CRI GCE e2e 0247a9a link @k8s-bot cri e2e test this

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.

@k8s-github-robot
Copy link

Automatic merge from submit-queue

@k8s-github-robot k8s-github-robot merged commit 6f3e5ba into kubernetes:master Apr 3, 2017
@sjenning sjenning deleted the imagegc-default branch August 16, 2017 02:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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. 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. 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.