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

Adapt pod killing and cleanup for generic container runtime #7525

Merged
merged 1 commit into from Apr 30, 2015

Conversation

yujuhong
Copy link
Contributor

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.

@vmarmol vmarmol self-assigned this Apr 29, 2015
@yujuhong
Copy link
Contributor Author

/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
Copy link
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.

// 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)
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

@yifan-gu
Copy link
Contributor

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

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
Copy link
Contributor Author

Rebased. Thanks!

@yifan-gu
Copy link
Contributor

LGTM

@vmarmol vmarmol added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 29, 2015
vmarmol added a commit that referenced this pull request Apr 30, 2015
Adapt pod killing and cleanup for generic container runtime
@vmarmol vmarmol merged commit 70830ad into kubernetes:master Apr 30, 2015
@yujuhong yujuhong deleted the container_runtime branch May 8, 2015 17:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lgtm "Looks good to me", indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants