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

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

Merged
merged 1 commit into from
Jan 24, 2018
Merged

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

merged 1 commit into from
Jan 24, 2018
+119 −22

Conversation

verb
Copy link
Contributor

@verb verb commented May 5, 2017 •
edited
Loading

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:

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label May 5, 2017
@k8s-github-robot k8s-github-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. release-note-none Denotes a PR that doesn't merit a release note. labels May 5, 2017
@k8s-ci-robot k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label May 5, 2017
@k8s-reviewable
Copy link

This change is Reviewable

@spiffxp
Copy link
Member

spiffxp commented May 8, 2017

@k8s-bot ok to test

@k8s-ci-robot k8s-ci-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label May 8, 2017
@verb verb changed the title Add a container type to the runtime manager's container status WIP: Add a container type to the runtime manager's container status May 8, 2017
@verb verb changed the title WIP: Add a container type to the runtime manager's container status Add a container type to the runtime manager's container status May 8, 2017
@verb
Copy link
Contributor Author

verb commented May 8, 2017

/assign @dashpole

@dashpole
Copy link
Contributor

dashpole commented May 9, 2017

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

verb commented May 9, 2017

@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.

@verb verb changed the title Add a container type to the runtime manager's container status WIP: Add a container type to the runtime manager's container status May 9, 2017
@dashpole
Copy link
Contributor

dashpole commented May 9, 2017

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

verb commented May 9, 2017

@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.

@verb verb changed the title WIP: Add a container type to the runtime manager's container status Add a container type to the runtime manager's container status May 9, 2017
@dashpole
Copy link
Contributor

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

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

@verb
Copy link
Contributor Author

verb commented May 10, 2017

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.

@k8s-github-robot k8s-github-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels May 10, 2017
@k8s-github-robot k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 15, 2017
@k8s-github-robot k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 17, 2017
@k8s-ci-robot k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 20, 2017
@verb verb changed the title WIP: Add a container type to the runtime manager's container status Add a container type to the runtime manager's container status Oct 20, 2017
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 20, 2017
@verb
Copy link
Contributor Author

verb commented Oct 20, 2017

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

@k8s-ci-robot k8s-ci-robot assigned yujuhong and unassigned vishh and dchen1107 Oct 20, 2017
@dashpole
Copy link
Contributor

Took another look over.
/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 20, 2017
@k8s-github-robot k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 7, 2017
@k8s-github-robot
Copy link

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

@dchen1107 dchen1107 self-assigned this Nov 30, 2017
@kubernetes kubernetes deleted a comment from k8s-github-robot Jan 23, 2018
@k8s-github-robot k8s-github-robot removed lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Jan 23, 2018
@k8s-ci-robot
Copy link
Contributor

k8s-ci-robot commented Jan 23, 2018 •
edited
Loading

@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.

@verb
Add a container type to the runtime labels
ba6f31a 
This is part of the "Debug Containers" feature and is hidden behind
a feature gate. Debug containers have no stored spec, so this new
runtime label allows the kubelet to treat containers differently
without relying on spec.
@yujuhong yujuhong added this to the v1.10 milestone Jan 23, 2018
yujuhong
yujuhong reviewed Jan 23, 2018
View reviewed changes
pkg/kubelet/kuberuntime/labels.go
@@ -94,12 +97,15 @@ func newPodAnnotations(pod *v1.Pod) map[string]string {
}

// newContainerLabels creates container labels from v1.Container and v1.Pod.
func newContainerLabels(container *v1.Container, pod *v1.Pod) map[string]string {
func newContainerLabels(container *v1.Container, pod *v1.Pod, containerType kubecontainer.ContainerType) map[string]string {
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 24, 2018
@k8s-ci-robot
Copy link
Contributor

[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:

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

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 24, 2018
@k8s-github-robot
Copy link

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

@k8s-github-robot
Copy link

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

@k8s-github-robot k8s-github-robot merged commit 4024b59 into kubernetes:master Jan 24, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yujuhong yujuhong yujuhong left review comments

@dashpole dashpole dashpole left review comments

Assignees

@dashpole dashpole

@dchen1107 dchen1107

@yujuhong yujuhong

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note-none Denotes a PR that doesn't merit a release note. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
v1.10
Development

Successfully merging this pull request may close these issues.
9 participants
@verb @k8s-reviewable @spiffxp @dashpole @k8s-github-robot @k8s-ci-robot @yujuhong @vishh @dchen1107