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

Add support for Ephemeral Containers to the kubelet #59484

Merged
merged 5 commits into from Aug 21, 2019

Conversation

@verb
Copy link
Contributor

commented Feb 7, 2018

What this PR does / why we need it: This implements the Troubleshooting Running Pods in the kubelet, protected by a feature flag.

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

Special notes for your reviewer:
/assign @yujuhong

Release note:

Ephemeral containers have been added in alpha. These temporary containers can be added to running pods for purposes such as debugging, similar to how `kubectl exec` runs an process in an existing container. Also like `kubectl exec`, no resources are reserved for ephemeral containers and they are not restarted when they exit.

Note that container namespace targeting is not yet implemented, so [process namespace sharing](https://kubernetes.io/docs/tasks/configure-pod-container/share-process-namespace/) must be enabled to view process from other containers in the pod.

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

- [KEP]: https://git.k8s.io/enhancements/keps/sig-node/20190212-ephemeral-containers.md
@verb

This comment has been minimized.

Copy link
Contributor Author

commented Feb 8, 2018

/retest

@@ -170,6 +170,14 @@ type ContainerCommandRunner interface {
RunInContainer(id ContainerID, cmd []string, timeout time.Duration) ([]byte, error)
}

type EphemeralContainerStarter interface {
// RunDebugContainer runs a new container in the specified pod. This "Debug Container" is

This comment has been minimized.

Copy link
@tallclair

tallclair Mar 15, 2018

Member

s/RunDebugContainer/StartEphemeralContainer/

@@ -212,6 +215,14 @@ func (m *kubeGenericRuntimeManager) generateContainerConfig(container *v1.Contai
Linux: m.generateLinuxContainerConfig(container, pod, uid, username),
}

// Since this saved v1.Container is used for security decisions, and they're infrequent,
// we should spare a few cycles to make sure the spec is readable.

This comment has been minimized.

Copy link
@tallclair

tallclair Mar 15, 2018

Member

I don't understand this comment, and "readable" doesn't seem to match up with the condition.

type podActions struct {
// Stop all running (regular and init) containers and the sandbox for the pod.
// Stop all running (regular, init and debug) containers and the sandbox for the pod.

This comment has been minimized.

Copy link
@tallclair

tallclair Mar 15, 2018

Member

This seems to conflict with the above comment stating that the actions don't affect debug containers.

@@ -105,6 +106,14 @@ func newContainerLabels(container *v1.Container, pod *v1.Pod, containerType kube
labels[types.KubernetesContainerNameLabel] = container.Name
if utilfeature.DefaultFeatureGate.Enabled(features.DebugContainers) {
labels[types.KubernetesContainerTypeLabel] = string(containerType)
// It might be useful to store the spec for every container, but for now only store spec for debug containers
if containerType == kubecontainer.ContainerTypeEphemeral {
if containerJSON, e := json.Marshal(container); e != nil {

This comment has been minimized.

Copy link
@tallclair

tallclair Mar 15, 2018

Member

Does size matter here? If it does a base64 encoded protobuf will probably be more efficient.

// It might be useful to store the spec for every container, but for now only store spec for debug containers
if containerType == kubecontainer.ContainerTypeEphemeral {
if containerJSON, e := json.Marshal(container); e != nil {
glog.Errorf("Unable to marshal %v container %q of pod %v: %v", containerType, container, pod.Name, e)

This comment has been minimized.

Copy link
@tallclair

tallclair Mar 15, 2018

Member

nit: use format.Pod(pod) in place of the pod name.

return errors.New("Debug Containers feature disabled")
}

if kubecontainer.GetContainerSpec(pod, container.Name) != nil {

This comment has been minimized.

Copy link
@tallclair

tallclair Mar 15, 2018

Member

What if there's another ephemeral container with the same name? Is there a race condition here?

@verb verb changed the title Add support for Ephemeral Containers to the kubelet Runtime Manager WIP: Add support for Ephemeral Containers to the kubelet Runtime Manager Mar 15, 2018
@verb verb force-pushed the verb:debug-kubelet-1 branch from 6e069be to 5d67c83 May 15, 2018
@verb

This comment has been minimized.

Copy link
Contributor Author

commented May 15, 2018

I've updated this PR to reflect updates to the API from kubernetes/community#1269, which makes the kubelet change simpler.

Note that tests will fail until this PR is rebased to include #59416.

@verb verb changed the title WIP: Add support for Ephemeral Containers to the kubelet Runtime Manager Add support for Ephemeral Containers to the kubelet May 15, 2018
@verb verb force-pushed the verb:debug-kubelet-1 branch from 5d67c83 to 5a735ef May 15, 2018
@fejta-bot

This comment has been minimized.

Copy link

commented Sep 5, 2018

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

@verb verb changed the title Add support for Ephemeral Containers to the kubelet WIP: Add support for Ephemeral Containers to the kubelet Sep 6, 2018
@verb

This comment has been minimized.

Copy link
Contributor Author

commented Sep 6, 2018

/remove-lifecycle stale

@spiffxp spiffxp referenced this pull request Oct 16, 2018
3 of 5 tasks complete
@spiffxp

This comment has been minimized.

Copy link
Member

commented Oct 16, 2018

ping, needs rebase

@spiffxp

This comment has been minimized.

Copy link
Member

commented Oct 16, 2018

/milestone v1.13
I'm adding this to the v1.13 milestone since it relates to kubernetes/enhancements#277 which is currently planned for v1.13

@k8s-ci-robot k8s-ci-robot added this to the v1.13 milestone Oct 16, 2018
@guineveresaenger

This comment has been minimized.

Copy link
Contributor

commented Oct 22, 2018

@verb given that this PR still is a WIP, what is your level of confidence this can be ready for the 1.13 milestone? Enhancement freeze is tomorrow end of day. If there is no communication or update on the PR, this is going to be pulled from the milestone as it doesn't fit with our "stability" theme. If there is no communication after COB tomorrow, an exception will be required to add it back to the milestone. Please let me or Enhancements Lead @kacole2 know where we stand. Thanks!

Copy link
Member

left a comment

Just one minor comment for volumemanager changes.

/approve

@verb

This comment has been minimized.

Copy link
Contributor Author

commented Aug 3, 2019

Cleaning up labels from before api change merged.

/remove-kind api-change
/remove-area test
/remove-area release-eng
/remove-sig apps
/remove-sig auth
/remove-sig api-machinery
/remove-sig release
/remove-sig storage
/remove-sig testing

@verb

This comment has been minimized.

Copy link
Contributor Author

commented Aug 3, 2019

/kind feature

@k8s-ci-robot k8s-ci-robot added kind/feature and removed needs-kind labels Aug 3, 2019
@verb

This comment has been minimized.

Copy link
Contributor Author

commented Aug 15, 2019

/retest

@yujuhong

This comment has been minimized.

Copy link
Member

commented Aug 20, 2019

/approve
/lgtm

/retest

@k8s-ci-robot k8s-ci-robot added the lgtm label Aug 20, 2019
@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

commented Aug 20, 2019

[APPROVALNOTIFIER] This PR is APPROVED

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

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

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@fejta-bot

This comment has been minimized.

Copy link

commented Aug 21, 2019

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

commented Aug 21, 2019

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

Test name Commit Details Rerun command
pull-kubernetes-e2e-kops-aws 5a735ef link /test pull-kubernetes-e2e-kops-aws
pull-kubernetes-kubemark-e2e-gce 5a735ef link /test pull-kubernetes-kubemark-e2e-gce
pull-kubernetes-local-e2e-containerized 5a735ef link /test pull-kubernetes-local-e2e-containerized

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

This comment has been minimized.

Copy link
Contributor Author

commented Aug 21, 2019

/retest

@k8s-ci-robot k8s-ci-robot merged commit 59d29b6 into kubernetes:master Aug 21, 2019
23 checks passed
23 checks passed
cla/linuxfoundation verb authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-conformance-image-test Skipped.
pull-kubernetes-cross Skipped.
pull-kubernetes-dependencies Job succeeded.
Details
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-100-performance Job succeeded.
Details
pull-kubernetes-e2e-gce-csi-serial Job succeeded.
Details
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-gce-iscsi Skipped.
pull-kubernetes-e2e-gce-iscsi-serial Skipped.
pull-kubernetes-e2e-gce-storage-slow Job succeeded.
Details
pull-kubernetes-godeps Skipped.
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce-big Job succeeded.
Details
pull-kubernetes-local-e2e Skipped.
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-node-e2e-containerd Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details
pull-publishing-bot-validate Skipped.
tide In merge pool.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.