diff --git a/src/control/cmd/ddb/ddb_commands.go b/src/control/cmd/ddb/ddb_commands.go index aa7f245be9e..ee6850e9ef7 100644 --- a/src/control/cmd/ddb/ddb_commands.go +++ b/src/control/cmd/ddb/ddb_commands.go @@ -15,6 +15,21 @@ import ( "github.com/desertbit/grumble" ) +const vosPathMissErr = "Cannot use sys db path without a VOS path" +const dtxAggrMutuallyExclusiveErr = "'--cmt_time' and '--cmt_date' options are mutually exclusive" +const dtxAggrRequiredOptErr = "'--cmt_time' or '--cmt_date' option has to be defined" +const featureOnlyOneOptErr = "exactly one of --enable, --disable, --show must be provided" + +func onlyOne(bools ...bool) bool { + count := 0 + for _, b := range bools { + if b { + count++ + } + } + return count == 1 +} + func addAppCommands(app *grumble.App, ctx *DdbContext) { // Command: ls app.AddCommand(&grumble.Command{ @@ -305,10 +320,11 @@ the path must include the extent, otherwise, it must not.`, }) // Command: feature app.AddCommand(&grumble.Command{ - Name: "feature", - Aliases: nil, - Help: "Manage VOS pool features", - LongHelp: "", + Name: "feature", + Aliases: nil, + Help: "Manage VOS pool features", + LongHelp: `Manage VOS pool features. Exactly one of --enable, --disable, or --show must be provided. +If --db_path is provided, a VOS file path must also be given as a positional argument.`, HelpGroup: "vos", Flags: func(f *grumble.Flags) { f.String("e", "enable", "", "Enable VOS pool features") @@ -320,7 +336,18 @@ the path must include the extent, otherwise, it must not.`, a.String("path", "Optional, Path to the VOS file", grumble.Default("")) }, Run: func(c *grumble.Context) error { - return ctx.Feature(c.Args.String("path"), c.Flags.String("db_path"), c.Flags.String("enable"), c.Flags.String("disable"), c.Flags.Bool("show")) + path := c.Args.String("path") + dbPath := c.Flags.String("db_path") + enable := c.Flags.String("enable") + disable := c.Flags.String("disable") + show := c.Flags.Bool("show") + if path == "" && dbPath != "" { + return fmt.Errorf(vosPathMissErr) + } + if !onlyOne(enable != "", disable != "", show) { + return fmt.Errorf(featureOnlyOneOptErr) + } + return ctx.Feature(path, dbPath, enable, disable, show) }, Completer: featureCompleter, }) @@ -445,10 +472,10 @@ the path must include the extent, otherwise, it must not.`, cmtTime := c.Flags.Uint64("cmt_time") cmtDate := c.Flags.String("cmt_date") if cmtTime != math.MaxUint64 && cmtDate != "" { - return fmt.Errorf("'--cmt_time' and '--cmt_date' options are mutually exclusive") + return fmt.Errorf(dtxAggrMutuallyExclusiveErr) } if cmtTime == math.MaxUint64 && cmtDate == "" { - return fmt.Errorf("'--cmt_time' or '--cmt_date' option has to be defined") + return fmt.Errorf(dtxAggrRequiredOptErr) } return ctx.DtxAggr(c.Args.String("path"), cmtTime, cmtDate) }, diff --git a/src/control/cmd/ddb/ddb_commands_test.go b/src/control/cmd/ddb/ddb_commands_test.go index a01c5c16d05..8e865497766 100644 --- a/src/control/cmd/ddb/ddb_commands_test.go +++ b/src/control/cmd/ddb/ddb_commands_test.go @@ -97,9 +97,26 @@ func TestDdb_Cmds(t *testing.T) { } } - featureFnCheckingShow := func(t *testing.T, wantShow bool) func(string, string, string, string, bool) error { - return func(_, _, _, _ string, show bool) error { + string2FlagsCapturing := func(captured *string) func(string) (uint64, uint64, error) { + return func(s string) (uint64, uint64, error) { + *captured = s + return 0, 0, nil + } + } + + featureFnChecking := func(t *testing.T, wantPath, wantDbPath string, + capturedEnable *string, capturedDisable *string, wantFlagValue string, + wantShow bool) func(string, string, string, string, bool) error { + return func(path, dbPath, enable, disable string, show bool) error { fmt.Println("feature called") + test.CmpAny(t, "path", wantPath, path) + test.CmpAny(t, "dbPath", wantDbPath, dbPath) + if capturedEnable != nil { + test.CmpAny(t, "enable", wantFlagValue, *capturedEnable) + } + if capturedDisable != nil { + test.CmpAny(t, "disable", wantFlagValue, *capturedDisable) + } test.CmpAny(t, "show", wantShow, show) return nil } @@ -120,12 +137,8 @@ func TestDdb_Cmds(t *testing.T) { setup func(*testing.T) expStdout []string expErr error - // skipCmdLine skips the command-line sub-test with a message. Use when - // a flag is shared between the CLI layer and the grumble command: go-flags - // consumes it before grumble can see it, making a clean command-line test - // impossible for that particular flag. - skipCmdLine string }{ + // --- ls command --- "ls invalid options": { args: []string{"ls", "--bar"}, expErr: ddbTestErr("invalid flag: --bar"), @@ -167,11 +180,6 @@ func TestDdb_Cmds(t *testing.T) { }, // --- open command --- - // Note: the -w/--write_mode and -p/--db_path flags of the grumble 'open' - // command share names with CLI-level flags that are consumed by go-flags - // before reaching grumble in command-line mode. The command-line test for - // those flags would silently test wrong values. They are correctly exercised - // in command-file mode; see TestRun for CLI-level flag coverage. "open default": { args: []string{"open", "/path/to/vos-0"}, setup: func(t *testing.T) { @@ -179,17 +187,29 @@ func TestDdb_Cmds(t *testing.T) { }, expStdout: []string{"open called"}, }, - "open write mode": { - args: []string{"open", "-w", "/path/to/vos-0"}, - skipCmdLine: "-w is consumed by the CLI write_mode flag before reaching grumble", + "open short write mode": { + args: []string{"open", "-w", "/path/to/vos-0"}, + setup: func(t *testing.T) { + ddb_run_open_Fn = openFnChecking(t, "/path/to/vos-0", "", true) + }, + expStdout: []string{"open called"}, + }, + "open long write mode": { + args: []string{"open", "--write_mode", "/path/to/vos-0"}, setup: func(t *testing.T) { ddb_run_open_Fn = openFnChecking(t, "/path/to/vos-0", "", true) }, expStdout: []string{"open called"}, }, - "open with db path": { - args: []string{"open", "-p", "/sysdb", "/path/to/vos-0"}, - skipCmdLine: "-p is consumed by the CLI db_path flag before reaching grumble", + "open with short db path": { + args: []string{"open", "-p", "/sysdb", "/path/to/vos-0"}, + setup: func(t *testing.T) { + ddb_run_open_Fn = openFnChecking(t, "/path/to/vos-0", "/sysdb", false) + }, + expStdout: []string{"open called"}, + }, + "open with long db path": { + args: []string{"open", "--db_path", "/sysdb", "/path/to/vos-0"}, setup: func(t *testing.T) { ddb_run_open_Fn = openFnChecking(t, "/path/to/vos-0", "/sysdb", false) }, @@ -197,45 +217,80 @@ func TestDdb_Cmds(t *testing.T) { }, // --- feature command --- - // feature --show: verifies the show flag is forwarded to the C layer. - "feature show": { + "feature without flags": { + args: []string{"feature"}, + expErr: ddbTestErr(featureOnlyOneOptErr), + }, + "feature with enable and disable flags": { + args: []string{"feature", "--enable=a", "--disable=b"}, + expErr: ddbTestErr(featureOnlyOneOptErr), + }, + "feature with enable and show flags": { + args: []string{"feature", "--enable=a", "--show"}, + expErr: ddbTestErr(featureOnlyOneOptErr), + }, + "feature with disable and show flags": { + args: []string{"feature", "--disable=a", "--show"}, + expErr: ddbTestErr(featureOnlyOneOptErr), + }, + "feature with db_path but no path": { + args: []string{"feature", "--db_path=/sysdb", "--show"}, + expErr: ddbTestErr(vosPathMissErr), + }, + "feature with long show flag": { args: []string{"feature", "--show"}, setup: func(t *testing.T) { - ddb_run_feature_Fn = featureFnCheckingShow(t, true) + ddb_run_feature_Fn = featureFnChecking(t, "", "", nil, nil, "", true) + }, + expStdout: []string{"feature called"}, + }, + "feature with short show flag": { + args: []string{"feature", "-s"}, + setup: func(t *testing.T) { + ddb_run_feature_Fn = featureFnChecking(t, "", "", nil, nil, "", true) }, expStdout: []string{"feature called"}, }, - // feature --enable: verifies that the enable string reaches ddb_feature_string2flags. - "feature enable": { + "feature with long enable flag": { args: []string{"feature", "--enable=myflag"}, setup: func(t *testing.T) { var capturedFlag string - ddb_feature_string2flags_Fn = func(s string) (uint64, uint64, error) { - capturedFlag = s - return 0, 0, nil - } - ddb_run_feature_Fn = func(path, dbPath, enable, disable string, show bool) error { - fmt.Println("feature called") - test.CmpAny(t, "enable flag string", "myflag", capturedFlag) - return nil - } + ddb_feature_string2flags_Fn = string2FlagsCapturing(&capturedFlag) + ddb_run_feature_Fn = featureFnChecking(t, "", "", &capturedFlag, nil, "myflag", false) }, expStdout: []string{"feature called"}, }, - // feature --disable: verifies that the disable string reaches ddb_feature_string2flags. - "feature disable": { - args: []string{"feature", "--disable=otherflag"}, + "feature with short enable flag": { + args: []string{"feature", "-e", "myflag"}, setup: func(t *testing.T) { var capturedFlag string - ddb_feature_string2flags_Fn = func(s string) (uint64, uint64, error) { - capturedFlag = s - return 0, 0, nil - } - ddb_run_feature_Fn = func(path, dbPath, enable, disable string, show bool) error { - fmt.Println("feature called") - test.CmpAny(t, "disable flag string", "otherflag", capturedFlag) - return nil - } + ddb_feature_string2flags_Fn = string2FlagsCapturing(&capturedFlag) + ddb_run_feature_Fn = featureFnChecking(t, "", "", &capturedFlag, nil, "myflag", false) + }, + expStdout: []string{"feature called"}, + }, + "feature with long disable flag": { + args: []string{"feature", "--disable=myflag"}, + setup: func(t *testing.T) { + var capturedFlag string + ddb_feature_string2flags_Fn = string2FlagsCapturing(&capturedFlag) + ddb_run_feature_Fn = featureFnChecking(t, "", "", nil, &capturedFlag, "myflag", false) + }, + expStdout: []string{"feature called"}, + }, + "feature with short disable flag": { + args: []string{"feature", "-d", "myflag"}, + setup: func(t *testing.T) { + var capturedFlag string + ddb_feature_string2flags_Fn = string2FlagsCapturing(&capturedFlag) + ddb_run_feature_Fn = featureFnChecking(t, "", "", nil, &capturedFlag, "myflag", false) + }, + expStdout: []string{"feature called"}, + }, + "feature with cmd-level db_path": { + args: []string{"feature", "--db_path=/sysdb", "--show", "/path/to/vos-0"}, + setup: func(t *testing.T) { + ddb_run_feature_Fn = featureFnChecking(t, "/path/to/vos-0", "/sysdb", nil, nil, "", true) }, expStdout: []string{"feature called"}, }, @@ -245,11 +300,11 @@ func TestDdb_Cmds(t *testing.T) { // --cmt_date is provided. These tests exercise that Go-layer validation. "dtx_aggr both cmt_time and cmt_date": { args: []string{"dtx_aggr", "--cmt_time=0", "--cmt_date=2024-01-01"}, - expErr: ddbTestErr("mutually exclusive"), + expErr: ddbTestErr(dtxAggrMutuallyExclusiveErr), }, "dtx_aggr neither cmt_time nor cmt_date": { args: []string{"dtx_aggr"}, - expErr: ddbTestErr("has to be defined"), + expErr: ddbTestErr(dtxAggrRequiredOptErr), }, "dtx_aggr cmt_time": { args: []string{"dtx_aggr", "--cmt_time=1000"}, @@ -297,14 +352,57 @@ func TestDdb_Cmds(t *testing.T) { expStdout: []string{"version called"}, }, + // --- rm_pool command --- + "rm_pool with db_path": { + args: []string{"rm_pool", "--db_path", "/sysdb", "/mnt/pool/rdb-pool"}, + setup: func(t *testing.T) { + ddb_run_rm_pool_Fn = func(path, dbPath string) error { + fmt.Println("rm_pool called") + test.CmpAny(t, "path", "/mnt/pool/rdb-pool", path) + test.CmpAny(t, "dbPath", "/sysdb", dbPath) + return nil + } + }, + expStdout: []string{"rm_pool called"}, + }, + "rm_pool without db_path": { + args: []string{"rm_pool", "/mnt/pool/rdb-pool"}, + setup: func(t *testing.T) { + ddb_run_rm_pool_Fn = func(path, dbPath string) error { + fmt.Println("rm_pool called") + test.CmpAny(t, "path", "/mnt/pool/rdb-pool", path) + test.CmpAny(t, "dbPath", "", dbPath) + return nil + } + }, + expStdout: []string{"rm_pool called"}, + }, + + // --- prov_mem command: flag conflict --- + // -s / --tmpfs_size: short flag -s was consumed as global VosPath before PassAfterNonOption. + "prov_mem with tmpfs_size short flag": { + args: []string{"prov_mem", "-s", "10", "/db", "/mnt"}, + setup: func(t *testing.T) { + ddb_run_prov_mem_Fn = func(dbPath, tmpfsMount string, tmpfsMountSize uint) error { + fmt.Println("prov_mem called") + test.CmpAny(t, "dbPath", "/db", dbPath) + test.CmpAny(t, "tmpfsMount", "/mnt", tmpfsMount) + test.CmpAny(t, "tmpfsMountSize", uint(10), tmpfsMountSize) + return nil + } + }, + expStdout: []string{"prov_mem called"}, + }, + // TODO(follow-up PR): Add TestCmds cases for the remaining commands. // Each new test case follows the same pattern as the cases above: set the // corresponding ddb_run__Fn hook in setup() to verify argument passing, // then add the case to this table. // Commands still to be covered: superblock_dump, value_dump, rm, // value_load, ilog_dump, ilog_commit, ilog_clear, dtx_dump, dtx_cmt_clear, - // smd_sync, vea_dump, vea_update, dtx_act_commit, dtx_act_abort, rm_pool, - // dtx_act_discard_invalid, dev_list, dev_replace, dtx_stat, prov_mem. + // smd_sync, vea_dump, vea_update, dtx_act_commit, dtx_act_abort, + // dtx_act_discard_invalid, dev_list, dev_replace, dtx_stat, + // prov_mem (default, no flag). } { t.Run(name, func(t *testing.T) { checkCmd := func(t *testing.T, stdout string, err error) { @@ -320,9 +418,6 @@ func TestDdb_Cmds(t *testing.T) { } t.Run("command-line", func(t *testing.T) { - if tc.skipCmdLine != "" { - t.Skipf("skipping command-line mode: %s", tc.skipCmdLine) - } ctx := newTestContext(t) if tc.setup != nil { tc.setup(t) diff --git a/src/control/cmd/ddb/main.go b/src/control/cmd/ddb/main.go index 97e2603b76a..1fc2f98c3cb 100644 --- a/src/control/cmd/ddb/main.go +++ b/src/control/cmd/ddb/main.go @@ -92,9 +92,11 @@ shell mode. If neither a single command or '-f' option is provided, then the tool will run in interactive mode. In order to modify the VOS file, the '-w' option must be included. -If the command requires it, the VOS file must be provided with the parameter ---vos-path. The VOS file will be opened before any commands are executed. See -the command‑specific help for details. +If the command requires it, the VOS file must be provided with the parameter +--vos_path. The VOS file will be opened before any commands are executed, +except for commands that manage their own pool lifecycle (open, close, feature, +rm_pool, prov_mem, smd_sync, dev_list, dev_replace). See the command-specific +help for details. A DAOS file system can operate in different modes depending on the available hardware resources. The two primary modes are MD-on-SSD and PMEM. In MD-on-SSD mode (the default), metadata is stored @@ -104,7 +106,6 @@ MODE section of the manpage for details. const grumbleUnknownCmdErr = "unknown command, try 'help'" const runCmdArgsErr = "Cannot use both command file and a command string" -const vosPathMissErr = "Cannot use sys db path without a VOS path" const loggerInitErr = "Logging facilities cannot be initialized" const ctxInitErr = "DDB Context cannot be initialized" const vosPathOpenErr = "Error opening VOS path '%s'" @@ -286,7 +287,7 @@ func closePoolIfOpen(ctx *DdbContext, log *logging.LeveledLogger) { func parseOpts(args []string, ctx *DdbContext) (cliOptions, *flags.Parser, error) { var opts cliOptions - parser := flags.NewParser(&opts, flags.HelpFlag|flags.IgnoreUnknown) + parser := flags.NewParser(&opts, flags.HelpFlag|flags.IgnoreUnknown|flags.PassAfterNonOption) parser.Name = "ddb" parser.Usage = "[OPTIONS]" parser.ShortDescription = "daos debug tool" @@ -383,8 +384,8 @@ func runDdb(ctx *DdbContext, args []string) error { return nil } - var log *logging.LeveledLogger - if log, err = newLogger(opts); err != nil { + log, err := newLogger(opts) + if err != nil { return errors.Wrap(err, loggerInitErr) } log.Debug("Logging facilities initialized") diff --git a/src/control/cmd/ddb/main_test.go b/src/control/cmd/ddb/main_test.go index 03c761b94b8..fb5b0176e3c 100644 --- a/src/control/cmd/ddb/main_test.go +++ b/src/control/cmd/ddb/main_test.go @@ -46,12 +46,37 @@ func TestDdb_parseOpts(t *testing.T) { expErr: errHelpRequested, }, "Unknown commands with help": { - args: []string{"foo", "--help"}, - expErr: errUnknownCmd, + // With PassAfterNonOption, --help that appears after the subcommand name is no longer + // processed by go-flags. It lands in RunCmdArgs and is forwarded to grumble, which + // handles it (and returns an unknown-command error for "foo"). The full flow is + // exercised in TestDdb_runDdb. + args: []string{"foo", "--help"}, + checkFunc: func(opts *cliOptions) error { + if opts.Args.RunCmd != "foo" { + return fmt.Errorf("expected RunCmd to be 'foo', got %q", opts.Args.RunCmd) + } + if len(opts.Args.RunCmdArgs) == 0 || opts.Args.RunCmdArgs[0] != "--help" { + return fmt.Errorf("expected RunCmdArgs[0] to be '--help', got %v", opts.Args.RunCmdArgs) + } + return nil + }, }, "Unknown commands with help and opt": { - args: []string{"-w", "foo", "--help"}, - expErr: errUnknownCmd, + // Same as above: -w is consumed globally (it appears before the subcommand), + // while --help after "foo" goes into RunCmdArgs. + args: []string{"-w", "foo", "--help"}, + checkFunc: func(opts *cliOptions) error { + if !opts.WriteMode { + return fmt.Errorf("expected WriteMode to be true") + } + if opts.Args.RunCmd != "foo" { + return fmt.Errorf("expected RunCmd to be 'foo', got %q", opts.Args.RunCmd) + } + if len(opts.Args.RunCmdArgs) == 0 || opts.Args.RunCmdArgs[0] != "--help" { + return fmt.Errorf("expected RunCmdArgs[0] to be '--help', got %v", opts.Args.RunCmdArgs) + } + return nil + }, }, "Default option values": { args: []string{"ls", "-d", "-r"}, @@ -194,6 +219,36 @@ func TestDdb_parseOpts(t *testing.T) { return nil }, }, + // PassAfterNonOption regression: a known global flag (--db_path) that appears AFTER + // the subcommand name must NOT be consumed by go-flags. It should land in RunCmdArgs + // so grumble can process it as a command-level flag. + "cmd-level --db_path after subcommand not consumed globally": { + args: []string{"rm_pool", "--db_path", "/sysdb", "/mnt/pool/rdb-pool"}, + checkFunc: func(opts *cliOptions) error { + if opts.SysdbPath != "" { + return fmt.Errorf("SysdbPath should be empty (PassAfterNonOption), got %q", opts.SysdbPath) + } + if opts.Args.RunCmd != "rm_pool" { + return fmt.Errorf("expected RunCmd to be 'rm_pool', got %q", opts.Args.RunCmd) + } + want := []string{"--db_path", "/sysdb", "/mnt/pool/rdb-pool"} + if len(opts.Args.RunCmdArgs) != len(want) { + return fmt.Errorf("expected RunCmdArgs %v, got %v", want, opts.Args.RunCmdArgs) + } + for i, w := range want { + if opts.Args.RunCmdArgs[i] != w { + return fmt.Errorf("RunCmdArgs[%d]: want %q, got %q", i, w, opts.Args.RunCmdArgs[i]) + } + } + return nil + }, + }, + // PassAfterNonOption does not affect flags that appear BEFORE the subcommand: those + // are still consumed globally, so the existing vosPathMissErr validation still fires. + "global --db_path before subcommand still consumed and validation fires": { + args: []string{"--db_path=/sysdb", "rm_pool", "/mnt/pool/rdb-pool"}, + expErr: ddbTestErr(vosPathMissErr), + }, } { t.Run(name, func(t *testing.T) { ctx := newTestContext(t) @@ -341,6 +396,36 @@ func TestDdb_runDdb(t *testing.T) { ddb_run_open_Fn = openFnMustNotBeCalled }, }, + "No auto-open for rm_pool": { + args: []string{"-s", "/foo/vos-0", "rm_pool", "/mnt/rdb-pool"}, + setup: func(t *testing.T) { + ddb_run_open_Fn = openFnMustNotBeCalled + }, + }, + "No auto-open for close": { + args: []string{"-s", "/foo/vos-0", "close"}, + setup: func(t *testing.T) { + ddb_run_open_Fn = openFnMustNotBeCalled + }, + }, + "No auto-open for prov_mem": { + args: []string{"-s", "/foo/vos-0", "prov_mem", "/db", "/mnt"}, + setup: func(t *testing.T) { + ddb_run_open_Fn = openFnMustNotBeCalled + }, + }, + "No auto-open for dev_list": { + args: []string{"-s", "/foo/vos-0", "dev_list", "/db"}, + setup: func(t *testing.T) { + ddb_run_open_Fn = openFnMustNotBeCalled + }, + }, + "No auto-open for dev_replace": { + args: []string{"-s", "/foo/vos-0", "dev_replace", "/db", "old-uuid", "new-uuid"}, + setup: func(t *testing.T) { + ddb_run_open_Fn = openFnMustNotBeCalled + }, + }, "Init failure": { args: []string{"ls"}, expErr: ddbTestErr(ctxInitErr),