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

Fix kubectl logs for init containers #27042

Merged
merged 1 commit into from
Jun 17, 2016
Merged

Fix kubectl logs for init containers #27042

merged 1 commit into from
Jun 17, 2016

Conversation

lukaszo
Copy link
Contributor

@lukaszo lukaszo commented Jun 8, 2016

Related issues: #25818 #27040

@k8s-bot
Copy link

k8s-bot commented Jun 8, 2016

Can one of the admins verify that this patch is reasonable to test? If so, please reply "ok to test".
(Note: "add to whitelist" is no longer supported. Please update configurations in kubernetes/test-infra/jenkins/job-configs/kubernetes-jenkins-pull instead.)

This message may repeat a few times in short succession due to jenkinsci/ghprb-plugin#292. Sorry.

Otherwise, if this message is too spammy, please complain to ixdy.

2 similar comments
@k8s-bot
Copy link

k8s-bot commented Jun 8, 2016

Can one of the admins verify that this patch is reasonable to test? If so, please reply "ok to test".
(Note: "add to whitelist" is no longer supported. Please update configurations in kubernetes/test-infra/jenkins/job-configs/kubernetes-jenkins-pull instead.)

This message may repeat a few times in short succession due to jenkinsci/ghprb-plugin#292. Sorry.

Otherwise, if this message is too spammy, please complain to ixdy.

@k8s-bot
Copy link

k8s-bot commented Jun 8, 2016

Can one of the admins verify that this patch is reasonable to test? If so, please reply "ok to test".
(Note: "add to whitelist" is no longer supported. Please update configurations in kubernetes/test-infra/jenkins/job-configs/kubernetes-jenkins-pull instead.)

This message may repeat a few times in short succession due to jenkinsci/ghprb-plugin#292. Sorry.

Otherwise, if this message is too spammy, please complain to ixdy.

@k8s-github-robot k8s-github-robot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. release-note-label-needed labels Jun 8, 2016
dims
dims reviewed Jun 8, 2016
View reviewed changes
pkg/kubelet/kubelet.go
@@ -2845,6 +2845,10 @@ func (kl *Kubelet) validateContainerLogStatus(podName string, podStatus *api.Pod
var cID string

cStatus, found := api.GetContainerStatus(podStatus.ContainerStatuses, containerName)
// if not found, check the init containers
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lukaszo Trying to understand, this covers only the case where the container name is specified in the command line. right? (and not the problems mentioned in #27040)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one fixes the issue of pulling init container's log. Not addressed #27040.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dims yes, exactly. Working on #27040 without this doesn't make sense

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dchen1107 what is a init container logs

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@krmayankk see here #23666

@dchen1107 dchen1107 added release-note-none Denotes a PR that doesn't merit a release note. and removed release-note-label-needed labels Jun 8, 2016
@dchen1107 dchen1107 added this to the v1.3 milestone Jun 8, 2016
@dchen1107
Copy link
Member

LGTM

cc/ @smarterclayton

@dchen1107 dchen1107 added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 8, 2016
@smarterclayton
Copy link
Contributor

Also Lgtm, thanks

@k8s-github-robot k8s-github-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 13, 2016
@dchen1107
Copy link
Member

@k8s-bot test this github issue: #27276

@dchen1107
Copy link
Member

The failure has nothing to do with this pr. Reapply lgtm

@dchen1107 dchen1107 added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 13, 2016
@k8s-bot
Copy link

k8s-bot commented Jun 14, 2016

Can one of the admins verify that this patch is reasonable to test? If so, please reply "ok to test".
(Note: "add to whitelist" is no longer supported. Please update configurations in kubernetes/test-infra/jenkins/job-configs/kubernetes-jenkins-pull instead.)

This message may repeat a few times in short succession due to jenkinsci/ghprb-plugin#292. Sorry.

Otherwise, if this message is too spammy, please complain to ixdy.

1 similar comment
@k8s-bot
Copy link

k8s-bot commented Jun 15, 2016

Can one of the admins verify that this patch is reasonable to test? If so, please reply "ok to test".
(Note: "add to whitelist" is no longer supported. Please update configurations in kubernetes/test-infra/jenkins/job-configs/kubernetes-jenkins-pull instead.)

This message may repeat a few times in short succession due to jenkinsci/ghprb-plugin#292. Sorry.

Otherwise, if this message is too spammy, please complain to ixdy.

@krmayankk
Copy link

@lukaszo thanks for the pointer, this feature looks very interesting and usable. Can someone also point to examples of how to use it ? I very often see the PR for new features get merged without an example in the docs/user guides or contrib. It would help to make that as requirement for these new PR merges imho

@lukaszo
Copy link
Contributor Author

lukaszo commented Jun 15, 2016

@krmayankk init containers are for example used in petsets https://github.com/kubernetes/contrib/blob/master/pets/mysql/galera/mysql-galera.yaml#L32-L76

@smarterclayton
Copy link
Contributor

We definitely want people to try alpha features, but they're also somewhat alpha and subject to change. We'll probably have some core documentation for petset but I would expect them to change in 1.4 as we learn more about what we need to implement.

@idvoretskyi
Copy link
Member

@smarterclayton so, are we going to move this feature to 1.4 milestone?

@smarterclayton
Copy link
Contributor

Not sure what you're asking? I had no plans to move this PR out of v1.3 since it's contained, reviewed, and has been in the queue for a few days.

@lukaszo
Copy link
Contributor Author

lukaszo commented Jun 16, 2016

@smarterclayton are P3 PRs still merged?

@smarterclayton
Copy link
Contributor

Anything in the queue is being left in the queue - we may decide to cut the RC without everything in the queue being merged but that decision hasn't been made.

This also probably isn't a P3.

@smarterclayton smarterclayton added the priority/backlog Higher priority than priority/awaiting-more-evidence. label Jun 16, 2016
@smarterclayton smarterclayton added priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. and removed priority/backlog Higher priority than priority/awaiting-more-evidence. labels Jun 16, 2016
@smarterclayton
Copy link
Contributor

This is P1 because it would block debugging of pet sets for end users. We can kick it to P2 if necessary, but I'd be more concerned without this than with other PRs related to init containers.

@k8s-bot
Copy link

k8s-bot commented Jun 17, 2016

GCE e2e build/test passed for commit 07d13c1.

@k8s-github-robot
Copy link

@k8s-bot test this [submit-queue is verifying that this PR is safe to merge]

@k8s-bot
Copy link

k8s-bot commented Jun 17, 2016

GCE e2e build/test passed for commit 07d13c1.

@k8s-github-robot
Copy link

Automatic merge from submit-queue

@k8s-github-robot k8s-github-robot merged commit 983b478 into kubernetes:master Jun 17, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@dchen1107 dchen1107

Labels
area/usability lgtm "Looks good to me", indicates that a PR is ready to be merged. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. release-note-none Denotes a PR that doesn't merit a release note. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Milestone
v1.3
Development

Successfully merging this pull request may close these issues.
9 participants
@lukaszo @k8s-bot @dchen1107 @smarterclayton @krmayankk @idvoretskyi @k8s-github-robot @dims @googlebot