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

Allow a selector when retrieving logs #32752

Merged
merged 1 commit into from
Dec 9, 2016
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
74 changes: 57 additions & 17 deletions pkg/kubectl/cmd/logs.go
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,9 @@ var (
# Return snapshot logs from pod nginx with only one container
kubectl logs nginx

# Return snapshot logs for the pods defined by label app=nginx
kubectl logs -lapp=nginx

# Return snapshot of previous terminated ruby container logs from pod web-1
kubectl logs -p -c ruby web-1

Expand All @@ -51,6 +54,8 @@ var (

# Show all logs from pod nginx written in the last hour
kubectl logs --since=1h nginx`)

Choose a reason for hiding this comment

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

can you update the comment here to give some examples of what this supports. Like kubectl logs -lapp=nginx. I am guessing this will only select pods with that label. Also what happens if the pod contains multiple containers, does it show logs for all container as well ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If you have multiple containers, it will fail. See the discussion below.

selectorTail int64 = 10
)

const (
Expand Down Expand Up @@ -91,33 +96,39 @@ func NewCmdLogs(f cmdutil.Factory, out io.Writer) *cobra.Command {
if err := o.Validate(); err != nil {
cmdutil.CheckErr(cmdutil.UsageError(cmd, err.Error()))
}
_, err := o.RunLogs()
cmdutil.CheckErr(err)
cmdutil.CheckErr(o.RunLogs())
},
Aliases: []string{"log"},
}
cmd.Flags().BoolP("follow", "f", false, "Specify if the logs should be streamed.")
cmd.Flags().Bool("timestamps", false, "Include timestamps on each line in the log output")
cmd.Flags().Int64("limit-bytes", 0, "Maximum bytes of logs to return. Defaults to no limit.")
cmd.Flags().BoolP("previous", "p", false, "If true, print the logs for the previous instance of the container in a pod if it exists.")
cmd.Flags().Int64("tail", -1, "Lines of recent log file to display. Defaults to -1, showing all log lines.")
cmd.Flags().Int64("tail", -1, "Lines of recent log file to display. Defaults to -1 with no selector, showing all log lines otherwise 10, if a selector is provided.")
cmd.Flags().String("since-time", "", "Only return logs after a specific date (RFC3339). Defaults to all logs. Only one of since-time / since may be used.")
cmd.Flags().Duration("since", 0, "Only return logs newer than a relative duration like 5s, 2m, or 3h. Defaults to all logs. Only one of since-time / since may be used.")
cmd.Flags().StringP("container", "c", "", "Print the logs of this container")

cmd.Flags().Bool("interactive", false, "If true, prompt the user for input when required.")
cmd.Flags().MarkDeprecated("interactive", "This flag is no longer respected and there is no replacement.")
cmdutil.AddInclude3rdPartyFlags(cmd)
cmd.Flags().StringP("selector", "l", "", "Selector (label query) to filter on.")
return cmd
}

func (o *LogsOptions) Complete(f cmdutil.Factory, out io.Writer, cmd *cobra.Command, args []string) error {
containerName := cmdutil.GetFlagString(cmd, "container")
selector := cmdutil.GetFlagString(cmd, "selector")
switch len(args) {
case 0:
return cmdutil.UsageError(cmd, logsUsageStr)
if len(selector) == 0 {
return cmdutil.UsageError(cmd, logsUsageStr)
}
case 1:
o.ResourceArg = args[0]
if len(selector) != 0 {
return cmdutil.UsageError(cmd, "only a selector (-l) or a POD name is allowed")
}
case 2:
if cmd.Flag("container").Changed {
return cmdutil.UsageError(cmd, "only one of -c or an inline [CONTAINER] arg is allowed")
Expand Down Expand Up @@ -162,18 +173,35 @@ func (o *LogsOptions) Complete(f cmdutil.Factory, out io.Writer, cmd *cobra.Comm
o.ClientMapper = resource.ClientMapperFunc(f.ClientForMapping)
o.Out = out

if len(selector) != 0 {
if logOptions.Follow {
return cmdutil.UsageError(cmd, "only one of follow (-f) or selector (-l) is allowed")

Choose a reason for hiding this comment

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

is this because it will be cpu intensive for the master ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The rules stated in the issue was to disallow follow when using a selector. The concern was that you could be following more than one log which would be more an issue with your cluster and then you would also have to tag every log line.

Copy link
Contributor

Choose a reason for hiding this comment

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

It has to do with interleaving logs (@smarterclayton was talking about) in case when getting logs from several pods.

}
if len(logOptions.Container) != 0 {
return cmdutil.UsageError(cmd, "a container cannot be specified when using a selector (-l)")
}
if logOptions.TailLines == nil {
logOptions.TailLines = &selectorTail
Copy link
Contributor

Choose a reason for hiding this comment

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

Why using a selector would give me different behavior than without one? By default this is not set, iow. it's unlimited. I'd like this to be consistent.

Copy link
Contributor Author

@fraenkel fraenkel Oct 10, 2016

Choose a reason for hiding this comment

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

We can allow this. I was just follow what was described in the issue. I am assuming the concern was that your selector may match pods that are not guaranteed to have the matching container. One could choose to just ignore pods that do not have the container, although the current code would display an error. So you would have different behavior either one the parsing or on the output.

Copy link
Contributor

Choose a reason for hiding this comment

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

I missed that comment, ok, but please make it clear in the command description.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated.

}
}

mapper, typer := f.Object()
decoder := f.Decoder(true)
if o.Object == nil {
infos, err := resource.NewBuilder(mapper, typer, o.ClientMapper, decoder).
builder := resource.NewBuilder(mapper, typer, o.ClientMapper, decoder).
NamespaceParam(o.Namespace).DefaultNamespace().
ResourceNames("pods", o.ResourceArg).
SingleResourceType().
Do().Infos()
SingleResourceType()
if o.ResourceArg != "" {
builder.ResourceNames("pods", o.ResourceArg)
}
if selector != "" {
builder.ResourceTypes("pods").SelectorParam(selector)
}
infos, err := builder.Do().Infos()
if err != nil {
return err
}
if len(infos) != 1 {
if selector == "" && len(infos) != 1 {
return errors.New("expected a resource")
}
o.Object = infos[0].Object
Expand All @@ -183,9 +211,6 @@ func (o *LogsOptions) Complete(f cmdutil.Factory, out io.Writer, cmd *cobra.Comm
}

func (o LogsOptions) Validate() error {
if len(o.ResourceArg) == 0 {
return errors.New("a pod must be specified")
}
logsOptions, ok := o.Options.(*api.PodLogOptions)
if !ok {
return errors.New("unexpected logs options object")
Expand All @@ -198,17 +223,32 @@ func (o LogsOptions) Validate() error {
}

// RunLogs retrieves a pod log
func (o LogsOptions) RunLogs() (int64, error) {
req, err := o.LogsForObject(o.Object, o.Options)
func (o LogsOptions) RunLogs() error {
switch t := o.Object.(type) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why this code isn't part of the LogsForObject defined in pkg/kubectl/cmd/util/factory.go?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It can move there but I am now I have to build and return a fake Request since it would need to chain all the Pod logs in some manner.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes please, move.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looking closer, this isn't so simple without making the request completely bogus. Most of the code would end up living inside this fake request instead of LogsForObject.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not quite convinced.You have to iterate over list of Pods and get logs for each of them. You're basically doing just that in here. What I'm asking is to move that code into the other file (pkg/kubectl/cmd/util/factory.go) which actually invokes client calls to get logs for each container. If that is not clear lemme know, I can submit a PR against your branch with the solution.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, you lost me again. It might be easier for you do just show me what you want rather than me guess. I guess I don't see how it will all work in the end.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think I only now realized what you're speaking about. Still the biggest problem here is that depending on whether you use the selector or now you're getting different results. Since only PodList will trigger getting logs from all the containers, whereas single Pod will only return the first container (if none is specified). We need to make this consistent, I'm trying to come up with some reasonable solution.

Copy link
Contributor

Choose a reason for hiding this comment

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

I've re-read the issue and all the comments on this issue and I see that @smarterclayton was clear that for this PR we should have only the current behavior (just the first container) and the followup should handle --all-containers flag that will solve the problem globally.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No problem. Will adjust the behavior and add --all-containers.

Copy link
Contributor

Choose a reason for hiding this comment

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

My proposed changes are I've included in fraenkel#1

case *api.PodList:
for _, p := range t.Items {
if err := o.getLogs(&p); err != nil {
return err
}
}
return nil
default:
return o.getLogs(o.Object)
}
}

func (o LogsOptions) getLogs(obj runtime.Object) error {
req, err := o.LogsForObject(obj, o.Options)
if err != nil {
return 0, err
return err
}

readCloser, err := req.Stream()
if err != nil {
return 0, err
return err
}
defer readCloser.Close()

return io.Copy(o.Out, readCloser)
_, err = io.Copy(o.Out, readCloser)
return err
}