DAOS-19122 ddb: Fix flag name collision between global and subcommand parsers

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)
 		},

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,75 +180,117 @@ 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) {
 				ddb_run_open_Fn = openFnChecking(t, "/path/to/vos-0", "", false)
 			},
 			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)
 			},
 			expStdout: []string{"open called"},
 		},
 
 		// --- 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_<cmd>_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)

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")