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

Allow getting logs directly from deployment, job and statefulset #40927

Merged
merged 2 commits into from
Feb 27, 2017
Merged

Allow getting logs directly from deployment, job and statefulset #40927

merged 2 commits into from
Feb 27, 2017

Conversation

soltysh
Copy link
Contributor

@soltysh soltysh commented Feb 3, 2017 •
edited
Loading

Special notes for your reviewer:
@smarterclayton you asked for it in OpenShift

kubectl logs allows getting logs directly from deployment, job and statefulset

@soltysh soltysh added the release-note Denotes a PR that will be considered when it comes time to generate release notes. label Feb 3, 2017
@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Feb 3, 2017
@k8s-reviewable
Copy link

This change is Reviewable

@soltysh
Copy link
Contributor Author

soltysh commented Feb 3, 2017

@kubernetes/sig-apps-pr-reviews

@k8s-github-robot k8s-github-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Feb 3, 2017
@pwittrock
Copy link
Member

Do tests exist for these?

@soltysh
Copy link
Contributor Author

soltysh commented Feb 3, 2017

@pwittrock I haven't seen any, I'll add one.

@pwittrock
Copy link
Member

That would be wonderful. Thank you.

krmayankk
krmayankk reviewed Feb 4, 2017
View reviewed changes
pkg/kubectl/cmd/util/factory_object_mapping.go Outdated
return nil, fmt.Errorf("invalid label selector: %v", err)
}
sortBy := func(pods []*v1.Pod) sort.Interface { return controller.ByLogging(pods) }
pod, numPods, err := GetFirstPod(clientset.Core(), t.Namespace, selector, 20*time.Second, sortBy)
Copy link

Choose a reason for hiding this comment

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

Will this only work for the first pod ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, consistent with the others. See #19873 for proposed extension.

Copy link
Contributor

Choose a reason for hiding this comment

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

shouldn't we check if replicas > 0 ? AFAIK this function will wait for 20s for a Pod to appear, but I think we should return faster

Copy link
Contributor

Choose a reason for hiding this comment

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

If it's consistent with the others, why would we change the behavior here?

@0xmichalis
Copy link
Contributor

@soltysh I think the help message of the command needs an update

mfojtik
mfojtik reviewed Feb 6, 2017
View reviewed changes
pkg/kubectl/cmd/util/factory_object_mapping.go Outdated
return nil, err
}
if numPods > 1 {
fmt.Fprintf(os.Stderr, "Found %v pods, using pod/%v\n", numPods, pod.Name)
Copy link
Contributor

Choose a reason for hiding this comment

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

why is this error?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Consistently with others.

@soltysh
Copy link
Contributor Author

soltysh commented Feb 8, 2017

I've added more objects per @smarterclayton request, that is StatefulSet, Job and CronJob. I've also added them to AttachablePodForObject. I still need to figure out the test for this.

@k8s-github-robot k8s-github-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Feb 8, 2017
0xmichalis
0xmichalis reviewed Feb 8, 2017
View reviewed changes
pkg/kubectl/cmd/util/factory_object_mapping.go Outdated
@@ -330,6 +390,16 @@ func (f *ring1Factory) AttachablePodForObject(object runtime.Object) (*api.Pod,
sortBy := func(pods []*v1.Pod) sort.Interface { return sort.Reverse(controller.ActivePods(pods)) }
pod, _, err := GetFirstPod(clientset.Core(), t.Namespace, selector, 1*time.Minute, sortBy)
return pod, err

case *extensions.ReplicaSet:
Copy link
Contributor

Choose a reason for hiding this comment

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

There is an open PR for attach, mind leaving that aside?
#40365

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure thing.

@k8s-github-robot k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 14, 2017
@k8s-github-robot k8s-github-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Feb 16, 2017
@soltysh
Copy link
Contributor Author

soltysh commented Feb 16, 2017

@Kargakis I've removed the attach part
@pwittrock added tests, although now that we have a placeholder for more tests probably AttachablePodForObject could deserve its own tests as well :) (as a followup)
@deads2k and @fabianofranz you might be interested in the first commit, for the ClientAccessFactory to be usable for tests I had to change method signatures to use internalclientset.Interface instead of actual type, can one of you approve that change?
@smarterclayton ready for final review

@soltysh soltysh changed the title Allow getting logs directly from deployment Allow getting logs directly from deployment, job and statefulset Feb 16, 2017
0xmichalis
0xmichalis reviewed Feb 16, 2017
View reviewed changes
pkg/kubectl/cmd/logs.go Outdated
kubectl logs job/hello

# Return snapshot logs from from container hello-1 of a job named hello
kubectl logs job/hello -c hello-1`)
Copy link
Contributor

Choose a reason for hiding this comment

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

Provide an example with a different resource since you already have one with jobs above

@0xmichalis
Copy link
Contributor

The relase note needs an update

@0xmichalis
Copy link
Contributor

Apart from a couple of comments and the clientset interface update, lgtm

@soltysh
Copy link
Contributor Author

soltysh commented Feb 16, 2017

@Kargakis nits fixed, release note updated

@soltysh
Copy link
Contributor Author

soltysh commented Feb 27, 2017

Rebased.

@k8s-github-robot k8s-github-robot removed lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Feb 27, 2017
@soltysh soltysh added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 27, 2017
@soltysh
Copy link
Contributor Author

soltysh commented Feb 27, 2017

Re-applying lgtm label, since this was just mechanical rebase.

@k8s-github-robot k8s-github-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 27, 2017
@k8s-github-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

The following people have approved this PR: deads2k, k8s-merge-robot, madhusudancs, soltysh

Needs approval from an approver in each of these OWNERS Files:

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

@soltysh soltysh added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 27, 2017
@soltysh
Copy link
Contributor Author

soltysh commented Feb 27, 2017

@k8s-bot unit test this

@soltysh
Copy link
Contributor Author

soltysh commented Feb 27, 2017

@k8s-bot gce etcd3 e2e test this

@k8s-github-robot
Copy link

Automatic merge from submit-queue (batch tested with PRs 42053, 41282, 42056, 41663, 40927)

@k8s-github-robot k8s-github-robot merged commit 6d9e2af into kubernetes:master Feb 27, 2017
@soltysh soltysh deleted the deployment_logs branch February 27, 2017 21:09
@crassirostris
Copy link

/cc @crassirostris @piosz @fgrzadkowski

@fgrzadkowski
Copy link
Contributor

@soltysh What's the motivation for this PR? How users should use it? AFAIU the semantic is that we will show logs from a random pod within Deployment. How is it useful? To be hones I find it pretty confusing.

@soltysh
Copy link
Contributor Author

soltysh commented May 11, 2017

@fgrzadkowski this is more of a convenience when you have a simple deployment/job/stateful set. We've had similar logic in place for RC/RS already, this was only adding the same for other objects.

@fgrzadkowski
Copy link
Contributor

I see. Though I find it very confusing. So we you saying that this is intended for Deployments/RS/RC with only one instance where it's clear what this mean? Do you know what was the motivation for label selectors?

@soltysh
Copy link
Contributor Author

soltysh commented May 11, 2017

Not sure I understand which label selectors you're talking about, here. The ones used in this impl are only to get the pods owned by the controller and then pick the first pod.

@0xmichalis
Copy link
Contributor

The way pods are picked up is not random but we may want to document it.

@fgrzadkowski
Copy link
Contributor

To be honest I find that logic even more confusing.

I was referring to the following option in kubectl logs:

  -l, --selector='': Selector (label query) to filter on.

Maybe it'd useful to at least let the user choose which pod they are interested in?

@soltysh
Copy link
Contributor Author

soltysh commented May 11, 2017

See #44282 for the relevant discussion.

@KnVerey KnVerey mentioned this pull request Jun 1, 2017
Merged
3 tasks
@piosz piosz removed their request for review July 13, 2017 20:07
@KnVerey KnVerey mentioned this pull request Jul 19, 2017
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@krmayankk krmayankk krmayankk left review comments

@shiywang shiywang shiywang left review comments

@0xmichalis 0xmichalis 0xmichalis left review comments

@smarterclayton smarterclayton smarterclayton left review comments

@mfojtik mfojtik mfojtik left review comments

@crassirostris crassirostris Awaiting requested review from crassirostris

@fgrzadkowski fgrzadkowski Awaiting requested review from fgrzadkowski

Assignees

@smarterclayton smarterclayton

@deads2k deads2k

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
v1.6
Development

Successfully merging this pull request may close these issues.