-
Notifications
You must be signed in to change notification settings - Fork 772
refactor: container logs flagging process #1881
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,116 @@ | ||
| /* | ||
| Copyright The containerd Authors. | ||
|
|
||
| Licensed under the Apache License, Version 2.0 (the "License"); | ||
| you may not use this file except in compliance with the License. | ||
| You may obtain a copy of the License at | ||
|
|
||
| http://www.apache.org/licenses/LICENSE-2.0 | ||
|
|
||
| Unless required by applicable law or agreed to in writing, software | ||
| distributed under the License is distributed on an "AS IS" BASIS, | ||
| WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| See the License for the specific language governing permissions and | ||
| limitations under the License. | ||
| */ | ||
|
|
||
| package main | ||
|
|
||
| import ( | ||
| "fmt" | ||
| "strconv" | ||
|
|
||
| "github.com/containerd/nerdctl/pkg/api/types" | ||
| "github.com/containerd/nerdctl/pkg/cmd/container" | ||
| "github.com/spf13/cobra" | ||
| ) | ||
|
|
||
| func newLogsCommand() *cobra.Command { | ||
| var logsCommand = &cobra.Command{ | ||
| Use: "logs [flags] CONTAINER", | ||
| Args: IsExactArgs(1), | ||
| Short: "Fetch the logs of a container. Currently, only containers created with `nerdctl run -d` are supported.", | ||
| RunE: logsAction, | ||
| ValidArgsFunction: logsShellComplete, | ||
| SilenceUsage: true, | ||
| SilenceErrors: true, | ||
| } | ||
| logsCommand.Flags().BoolP("follow", "f", false, "Follow log output") | ||
| logsCommand.Flags().BoolP("timestamps", "t", false, "Show timestamps") | ||
| logsCommand.Flags().StringP("tail", "n", "all", "Number of lines to show from the end of the logs") | ||
| logsCommand.Flags().String("since", "", "Show logs since timestamp (e.g. 2013-01-02T13:23:37Z) or relative (e.g. 42m for 42 minutes)") | ||
| logsCommand.Flags().String("until", "", "Show logs before a timestamp (e.g. 2013-01-02T13:23:37Z) or relative (e.g. 42m for 42 minutes)") | ||
| return logsCommand | ||
| } | ||
|
|
||
| func processContainerLogsOptions(cmd *cobra.Command) (types.ContainerLogsOptions, error) { | ||
| globalOptions, err := processRootCmdFlags(cmd) | ||
| if err != nil { | ||
| return types.ContainerLogsOptions{}, err | ||
| } | ||
| follow, err := cmd.Flags().GetBool("follow") | ||
| if err != nil { | ||
| return types.ContainerLogsOptions{}, err | ||
| } | ||
| tailArg, err := cmd.Flags().GetString("tail") | ||
| if err != nil { | ||
| return types.ContainerLogsOptions{}, err | ||
| } | ||
| var tail uint | ||
| if tailArg != "" { | ||
| tail, err = getTailArgAsUint(tailArg) | ||
| if err != nil { | ||
| return types.ContainerLogsOptions{}, err | ||
| } | ||
| } | ||
| timestamps, err := cmd.Flags().GetBool("timestamps") | ||
| if err != nil { | ||
| return types.ContainerLogsOptions{}, err | ||
| } | ||
| since, err := cmd.Flags().GetString("since") | ||
| if err != nil { | ||
| return types.ContainerLogsOptions{}, err | ||
| } | ||
| until, err := cmd.Flags().GetString("until") | ||
| if err != nil { | ||
| return types.ContainerLogsOptions{}, err | ||
| } | ||
| return types.ContainerLogsOptions{ | ||
| Stdout: cmd.OutOrStdout(), | ||
| Stderr: cmd.OutOrStderr(), | ||
| GOptions: globalOptions, | ||
| Follow: follow, | ||
| Timestamps: timestamps, | ||
| Tail: tail, | ||
| Since: since, | ||
| Until: until, | ||
| }, nil | ||
| } | ||
|
|
||
| func logsAction(cmd *cobra.Command, args []string) error { | ||
| options, err := processContainerLogsOptions(cmd) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| return container.Logs(cmd.Context(), args[0], options) | ||
| } | ||
|
|
||
| func logsShellComplete(cmd *cobra.Command, args []string, toComplete string) ([]string, cobra.ShellCompDirective) { | ||
| // show container names (TODO: only show containers with logs) | ||
| return shellCompleteContainerNames(cmd, nil) | ||
| } | ||
|
|
||
| // Attempts to parse the argument given to `-n/--tail` as a uint. | ||
| func getTailArgAsUint(arg string) (uint, error) { | ||
| if arg == "all" { | ||
| return 0, nil | ||
| } | ||
| num, err := strconv.Atoi(arg) | ||
| if err != nil { | ||
| return 0, fmt.Errorf("failed to parse `-n/--tail` argument %q: %s", arg, err) | ||
| } | ||
| if num < 0 { | ||
| return 0, fmt.Errorf("`-n/--tail` argument must be positive, got: %d", num) | ||
| } | ||
| return uint(num), nil | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -14,93 +14,42 @@ | |
| limitations under the License. | ||
| */ | ||
|
|
||
| package main | ||
| package container | ||
|
|
||
| import ( | ||
| "context" | ||
| "fmt" | ||
| "os" | ||
| "os/signal" | ||
| "strconv" | ||
| "syscall" | ||
|
|
||
| "github.com/containerd/containerd" | ||
| "github.com/containerd/nerdctl/pkg/api/types" | ||
| "github.com/containerd/nerdctl/pkg/api/types/cri" | ||
| "github.com/containerd/nerdctl/pkg/clientutil" | ||
| "github.com/containerd/nerdctl/pkg/idutil/containerwalker" | ||
| "github.com/containerd/nerdctl/pkg/labels" | ||
| "github.com/containerd/nerdctl/pkg/labels/k8slabels" | ||
| "github.com/containerd/nerdctl/pkg/logging" | ||
| "github.com/sirupsen/logrus" | ||
| "github.com/spf13/cobra" | ||
| ) | ||
|
|
||
| func newLogsCommand() *cobra.Command { | ||
| var logsCommand = &cobra.Command{ | ||
| Use: "logs [flags] CONTAINER", | ||
| Args: IsExactArgs(1), | ||
| Short: "Fetch the logs of a container. Currently, only containers created with `nerdctl run -d` are supported.", | ||
| RunE: logsAction, | ||
| ValidArgsFunction: logsShellComplete, | ||
| SilenceUsage: true, | ||
| SilenceErrors: true, | ||
| } | ||
| logsCommand.Flags().BoolP("follow", "f", false, "Follow log output") | ||
| logsCommand.Flags().BoolP("timestamps", "t", false, "Show timestamps") | ||
| logsCommand.Flags().StringP("tail", "n", "all", "Number of lines to show from the end of the logs") | ||
| logsCommand.Flags().String("since", "", "Show logs since timestamp (e.g. 2013-01-02T13:23:37Z) or relative (e.g. 42m for 42 minutes)") | ||
| logsCommand.Flags().String("until", "", "Show logs before a timestamp (e.g. 2013-01-02T13:23:37Z) or relative (e.g. 42m for 42 minutes)") | ||
| return logsCommand | ||
| } | ||
|
|
||
| func logsAction(cmd *cobra.Command, args []string) error { | ||
| globalOptions, err := processRootCmdFlags(cmd) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| dataStore, err := clientutil.DataStore(globalOptions.DataRoot, globalOptions.Address) | ||
| func Logs(ctx context.Context, container string, options types.ContainerLogsOptions) error { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you add a brief comment here? nit: also maybe change
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Do you mean adding a function comment for
Maybe
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Rebased.
Keeping it as is as there's no conflict. |
||
| dataStore, err := clientutil.DataStore(options.GOptions.DataRoot, options.GOptions.Address) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| switch globalOptions.Namespace { | ||
| switch options.GOptions.Namespace { | ||
| case "moby": | ||
| logrus.Warn("Currently, `nerdctl logs` only supports containers created with `nerdctl run -d` or CRI") | ||
| } | ||
| client, ctx, cancel, err := clientutil.NewClient(cmd.Context(), globalOptions.Namespace, globalOptions.Address) | ||
| client, ctx, cancel, err := clientutil.NewClient(ctx, options.GOptions.Namespace, options.GOptions.Address) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| defer cancel() | ||
|
|
||
| follow, err := cmd.Flags().GetBool("follow") | ||
| if err != nil { | ||
| return err | ||
| } | ||
| tailArg, err := cmd.Flags().GetString("tail") | ||
| if err != nil { | ||
| return err | ||
| } | ||
| var tail uint | ||
| if tailArg != "" { | ||
| tail, err = getTailArgAsUint(tailArg) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| } | ||
| timestamps, err := cmd.Flags().GetBool("timestamps") | ||
| if err != nil { | ||
| return err | ||
| } | ||
| since, err := cmd.Flags().GetString("since") | ||
| if err != nil { | ||
| return err | ||
| } | ||
| until, err := cmd.Flags().GetString("until") | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| stopChannel := make(chan os.Signal, 1) | ||
| // catch OS signals: | ||
| signal.Notify(stopChannel, syscall.SIGTERM, syscall.SIGINT) | ||
|
|
@@ -130,10 +79,10 @@ func logsAction(cmd *cobra.Command, args []string) error { | |
| return err | ||
| } | ||
| if status.Status != containerd.Running { | ||
| follow = false | ||
| options.Follow = false | ||
| } | ||
|
|
||
| if follow { | ||
| if options.Follow { | ||
| waitCh, err := task.Wait(ctx) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to get wait channel for task %#v: %s", task, err) | ||
|
|
@@ -152,26 +101,25 @@ func logsAction(cmd *cobra.Command, args []string) error { | |
| Namespace: l[labels.Namespace], | ||
| DatastoreRootPath: dataStore, | ||
| LogPath: logPath, | ||
| Follow: follow, | ||
| Timestamps: timestamps, | ||
| Tail: tail, | ||
| Since: since, | ||
| Until: until, | ||
| Follow: options.Follow, | ||
| Timestamps: options.Timestamps, | ||
| Tail: options.Tail, | ||
| Since: options.Since, | ||
| Until: options.Until, | ||
| } | ||
| logViewer, err := logging.InitContainerLogViewer(l, logViewOpts, stopChannel) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| return logViewer.PrintLogsTo(os.Stdout, os.Stderr) | ||
| return logViewer.PrintLogsTo(options.Stdout, options.Stderr) | ||
| }, | ||
| } | ||
| req := args[0] | ||
| n, err := walker.Walk(ctx, req) | ||
| n, err := walker.Walk(ctx, container) | ||
| if err != nil { | ||
| return err | ||
| } else if n == 0 { | ||
| return fmt.Errorf("no such container %s", req) | ||
| return fmt.Errorf("no such container %s", container) | ||
| } | ||
| return nil | ||
| } | ||
|
|
@@ -192,23 +140,3 @@ func getLogPath(ctx context.Context, container containerd.Container) (string, er | |
|
|
||
| return meta.LogPath, nil | ||
| } | ||
|
|
||
| func logsShellComplete(cmd *cobra.Command, args []string, toComplete string) ([]string, cobra.ShellCompDirective) { | ||
| // show container names (TODO: only show containers with logs) | ||
| return shellCompleteContainerNames(cmd, nil) | ||
| } | ||
|
|
||
| // Attempts to parse the argument given to `-n/--tail` as a uint. | ||
| func getTailArgAsUint(arg string) (uint, error) { | ||
| if arg == "all" { | ||
| return 0, nil | ||
| } | ||
| num, err := strconv.Atoi(arg) | ||
| if err != nil { | ||
| return 0, fmt.Errorf("failed to parse `-n/--tail` argument %q: %s", arg, err) | ||
| } | ||
| if num < 0 { | ||
| return 0, fmt.Errorf("`-n/--tail` argument must be positive, got: %d", num) | ||
| } | ||
| return uint(num), nil | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can keep the file name unchanged as
logs.go(andlogs_test.go)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, I was following the pattern in #1869 as
container logsis the canonical name of this command. @AkihiroSuda Do you know which pattern we should follow?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ops, didn't notice that. I'm okay with either one.
Currently it seems we only prefix
container/imageto file names if there're mutiple commands (e.g.,container_inspect.go/container_prune.go v.s. image_inspect.go/image_prune.go;run.goinstead ofcontainer_run.go).But yeah adding a prefix can make those files closer :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My thoughts:
pkg/cmd, maybe we want to do so consistently in other places.Just my 2 cents :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes,
containers_logs.gois preferred as it corresponds to the canonical formnerdctl containers logsThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wondering if I should do a one-shot PR for this. Maybe after #1900 is merged? /cc @Zheaoli
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, @davidhsingyuchen, it's better to be merged after #1900