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

Regression in kubectl logs: the --all-containers=true option doesn't work in master #67314

Closed
m1kola opened this issue Aug 12, 2018 · 1 comment · Fixed by #67316
Closed

Regression in kubectl logs: the --all-containers=true option doesn't work in master #67314

m1kola opened this issue Aug 12, 2018 · 1 comment · Fixed by #67316
Labels
kind/bug Categorizes issue or PR as related to a bug. sig/cli Categorizes an issue or PR as relevant to SIG CLI.

Comments

@m1kola
Copy link
Member

m1kola commented Aug 12, 2018 •
edited

Is this a BUG REPORT or FEATURE REQUEST?:

/kind bug

What happened:

The --all-containers=true option was added in #45275. It allows to get logs from pods/deployments with multiple containers.

It doesn't seem to work in the master branch (I tested on 6274590).

So the following commands:

kubectl logs -l somelabel=somevalue --all-containers=true
kubectl logs pod-name-with-multiple-containers --all-containers=true

shows an errors like this:

Error from server (BadRequest): a container name must be specified for pod pod-name-with-multiple-containers, choose one of: [container-1 container-2]

What you expected to happen:

kubectl logs prints logs from all pods event if pod has multiple containers.

How to reproduce it (as minimally and precisely as possible):

  1. Create a pod with multiple containers
  2. Run kubectl logs pod-name-with-multiple-containers --all-containers=true

Anything else we need to know?:

It works in kubectl 1.11:

Client Version: version.Info{Major:"1", Minor:"11", GitVersion:"v1.11.2", GitCommit:"bb9ffb1654d4a729bb4cec18ff088eacc153c239", GitTreeState:"clean", BuildDate:"2018-08-07T23:17:28Z", GoVersion:"go1.10.3", Compiler:"gc", Platform:"darwin/amd64"}
Server Version: version.Info{Major:"1", Minor:"10", GitVersion:"v1.10.0", GitCommit:"fc32d2f3698e36b93322a3465f63a14e9f0eaead", GitTreeState:"clean", BuildDate:"2018-04-10T12:46:31Z", GoVersion:"go1.9.4", Compiler:"gc", Platform:"linux/amd64"}

but doesn't work in kubectl built from current master:

Client Version: version.Info{Major:"1", Minor:"12+", GitVersion:"v1.12.0-alpha.1.409+62745905188c6b-dirty", GitCommit:"62745905188c6b5e0d3ae4f1cc1d20ca58d27a87", GitTreeState:"dirty", BuildDate:"2018-08-12T13:01:41Z", GoVersion:"go1.10.3", Compiler:"gc", Platform:"darwin/amd64"}
Server Version: version.Info{Major:"1", Minor:"10", GitVersion:"v1.10.0", GitCommit:"fc32d2f3698e36b93322a3465f63a14e9f0eaead", GitTreeState:"clean", BuildDate:"2018-04-10T12:46:31Z", GoVersion:"go1.9.4", Compiler:"gc", Platform:"linux/amd64"}

Environment:

  • Kubernetes version (use kubectl version): see above
  • Cloud provider or hardware configuration: minikube & GKE

/sig cli
@k8s-ci-robot k8s-ci-robot added kind/bug Categorizes issue or PR as related to a bug. sig/cli Categorizes an issue or PR as relevant to SIG CLI. labels Aug 12, 2018
@m1kola
Copy link
Member Author

m1kola commented Aug 12, 2018

I think, I found the reason of this issue. Regression was introduced in #66398

I'll fix it and add some extra tests.

This was referenced Aug 12, 2018
Merged
Open
k8s-github-robot pushed a commit that referenced this issue Aug 17, 2018
Merge pull request #67316 from m1kola/67314_fix_regression
4fff352 
Automatic merge from submit-queue (batch tested with PRs 66920, 67316, 67363, 67528, 66963). If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>.

Fixes regression in kubectl logs: the --all-containers=true option didn't work

**What this PR does / why we need it**:

Fixes regression introduced in #66398 and adds unit tests for logging with `--all-containers=true`. See #67314 for more details.

**Which issue(s) this PR fixes**:
Fixes #67314

**Special notes for your reviewer**:

I didn't cover cases with `coreinternal.PodList` and `coreinternal.Pod` in tests, because it doesn't look like we need them: I didn't manage to find any callers of the `logsForObjectWithClient` and `logsForObject` functions, so, probably, we can remove them. I'll double check and try to do that separately once this PR is merged.

**Release note**:
```release-note
NONE
```

/sig cli
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
kind/bug Categorizes issue or PR as related to a bug. sig/cli Categorizes an issue or PR as relevant to SIG CLI.
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Fixes regression in kubectl logs: the --all-containers=true option didn't work
2 participants
@m1kola @k8s-ci-robot