DAOS-19122 ddb: Fix flag name collision between global and subcommand parsers by knard38 · Pull Request #18466 · daos-stack/daos

knard38 · 2026-06-09T08:10:34Z

Description

Commit 86f31a2 (DAOS-18304) introduced a regression: ddb rm_pool --db_path ... fails with:

ERROR: ddb: Cannot use sys db path without a VOS path

This breaks the recovery/pool_list_consolidation.py test (test_lost_majority_ps_replicas) on MD-on-SSD clusters, as observed in PR #17965 CI build 22.

Root cause: go-flags processes the full command line before dispatching to grumble. Because --db_path is registered as a global option in cliOptions, go-flags intercepts it — even when the user supplies it as a subcommand-level argument (e.g. ddb rm_pool --db_path ...). The new validation added by 86f31a2bd3 then rejects the invocation because the global --vos_path was not also provided. The same collision affects open (-w, -p), feature (-p, -s), and prov_mem (-s).

Solution

This PR fixes the regression and improves the feature command with better input validation, all covered by new unit tests. The PR is split into four focused commits, each independently buildable and testable.

Commit 1 — Go style: idiomatic logger initialisation

Replace a two-statement var declaration + assignment with a single := short variable declaration, as suggested in code review of PR #18086.

Commit 2 — Fix: `PassAfterNonOption`

Add flags.PassAfterNonOption to the go-flags parser. This makes go-flags stop consuming flags after the first positional argument (the subcommand name). Everything after the subcommand lands in RunCmdArgs and is forwarded to grumble's own flag parser, which correctly handles command-level flags.

parser := flags.NewParser(&opts, flags.HelpFlag|flags.IgnoreUnknown|flags.PassAfterNonOption)

Also includes the two TestDdb_parseOpts cases whose expected behavior changed with PassAfterNonOption (see Side effects below), a doc fix (--vos-path → --vos_path), and error message constants for dtxAggr.

Commit 3 — Regression tests

TestDdb_parseOpts: two new cases — --db_path after the subcommand is not consumed globally (core regression case); --db_path before the subcommand still triggers the vosPathMissErr validation (correct misuse detection preserved).
TestDdb_runDdb: complete noAutoOpen coverage for all 8 commands (close, prov_mem, dev_list, dev_replace were previously missing).
TestDdb_Cmds: remove skipCmdLine (now obsolete); add regression tests for open (-w, --db_path), rm_pool (--db_path), and prov_mem (-s).

Commit 4 — `feature` command improvements

Add Go-layer validation: exactly one of --enable/--disable/--show required; --db_path requires a VOS path argument.
Add featureOnlyOneOptErr constant and onlyOne() helper.
Add LongHelp documenting the validation rules.
Replace featureFnCheckingShow with the more general featureFnChecking factory; add string2FlagsCapturing to verify enable/disable routing.
Full test coverage: short/long forms of all flags, all validation error paths.

Side effects

PassAfterNonOption changes one existing user-visible behavior: flags that appear after the subcommand name are now passed to grumble instead of go-flags. The most visible effect is that ddb <cmd> --help now shows the subcommand-specific help rather than the general ddb help:

# Before: showed general ddb help (go-flags consumed --help globally)
# After:  shows ls subcommand help (grumble handles it correctly)
ddb ls --help

This is a correction of incorrect behavior — users naturally expect ddb ls --help to document the ls command. Flags placed before the subcommand (e.g. ddb --vos_path /foo ls) continue to work as before.

Steps for the author:

Commit message follows the guidelines.
Appropriate Features or Test-tag pragmas were used.
Appropriate Functional Test Stages were run.
At least two positive code reviews including at least one code owner from each category referenced in the PR.
Testing is complete. If necessary, forced-landing label added and a reason added in a comment.

After all prior steps are complete:

Gatekeeper requested (daos-gatekeeper added as a reviewer).

Fix test race condition where status was checked outside of the completion callback. Signed-off-by: Alexander A Oganezov <alexander.oganezov@hpe.com>

opts_add_pci_addr() allocates opts.pci_allowed via realloc() but daos_spdk_init() never frees it after calling spdk_env_init(). Add free(opts.pci_allowed) at the out: label so both the success and error paths release the buffer. _collect() allocates ctrlr->pci_addr via calloc() for each controller, but free_ctrlr_fields() freed all other heap fields (model, serial, fw_rev, vendor_id, pci_type) while omitting pci_addr. Add the missing free(). Two new unit tests are added to nvme_control_ut.c to verify under ASan that opts.pci_allowed and pci_addr are both freed on return. Also fix mock_spdk_nvme_ctrlr_get_pci_device() to use a static local pool instead of calloc(), which was itself leaking under ASan. Signed-off-by: Cedric Koch-Hofer <cedric.koch-hofer@hpe.com>

It is unnecessary to evict the vos object from cache after related DTX committed; otherwise, other concurrent modification against the same object shard maybe required to retry. Signed-off-by: Fan Yong <fan.yong@hpe.com>

Retire these release branches now that 2.6.5 is released. Signed-off-by: Dalton Bohning <dalton.bohning@hpe.com>

Speedup ftest/nvme/io.py by: - Increasing the client processes - Reducing some aggregate file sizes Signed-off-by: Dalton Bohning <dalton.bohning@hpe.com>

github-actions · 2026-06-09T08:10:52Z

Ticket title is 'ddb: rm_pool fails with "Cannot use sys db path without a VOS path" regression'
Status is 'In Review'
https://daosio.atlassian.net/browse/DAOS-19122

Replace the two-statement var declaration + assignment with a single ':=' short variable declaration, as suggested in code review. Signed-off-by: Cedric Koch-Hofer <cedric.koch-hofer@hpe.com>

… parsers go-flags processes the full argument list before dispatching to grumble. When a subcommand defines a flag with the same name as a global option (e.g. 'rm_pool --db_path', 'open -w', 'feature -s'), go-flags intercepts it as the global option and the subcommand never receives it. This was introduced as a regression by commit 86f31a2 (DAOS-18304), which added a validation that rejected any invocation where --db_path was set (globally, by go-flags) without --vos_path -- even when the user correctly passed --db_path as a subcommand-level argument. Fix: add flags.PassAfterNonOption to the go-flags parser so it stops consuming flags after the first positional argument (the subcommand name). Everything after the subcommand lands in RunCmdArgs and is forwarded to grumble's own flag parser. Also: - Move vosPathMissErr to ddb_commands.go (sole use site after this fix) and add dtxAggrMutuallyExclusiveErr and dtxAggrRequiredOptErr constants to eliminate raw string literals in Go-layer error messages. - Fix a typo in the CLI long description (--vos-path -> --vos_path) and accurately list commands that manage their own pool lifecycle. - Update the two TestDdb_parseOpts cases whose behavior changed with PassAfterNonOption (--help after a subcommand now lands in RunCmdArgs instead of being processed by go-flags). Signed-off-by: Cedric Koch-Hofer <cedric.koch-hofer@hpe.com>

- TestDdb_parseOpts: add two cases verifying PassAfterNonOption behavior: - flags after the subcommand name are NOT consumed by go-flags (regression case) - flags before the subcommand name are still consumed globally (validation still fires) - TestDdb_runDdb: complete noAutoOpen coverage for all 8 commands in the noAutoOpen list (close, prov_mem, dev_list, dev_replace were missing). - TestDdb_Cmds: remove the now-obsolete skipCmdLine escape hatch and add regression coverage for the fixed flag conflicts: - open: long/short forms of -w/--write_mode and -p/--db_path now work correctly in command-line mode - rm_pool: --db_path after subcommand correctly reaches grumble - prov_mem: -s/--tmpfs_size flag no longer consumed as global VosPath Also update dtxAggr error assertions to use the new named constants. Signed-off-by: Cedric Koch-Hofer <cedric.koch-hofer@hpe.com>

- Add Go-layer input validation to the feature command Run handler: - Enforce that exactly one of --enable, --disable, --show is provided - Reject --db_path when no VOS path argument is given - Add featureOnlyOneOptErr constant and onlyOne() helper - Add LongHelp to the feature command documenting the validation rules - Replace featureFnCheckingShow with the more general featureFnChecking factory and add string2FlagsCapturing to verify enable/disable routing - Add full test coverage for all feature flags (short/long enable, disable, show, db_path) and all validation error paths Features: recovery Signed-off-by: Cedric Koch-Hofer <cedric.koch-hofer@hpe.com>

daosbuild3 · 2026-06-09T12:20:18Z

Test stage Functional on EL 9 completed with status FAILURE. https://jenkins-3.daos.hpc.amslabs.hpecorp.net//job/daos-stack/job/daos/view/change-requests/job/PR-18466/1/execution/node/1039/log

The syscall function signature changed, so suppressions needed to be updated. Also added a suppression related to an internal runtime context.Context implementation. Signed-off-by: Kris Jacque <kris.jacque@hpe.com>

daosbuild3 · 2026-06-09T23:59:35Z

Test stage Functional Hardware Medium MD on SSD completed with status FAILURE. https://jenkins-3.daos.hpc.amslabs.hpecorp.net//job/daos-stack/job/daos/view/change-requests/job/PR-18466/2/execution/node/1369/log

knard38 · 2026-06-10T07:34:43Z

The two functional test failures of the build #2 are known issues:

osa/online_drain.py:OSAOnlineDrain.test_osa_online_drain_oclass - TIMEOUT detected after 120 seconds while for waiting for rebuild to be completed: JIRA ticket DAOS-18218
recovery/check_start_corner_case.py:DMGCheckStartCornerCaseTest.test_two_pools_corrupted- ERROR: dmg: finding 0x24d1903e4c is already resolved: JIRA ticket DAOS-18773

tanabarr · 2026-06-10T09:22:42Z

+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 {


could this go in a generic utility file for reuse?

I hesitated to add it, but I am not sure it is worth of it ?
@kjacque , @mjmac and @tanabarr any opinion on this ?

Worker processes spawned by _Dfs.parallel_list may raise exceptions that never reached the calling process. This results in indefinite hang during Dataset and IterableDataset construction with no surfaced error to the user. Replacing manual Process + Queue scheme and its queued/processed counter with a multiprocessing.Pool driven by imap_unordered. Pool re-raises worker exceptions in the parent when their results are consumed, so a worker error now propagates as a raised OSError instead of a deadlock, and the Pool context manager reaps all workers on any exit path. `concurrent.futures.ProcessPoolExecutor` would be even better but its initializer/initargs arguments are unavailable before Python 3.7, and the target runtime includes EL8.8 / Python 3.6. Signed-off-by: Denis Barakhtanov <dbarahtanov@enakta.com>

…/daos-19122/patch-001 Features: recovery

daltonbohning

This PR seems to have a lot of commits already on master

knard38 · 2026-06-12T19:03:49Z

This PR seems to have a lot of commits already on master

I think, it is because I have recently merged the target PR with master.
Not sure of the best process to do in such situations, maybe I should have kept this PR as a draft until the target PR will have been merged with master.

daltonbohning · 2026-06-12T19:09:15Z

This PR seems to have a lot of commits already on master

I think, it is because I have recently merged the target PR with master. Not sure of the best process to do in such situations, maybe I should have kept this PR as a draft until the target PR will have been merged with master.

I think it's because the target branch daos-18304-patch-002 is behind this branch daos-19122/patch-001.
I would recommend NOT merging master into this branch, but instead merge whatever the TARGET is into this branch.

Nasf-Fan

The fixes looks fine for me. As my understand, current patch will be merged into ckochhof/fix/master/daos-18304-patch-002 instead of directly into master.

daosbuild3 · 2026-06-13T09:56:14Z

Test stage Functional Hardware Medium MD on SSD completed with status FAILURE. https://jenkins-3.daos.hpc.amslabs.hpecorp.net//job/daos-stack/job/daos/view/change-requests/job/PR-18466/3/execution/node/1311/log

frostedcmos and others added 5 commits June 5, 2026 12:30

DAOS-19031 cart: Fix test_ep_cred_client race condition (#18439)

ba24711

Fix test race condition where status was checked outside of the completion callback. Signed-off-by: Alexander A Oganezov <alexander.oganezov@hpe.com>

DAOS-19059 vos: cache vos object after DTX commit (#18416)

0ef5142

It is unnecessary to evict the vos object from cache after related DTX committed; otherwise, other concurrent modification against the same object shard maybe required to retry. Signed-off-by: Fan Yong <fan.yong@hpe.com>

DAOS-19085 cq: retire the release/2.6.4-* branches (#18452)

5500b14

Retire these release branches now that 2.6.5 is released. Signed-off-by: Dalton Bohning <dalton.bohning@hpe.com>

DAOS-18903 test: speedup ftest/nvme/io.py (#18370)

1e28381

Speedup ftest/nvme/io.py by: - Increasing the client processes - Reducing some aggregate file sizes Signed-off-by: Dalton Bohning <dalton.bohning@hpe.com>

knard38 self-assigned this Jun 9, 2026

knard38 added the CR Catastrophic Recovery Feature label Jun 9, 2026

knard38 changed the title ~~Ckochhof/fix/master/daos 19122/patch 001~~ DAOS-19122 ddb: Fix flag name collision between global and subcommand parsers Jun 9, 2026

kanard38 added 4 commits June 9, 2026 12:18

DAOS-19122 ddb: Use idiomatic short variable declaration for logger init

04cdf46

Replace the two-statement var declaration + assignment with a single ':=' short variable declaration, as suggested in code review. Signed-off-by: Cedric Koch-Hofer <cedric.koch-hofer@hpe.com>

knard38 force-pushed the ckochhof/fix/master/daos-19122/patch-001 branch from 8a13591 to c101fde Compare June 9, 2026 12:19

knard38 mentioned this pull request Jun 9, 2026

DAOS-18304 ddb: Add Go unit tests for ddb command layer #18086

Open

6 tasks

DAOS-19084 test: Suppress Cgo syscall false positives (#18448)

05984ff

The syscall function signature changed, so suppressions needed to be updated. Also added a suppression related to an internal runtime context.Context implementation. Signed-off-by: Kris Jacque <kris.jacque@hpe.com>

knard38 marked this pull request as ready for review June 10, 2026 07:35

knard38 requested review from a team as code owners June 10, 2026 07:35

knard38 requested a review from janekmi June 10, 2026 07:35

knard38 assigned mjmac, tanabarr, kjacque and Nasf-Fan and unassigned mjmac and tanabarr Jun 10, 2026

knard38 assigned knard38 and Nasf-Fan and unassigned tanabarr, kjacque, Nasf-Fan and knard38 Jun 10, 2026

knard38 requested review from Nasf-Fan, kjacque, mjmac and tanabarr June 10, 2026 07:37

tanabarr approved these changes Jun 10, 2026

View reviewed changes

enakta and others added 2 commits June 11, 2026 12:11

Merge remote-tracking branch 'origin/master' into ckochhof/fix/master…

9a6c7ec

…/daos-19122/patch-001 Features: recovery

knard38 requested review from a team as code owners June 12, 2026 14:00

daltonbohning reviewed Jun 12, 2026

View reviewed changes

Nasf-Fan approved these changes Jun 13, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

DAOS-19122 ddb: Fix flag name collision between global and subcommand parsers#18466
knard38 wants to merge 12 commits into
ckochhof/fix/master/daos-18304-patch-002from
ckochhof/fix/master/daos-19122/patch-001

knard38 commented Jun 9, 2026 •

edited

Loading

Uh oh!

github-actions Bot commented Jun 9, 2026 •

edited

Loading

Uh oh!

daosbuild3 commented Jun 9, 2026

Uh oh!

daosbuild3 commented Jun 9, 2026

Uh oh!

knard38 commented Jun 10, 2026

Uh oh!

tanabarr Jun 10, 2026

Uh oh!

knard38 Jun 10, 2026

Uh oh!

daltonbohning left a comment

Uh oh!

knard38 commented Jun 12, 2026

Uh oh!

daltonbohning commented Jun 12, 2026

Uh oh!

Nasf-Fan left a comment •

edited

Loading

Uh oh!

daosbuild3 commented Jun 13, 2026

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

10 participants

Conversation

knard38 commented Jun 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Solution

Commit 1 — Go style: idiomatic logger initialisation

Commit 2 — Fix: PassAfterNonOption

Commit 3 — Regression tests

Commit 4 — feature command improvements

Side effects

Steps for the author:

After all prior steps are complete:

Uh oh!

github-actions Bot commented Jun 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

daosbuild3 commented Jun 9, 2026

Uh oh!

daosbuild3 commented Jun 9, 2026

Uh oh!

knard38 commented Jun 10, 2026

Uh oh!

tanabarr Jun 10, 2026

Choose a reason for hiding this comment

Uh oh!

knard38 Jun 10, 2026

Choose a reason for hiding this comment

Uh oh!

daltonbohning left a comment

Choose a reason for hiding this comment

Uh oh!

knard38 commented Jun 12, 2026

Uh oh!

daltonbohning commented Jun 12, 2026

Uh oh!

Nasf-Fan left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

daosbuild3 commented Jun 13, 2026

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

10 participants

knard38 commented Jun 9, 2026 •

edited

Loading

Commit 2 — Fix: `PassAfterNonOption`

Commit 4 — `feature` command improvements

github-actions Bot commented Jun 9, 2026 •

edited

Loading

Nasf-Fan left a comment •

edited

Loading