Skip to content

out of resource killing (memory)#21274

Merged
k8s-github-robot merged 1 commit intokubernetes:masterfrom
derekwaynecarr:kubelet_eviction
May 17, 2016
Merged

out of resource killing (memory)#21274
k8s-github-robot merged 1 commit intokubernetes:masterfrom
derekwaynecarr:kubelet_eviction

Conversation

@derekwaynecarr
Copy link
Copy Markdown
Member

@derekwaynecarr derekwaynecarr commented Feb 15, 2016

Adds the core framework for low-resource killing in the kubelet.

Implements support for out of memory killing.

Related:
#18724


This change is Reviewable

@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/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Feb 15, 2016
@derekwaynecarr derekwaynecarr force-pushed the kubelet_eviction branch 3 times, most recently from b80c763 to 8bb4fed Compare February 15, 2016 22:33
@k8s-github-robot k8s-github-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Feb 20, 2016
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

recording a note here so when i get time to get back to this PR, but I will look to use kubelet.HandlePodDeletions

@k8s-github-robot k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 4, 2016
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

@k8s-github-robot k8s-github-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Apr 13, 2016
@derekwaynecarr derekwaynecarr force-pushed the kubelet_eviction branch 2 times, most recently from c03d24e to 9715e29 Compare April 18, 2016 21:42
@derekwaynecarr derekwaynecarr force-pushed the kubelet_eviction branch 2 times, most recently from 173ad77 to f482830 Compare April 25, 2016 17:00
@k8s-github-robot k8s-github-robot added kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API kind/old-docs size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Apr 25, 2016
@derekwaynecarr derekwaynecarr assigned vishh and unassigned dchen1107 May 12, 2016
@k8s-github-robot k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 13, 2016
@derekwaynecarr derekwaynecarr force-pushed the kubelet_eviction branch 2 times, most recently from e9b2928 to 4a7a319 Compare May 13, 2016 17:53
@derekwaynecarr
Copy link
Copy Markdown
Member Author

@vishh - right now, this includes #25163 , but once that merges, I will do a final rebase. This should be good to do a code review against now. I actually was pleased how much I was able to cover in unit testing.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

What about usage of volumes?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I will leave a // TODO for volume tracking, we can handle that in a follow-on when we finish up disk.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

SGTM

@vishh
Copy link
Copy Markdown
Contributor

vishh commented May 13, 2016

I added a few comments. Overall this PR LGTM. @derekwaynecarr Thanks for coming up with super readable code :)

@derekwaynecarr
Copy link
Copy Markdown
Member Author

@vishh - updated code per your comments.

/cc @kubernetes/rh-cluster-infra @kubernetes/sig-node - if anyone wants to take a pass at memory eviction.

@vishh
Copy link
Copy Markdown
Contributor

vishh commented May 17, 2016

@derekwaynecarr I hope at-least one of the memory eviction PRs (ideally the PR that adds flags) has a release-note flag.

@vishh
Copy link
Copy Markdown
Contributor

vishh commented May 17, 2016

@derekwaynecarr Next step is to add a node e2e to test all this logic. The test might need some changes to the framework - setting eviction flags.

@derekwaynecarr
Copy link
Copy Markdown
Member Author

I have a follow on PR to enable the max pod eviction period and will have a release note for that PR that covers the feature.

@derekwaynecarr
Copy link
Copy Markdown
Member Author

@k8s-bot test this issue #25652

@k8s-bot
Copy link
Copy Markdown

k8s-bot commented May 17, 2016

GCE e2e build/test passed for commit edc76f6.

@k8s-github-robot
Copy link
Copy Markdown

@k8s-bot test this [submit-queue is verifying that this PR is safe to merge]

@k8s-bot
Copy link
Copy Markdown

k8s-bot commented May 17, 2016

GCE e2e build/test passed for commit edc76f6.

@k8s-github-robot
Copy link
Copy Markdown

Automatic merge from submit-queue

type signalObservations map[Signal]resource.Quantity

// thresholdsObservedAt maps a threshold to a time that it was observed
type thresholdsObservedAt map[Threshold]time.Time
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think this is broken, because you're assuming that resource.Quantity contributes to the uniqueness of the the threshold. It does not, because it's a pointer to an inf.Dec, which is only compared on pointer equality, not actual equality.

If you are depending on uniqueness of Quantity to signal value, you'll need to change your map keys.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Basically, map[Threshold1{Quantity: NewQuantity("1m")}] is not guaranteed to return the same value if you call it twice.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

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-none Denotes a PR that doesn't merit a release note. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants