Kubelet: add garbage collection for new runtime API

[feiskyer]

This PR adds garbage collection for new runtime API.

Note that this PR also adds CreatedAt and PodSandboxId to ListContainers() result.

CC @yujuhong @Random-Liu @kubernetes/sig-node @kubernetes/sig-rktnetes

---

This change is

[yifan-gu]

The second commit LGTM

[yujuhong]

Reviewed 9 of 9 files at r1.
Review status: 8 of 11 files reviewed at latest revision, 2 unresolved discussions.

---

pkg/kubelet/kuberuntime/kuberuntime_gc.go, line 28 [r2] (raw file):

  kubecontainer "k8s.io/kubernetes/pkg/kubelet/container"
)

I'd suggesting writing a brief explanation on how kubelet GC policy is interpreted in this file.

---

pkg/kubelet/kuberuntime/kuberuntime_gc.go, line 84 [r2] (raw file):

// evictableSandboxes gets evictable pod sandboxes.
// Evictable pod sandboxes are not active and created more than MinAge ago)

I think podsandbox should also comply to MaxPerPodContainer policy, so that we at least keep one exited pod (sandbox+containers) .
In the future, we'll change the GC policy to be more podsandbox friendly, but right now, I think this is the right interpretation.

---

Comments from Reviewable

[feiskyer]

Review status: 6 of 11 files reviewed at latest revision, 2 unresolved discussions.

---

pkg/kubelet/kuberuntime/kuberuntime_gc.go, line 28 [r2] (raw file):

Previously, yujuhong (Yu-Ju Hong) wrote…

I'd suggesting writing a brief explanation on how kubelet GC policy is interpreted in this file.

Done.

---

pkg/kubelet/kuberuntime/kuberuntime_gc.go, line 84 [r2] (raw file):

Previously, yujuhong (Yu-Ju Hong) wrote…

I think podsandbox should also comply to MaxPerPodContainer policy, so that we at least keep one exited pod (sandbox+containers) .
In the future, we'll change the GC policy to be more podsandbox friendly, but right now, I think this is the right interpretation.

Done.

---

Comments from Reviewable

[feiskyer]

@yujuhong The latest commit rewrites the gc logic and addresses the comments. PTAL.

[yujuhong]

I think podsandbox should also comply to MaxPerPodContainer policy, so that we at least keep one exited pod (sandbox+containers) .
In the future, we'll change the GC policy to be more podsandbox friendly, but right now, I think this is the right interpretation.

Hmm......I thought about it more, and I think the correct/easy way to do this is:

1. remove containers based on MaxContainerPerPod
2. only remove a sandbox if it's empty (i.e., no containers are using it).

@feiskyer WDYT? (and sorry for the churn).

[feiskyer]

@yujuhong So this means there will be no something like MaxSandboxPerPod in the future. SGTM.

And the entire GC procedure will be:

• removes oldest dead sandboxes for each Pod by enforcing MaxPerPodContainer
• remove oldest dead containers for each Pod by enforcing MaxPerPodContainer
• remove oldest dead containers by enforcing MaxContainers
• remove dead sandboxes with zero containers.

@euank @yifan-gu Do you think this is ok for rkt runtime?

[euank]

@feiskyer because that can be expressed by CRI it'll have to be okay for any runtime that integrates that way (that is future rkt, if not current rkt).

I agree with @yujuhong about that being the logical way to do it so long as I'm reading that correctly.

I also think it might make sense to not count sandboxes as "containers" anywhere or in any of the math. Sandboxes should simply be removed immediately when there are no remaining containers in them and no intention of creating new containers in them.

I think we can potentially make the code slightly simpler if we can assume that when all containers for a pod are laid out by age, each sandbox encompasses a non-overlapping contiguous segment of containers. I think that's always true, and makes it so that avoiding counting sandboxes and just auto-removing them when empty does more or less "the right thing".
In any case, more than one sandbox should be a rarity/edge case.

It's possible the above behaviour and assumptions are implicit in your comment, but I wanted to be a little more explicit about that part.

[yujuhong]

I also think it might make sense to not count sandboxes as "containers" anywhere or in any of the math. Sandboxes should simply be removed immediately when there are no remaining containers in them and no intention of creating new containers in them.

Thanks for summarizing! I think this is what @feiskyer plans to do, if I read his comments correctly.
Essentially, all the existing GC policies applies to "non-sandbox" containers, and sandboxes are only removed when all containers associated with the sandbox are removed.
This interprets the "MaxContainers" in the GC policy a bit differently than its original intention, i.e., we don't count sandboxes at all. I think this is acceptable, and we just have to document this behavior clearly.

[feiskyer]

I also think it might make sense to not count sandboxes as "containers" anywhere or in any of the math. Sandboxes should simply be removed immediately when there are no remaining containers in them and no intention of creating new containers in them.
Thanks for summarizing! I think this is what @feiskyer plans to do, if I read his comments correctly.
Essentially, all the existing GC policies applies to "non-sandbox" containers, and sandboxes are only removed when all containers associated with the sandbox are removed.
This interprets the "MaxContainers" in the GC policy a bit differently than its original intention, i.e., we don't count sandboxes at all. I think this is acceptable, and we just have to document this behavior clearly.

@euank @yujuhong Exactly. Will make an update today.

[k8s-bot]

Can one of the admins verify that this patch is reasonable to test? If so, please reply "ok to test".
(Note: "add to whitelist" is no longer supported. Please update configurations in kubernetes/test-infra/jenkins/job-configs/kubernetes-jenkins-pull instead.)

This message will repeat several times in short succession due to jenkinsci/ghprb-plugin#292. Sorry.

[feiskyer]

@euank @yujuhong updated. PTAL.

[tmrts]

no need to include services.go in the path, pkg name is sufficient

[feiskyer]

Fixed.

[yujuhong]

lgtm. Leaving up to @yifan-gu for a final round.

[yifan-gu]

LGTM, thank you @feiskyer !

[yujuhong]

Need to squash the commits. Removed the lgtm label.

[googlebot]

We found a Contributor License Agreement for you (the sender of this pull request) and all commit authors, but as best as we can tell these commits were authored by someone else. If that's the case, please add them to this pull request and have them confirm that they're okay with these commits being contributed to Google. If we're mistaken and you did author these commits, just reply here to confirm.

[googlebot]

CLAs look good, thanks!

[feiskyer]

@yujuhong squashed.

[yujuhong]

@feiskyer uh..more rebase.

[k8s-bot]

GCE e2e build/test passed for commit f774a68.

• Test Results
• Build Log
• Test Artifacts
• Internal Jenkins Results

[k8s-github-robot]

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

[k8s-bot]

GCE e2e build/test passed for commit f774a68.

• Test Results
• Build Log
• Test Artifacts
• Internal Jenkins Results

[k8s-github-robot]

Automatic merge from submit-queue