Add a container type to the runtime manager's container status

[verb]

What this PR does / why we need it:
This is Step 1 of the "Debug Containers" feature proposed in #35584 and is hidden behind a feature gate. Debug containers exist as container status with no associated spec, so this new runtime label allows the kubelet to treat containers differently without relying on spec.

Which issue this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close that issue when PR gets merged): cc #27140

Special notes for your reviewer:

Release note:

NONE

Integrating feedback:

• Remove Type field in favor of a help method

Dependencies:

• Add a feature gate for Debug Containers #46261 Feature gate for Debug Containers

[k8s-reviewable]

This change is 

[spiffxp]

@k8s-bot ok to test

[verb]

/assign @dashpole

[dashpole]

Apologies if this has already been hashed out elsewhere, but why do we need a field as part of kubecontainer? What does the field do that the label does not already do?

For example, the model used for enabling and feature gating "critical pods" uses a helper method to define a critical pod. You could define a IsDebugContainer( kubecontainer) helper method, which would avoid adding a field to kubecontainer

[verb]

@dashpole Nope, no one has brought that up. We don't need the field, it's just that avoiding it didn't occur to me. I like that though and will switch to a helper method.

[dashpole]

Btw, I don't really have context on the overall proposal, especially some of the design intricacies with how we represent debug containers. I plan to understand the proposal for the sake of guiding the implementation, but not to work through all of the edge cases associated with it. With that being said, I'll leave approving the proposal to vish and yuju. It goes without saying that this PR cannot go in until the proposal is approved.

[verb]

@dashpole it turns out the kubelet doesn't keep the runtime labels after populating kubecontainer.ContainerStatus, and neither Container nor ContainerStatus have sufficient information to calculate a container type.

Without type being part of ContainerStatus we'd have to query the labels from the runtime, bypassing the cache. It would need to happen from SyncPod(), so it's probably better that we cache it as part of kubecontainer.ContainerStatus.

[dashpole]

Interesting. Why use labels if they arent kept here? I was under the impression that the reason to add this as a label was to pass the container type.

[dashpole]

It might be nice to schedule a VC just so that you can walk me through how you plan to implement this.

[verb]

I will definitely do that. In the mean time, there's no reason we actually need to add a field to ContainerStatus in this PR where it would be unused. I've moved adding the field to my second PR, which will give context for its use.

I've also updated the unit tests to check for backwards compatibility with containers created prior to the feature being enabled.

[verb]

/unassign @vishh @dchen1107
/assign @yujuhong

@dashpole @yujuhong This is the first PR of debug containers that David first reviewed back in May, rebased a few times. Since kubernetes/community#649 is merged I think we can pick it up again.

[dashpole]

Took another look over.
/lgtm

[k8s-github-robot]

This PR hasn't been active in 30 days. It will be closed in 59 days (Jan 18, 2018).

cc @dashpole @verb @yujuhong

You can add 'keep-open' label to prevent this from happening, or add a comment to keep it open another 90 days

[k8s-ci-robot]

@verb: The following test failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
pull-kubernetes-e2e-gce-bazel	d8e31bb53a10b6a1f9e4f12916dc521777b4a159	link	/test pull-kubernetes-e2e-gce-bazel

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

[yujuhong]

Do you plan to use the container type as a filter to select containers?
If not, you could also consider writing it as an annotation.

[verb]

My plan was to add a ContainerType field to kubecontainer.ContainerStatus since most of the places where I need to reference the type I already have the kubecontainer.PodStatus with its list of ContainerStatuses.

I won't need to filter by type unless you think we should avoid the new field in kubecontainer.ContainerStatus. I chose a label of annotation because it "felt" like a more "label-y" thing that someone might want to filter, but it was an arbitrary choice and I'm happy to switch to annotation if you think that's a better fit.

[yujuhong]

/lgtm

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dashpole, verb, yujuhong

Associated issue: #35584

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these OWNERS Files:

• pkg/kubelet/OWNERS [yujuhong]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

[k8s-github-robot]

/test all [submit-queue is verifying that this PR is safe to merge]

[k8s-github-robot]

Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions here.