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

Adapt pod killing and cleanup for generic container runtime #7525

Merged
merged 1 commit into from Apr 30, 2015

Conversation

Projects
None yet
4 participants
@yujuhong
Contributor

yujuhong commented Apr 29, 2015

This change removes docker-specifc code in killUnwantedPods. It
also instructs the cleanup code to move away from interacting with
containers directly. They should always deal with the pod-level
abstraction if at all possible.

@googlebot googlebot added the cla: yes label Apr 29, 2015

@vmarmol vmarmol self-assigned this Apr 29, 2015

@yujuhong

This comment has been minimized.

Show comment
Hide comment
@yujuhong

yujuhong Apr 29, 2015

Contributor

/cc @vmarmol, @yifan-gu

To avoid inspecting individual containers, I had to use containerManager.GetPods. This bypasses the runtime cache and may affect performance (?). I am doubtful that this operation is so expensive that we need to use the runtime cache at all times (since this is called only once per sync). But I suppose the cache was added for a reason.... If we absolutely want to avoid calling this function, I could modify the container runtime interface to support a function similar to GetRunningContainers to take container ids, inspect them, and return running pods. This doesn't seem to play well with rkt though.

Contributor

yujuhong commented Apr 29, 2015

/cc @vmarmol, @yifan-gu

To avoid inspecting individual containers, I had to use containerManager.GetPods. This bypasses the runtime cache and may affect performance (?). I am doubtful that this operation is so expensive that we need to use the runtime cache at all times (since this is called only once per sync). But I suppose the cache was added for a reason.... If we absolutely want to avoid calling this function, I could modify the container runtime interface to support a function similar to GetRunningContainers to take container ids, inspect them, and return running pods. This doesn't seem to play well with rkt though.

@vmarmol

This comment has been minimized.

Show comment
Hide comment
@vmarmol

vmarmol Apr 29, 2015

Contributor

LGTM. Nice! We take out a bunch of code and end up with something more generic :)

I agree that the performance impact is probably minimal since this is only a single extra call per sync loop. If anything, with time this operation will become async too.

Contributor

vmarmol commented Apr 29, 2015

LGTM. Nice! We take out a bunch of code and end up with something more generic :)

I agree that the performance impact is probably minimal since this is only a single extra call per sync loop. If anything, with time this operation will become async too.

Show outdated Hide outdated pkg/kubelet/kubelet.go
// in the cache. We need to bypass the cach to get the latest set of
// running pods to clean up the volumes.
// TODO: Evaluate the performance impact of bypassing the runtime cache.
runningPods, err = kl.containerManager.GetPods(true)

This comment has been minimized.

@yifan-gu

yifan-gu Apr 29, 2015

Member

Why we pass true here? I remember true is for getting all pods including dead ones?

@yifan-gu

yifan-gu Apr 29, 2015

Member

Why we pass true here? I remember true is for getting all pods including dead ones?

This comment has been minimized.

@yujuhong

yujuhong Apr 29, 2015

Contributor

Good catch. I meant to say false...Thanks!

@yujuhong

yujuhong Apr 29, 2015

Contributor

Good catch. I meant to say false...Thanks!

@yifan-gu

This comment has been minimized.

Show comment
Hide comment
@yifan-gu

yifan-gu Apr 29, 2015

Member

Great! This is what I want to do! Thanks @yujuhong !

Member

yifan-gu commented Apr 29, 2015

Great! This is what I want to do! Thanks @yujuhong !

Adapt pod killing and cleanup for generic container runtime
This change removes docker-specifc code in killUnwantedPods. It
also instructs the cleanup code to move away from interacting with
containers directly. They should always deal with the pod-level
abstraction if at all possible.
@yujuhong

This comment has been minimized.

Show comment
Hide comment
@yujuhong

yujuhong Apr 29, 2015

Contributor

Rebased. Thanks!

Contributor

yujuhong commented Apr 29, 2015

Rebased. Thanks!

@yifan-gu

This comment has been minimized.

Show comment
Hide comment
@yifan-gu

yifan-gu Apr 29, 2015

Member

LGTM

Member

yifan-gu commented Apr 29, 2015

LGTM

@vmarmol vmarmol added the lgtm label Apr 29, 2015

vmarmol added a commit that referenced this pull request Apr 30, 2015

Merge pull request #7525 from yujuhong/container_runtime
Adapt pod killing and cleanup for generic container runtime

@vmarmol vmarmol merged commit 70830ad into kubernetes:master Apr 30, 2015

2 of 4 checks passed

coverage/coveralls First build on master at 48.49%
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
Shippable Shippable builds completed
Details
cla/google All necessary CLAs are signed

@yujuhong yujuhong deleted the yujuhong:container_runtime branch May 8, 2015

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