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

default container annotation that to be used by kubectl #2227 #2227

Closed
11 of 13 tasks
pacoxu opened this issue Jan 3, 2021 · 59 comments · Fixed by kubernetes/kubernetes#109254, #3457 or kubernetes/website#39899
Closed
11 of 13 tasks
Assignees
Labels
kind/feature Categorizes issue or PR as related to a new feature. lifecycle/frozen Indicates that an issue or PR should not be auto-closed due to staleness. sig/cli Categorizes an issue or PR as relevant to SIG CLI. stage/stable Denotes an issue tracking an enhancement targeted for Stable/GA status tracked/yes Denotes an enhancement issue is actively being tracked by the Release Team
Milestone

Comments

@pacoxu
Copy link
Member

pacoxu commented Jan 3, 2021

I opened kubernetes/kubernetes#97099
Fixes kubernetes/kubernetes#96986, a similar solution like kubernetes/kubernetes#87809

Enhancement Description

const defaultLogsContainerAnnotationName = "kubectl.kubernetes.io/default-logs-container"
It is deprecated and will be removed in 1.25.

Please keep this description up to date. This will help the Enhancement Team to track the evolution of the enhancement efficiently.

@k8s-ci-robot k8s-ci-robot added the needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. label Jan 3, 2021
@pacoxu
Copy link
Member Author

pacoxu commented Jan 3, 2021

/sig cli

@k8s-ci-robot k8s-ci-robot added sig/cli Categorizes an issue or PR as relevant to SIG CLI. and removed needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Jan 3, 2021
@pacoxu
Copy link
Member Author

pacoxu commented Jan 13, 2021

#2189 is updated now as well as the PR #97099.
/assign

@dougsland @howardjohn Thanks for review of the KEP. What would be the next step for this? I'm not quite familiar with the process.

@dougsland
Copy link
Member

#2189 is updated now as well as the PR #97099.
/assign

@dougsland @howardjohn Thanks for review of the KEP. What would be the next step for this? I'm not quite familiar with the
process.

I have added the enhancement #2189 to SIG Cli agenda asking people to take a look when they have free time. We need more people to review.

@pacoxu pacoxu changed the title default container annotation that to be used by kubectl #2189 default container annotation that to be used by kubectl #2227 Feb 2, 2021
@rikatz
Copy link
Contributor

rikatz commented Feb 2, 2021

Hey,

We have #2227 and #2368 , what is the difference between them?

@pacoxu
Copy link
Member Author

pacoxu commented Feb 3, 2021

@rikatz I closed #2368 and will update the KEP pr.

@soltysh
Copy link
Contributor

soltysh commented Feb 3, 2021

/stage alpha
/milestone v1.21
/kind feature

@k8s-ci-robot k8s-ci-robot added stage/alpha Denotes an issue tracking an enhancement targeted for Alpha status kind/feature Categorizes issue or PR as related to a new feature. labels Feb 3, 2021
@k8s-ci-robot k8s-ci-robot added this to the v1.21 milestone Feb 3, 2021
@annajung annajung added the tracked/yes Denotes an enhancement issue is actively being tracked by the Release Team label Feb 3, 2021
@jrsapi
Copy link

jrsapi commented Feb 5, 2021

Greetings @pacoxu,

This is Joseph v1.21 enhancement shadow checking in. For the enhancement to be included in the 1.21 milestone, it must meet the following criteria:

The KEP must be merged in an implementable state
The KEP must have test plans
The KEP must have graduation criteria
The KEP must have a production readiness review

Starting v1.21, all KEPs must include a production readiness review. Please make sure to take a look at the instructions and update the KEP to include all steps. Let us know if we can be of any assistance.

Thank you!

@soltysh
Copy link
Contributor

soltysh commented Feb 5, 2021

@jrsapi the enhancement is approved both by sig-leads and PRR approvers, it's awaiting small update and should merge on time.

@jrsapi
Copy link

jrsapi commented Feb 8, 2021

Greetings @soltysh @pacoxu,
Thanks for the update. Reminder that enhancement freeze is 2 days away, Feb 9th EOD PST

Enhancements team is aware that KEP update is currently in progress (PR [#2189]. Please make sure the updates get done and pr merged before the freeze. For PRR related questions or to boost the PR for PRR review, please reach out in slack #prod-readiness

Any enhancements that do not complete the following requirements by the freeze will require an exception.

[DONE] The KEP must be merged in an implementable state
[DONE] The KEP must have test plans
[DONE] The KEP must have graduation criteria
[DONE] The KEP must have a production readiness review

@pacoxu
Copy link
Member Author

pacoxu commented Feb 8, 2021

/assign

@soltysh
Copy link
Contributor

soltysh commented Feb 8, 2021

[IN PROGRESS] The KEP must have a production readiness review

@jrsapi #2189 merged on Friday with PRR approved, what else is missing?

@jrsapi
Copy link

jrsapi commented Feb 8, 2021

@soltysh Verified the kep.yaml for PRR. We're good to go. Thanks for your efforts and patience with this new step.

@jrsapi
Copy link

jrsapi commented Feb 18, 2021

Greetings @pacoxu,
Since your Enhancement is scheduled to be in 1.21, please keep in mind the important upcoming dates:
• Tuesday, March 9th: Week 9 - Code Freeze
• Tuesday, March 16th: Week 10 - Docs Placeholder PR deadline
• If this enhancement requires new docs or modification to existing docs, please follow the steps in the Open a placeholder PR doc to open a PR against k/website repo.
As a reminder, please link all of your k/k PR(s) and k/website PR(s) to this issue so we can track them.
Thanks!

@pacoxu
Copy link
Member Author

pacoxu commented Feb 20, 2021

kubernetes/kubernetes#97099 is ready for log and exec.

I will work on it late next week and the first week of March.

Thanks for reminding @jrsapi

@pacoxu
Copy link
Member Author

pacoxu commented Mar 1, 2021

/assign @mengjiao-liu

@k8s-ci-robot
Copy link
Contributor

@pacoxu: GitHub didn't allow me to assign the following users: mengjiao-liu.

Note that only kubernetes members, repo collaborators and people who have commented on this issue/PR can be assigned. Additionally, issues/PRs can only have 10 assignees at the same time.
For more information please see the contributor guide

In response to this:

/assign @mengjiao-liu

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.

@mengjiao-liu
Copy link
Member

mengjiao-liu commented Mar 1, 2021

I will work in cp, which will be finished this week.

@soltysh
Copy link
Contributor

soltysh commented Oct 21, 2022

@soltysh what do we need to do to promote to stable in this case? I moved to 1.27 because we didn't make the opt-in period for 1.26.

sgtm - thx!

@soltysh
Copy link
Contributor

soltysh commented Jan 12, 2023

/label lead-opt-in

@k8s-ci-robot
Copy link
Contributor

@soltysh: The label(s) /label lead-opt-in cannot be applied. These labels are supported: api-review, tide/merge-method-merge, tide/merge-method-rebase, tide/merge-method-squash, team/katacoda, refactor, lead-opted-in, tracked/no, tracked/out-of-tree, tracked/yes. Is this label configured under labels -> additional_labels or labels -> restricted_labels in plugin.yaml?

In response to this:

/label lead-opt-in

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.

@soltysh
Copy link
Contributor

soltysh commented Jan 12, 2023

/label lead-opted-in

@k8s-ci-robot k8s-ci-robot added the lead-opted-in Denotes that an issue has been opted in to a release label Jan 12, 2023
@marosset
Copy link
Contributor

marosset commented Feb 1, 2023

Hello @pacoxu 👋, Enhancements team here.

Just checking in as we approach enhancements freeze on 18:00 PDT Thursday 9th February 2023.

This enhancement is targeting for stage stable for v1.27 (correct me, if otherwise)

Here's where this enhancement currently stands:

  • KEP readme using the latest template has been merged into the k/enhancements repo.
  • KEP status is marked as implementable for latest-milestone: v1.27
  • KEP readme has a updated detailed test plan section filled out
  • KEP readme has up to date graduation criteria
  • KEP has a production readiness review that has been completed and merged into k/enhancements.

For this enhancement it looks like #3457 will address all of the remaining requirements.

The status of this enhancement is marked as at risk. Please keep the issue description up-to-date with appropriate stages as well.
Thank you!

@pacoxu
Copy link
Member Author

pacoxu commented Feb 2, 2023

#3457 is waiting for prr approval.

  • there will be no k/k or k/website pr for this, IMO.

@sftim
Copy link
Contributor

sftim commented Feb 2, 2023

For graduation to GA, we should document kubectl.kubernetes.io/default-logs-container as deprecated. See https://kubernetes.io/docs/reference/labels-annotations-taints/ - it's not there yet.

Also, ideally, we wait a couple of releases between documenting the deprecation and dropping the support.

@pacoxu
Copy link
Member Author

pacoxu commented Feb 3, 2023

For graduation to GA, we should document kubectl.kubernetes.io/default-logs-container as deprecated. See https://kubernetes.io/docs/reference/labels-annotations-taints/ - it's not there yet.

  • kubectl.kubernetes.io/default-logs-container was deprecated in v1.21 when we add kubectl.kubernetes.io/default-container
  • kubectl.kubernetes.io/default-logs-container support was removed in v1.25

https://github.com/kubernetes/website/pull/27095/files

  • kubectl.kubernetes.io/default-logs-container has not been added to the document ever
  • The PR above add kubectl.kubernetes.io/default-container since v1.21

@sftim
Copy link
Contributor

sftim commented Feb 3, 2023

I agree with those things, and we should document the deprecated annotation. Graduation to GA is a trigger point for checking we didn't miss a detail; it turns out that we did.

Once we register an annotation, even as deprecated or no longer respected, we leave it documented forever.

@marosset
Copy link
Contributor

marosset commented Feb 3, 2023

With #3457 merged I'm marking this enhancement as tracked for v1.27.

/remove-label tracked/no
/label tracked/yes

@k8s-ci-robot k8s-ci-robot added tracked/yes Denotes an enhancement issue is actively being tracked by the Release Team and removed tracked/no Denotes an enhancement issue is NOT actively being tracked by the Release Team labels Feb 3, 2023
@katmutua
Copy link
Member

katmutua commented Mar 9, 2023

Hello @pacoxu 👋🏾 !

@katmutua 1.27 Release Docs shadow here. This enhancement is marked as ‘Needs Docs’ for 1.27 release.

Please follow the steps detailed in the documentation to open a PR against dev-1.27 branch in the k/website repo. This PR can be just a placeholder at this time, and must be created by March 16. For more information, please take a look at Documenting for a release to familiarize yourself with the documentation requirements for the release.

If you already have existing open PRs please link them to the description so we can easily track them. Thanks!

@pacoxu
Copy link
Member Author

pacoxu commented Mar 10, 2023

I agree with those things, and we should document the deprecated annotation. Graduation to GA is a trigger point for checking we didn't miss a detail; it turns out that we did.

Once we register an annotation, even as deprecated or no longer respected, we leave it documented forever.

I opened kubernetes/website#39899.

@shatoboar
Copy link

Hi @pacoxu 👋,
Checking in as we approach 1.27 code freeze at 17:00 PDT on Tuesday 14th March 2023.
Please ensure the following items are completed:

Please let me know what other PRs in k/k I should be tracking for this KEP.
As always, we are here to help should questions come up. Thanks!

@pacoxu
Copy link
Member Author

pacoxu commented Mar 15, 2023

If I understand correctly only kubernetes/kubernetes#115046 and kubernetes/website#39899 are merged for v1.27.
And there are no todo items for this KEP.

@Atharva-Shinde
Copy link
Contributor

We usually wait until the code-freeze to check with the authors if there are any more PR's left to acknowledge and merge. And since its now past the code-freeze and there are no action items for this KEP, we can close it :)

/close

@k8s-ci-robot
Copy link
Contributor

@Atharva-Shinde: Closing this issue.

In response to this:

We usually wait until the code-freeze to check with the authors if there are any more PR's left to acknowledge and merge. And since its now past the code-freeze and there are no action items for this KEP, we can close it :)

/close

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.

@salehsedghpour
Copy link
Contributor

/remove-label lead-opted-in

@k8s-ci-robot k8s-ci-robot removed the lead-opted-in Denotes that an issue has been opted in to a release label Jan 6, 2024
@antevens
Copy link

Sorry to resurrect this issue/thread but is there a specific reason why the logs-container annotation was deprecated rather than retained and a default container or a default exec container added?

I'm asking because I have a use-case where I have a daemon container which creates the log output that is relevant for the Pod but I also have a CLI container which is what users exec into for debugging and with default-container deprecating default-logs-container I'm forced to pick one or the other. I'd love to be able to specify a default exec container and a default logs container.

kubectl.kubernetes.io/default-container: "cli"
kubectl.kubernetes.io/default-logs-container: "daemon"

@pacoxu
Copy link
Member Author

pacoxu commented May 23, 2024

An interesting scenario. This KEP currently cannot support this. This KEP focus on define a default container for all kubectl.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/feature Categorizes issue or PR as related to a new feature. lifecycle/frozen Indicates that an issue or PR should not be auto-closed due to staleness. sig/cli Categorizes an issue or PR as relevant to SIG CLI. stage/stable Denotes an issue tracking an enhancement targeted for Stable/GA status tracked/yes Denotes an enhancement issue is actively being tracked by the Release Team
Projects
Status: Tracked