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

Use runtime instread of dockerclient in container gc #15051

Merged
merged 6 commits into from
Oct 9, 2015
Merged

Use runtime instread of dockerclient in container gc #15051

merged 6 commits into from
Oct 9, 2015

Conversation

feiskyer
Copy link
Member

@feiskyer feiskyer commented Oct 3, 2015

Refactor container garbage collection to use runtime interface instead.

This refactor is a pre-requirement for client/server runtime at #13768

@k8s-bot
Copy link

k8s-bot commented Oct 3, 2015

Can one of the admins verify that this patch is reasonable to test? (reply "ok to test", or if you trust the user, reply "add to whitelist")

If this message is too spammy, please complain to ixdy.

@feiskyer
Copy link
Member Author

feiskyer commented Oct 3, 2015

CC @dchen1107 @yujuhong

@k8s-github-robot k8s-github-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Oct 3, 2015
@k8s-github-robot
Copy link

Labelling this PR as size/XL

@yujuhong yujuhong assigned yujuhong and unassigned thockin Oct 5, 2015
derekwaynecarr
derekwaynecarr reviewed Oct 5, 2015
View reviewed changes
pkg/kubelet/container/runtime.go
@@ -57,6 +58,8 @@ type Runtime interface {
// specifies whether the runtime returns all containers including those already
// exited and dead containers (used for garbage collection).
GetPods(all bool) ([]*Pod, error)
// Garbage collection of dead containers
Copy link
Member

Choose a reason for hiding this comment

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

use proper go doc conventions.

// GarbageCollect removes dead containers using the specified criteria

Copy link
Member Author

Choose a reason for hiding this comment

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

@derekwaynecarr thanks

@yujuhong
Copy link
Contributor

yujuhong commented Oct 5, 2015

@vishh, this pushes garbage collection down to the container runtime. Is this in line with your plans to improve the disk manager? We may need a better GC policy that's generic for all the runtimes in the near future.

@yifan-gu, FYI, as this changes the runtime interface.

@vishh
Copy link
Contributor

vishh commented Oct 5, 2015

@yujuhong: I haven't thought through disk management yet. We might end up exposing additional runtime APIs. Image management is specific to the runtime and so this PR makes sense overall.
I have not reviewed the code.

@yujuhong
Copy link
Contributor

yujuhong commented Oct 5, 2015

@feiskyer, just a heads-up this may conflict with #14686, which hopefully will land soon.

@k8s-github-robot k8s-github-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Oct 5, 2015
@k8s-github-robot
Copy link

Labelling this PR as size/L

yifan-gu
yifan-gu reviewed Oct 5, 2015
View reviewed changes
pkg/kubelet/rkt/rkt.go
@@ -1082,7 +1082,7 @@ func (r *runtime) GetContainerLogs(pod *api.Pod, containerID string, logOptions
}

// GarbageCollect collects the pods/containers. TODO(yifan): Enforce the gc policy.
func (r *runtime) GarbageCollect() error {
func (r *runtime) GarbageCollect(gcPolicy kubecontainer.ContainerGCPolicy) error {
Copy link
Contributor

Choose a reason for hiding this comment

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

So if #14686 gets in first, then the change we need to make is to derive the --grace-period and --expire-prepared from the gcPolicy.MinAge. AFAICS we can just set both the the gcPolicy.MinAge

Copy link
Member Author

Choose a reason for hiding this comment

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

@yifan-gu OK

@feiskyer
Copy link
Member Author

feiskyer commented Oct 5, 2015

@yujuhong OK, I'll rebase when #14686 gets merged

@yujuhong
Copy link
Contributor

yujuhong commented Oct 6, 2015

@k8s-bot ok to test

@yujuhong
Copy link
Contributor

yujuhong commented Oct 6, 2015

PR looks good to me overall. Will review again after rebase and apply the lgtm label. Thanks for the PR!

@k8s-bot
Copy link

k8s-bot commented Oct 6, 2015

Unit, integration and GCE e2e test build/test passed for commit 42d72027d802a951d4fdbb6fc9b9a26602366fd4.

@feiskyer
Copy link
Member Author

feiskyer commented Oct 6, 2015

@yujuhong Shippable is so slow now

@k8s-bot
Copy link

k8s-bot commented Oct 6, 2015

GCE e2e test build/test passed for commit f0d1b6935df4e9d5242b103380b2874125dd2a71.

@k8s-bot
Copy link

k8s-bot commented Oct 6, 2015

GCE e2e test build/test passed for commit 673a9a7c69e97f877193f1ca7dce7c768214f21f.

@yujuhong
Copy link
Contributor

yujuhong commented Oct 6, 2015

@yujuhong Shippable is so slow now

Mergebot is also slow or not working :\ Sorry for the wait.

@k8s-bot
Copy link

k8s-bot commented Oct 7, 2015

GCE e2e build/test failed for commit cfa60a56fbe08f913b2314b23941acbaecddd9c1.

@k8s-bot
Copy link

k8s-bot commented Oct 7, 2015

GCE e2e test build/test passed for commit 74b5b0c25fc186ec6221b6fe80d58608188bb9ed.

@feiskyer
Copy link
Member Author

feiskyer commented Oct 7, 2015

@yujuhong @yifan-gu rebased since #14686 has been merged

@yifan-gu
Copy link
Contributor

yifan-gu commented Oct 7, 2015

LGTM

@yujuhong yujuhong added lgtm "Looks good to me", indicates that a PR is ready to be merged. ok-to-merge labels Oct 7, 2015
@yujuhong yujuhong mentioned this pull request Oct 7, 2015
Merged
@yifan-gu yifan-gu mentioned this pull request Oct 7, 2015
Merged
@k8s-github-robot k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 8, 2015
@k8s-bot
Copy link

k8s-bot commented Oct 8, 2015

GCE e2e test build/test passed for commit fb04ede.

@yujuhong yujuhong removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 8, 2015
@yujuhong
Copy link
Contributor

yujuhong commented Oct 8, 2015

Thanks for rebasing.

@yujuhong yujuhong added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 8, 2015
@k8s-github-robot k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 9, 2015
@k8s-github-robot
Copy link

Continuous integration appears to have missed, closing and re-opening to trigger it

@k8s-github-robot
Copy link

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

@k8s-bot
Copy link

k8s-bot commented Oct 9, 2015

GCE e2e test build/test passed for commit fb04ede.

@k8s-github-robot
Copy link

Automatic merge from submit-queue

k8s-github-robot pushed a commit that referenced this pull request Oct 9, 2015
Merge pull request #15051 from feiskyer/kubelet/garbage-collection
538cf72 
Auto commit by PR queue bot
@k8s-github-robot k8s-github-robot merged commit 538cf72 into kubernetes:master Oct 9, 2015
@feiskyer feiskyer deleted the kubelet/garbage-collection branch April 26, 2016 06:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@yujuhong yujuhong

Labels
lgtm "Looks good to me", indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
9 participants
@feiskyer @k8s-bot @k8s-github-robot @yujuhong @vishh @yifan-gu @derekwaynecarr @googlebot @thockin