From 82c9c0e58ddcc1f791f85da1f1eaa66891c70704 Mon Sep 17 00:00:00 2001 From: Rico Chiu Date: Tue, 6 Jun 2023 14:33:07 -0700 Subject: [PATCH 1/7] Add info subcommand to CLI --- cli/README.md | 6 +- cli/src/alluxio.org/cli/cmd/info/cache.go | 68 +++++++++ cli/src/alluxio.org/cli/cmd/info/collect.go | 147 ++++++++++++++++++++ cli/src/alluxio.org/cli/cmd/info/info.go | 27 ++++ cli/src/alluxio.org/cli/cmd/info/master.go | 49 +++++++ cli/src/alluxio.org/cli/cmd/info/report.go | 73 ++++++++++ cli/src/alluxio.org/cli/main.go | 2 + 7 files changed, 371 insertions(+), 1 deletion(-) create mode 100644 cli/src/alluxio.org/cli/cmd/info/cache.go create mode 100644 cli/src/alluxio.org/cli/cmd/info/collect.go create mode 100644 cli/src/alluxio.org/cli/cmd/info/info.go create mode 100644 cli/src/alluxio.org/cli/cmd/info/master.go create mode 100644 cli/src/alluxio.org/cli/cmd/info/report.go diff --git a/cli/README.md b/cli/README.md index 23f5eb240cad..0d8965f01f6e 100644 --- a/cli/README.md +++ b/cli/README.md @@ -29,12 +29,16 @@ bin/cli.sh item mark --unset name where it is expected that either `--set` or `--unset` are specified. This is preferred over the alternative of two separate commands with `setMark` and `unsetMark` as the operations. +## User input validation + +User inputs should be validated by the CLI command as much as possible as opposed to the resulting invocation. + ## Output conventions and java invocation A majority of commands result in invoking a java class with arguments to execute the expected operation and possibly return some output. The output returned from the java invocation should tend towards being plain or machine parseable, such as a JSON formatted string, rather than terminal friendly or human readable format. -When appropriate, the CLI command will default to formatting this output to be terminal friendly, with an option to output in a machine parseable format such as JSON. +When appropriate, the CLI command will default to formatting this output to be terminal friendly, with an option to output in a machine parseable format. ## References diff --git a/cli/src/alluxio.org/cli/cmd/info/cache.go b/cli/src/alluxio.org/cli/cmd/info/cache.go new file mode 100644 index 000000000000..c17bb1a3e3e7 --- /dev/null +++ b/cli/src/alluxio.org/cli/cmd/info/cache.go @@ -0,0 +1,68 @@ +/* + * The Alluxio Open Foundation licenses this work under the Apache License, version 2.0 + * (the "License"). You may not use this work except in compliance with the License, which is + * available at www.apache.org/licenses/LICENSE-2.0 + * + * This software is distributed on an "AS IS" basis, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, + * either express or implied, as more fully set forth in the License. + * + * See the NOTICE file distributed with this work for information regarding copyright ownership. + */ + +package info + +import ( + "strings" + + "github.com/spf13/cobra" + + "alluxio.org/cli/env" +) + +var Cache = &CacheCommand{ + BaseJavaCommand: &env.BaseJavaCommand{ + CommandName: "cache", + JavaClassName: "alluxio.cli.fsadmin.FileSystemAdminShell", + }, +} + +type CacheCommand struct { + *env.BaseJavaCommand + + liveWorkers bool + lostWorkers bool + workersList []string // list of worker hostnames or ip addresses +} + +func (c *CacheCommand) Base() *env.BaseJavaCommand { + return c.BaseJavaCommand +} + +func (c *CacheCommand) ToCommand() *cobra.Command { + cmd := c.Base().InitRunJavaClassCmd(&cobra.Command{ + Use: Cache.CommandName, + Short: "Reports worker capacity information", + Args: cobra.NoArgs, + RunE: func(cmd *cobra.Command, args []string) error { + return c.Run(nil) + }, + }) + cmd.Flags().BoolVar(&c.liveWorkers, "live", false, "Only show live workers for capacity report") + cmd.Flags().BoolVar(&c.lostWorkers, "lost", false, "Only show lost workers for capacity report") + cmd.Flags().StringSliceVar(&c.workersList, "workers", nil, "Only show specified workers for capacity report, labeled by hostname or IP address") + cmd.MarkFlagsMutuallyExclusive("live", "lost", "workers") + return cmd +} + +func (c *CacheCommand) Run(_ []string) error { + // TODO: output all in a serializable format and filter/trim as specified by flags + args := []string{"report", "capacity"} + if c.liveWorkers { + args = append(args, "-live") + } else if c.lostWorkers { + args = append(args, "-lost") + } else if len(c.workersList) > 0 { + args = append(args, "-workers", strings.Join(c.workersList, ",")) + } + return c.Base().Run(args) +} diff --git a/cli/src/alluxio.org/cli/cmd/info/collect.go b/cli/src/alluxio.org/cli/cmd/info/collect.go new file mode 100644 index 000000000000..364dc15ed5fb --- /dev/null +++ b/cli/src/alluxio.org/cli/cmd/info/collect.go @@ -0,0 +1,147 @@ +/* + * The Alluxio Open Foundation licenses this work under the Apache License, version 2.0 + * (the "License"). You may not use this work except in compliance with the License, which is + * available at www.apache.org/licenses/LICENSE-2.0 + * + * This software is distributed on an "AS IS" basis, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, + * either express or implied, as more fully set forth in the License. + * + * See the NOTICE file distributed with this work for information regarding copyright ownership. + */ + +package info + +import ( + "fmt" + "strconv" + "strings" + "time" + + "github.com/palantir/stacktrace" + "github.com/spf13/cobra" + + "alluxio.org/cli/env" +) + +var Collect = &CollectCommand{ + BaseJavaCommand: &env.BaseJavaCommand{ + CommandName: "collect", + JavaClassName: "alluxio.cli.bundler.CollectInfo", + }, +} + +type CollectCommand struct { + *env.BaseJavaCommand + + additionalLogs []string + endTime string + excludeLogs []string + excludeWorkerMetrics bool + includeLogs []string + local bool + maxThreads int + startTime string +} + +func (c *CollectCommand) Base() *env.BaseJavaCommand { + return c.BaseJavaCommand +} + +const dateFormat = "2006-01-02T15:04:05" + +func (c *CollectCommand) ToCommand() *cobra.Command { + cmd := c.Base().InitRunJavaClassCmd(&cobra.Command{ + Use: fmt.Sprintf("%v [command] [outputPath]", Collect.CommandName), + Short: "Collects information such as logs, config, metrics, and more from the running Alluxio cluster and bundle into a single tarball", + Long: `Collects information such as logs, config, metrics, and more from the running Alluxio cluster and bundle into a single tarball +[command] can be one of the following values: +all: runs all the commands below. +collectAlluxioInfo: runs a set of Alluxio commands to collect information about the Alluxio cluster. +collectConfig: collects the configuration files under ${ALLUXIO_HOME}/config/. +collectEnv: runs a set of linux commands to collect information about the cluster. +collectJvmInfo: collects jstack from the JVMs. +collectLog: collects the log files under ${ALLUXIO_HOME}/logs/. +collectMetrics: collects Alluxio system metrics. + +[outputPath] the directory you want the collected tarball to be in + +WARNING: This command MAY bundle credentials. To understand the risks refer to the docs here. +https://docs.alluxio.io/os/user/edge/en/operation/Troubleshooting.html#collect-alluxio-cluster-information +`, + Args: cobra.ExactArgs(2), + RunE: func(cmd *cobra.Command, args []string) error { + return c.Run(args) + }, + }) + cmd.Flags().StringSliceVar(&c.additionalLogs, "additional-logs", nil, "additional file name prefixes from ${ALLUXIO_HOME}/logs to include in the tarball, inclusive of the default log files") + cmd.Flags().StringVar(&c.endTime, "end-time", "", "logs that do not contain entries before this time will be ignored, format must be like "+dateFormat) + cmd.Flags().StringSliceVar(&c.excludeLogs, "exclude-logs", nil, "file name prefixes from ${ALLUXIO_HOME}/logs to exclude; this is evaluated after adding files from --additional-logs") + cmd.Flags().BoolVar(&c.excludeWorkerMetrics, "exclude-worker-metrics", false, "true to skip worker metrics collection") + cmd.Flags().StringSliceVar(&c.includeLogs, "include-logs", nil, "file name prefixes from ${ALLUXIO_HOME}/logs to include in the tarball, ignoring the default log files; cannot be used with --exclude-logs or --additional-logs") + cmd.Flags().BoolVar(&c.local, "local", false, "true to only collect information from the local machine") + cmd.Flags().IntVar(&c.maxThreads, "max-threads", 1, "parallelism of the command; use a smaller value to limit network I/O when transferring tarballs") + cmd.Flags().StringVar(&c.startTime, "start-time", "", "logs that do not contain entries after this time will be ignored, format must be like "+dateFormat) + return cmd +} + +func (c *CollectCommand) Run(args []string) error { + commands := map[string]struct{}{ + "all": {}, + "collectAlluxioInfo": {}, + "collectConfig": {}, + "collectEnv": {}, + "collectJvmInfo": {}, + "collectLog": {}, + "collectMetrics": {}, + } + if _, ok := commands[args[0]]; !ok { + var cmds []string + for c := range commands { + cmds = append(cmds, c) + } + return stacktrace.NewError("first argument must be one of %v", strings.Join(cmds, ", ")) + } + + var javaArgs []string + if c.additionalLogs != nil { + if c.includeLogs != nil { + return stacktrace.NewError("cannot set both --include-logs and --additional-logs") + } + javaArgs = append(javaArgs, "--additional-logs", strings.Join(c.additionalLogs, ",")) + } + if c.endTime != "" { + if _, err := time.Parse(dateFormat, c.endTime); err != nil { + return stacktrace.Propagate(err, "could not parse end time %v", c.endTime) + } + javaArgs = append(javaArgs, "--end-time", c.endTime) + } + if c.excludeLogs != nil { + if c.includeLogs != nil { + return stacktrace.NewError("cannot set both --include-logs and --exclude-logs") + } + javaArgs = append(javaArgs, "--exclude-logs", strings.Join(c.excludeLogs, ",")) + } + if c.excludeWorkerMetrics { + javaArgs = append(javaArgs, "--exclude-worker-metrics") + } + if c.includeLogs != nil { + // already checked exclusivity with --additional-logs and --exclude-logs + javaArgs = append(javaArgs, "--include-logs", strings.Join(c.includeLogs, ",")) + } + if c.local { + javaArgs = append(javaArgs, "--local") + } + if c.maxThreads > 1 { + javaArgs = append(javaArgs, "--max-threads", strconv.Itoa(c.maxThreads)) + } + if c.startTime != "" { + if _, err := time.Parse(dateFormat, c.startTime); err != nil { + return stacktrace.Propagate(err, "could not parse start time %v", c.startTime) + } + javaArgs = append(javaArgs, "--start-time", c.startTime) + } + + javaArgs = append(javaArgs, args...) + + return c.Base().Run(javaArgs) +} diff --git a/cli/src/alluxio.org/cli/cmd/info/info.go b/cli/src/alluxio.org/cli/cmd/info/info.go new file mode 100644 index 000000000000..39ea06d913ad --- /dev/null +++ b/cli/src/alluxio.org/cli/cmd/info/info.go @@ -0,0 +1,27 @@ +/* + * The Alluxio Open Foundation licenses this work under the Apache License, version 2.0 + * (the "License"). You may not use this work except in compliance with the License, which is + * available at www.apache.org/licenses/LICENSE-2.0 + * + * This software is distributed on an "AS IS" basis, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, + * either express or implied, as more fully set forth in the License. + * + * See the NOTICE file distributed with this work for information regarding copyright ownership. + */ + +package info + +import ( + "alluxio.org/cli/env" +) + +var Service = &env.Service{ + Name: "info", + Description: "Retrieve and/or display info about the running Alluxio cluster", + Commands: []env.Command{ + Cache, + Collect, + Master, + Report, + }, +} diff --git a/cli/src/alluxio.org/cli/cmd/info/master.go b/cli/src/alluxio.org/cli/cmd/info/master.go new file mode 100644 index 000000000000..2eceaab89862 --- /dev/null +++ b/cli/src/alluxio.org/cli/cmd/info/master.go @@ -0,0 +1,49 @@ +/* + * The Alluxio Open Foundation licenses this work under the Apache License, version 2.0 + * (the "License"). You may not use this work except in compliance with the License, which is + * available at www.apache.org/licenses/LICENSE-2.0 + * + * This software is distributed on an "AS IS" basis, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, + * either express or implied, as more fully set forth in the License. + * + * See the NOTICE file distributed with this work for information regarding copyright ownership. + */ + +package info + +import ( + "github.com/spf13/cobra" + + "alluxio.org/cli/env" +) + +var Master = &MasterCommand{ + BaseJavaCommand: &env.BaseJavaCommand{ + CommandName: "master", + JavaClassName: "alluxio.cli.fs.FileSystemShell", + }, +} + +type MasterCommand struct { + *env.BaseJavaCommand +} + +func (c *MasterCommand) Base() *env.BaseJavaCommand { + return c.BaseJavaCommand +} + +func (c *MasterCommand) ToCommand() *cobra.Command { + cmd := c.Base().InitRunJavaClassCmd(&cobra.Command{ + Use: Master.CommandName, + Short: "Prints information regarding master fault tolerance such as leader address and list of master addresses", + RunE: func(cmd *cobra.Command, args []string) error { + return c.Run(nil) + }, + }) + return cmd +} + +func (c *MasterCommand) Run(_ []string) error { + // TODO: output in a serializable format + return c.Base().Run([]string{"masterInfo"}) +} diff --git a/cli/src/alluxio.org/cli/cmd/info/report.go b/cli/src/alluxio.org/cli/cmd/info/report.go new file mode 100644 index 000000000000..02bfd85d42ed --- /dev/null +++ b/cli/src/alluxio.org/cli/cmd/info/report.go @@ -0,0 +1,73 @@ +/* + * The Alluxio Open Foundation licenses this work under the Apache License, version 2.0 + * (the "License"). You may not use this work except in compliance with the License, which is + * available at www.apache.org/licenses/LICENSE-2.0 + * + * This software is distributed on an "AS IS" basis, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, + * either express or implied, as more fully set forth in the License. + * + * See the NOTICE file distributed with this work for information regarding copyright ownership. + */ + +package info + +import ( + "fmt" + + "github.com/spf13/cobra" + + "alluxio.org/cli/env" +) + +var Report = &ReportCommand{ + BaseJavaCommand: &env.BaseJavaCommand{ + CommandName: "report", + JavaClassName: "alluxio.cli.fsadmin.FileSystemAdminShell", + }, +} + +type ReportCommand struct { + *env.BaseJavaCommand + + category string // can be empty or one of: jobservice, metrics, summary, ufs +} + +func (c *ReportCommand) Base() *env.BaseJavaCommand { + return c.BaseJavaCommand +} + +func (c *ReportCommand) ToCommand() *cobra.Command { + cmd := c.Base().InitRunJavaClassCmd(&cobra.Command{ + Use: fmt.Sprintf("%v [args]", Report.CommandName), + Short: "Reports Alluxio running cluster information", + Args: cobra.NoArgs, + RunE: func(cmd *cobra.Command, args []string) error { + return c.Run(nil) + }, + }) + // note that the capacity category is moved into its own cache command + var isJobservice, isMetrics, isSummary, isUfs bool + cmd.Flags().BoolVar(&isJobservice, "job-service", false, "Job service metrics information") + cmd.Flags().BoolVar(&isMetrics, "metrics", false, "Metrics information") + cmd.Flags().BoolVar(&isSummary, "summary", false, "Cluster summary") + cmd.Flags().BoolVar(&isUfs, "ufs", false, "Under storage system information") + cmd.MarkFlagsMutuallyExclusive("job-service", "metrics", "summary", "ufs") + if isJobservice { + c.category = "jobservice" + } else if isMetrics { + c.category = "metrics" + } else if isSummary { + c.category = "summary" + } else if isUfs { + c.category = "ufs" + } else { + c.category = "summary" + } + + return cmd +} + +func (c *ReportCommand) Run(_ []string) error { + // TODO: output all in a serializable format and filter/trim as specified by flags + return c.Base().Run([]string{"report", c.category}) +} diff --git a/cli/src/alluxio.org/cli/main.go b/cli/src/alluxio.org/cli/main.go index 5860604929dd..ef831b5d2fa5 100644 --- a/cli/src/alluxio.org/cli/main.go +++ b/cli/src/alluxio.org/cli/main.go @@ -15,6 +15,7 @@ import ( "os" "alluxio.org/cli/cmd/conf" + "alluxio.org/cli/cmd/info" "alluxio.org/cli/cmd/journal" "alluxio.org/cli/cmd/process" "alluxio.org/cli/env" @@ -37,6 +38,7 @@ func main() { conf.Service, journal.Service, process.Service, + info.Service, } { env.RegisterService(c) } From d78331196f1543b87dfeba12543c58bbc0efbb04 Mon Sep 17 00:00:00 2001 From: Rico Chiu Date: Tue, 6 Jun 2023 15:16:15 -0700 Subject: [PATCH 2/7] fix info args and cleanup --- cli/src/alluxio.org/cli/cmd/info/cache.go | 3 +- cli/src/alluxio.org/cli/cmd/info/collect.go | 16 ++--- cli/src/alluxio.org/cli/cmd/info/master.go | 3 +- cli/src/alluxio.org/cli/cmd/info/report.go | 60 +++++++++++-------- cli/src/alluxio.org/cli/cmd/journal/format.go | 2 +- cli/src/alluxio.org/cli/env/command.go | 6 +- 6 files changed, 50 insertions(+), 40 deletions(-) diff --git a/cli/src/alluxio.org/cli/cmd/info/cache.go b/cli/src/alluxio.org/cli/cmd/info/cache.go index c17bb1a3e3e7..d821d1d3ae0e 100644 --- a/cli/src/alluxio.org/cli/cmd/info/cache.go +++ b/cli/src/alluxio.org/cli/cmd/info/cache.go @@ -23,6 +23,7 @@ var Cache = &CacheCommand{ BaseJavaCommand: &env.BaseJavaCommand{ CommandName: "cache", JavaClassName: "alluxio.cli.fsadmin.FileSystemAdminShell", + Parameters: []string{"report", "capacity"}, }, } @@ -56,7 +57,7 @@ func (c *CacheCommand) ToCommand() *cobra.Command { func (c *CacheCommand) Run(_ []string) error { // TODO: output all in a serializable format and filter/trim as specified by flags - args := []string{"report", "capacity"} + var args []string if c.liveWorkers { args = append(args, "-live") } else if c.lostWorkers { diff --git a/cli/src/alluxio.org/cli/cmd/info/collect.go b/cli/src/alluxio.org/cli/cmd/info/collect.go index 364dc15ed5fb..48ef2f670d11 100644 --- a/cli/src/alluxio.org/cli/cmd/info/collect.go +++ b/cli/src/alluxio.org/cli/cmd/info/collect.go @@ -54,14 +54,14 @@ func (c *CollectCommand) ToCommand() *cobra.Command { Use: fmt.Sprintf("%v [command] [outputPath]", Collect.CommandName), Short: "Collects information such as logs, config, metrics, and more from the running Alluxio cluster and bundle into a single tarball", Long: `Collects information such as logs, config, metrics, and more from the running Alluxio cluster and bundle into a single tarball -[command] can be one of the following values: -all: runs all the commands below. -collectAlluxioInfo: runs a set of Alluxio commands to collect information about the Alluxio cluster. -collectConfig: collects the configuration files under ${ALLUXIO_HOME}/config/. -collectEnv: runs a set of linux commands to collect information about the cluster. -collectJvmInfo: collects jstack from the JVMs. -collectLog: collects the log files under ${ALLUXIO_HOME}/logs/. -collectMetrics: collects Alluxio system metrics. +[command] must be one of the following values: + all: runs all the commands below. + collectAlluxioInfo: runs a set of Alluxio commands to collect information about the Alluxio cluster. + collectConfig: collects the configuration files under ${ALLUXIO_HOME}/config/. + collectEnv: runs a set of linux commands to collect information about the cluster. + collectJvmInfo: collects jstack from the JVMs. + collectLog: collects the log files under ${ALLUXIO_HOME}/logs/. + collectMetrics: collects Alluxio system metrics. [outputPath] the directory you want the collected tarball to be in diff --git a/cli/src/alluxio.org/cli/cmd/info/master.go b/cli/src/alluxio.org/cli/cmd/info/master.go index 2eceaab89862..2c371f066723 100644 --- a/cli/src/alluxio.org/cli/cmd/info/master.go +++ b/cli/src/alluxio.org/cli/cmd/info/master.go @@ -21,6 +21,7 @@ var Master = &MasterCommand{ BaseJavaCommand: &env.BaseJavaCommand{ CommandName: "master", JavaClassName: "alluxio.cli.fs.FileSystemShell", + Parameters: []string{"masterInfo"}, }, } @@ -45,5 +46,5 @@ func (c *MasterCommand) ToCommand() *cobra.Command { func (c *MasterCommand) Run(_ []string) error { // TODO: output in a serializable format - return c.Base().Run([]string{"masterInfo"}) + return c.Base().Run(nil) } diff --git a/cli/src/alluxio.org/cli/cmd/info/report.go b/cli/src/alluxio.org/cli/cmd/info/report.go index 02bfd85d42ed..8de4c163f716 100644 --- a/cli/src/alluxio.org/cli/cmd/info/report.go +++ b/cli/src/alluxio.org/cli/cmd/info/report.go @@ -13,7 +13,9 @@ package info import ( "fmt" + "strings" + "github.com/palantir/stacktrace" "github.com/spf13/cobra" "alluxio.org/cli/env" @@ -23,13 +25,12 @@ var Report = &ReportCommand{ BaseJavaCommand: &env.BaseJavaCommand{ CommandName: "report", JavaClassName: "alluxio.cli.fsadmin.FileSystemAdminShell", + Parameters: []string{"report"}, }, } type ReportCommand struct { *env.BaseJavaCommand - - category string // can be empty or one of: jobservice, metrics, summary, ufs } func (c *ReportCommand) Base() *env.BaseJavaCommand { @@ -38,36 +39,43 @@ func (c *ReportCommand) Base() *env.BaseJavaCommand { func (c *ReportCommand) ToCommand() *cobra.Command { cmd := c.Base().InitRunJavaClassCmd(&cobra.Command{ - Use: fmt.Sprintf("%v [args]", Report.CommandName), + Use: fmt.Sprintf("%v [arg]", Report.CommandName), Short: "Reports Alluxio running cluster information", - Args: cobra.NoArgs, + Long: `Reports Alluxio running cluster information +[arg] can be one of the following values: + jobservice: job service metrics information + metrics: metrics information + summary: cluster summary + ufs: under storage system information + +Defaults to summary if no arg is provided +`, + Args: cobra.RangeArgs(0, 1), RunE: func(cmd *cobra.Command, args []string) error { - return c.Run(nil) + return c.Run(args) }, }) - // note that the capacity category is moved into its own cache command - var isJobservice, isMetrics, isSummary, isUfs bool - cmd.Flags().BoolVar(&isJobservice, "job-service", false, "Job service metrics information") - cmd.Flags().BoolVar(&isMetrics, "metrics", false, "Metrics information") - cmd.Flags().BoolVar(&isSummary, "summary", false, "Cluster summary") - cmd.Flags().BoolVar(&isUfs, "ufs", false, "Under storage system information") - cmd.MarkFlagsMutuallyExclusive("job-service", "metrics", "summary", "ufs") - if isJobservice { - c.category = "jobservice" - } else if isMetrics { - c.category = "metrics" - } else if isSummary { - c.category = "summary" - } else if isUfs { - c.category = "ufs" - } else { - c.category = "summary" - } - return cmd } -func (c *ReportCommand) Run(_ []string) error { +func (c *ReportCommand) Run(args []string) error { + reportArg := "summary" + if len(args) == 1 { + options := map[string]struct{}{ + "jobservice": {}, + "metrics": {}, + "summary": {}, + "ufs": {}, + } + if _, ok := options[args[0]]; !ok { + var cmds []string + for c := range options { + cmds = append(cmds, c) + } + return stacktrace.NewError("first argument must be one of %v", strings.Join(cmds, ", ")) + } + reportArg = args[0] + } // TODO: output all in a serializable format and filter/trim as specified by flags - return c.Base().Run([]string{"report", c.category}) + return c.Base().Run([]string{reportArg}) } diff --git a/cli/src/alluxio.org/cli/cmd/journal/format.go b/cli/src/alluxio.org/cli/cmd/journal/format.go index f94327296395..e76c0f68e629 100644 --- a/cli/src/alluxio.org/cli/cmd/journal/format.go +++ b/cli/src/alluxio.org/cli/cmd/journal/format.go @@ -26,7 +26,7 @@ var Format = &FormatCommand{ BaseJavaCommand: &env.BaseJavaCommand{ CommandName: "format", JavaClassName: "alluxio.cli.Format", - Parameter: "master", + Parameters: []string{"master"}, ShellJavaOpts: fmt.Sprintf(env.JavaOptFormat, env.ConfAlluxioLoggerType, "Console"), }, } diff --git a/cli/src/alluxio.org/cli/env/command.go b/cli/src/alluxio.org/cli/env/command.go index 2bc9c2634a9d..497bef0fc11e 100644 --- a/cli/src/alluxio.org/cli/env/command.go +++ b/cli/src/alluxio.org/cli/env/command.go @@ -30,7 +30,7 @@ type Command interface { type BaseJavaCommand struct { CommandName string JavaClassName string - Parameter string + Parameters []string DebugMode bool @@ -71,8 +71,8 @@ func (c *BaseJavaCommand) RunJavaClassCmd(args []string) *exec.Cmd { } } cmdArgs = append(cmdArgs, c.JavaClassName) - if c.Parameter != "" { - cmdArgs = append(cmdArgs, c.Parameter) + if len(c.Parameters) > 0 { + cmdArgs = append(cmdArgs, c.Parameters...) } cmdArgs = append(cmdArgs, args...) From 98d390ae7b70f7224b445f468d5c7c0c87a0fc15 Mon Sep 17 00:00:00 2001 From: Rico Chiu Date: Tue, 6 Jun 2023 16:53:39 -0700 Subject: [PATCH 3/7] address comments --- cli/README.md | 15 +++++++++++- cli/src/alluxio.org/cli/cmd/info/cache.go | 2 +- cli/src/alluxio.org/cli/cmd/info/collect.go | 26 ++++++++++++--------- cli/src/alluxio.org/cli/launch/launch.go | 2 +- 4 files changed, 31 insertions(+), 14 deletions(-) diff --git a/cli/README.md b/cli/README.md index 0d8965f01f6e..78c2354be1e6 100644 --- a/cli/README.md +++ b/cli/README.md @@ -29,7 +29,20 @@ bin/cli.sh item mark --unset name where it is expected that either `--set` or `--unset` are specified. This is preferred over the alternative of two separate commands with `setMark` and `unsetMark` as the operations. -## User input validation +## User input + +### Flags and arguments +After selecting the desired command, additional user input can be parsed, as a mix of arguments and/or flags: +- Arguments: `bin/cli.sh command arg1 arg2 ...` +- Flags: `bin/cli.sh command --flag1 --flag2 val2 ...` + +Flags are strictly preferred over arguments. +- The flag name conveys context; an argument does not +- Arguments must be ordered; flags can be declared arbitrarily +- Flags can be designated as required to ensure user input. +- Repeated flags can be defined to capture an ordered list of inputs, ex. `--target target1 --target target2` + +### Input validation User inputs should be validated by the CLI command as much as possible as opposed to the resulting invocation. diff --git a/cli/src/alluxio.org/cli/cmd/info/cache.go b/cli/src/alluxio.org/cli/cmd/info/cache.go index d821d1d3ae0e..81835a0cbae4 100644 --- a/cli/src/alluxio.org/cli/cmd/info/cache.go +++ b/cli/src/alluxio.org/cli/cmd/info/cache.go @@ -50,7 +50,7 @@ func (c *CacheCommand) ToCommand() *cobra.Command { }) cmd.Flags().BoolVar(&c.liveWorkers, "live", false, "Only show live workers for capacity report") cmd.Flags().BoolVar(&c.lostWorkers, "lost", false, "Only show lost workers for capacity report") - cmd.Flags().StringSliceVar(&c.workersList, "workers", nil, "Only show specified workers for capacity report, labeled by hostname or IP address") + cmd.Flags().StringSliceVar(&c.workersList, "worker", nil, "Only show specified workers for capacity report, labeled by hostname or IP address") cmd.MarkFlagsMutuallyExclusive("live", "lost", "workers") return cmd } diff --git a/cli/src/alluxio.org/cli/cmd/info/collect.go b/cli/src/alluxio.org/cli/cmd/info/collect.go index 48ef2f670d11..287d8c561bf3 100644 --- a/cli/src/alluxio.org/cli/cmd/info/collect.go +++ b/cli/src/alluxio.org/cli/cmd/info/collect.go @@ -40,6 +40,7 @@ type CollectCommand struct { includeLogs []string local bool maxThreads int + outputPath string startTime string } @@ -51,7 +52,7 @@ const dateFormat = "2006-01-02T15:04:05" func (c *CollectCommand) ToCommand() *cobra.Command { cmd := c.Base().InitRunJavaClassCmd(&cobra.Command{ - Use: fmt.Sprintf("%v [command] [outputPath]", Collect.CommandName), + Use: fmt.Sprintf("%v [command]", Collect.CommandName), Short: "Collects information such as logs, config, metrics, and more from the running Alluxio cluster and bundle into a single tarball", Long: `Collects information such as logs, config, metrics, and more from the running Alluxio cluster and bundle into a single tarball [command] must be one of the following values: @@ -68,23 +69,26 @@ func (c *CollectCommand) ToCommand() *cobra.Command { WARNING: This command MAY bundle credentials. To understand the risks refer to the docs here. https://docs.alluxio.io/os/user/edge/en/operation/Troubleshooting.html#collect-alluxio-cluster-information `, - Args: cobra.ExactArgs(2), + Args: cobra.ExactArgs(1), RunE: func(cmd *cobra.Command, args []string) error { return c.Run(args) }, }) - cmd.Flags().StringSliceVar(&c.additionalLogs, "additional-logs", nil, "additional file name prefixes from ${ALLUXIO_HOME}/logs to include in the tarball, inclusive of the default log files") - cmd.Flags().StringVar(&c.endTime, "end-time", "", "logs that do not contain entries before this time will be ignored, format must be like "+dateFormat) - cmd.Flags().StringSliceVar(&c.excludeLogs, "exclude-logs", nil, "file name prefixes from ${ALLUXIO_HOME}/logs to exclude; this is evaluated after adding files from --additional-logs") - cmd.Flags().BoolVar(&c.excludeWorkerMetrics, "exclude-worker-metrics", false, "true to skip worker metrics collection") - cmd.Flags().StringSliceVar(&c.includeLogs, "include-logs", nil, "file name prefixes from ${ALLUXIO_HOME}/logs to include in the tarball, ignoring the default log files; cannot be used with --exclude-logs or --additional-logs") - cmd.Flags().BoolVar(&c.local, "local", false, "true to only collect information from the local machine") - cmd.Flags().IntVar(&c.maxThreads, "max-threads", 1, "parallelism of the command; use a smaller value to limit network I/O when transferring tarballs") - cmd.Flags().StringVar(&c.startTime, "start-time", "", "logs that do not contain entries after this time will be ignored, format must be like "+dateFormat) + cmd.Flags().StringSliceVar(&c.additionalLogs, "additional-logs", nil, "Additional file name prefixes from ${ALLUXIO_HOME}/logs to include in the tarball, inclusive of the default log files") + cmd.Flags().StringVar(&c.endTime, "end-time", "", "Logs that do not contain entries before this time will be ignored, format must be like "+dateFormat) + cmd.Flags().StringSliceVar(&c.excludeLogs, "exclude-logs", nil, "File name prefixes from ${ALLUXIO_HOME}/logs to exclude; this is evaluated after adding files from --additional-logs") + cmd.Flags().BoolVar(&c.excludeWorkerMetrics, "exclude-worker-metrics", false, "True to skip worker metrics collection") + cmd.Flags().StringSliceVar(&c.includeLogs, "include-logs", nil, "File name prefixes from ${ALLUXIO_HOME}/logs to include in the tarball, ignoring the default log files; cannot be used with --exclude-logs or --additional-logs") + cmd.Flags().BoolVar(&c.local, "local", false, "True to only collect information from the local machine") + cmd.Flags().IntVar(&c.maxThreads, "max-threads", 1, "Parallelism of the command; use a smaller value to limit network I/O when transferring tarballs") + cmd.Flags().StringVar(&c.outputPath, "output-path", "", "Output directory to write collect info tarball to") + cmd.MarkFlagRequired("output-path") + cmd.Flags().StringVar(&c.startTime, "start-time", "", "Logs that do not contain entries after this time will be ignored, format must be like "+dateFormat) return cmd } func (c *CollectCommand) Run(args []string) error { + // TODO: use flags instead of arguments to parse user input commands := map[string]struct{}{ "all": {}, "collectAlluxioInfo": {}, @@ -141,7 +145,7 @@ func (c *CollectCommand) Run(args []string) error { javaArgs = append(javaArgs, "--start-time", c.startTime) } - javaArgs = append(javaArgs, args...) + javaArgs = append(javaArgs, args[0], c.outputPath) return c.Base().Run(javaArgs) } diff --git a/cli/src/alluxio.org/cli/launch/launch.go b/cli/src/alluxio.org/cli/launch/launch.go index 3772f7aa60b9..58267adf531c 100644 --- a/cli/src/alluxio.org/cli/launch/launch.go +++ b/cli/src/alluxio.org/cli/launch/launch.go @@ -34,7 +34,7 @@ func Run() error { return stacktrace.Propagate(err, "error marking %v flag hidden", rootPathName) } var flagDebugLog bool - rootCmd.PersistentFlags().BoolVar(&flagDebugLog, "debugLog", false, "True to enable debug logging") + rootCmd.PersistentFlags().BoolVar(&flagDebugLog, "debug-log", false, "True to enable debug logging") rootCmd.PersistentPreRunE = func(cmd *cobra.Command, args []string) error { if flagDebugLog { log.Logger.SetLevel(logrus.DebugLevel) From 23b4423efa7bee6e18a043a01fe8d667169ac62f Mon Sep 17 00:00:00 2001 From: Rico Chiu Date: Tue, 6 Jun 2023 20:29:41 -0700 Subject: [PATCH 4/7] fix name --- cli/src/alluxio.org/cli/cmd/info/cache.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cli/src/alluxio.org/cli/cmd/info/cache.go b/cli/src/alluxio.org/cli/cmd/info/cache.go index 81835a0cbae4..d7e48a21ed92 100644 --- a/cli/src/alluxio.org/cli/cmd/info/cache.go +++ b/cli/src/alluxio.org/cli/cmd/info/cache.go @@ -51,7 +51,7 @@ func (c *CacheCommand) ToCommand() *cobra.Command { cmd.Flags().BoolVar(&c.liveWorkers, "live", false, "Only show live workers for capacity report") cmd.Flags().BoolVar(&c.lostWorkers, "lost", false, "Only show lost workers for capacity report") cmd.Flags().StringSliceVar(&c.workersList, "worker", nil, "Only show specified workers for capacity report, labeled by hostname or IP address") - cmd.MarkFlagsMutuallyExclusive("live", "lost", "workers") + cmd.MarkFlagsMutuallyExclusive("live", "lost", "worker") return cmd } From ef72dbdf282c9ccd5df5aa4ad24d548bc976df97 Mon Sep 17 00:00:00 2001 From: Rico Chiu Date: Tue, 6 Jun 2023 22:49:19 -0700 Subject: [PATCH 5/7] cleanup flag names --- cli/src/alluxio.org/cli/cmd/conf/log.go | 5 +++-- cli/src/alluxio.org/cli/cmd/info/cache.go | 9 +++++---- cli/src/alluxio.org/cli/cmd/info/collect.go | 7 +++++-- 3 files changed, 13 insertions(+), 8 deletions(-) diff --git a/cli/src/alluxio.org/cli/cmd/conf/log.go b/cli/src/alluxio.org/cli/cmd/conf/log.go index 042f54c3b623..60ee82c52a2e 100644 --- a/cli/src/alluxio.org/cli/cmd/conf/log.go +++ b/cli/src/alluxio.org/cli/cmd/conf/log.go @@ -46,8 +46,9 @@ func (c *LogCommand) ToCommand() *cobra.Command { return c.Run(args) }, }) - cmd.Flags().StringVar(&c.LogName, "name", "", "Logger name (ex. alluxio.master.file.DefaultFileSystemMaster)") - if err := cmd.MarkFlagRequired("name"); err != nil { + const name = "name" + cmd.Flags().StringVar(&c.LogName, name, "", "Logger name (ex. alluxio.master.file.DefaultFileSystemMaster)") + if err := cmd.MarkFlagRequired(name); err != nil { panic(err) } cmd.Flags().StringVar(&c.Level, "level", "", "If specified, sets the specified logger at the given level") diff --git a/cli/src/alluxio.org/cli/cmd/info/cache.go b/cli/src/alluxio.org/cli/cmd/info/cache.go index d7e48a21ed92..ddf36010b83f 100644 --- a/cli/src/alluxio.org/cli/cmd/info/cache.go +++ b/cli/src/alluxio.org/cli/cmd/info/cache.go @@ -48,10 +48,11 @@ func (c *CacheCommand) ToCommand() *cobra.Command { return c.Run(nil) }, }) - cmd.Flags().BoolVar(&c.liveWorkers, "live", false, "Only show live workers for capacity report") - cmd.Flags().BoolVar(&c.lostWorkers, "lost", false, "Only show lost workers for capacity report") - cmd.Flags().StringSliceVar(&c.workersList, "worker", nil, "Only show specified workers for capacity report, labeled by hostname or IP address") - cmd.MarkFlagsMutuallyExclusive("live", "lost", "worker") + const live, lost, worker = "live", "lost", "worker" + cmd.Flags().BoolVar(&c.liveWorkers, live, false, "Only show live workers for capacity report") + cmd.Flags().BoolVar(&c.lostWorkers, lost, false, "Only show lost workers for capacity report") + cmd.Flags().StringSliceVar(&c.workersList, worker, nil, "Only show specified workers for capacity report, labeled by hostname or IP address") + cmd.MarkFlagsMutuallyExclusive(live, lost, worker) return cmd } diff --git a/cli/src/alluxio.org/cli/cmd/info/collect.go b/cli/src/alluxio.org/cli/cmd/info/collect.go index 287d8c561bf3..8a9caf841085 100644 --- a/cli/src/alluxio.org/cli/cmd/info/collect.go +++ b/cli/src/alluxio.org/cli/cmd/info/collect.go @@ -81,8 +81,11 @@ https://docs.alluxio.io/os/user/edge/en/operation/Troubleshooting.html#collect-a cmd.Flags().StringSliceVar(&c.includeLogs, "include-logs", nil, "File name prefixes from ${ALLUXIO_HOME}/logs to include in the tarball, ignoring the default log files; cannot be used with --exclude-logs or --additional-logs") cmd.Flags().BoolVar(&c.local, "local", false, "True to only collect information from the local machine") cmd.Flags().IntVar(&c.maxThreads, "max-threads", 1, "Parallelism of the command; use a smaller value to limit network I/O when transferring tarballs") - cmd.Flags().StringVar(&c.outputPath, "output-path", "", "Output directory to write collect info tarball to") - cmd.MarkFlagRequired("output-path") + const outputPath = "output-path" + cmd.Flags().StringVar(&c.outputPath, outputPath, "", "Output directory to write collect info tarball to") + if err := cmd.MarkFlagRequired(outputPath); err != nil { + panic(err) + } cmd.Flags().StringVar(&c.startTime, "start-time", "", "Logs that do not contain entries after this time will be ignored, format must be like "+dateFormat) return cmd } From 43f937f10c95972e29d34cc15dcf47915280e321 Mon Sep 17 00:00:00 2001 From: Rico Chiu Date: Tue, 6 Jun 2023 22:50:14 -0700 Subject: [PATCH 6/7] fix description --- cli/src/alluxio.org/cli/cmd/info/collect.go | 2 -- 1 file changed, 2 deletions(-) diff --git a/cli/src/alluxio.org/cli/cmd/info/collect.go b/cli/src/alluxio.org/cli/cmd/info/collect.go index 8a9caf841085..34e8c729c27f 100644 --- a/cli/src/alluxio.org/cli/cmd/info/collect.go +++ b/cli/src/alluxio.org/cli/cmd/info/collect.go @@ -64,8 +64,6 @@ func (c *CollectCommand) ToCommand() *cobra.Command { collectLog: collects the log files under ${ALLUXIO_HOME}/logs/. collectMetrics: collects Alluxio system metrics. -[outputPath] the directory you want the collected tarball to be in - WARNING: This command MAY bundle credentials. To understand the risks refer to the docs here. https://docs.alluxio.io/os/user/edge/en/operation/Troubleshooting.html#collect-alluxio-cluster-information `, From 4c4851fb79ccd909984f260701dc397b746393e3 Mon Sep 17 00:00:00 2001 From: Rico Chiu Date: Wed, 7 Jun 2023 09:55:12 -0700 Subject: [PATCH 7/7] abbreviate arg names for collect --- cli/src/alluxio.org/cli/cmd/info/collect.go | 35 +++++++++++---------- 1 file changed, 18 insertions(+), 17 deletions(-) diff --git a/cli/src/alluxio.org/cli/cmd/info/collect.go b/cli/src/alluxio.org/cli/cmd/info/collect.go index 34e8c729c27f..b339bab336a6 100644 --- a/cli/src/alluxio.org/cli/cmd/info/collect.go +++ b/cli/src/alluxio.org/cli/cmd/info/collect.go @@ -56,13 +56,13 @@ func (c *CollectCommand) ToCommand() *cobra.Command { Short: "Collects information such as logs, config, metrics, and more from the running Alluxio cluster and bundle into a single tarball", Long: `Collects information such as logs, config, metrics, and more from the running Alluxio cluster and bundle into a single tarball [command] must be one of the following values: - all: runs all the commands below. - collectAlluxioInfo: runs a set of Alluxio commands to collect information about the Alluxio cluster. - collectConfig: collects the configuration files under ${ALLUXIO_HOME}/config/. - collectEnv: runs a set of linux commands to collect information about the cluster. - collectJvmInfo: collects jstack from the JVMs. - collectLog: collects the log files under ${ALLUXIO_HOME}/logs/. - collectMetrics: collects Alluxio system metrics. + all runs all the commands below + cluster: runs a set of Alluxio commands to collect information about the Alluxio cluster + conf: collects the configuration files under ${ALLUXIO_HOME}/config/ + env: runs a set of linux commands to collect information about the cluster + jvm: collects jstack from the JVMs + log: collects the log files under ${ALLUXIO_HOME}/logs/ + metrics: collects Alluxio system metrics WARNING: This command MAY bundle credentials. To understand the risks refer to the docs here. https://docs.alluxio.io/os/user/edge/en/operation/Troubleshooting.html#collect-alluxio-cluster-information @@ -90,16 +90,17 @@ https://docs.alluxio.io/os/user/edge/en/operation/Troubleshooting.html#collect-a func (c *CollectCommand) Run(args []string) error { // TODO: use flags instead of arguments to parse user input - commands := map[string]struct{}{ - "all": {}, - "collectAlluxioInfo": {}, - "collectConfig": {}, - "collectEnv": {}, - "collectJvmInfo": {}, - "collectLog": {}, - "collectMetrics": {}, + commands := map[string]string{ + "all": "all", + "cluster": "collectAlluxioInfo", + "conf": "collectConfig", + "env": "collectEnv", + "jvm": "collectJvmInfo", + "log": "collectLog", + "metrics": "collectMetrics", } - if _, ok := commands[args[0]]; !ok { + commandArg, ok := commands[args[0]] + if !ok { var cmds []string for c := range commands { cmds = append(cmds, c) @@ -146,7 +147,7 @@ func (c *CollectCommand) Run(args []string) error { javaArgs = append(javaArgs, "--start-time", c.startTime) } - javaArgs = append(javaArgs, args[0], c.outputPath) + javaArgs = append(javaArgs, commandArg, c.outputPath) return c.Base().Run(javaArgs) }