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

Support multi-container pod for "kubectl logs" #45275

Merged
merged 1 commit into from Mar 30, 2018

Conversation

@CaoShuFeng
Member

CaoShuFeng commented May 3, 2017

kubectl logs -l will print logs for pods with the same label, however it doesn't support pods with multi containers. This change adds support to it with --all-containers.

Ussage:
$ kubectl logs my-pod --all-containers
$ kubectl logs -laa=bb --all-containers
$ kubectl logs my-pod my-container --all-containers (err: container should not combined with --all-containers)

Release note:

add --all-containers option to "kubectl log"

Fixes:
kubernetes/kubectl#371

@k8s-reviewable

This comment has been minimized.

k8s-reviewable commented May 3, 2017

This change is Reviewable

@spiffxp

This comment has been minimized.

Member

spiffxp commented May 8, 2017

I'll let @kubernetes/sig-cli-pr-reviews correct me here, but I think multi-container logging isn't supported for a single pod either, seems like this PR would break feature parity

@k8s-ci-robot k8s-ci-robot added the sig/cli label May 8, 2017

@smarterclayton

This comment has been minimized.

Contributor

smarterclayton commented May 9, 2017

I think this is just a bug in that it didn't respect -c at all.

@@ -238,8 +238,11 @@ func (o LogsOptions) RunLogs() error {
switch t := o.Object.(type) {
case *api.PodList:
for _, p := range t.Items {
if err := o.getLogs(&p); err != nil {
return err
for _, c := range p.Spec.Containers {

This comment has been minimized.

@smarterclayton

smarterclayton May 9, 2017

Contributor

You also need to search p.Spec.InitContainers

This comment has been minimized.

@smarterclayton

smarterclayton May 9, 2017

Contributor

Actually, this is already covered by getLogs? Why are you adding it here?

@k8s-merge-robot k8s-merge-robot added size/S and removed size/XS labels May 11, 2017

@CaoShuFeng

This comment has been minimized.

Member

CaoShuFeng commented May 11, 2017

@spiffxp @smarterclayton
Thanks for your review. Your concern makes sense.

I have another idea:
Currently -l doesn't support multi-container pods. So I want to introduce a new --all-containers option.
With this "--all-containers" option, we will not break backward compatibility.
Will wrok like this:
$ kubectl logs my-pod --all-containers
$ kubectl logs -laa=bb --all-containers
$ kubectl logs my-pod my-container --all-containers (err: container should not combined with --all-containers)

@CaoShuFeng CaoShuFeng changed the title from Support multi-container pod for "kubectl logs -l" to Support multi-container pod for "kubectl logs" May 11, 2017

@CaoShuFeng

This comment has been minimized.

Member

CaoShuFeng commented May 11, 2017

Code is updated, please review again.

@k8s-merge-robot k8s-merge-robot added size/M and removed size/S labels May 11, 2017

@@ -145,6 +151,9 @@ func (o *LogsOptions) Complete(f cmdutil.Factory, out io.Writer, cmd *cobra.Comm
default:
return cmdutil.UsageError(cmd, logsUsageStr)
}
if o.AllContainers && len(containerName) > 0 {
return cmdutil.UsageError(cmd, "--all-containers=True should not be specifiled with container name %s", containerName)

This comment has been minimized.

@shiywang

shiywang Jun 15, 2017

Member

nit: --all-containers=true, consistent with example above

This comment has been minimized.

@CaoShuFeng
@spiffxp

This comment has been minimized.

Member

spiffxp commented Jun 15, 2017

@k8s-bot ok to test

@spiffxp

This comment has been minimized.

Member

spiffxp commented Aug 15, 2017

/retest

@k8s-merge-robot

This comment has been minimized.

Contributor

k8s-merge-robot commented Nov 14, 2017

This PR hasn't been active in 90 days. Closing this PR. Please reopen if you would like to work towards merging this change, if/when the PR is ready for the next round of review.

cc @CaoShuFeng @ericchiang

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

@Nowaker

This comment has been minimized.

Nowaker commented Mar 23, 2018

@CaoShuFeng Can you pick it up? It'd be really nice to finalize this fix.

@CaoShuFeng

This comment has been minimized.

Member

CaoShuFeng commented Mar 24, 2018

/reopen

@k8s-ci-robot k8s-ci-robot reopened this Mar 24, 2018

@CaoShuFeng

This comment has been minimized.

Member

CaoShuFeng commented Mar 24, 2018

@Nowaker
I will rebase it.

@soltysh

A few nits.

@@ -144,6 +150,9 @@ func (o *LogsOptions) Complete(f cmdutil.Factory, out io.Writer, cmd *cobra.Comm
default:
return cmdutil.UsageErrorf(cmd, "%s", logsUsageStr)
}
if o.AllContainers && len(containerName) > 0 {

This comment has been minimized.

@soltysh

soltysh Mar 26, 2018

Contributor

This check should go to Validate

This comment has been minimized.

@CaoShuFeng
}
}
// getpodLogs checks whether the obj is a pod and o.AllContainers is set to true.

This comment has been minimized.

@soltysh

soltysh Mar 26, 2018

Contributor

getPodLogs <- uppercase P

This comment has been minimized.

@CaoShuFeng
// getpodLogs checks whether the obj is a pod and o.AllContainers is set to true.
// If so, it retrives all containers' log in the pod.
func (o LogsOptions) getPodLogs(obj runtime.Object) error {
if pod, ok := obj.(*api.Pod); ok && o.AllContainers {

This comment has been minimized.

@soltysh

soltysh Mar 26, 2018

Contributor

Since there's already a type switch where this method is being invoked, why not move the type check there? This will simplify this method to:

if !o.AllContainers {
    return o.getLogs(pod)
}
for ...
for ...
return nil

This comment has been minimized.

@CaoShuFeng
@soltysh

This comment has been minimized.

Contributor

soltysh commented Mar 26, 2018

/assign

Support multi-container pod for "kubectl logs"
kubectl logs -l will print logs for pods with the same label, however
it doesn't support pods with multi containers. This change adds support
to it with --all-containers.

Ussage:
$ kubectl logs my-pod --all-containers
$ kubectl logs -laa=bb --all-containers
$ kubectl logs my-pod my-container --all-containers (err: --all-containers=true should not be specifiled with container name my-container)
@CaoShuFeng

This comment has been minimized.

Member

CaoShuFeng commented Mar 27, 2018

/test pull-kubernetes-integration

@k8s-ci-robot

This comment has been minimized.

Contributor

k8s-ci-robot commented Mar 27, 2018

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

Test name Commit Details Rerun command
pull-kubernetes-e2e-gce-etcd3 25fa6f4 link /test pull-kubernetes-e2e-gce-etcd3
pull-kubernetes-unit 25fa6f4 link /test pull-kubernetes-unit
pull-kubernetes-federation-e2e-gce 25fa6f4 link /test pull-kubernetes-federation-e2e-gce
pull-kubernetes-bazel 25fa6f4 link /test pull-kubernetes-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.

@soltysh

/lgtm
/approve no-issue

@k8s-ci-robot k8s-ci-robot added the lgtm label Mar 30, 2018

@soltysh

This comment has been minimized.

Contributor

soltysh commented Mar 30, 2018

/retest

@k8s-ci-robot

This comment has been minimized.

Contributor

k8s-ci-robot commented Mar 30, 2018

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: CaoShuFeng, 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:

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

@k8s-merge-robot

This comment has been minimized.

Contributor

k8s-merge-robot commented Mar 30, 2018

Automatic merge from submit-queue (batch tested with PRs 60990, 60947, 45275, 60565, 61091). If you want to cherry-pick this change to another branch, please follow the instructions here.

@k8s-merge-robot k8s-merge-robot merged commit cea4284 into kubernetes:master Mar 30, 2018

14 checks passed

Submit Queue Queued to run github e2e tests a second time.
Details
cla/linuxfoundation CaoShuFeng authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-cross Skipped
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-gke Skipped
pull-kubernetes-e2e-kops-aws Job succeeded.
Details
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce Job succeeded.
Details
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details
return err
}
}
return nil
case *api.Pod:
return o.getPodLogs(t)

This comment has been minimized.

@deads2k

deads2k Jul 19, 2018

Contributor

@soltysh @juanvallejo This change breaks when run against general resources. That is against the goals of kubectl

If I'd caught it before 1.11 shipped, we'd've reverted it.

This comment has been minimized.

@CaoShuFeng

CaoShuFeng Jul 20, 2018

Member

I am sorry for the trouble I made here, apologize.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment