default container behavior with annotation `kubectl.kubernetes.io/default-container` #97099

pacoxu · 2020-12-07T07:35:39Z

What type of PR is this?
/kind feature

What this PR does / why we need it:
specify the default container for kubectl exec, mirroring kubectl.kubernetes.io/default-logs-container.
more details can be seen in the KEP KEP-2227: default container behavior

Which issue(s) this PR fixes:
/sig cli
Fixes #96986

Special notes for your reviewer:
KEP is created in kubernetes/enhancements#2189, and the KEP is merged for milestone 1.21.

There was a discussion in #95293 which is in process.

Does this PR introduce a user-facing change?:

Users might specify the `kubectl.kubernetes.io/default-container` annotation in a Pod to preselect container for kubectl commands.

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

- [KEP] https://github.com/kubernetes/enhancements/blob/master/keps/sig-cli/2227-kubectl-default-container/README.md

fejta-bot · 2020-12-07T08:26:28Z

This PR may require API review.

If so, when the changes are ready, complete the pre-review checklist and request an API review.

Status of requested reviews is tracked in the API Review project.

pacoxu · 2020-12-07T15:50:48Z

/retest
fail for etcd mvcc exceed.

pacoxu · 2020-12-08T03:51:25Z

@kubernetes/sig-cli-pr-reviews @kubernetes/api-reviewers
@zhouya0

pacoxu · 2020-12-09T06:38:47Z

/label api-review

staging/src/k8s.io/api/core/v1/annotation_key_constants.go

liggitt · 2020-12-09T21:18:18Z

/remove-label api-review

Thanks for tagging for API review. I don't see an accepted KEP for adding annotations to control kubectl behavior, per the pre-review checklist:

The goal of the proposed change has agreement from the appropriate sub-project owners and/or SIG leads

A KEP and tracking issue in kubernetes/enhancements has been created

pacoxu · 2020-12-24T03:19:51Z

/retest

staging/src/k8s.io/kubectl/pkg/polymorphichelpers/logsforobject.go

pacoxu · 2021-02-14T05:42:40Z

/hold cancel
As the KEP is merged.

Currently, it is implemented for logs/exec in this PR as before. I will work on it for other commands later.

dougsland

@pacoxu do you mind to squash the patches and rebase as the KEP has been merged?

staging/src/k8s.io/kubectl/pkg/cmd/util/helpers.go

- update according to KEP: move getContainerName to helper Signed-off-by: pacoxu <paco.xu@daocloud.io>

pacoxu · 2021-02-20T06:57:34Z

@dougsland squashed and add comments.

/cc @soltysh
The change request is missing and the KEP is merged.

This pr is a fix for #96986 (logs) standalone.

I will work to make it aligned with the KEP in a new PR if this PR is merged. Or I will work on the KEP into this PR later.
As I need to work on several other KEPs next week for 1.21:

deprecate cAdvisor json metrics, almost done. (a manually test task)
sysctl to GA: walk though ga criteria. (WIP by wgahnagl.)
dual-stack feature gate update for kubeadm. almost done
dual-stack docs for kubeadm. add example and docs.

🥱 I will get back ASAP.

pacoxu · 2021-02-20T10:51:11Z

/retest

pacoxu · 2021-02-20T12:50:05Z

/retest

pacoxu · 2021-02-24T11:08:04Z

@dougsland squashed and add comments.

/cc @soltysh
The change request is missing and the KEP is merged.

This pr is a fix for #96986 (logs) standalone.

I will work to make it aligned with the KEP in a new PR if this PR is merged. Or I will work on the KEP into this PR later.
As I need to work on several other KEPs next week for 1.21:

deprecate cAdvisor json metrics, almost done. (a manually test task)

sysctl to GA: walk though ga criteria. (WIP by wgahnagl.)

dual-stack feature gate update for kubeadm. almost done

dual-stack docs for kubeadm. add example and docs.

🥱 I will get back ASAP.

I will start to work on this tommorrow.

soltysh

Small nits and this is good to go.
/approve

soltysh · 2021-02-25T17:31:06Z

staging/src/k8s.io/api/core/v1/annotation_key_constants.go

@@ -95,6 +95,10 @@ const (
 	// configuration of a resource for use in a three way diff by UpdateApplyAnnotation.
 	LastAppliedConfigAnnotation = kubectlPrefix + "last-applied-configuration"

+	// DefaultContainerAnnotationName is an annotation name that can be used to preselect the interesting container
+	// from a pod when running kubectl.
+	DefaultContainerAnnotationName = kubectlPrefix + "default-container"


Yeah, I think there's no need to put that in the API package. Let's keep it inside kubectl only.

soltysh · 2021-02-25T17:32:49Z

staging/src/k8s.io/kubectl/pkg/polymorphichelpers/logsforobject.go

+			var containerName string
+			if len(annotations[defaultLogsContainerAnnotationName]) > 0 {
+				containerName = annotations[defaultLogsContainerAnnotationName]
+				fmt.Fprintf(os.Stderr, "Found deprecated `kubectl.kubernetes.io/default-logs-container` annotation %v in pod/%v\n", containerName, t.Name)


Suggest the new annotation name in this warning.

k8s-ci-robot · 2021-02-26T02:12:27Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: pacoxu, soltysh

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:

~~staging/src/k8s.io/kubectl/OWNERS~~ [soltysh]

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

zhouya0 · 2021-02-26T02:27:50Z

/lgtm

jrsapi · 2021-03-02T07:12:23Z

Greetings @soltysh,
The enhancement team is tracking the following PR:

#97099

With the PR merged, can we mark this enhancement complete for code freeze, or do you have other PR(s) that are being worked on as part of the release?

pacoxu · 2021-03-02T07:46:19Z

@jrsapi

Almost done for development. I am working on testing it. Two small PRs are for reviewing.

logs kubectl: print info for default container annotation usage #99615 a small bugfix is opened now.
exec done
kubectl cp: add some descriptions update kubectl cp flag description #99581 to be reviewed.
kubectl attach need some testing

k8s-ci-robot requested review from soltysh and vishh December 7, 2020 07:36

pacoxu force-pushed the fix/96986 branch from 455a715 to 31f7b2e Compare December 7, 2020 14:53

k8s-ci-robot added the api-review Categorizes an issue or PR as actively needing an API review. label Dec 9, 2020

liggitt reviewed Dec 9, 2020

View reviewed changes

staging/src/k8s.io/api/core/v1/annotation_key_constants.go Outdated Show resolved Hide resolved

k8s-ci-robot removed the api-review Categorizes an issue or PR as actively needing an API review. label Dec 9, 2020

pacoxu changed the title ~~feature: add exec default container according to annotation~~ [Need KEP/sig approve]feature: add exec default container according to annotation Dec 12, 2020

k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-none Denotes a PR that doesn't merit a release note. labels Dec 16, 2020

pacoxu mentioned this pull request Dec 16, 2020

add KEP-2227 for default container used by kubectl kubernetes/enhancements#2189

Merged

pacoxu force-pushed the fix/96986 branch from 31f7b2e to dcce863 Compare December 24, 2020 02:16

pacoxu changed the title ~~[Need KEP/sig approve]feature: add exec default container according to annotation~~ feature: add exec default container according to annotation Dec 24, 2020

dougsland reviewed Jan 2, 2021

View reviewed changes

staging/src/k8s.io/kubectl/pkg/polymorphichelpers/logsforobject.go Show resolved Hide resolved

staging/src/k8s.io/kubectl/pkg/polymorphichelpers/logsforobject.go Show resolved Hide resolved

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 14, 2021

pacoxu force-pushed the fix/96986 branch 3 times, most recently from e45158d to 9c052a0 Compare February 14, 2021 06:20

dougsland reviewed Feb 20, 2021

View reviewed changes

staging/src/k8s.io/kubectl/pkg/cmd/util/helpers.go Show resolved Hide resolved

pacoxu force-pushed the fix/96986 branch from 9c052a0 to e171fe1 Compare February 20, 2021 05:48

feature: use default container annotation for logs and exec

b54e823

- update according to KEP: move getContainerName to helper Signed-off-by: pacoxu <paco.xu@daocloud.io>

pacoxu force-pushed the fix/96986 branch from e171fe1 to b54e823 Compare February 20, 2021 06:30

pacoxu requested review from dougsland and rikatz February 20, 2021 06:58

This comment has been minimized.

Sign in to view

soltysh approved these changes Feb 25, 2021

View reviewed changes

move default container annotation to kubectl pkg

27bd94e

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 26, 2021

k8s-ci-robot assigned zhouya0 Feb 26, 2021

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 26, 2021

k8s-ci-robot merged commit 3cb61de into kubernetes:master Feb 26, 2021

k8s-ci-robot added this to the v1.21 milestone Feb 26, 2021

pacoxu mentioned this pull request Mar 1, 2021

kubectl logs: don't check default container annotation if --all-containers is specified #99569

Merged

mengjiao-liu mentioned this pull request Mar 1, 2021

update kubectl cp flag description #99581

Merged

pacoxu deleted the fix/96986 branch June 23, 2021 05:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

default container behavior with annotation `kubectl.kubernetes.io/default-container` #97099

default container behavior with annotation `kubectl.kubernetes.io/default-container` #97099

pacoxu commented Dec 7, 2020 •

edited

Loading

fejta-bot commented Dec 7, 2020

pacoxu commented Dec 7, 2020

pacoxu commented Dec 8, 2020

pacoxu commented Dec 9, 2020

liggitt commented Dec 9, 2020

pacoxu commented Dec 24, 2020

pacoxu commented Feb 14, 2021

dougsland left a comment

pacoxu commented Feb 20, 2021 •

edited

Loading

This comment has been minimized.

pacoxu commented Feb 20, 2021

pacoxu commented Feb 20, 2021

pacoxu commented Feb 24, 2021

soltysh left a comment

soltysh Feb 25, 2021

soltysh Feb 25, 2021

k8s-ci-robot commented Feb 26, 2021

zhouya0 commented Feb 26, 2021

jrsapi commented Mar 2, 2021 •

edited

Loading

pacoxu commented Mar 2, 2021 •

edited

Loading

default container behavior with annotation kubectl.kubernetes.io/default-container #97099

default container behavior with annotation kubectl.kubernetes.io/default-container #97099

Conversation

pacoxu commented Dec 7, 2020 • edited Loading

fejta-bot commented Dec 7, 2020

pacoxu commented Dec 7, 2020

pacoxu commented Dec 8, 2020

pacoxu commented Dec 9, 2020

liggitt commented Dec 9, 2020

pacoxu commented Dec 24, 2020

pacoxu commented Feb 14, 2021

dougsland left a comment

Choose a reason for hiding this comment

pacoxu commented Feb 20, 2021 • edited Loading

This comment has been minimized.

pacoxu commented Feb 20, 2021

pacoxu commented Feb 20, 2021

pacoxu commented Feb 24, 2021

soltysh left a comment

Choose a reason for hiding this comment

soltysh Feb 25, 2021

Choose a reason for hiding this comment

soltysh Feb 25, 2021

Choose a reason for hiding this comment

k8s-ci-robot commented Feb 26, 2021

zhouya0 commented Feb 26, 2021

jrsapi commented Mar 2, 2021 • edited Loading

pacoxu commented Mar 2, 2021 • edited Loading

default container behavior with annotation `kubectl.kubernetes.io/default-container` #97099

default container behavior with annotation `kubectl.kubernetes.io/default-container` #97099

pacoxu commented Dec 7, 2020 •

edited

Loading

pacoxu commented Feb 20, 2021 •

edited

Loading

jrsapi commented Mar 2, 2021 •

edited

Loading

pacoxu commented Mar 2, 2021 •

edited

Loading