DAOS-18304 ddb: Introduce DdbAPI interface and fix db_path propagation#18124
Conversation
…limits - Add vf_vos_file_path field to vos_file_parts to retain the original VOS file path through the parsing pipeline, so callers no longer need to keep the input pointer alive after parse_vos_file_parts() returns. - Rename the enum to introduce VOS_PATH_SIZE (alongside DB_PATH_SIZE) and raise both constants from 256 to PATH_MAX. - Validate the VOS path length in parse_vos_file_parts() and return -DER_EXCEEDS_PATH_LEN when the limit is exceeded. Signed-off-by: Cedric Koch-Hofer <cedric.koch-hofer@hpe.com>
…hing from ddb_ctx Remove dc_pool_path and dc_db_path from ddb_ctx: callers now pass paths as explicit arguments instead of storing them in the context, eliminating the need for the SetCString() helper. dv_pool_open() and dv_pool_destroy() signatures are simplified from (path, path_parts, poh, flags, write_mode) to (path, db_path, ctx, flags): - vos_file_parts allocation is moved into the new internal helper create_vos_file_parts(), keeping the API surface minimal. - dc_write_mode and dc_poh are set directly on the ctx, so the caller gets the pool handle back without an extra out-parameter. ddb_run_open(), ddb_run_feature() and ddb_run_rm_pool() in ddb_commands.c are simplified accordingly. ddb_run_feature() also gains an explicit error message on dv_pool_open() failure. ddb_run_dtx_stat() no longer prints ctx->dc_pool_path (which no longer exists). Signed-off-by: Cedric Koch-Hofer <cedric.koch-hofer@hpe.com>
Update all dv_pool_open() and dv_pool_destroy() call sites to use the new (path, db_path, ctx) signature: path_parts is no longer allocated by callers, and the pool handle is read back from ctx->dc_poh. ddb_parse_tests.c: add vf_vos_file_path assertions in success cases; add a test for VOS paths exceeding VOS_PATH_SIZE; add a dedicated MD-on-SSD test for DB_PATH_SIZE independently of VOS_PATH_SIZE; introduce the MOCKED_VOS_PATH_STR macro to avoid repeated string literals. ddb_commands_tests.c: move dvt_vos_insert_2_records_with_dtx() into dcv_suit_setup() so dtx records are present for all tests in the suite; correct VOS tree path indices for ls, value_dump and ilog_dump tests; update the dtx_stat expected message. ddb_vos_tests.c: update open_pool_test(), pool_flags_tests(), dv_test_setup() and helper_stat_open_modify_close_stat() for the new API. Signed-off-by: Cedric Koch-Hofer <cedric.koch-hofer@hpe.com>
…methods Add the DdbAPI interface that lists every ddb sub-command as a method. It decouples the command layer from the concrete CGo implementation so tests can inject a stub without a live VOS environment. Convert all ddbXxx() free functions to (ctx *DdbContext) Xxx() methods, matching the DdbAPI interface. InitDdb() becomes (ctx *DdbContext) Init() so the caller (or a stub) controls object allocation. Open(), Feature() and RmPool() now receive db_path as an explicit argument instead of reading it from ctx.ctx.dc_db_path (which was removed from the C struct in the previous commit). Signed-off-by: Cedric Koch-Hofer <cedric.koch-hofer@hpe.com>
ddb_commands.go: addAppCommands() now takes a DdbAPI instead of a *DdbContext; every command Run closure calls api.Xxx() instead of the former ddbXxx(ctx, ...) free functions. The 'open' and 'feature' commands pass db_path explicitly; 'rm_pool' now requires --vos_path. main.go: - parseOpts() no longer calls os.Exit(); it returns (cliOptions, *flags.Parser, error) so the function is fully unit-testable. - A new run() function owns the DDB context lifetime: Init/defer-cleanup, auto-open logic, non-interactive command dispatch, interactive loop. - --version is handled in main() before run(), printing directly instead of synthesising a fake command. - --db_path without --vos_path returns an explicit error. - The HasPrefix loop for auto-open exclusions is replaced by a noAutoOpen map for exact command-name matching. - Pool close is consolidated into a closePoolIfOpen() helper used with defer. - Errors from runCmdStr() and runFileCmds() are now propagated. - unknownCmdError typed struct enables type-safe detection; exitWithError() prints the command list on unknown commands and fixes the duplicated ERROR: prefix in fault resolution messages. - errHelpRequested sentinel avoids os.Exit() in the --help path. Signed-off-by: Cedric Koch-Hofer <cedric.koch-hofer@hpe.com>
Fix reviewers comments: - Use MOCKED_VOS_PATH_STR instead of the hardcoded "/" MOCKED_POOL_UUID_STR "/vos-0" string literal. Signed-off-by: Cedric Koch-Hofer <cedric.koch-hofer@hpe.com>
Fixed reviewers comments: - Use slice instead of dictionary Signed-off-by: Cedric Koch-Hofer <cedric.koch-hofer@hpe.com>
|
Ticket title is 'Add unit test to ddb go code' |
|
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-18124/2/execution/node/1184/log |
janekmi
left a comment
There was a problem hiding this comment.
A few minor issues. I am very happy with this change otherwise. 👍
|
|
||
| /* Test invalid vos paths with too long db path */ | ||
| /* Test invalid vos paths with too long vos path */ | ||
| D_ALLOC_ARRAY_CHECK(buf, VOS_PATH_SIZE + 1); |
There was a problem hiding this comment.
Nitpick. But you do not have to allocate, free and re-allocate again. You can make it big enough to handle all cases and just populate it with what is necessary.
There was a problem hiding this comment.
The reason I allocate, free, and re-allocate is to ensure the buffer is always freed even when an assertion fails. From what I know, cmocka's assert_* macros use longjmp under the hood, so a failing assert exits the test immediately, bypassing any cleanup that follows.
By freeing buf before calling assert_rc_equal, I guarantee no leak regardless of the test outcome.
There was a problem hiding this comment.
There is no leak when the process is terminated. AFAIK OS reclaims all of the process' memory on termination. So, you do not have to worry about it.
You may argue it is a bad practice. But IMHO the happy day scenario is even worse since each of the successful rounds do a lot more than is necessary just for the sake of testing what you want to test.
Please reconsider.
There was a problem hiding this comment.
You are right that the OS reclaims the process memory on exit.
My concern is that valgrind memcheck, which is integrated in the CI, will still report unfreed heap allocations at process exit — and the same issue would arise if DAOS is compiled with libasan.
The current free-before-assert pattern is the simplest way that I find to ensure clean output from these tools. The only acceptable alternative I can think of would be to replace assert_rc_equal with an if + goto to an error label where the free happens before asserting — but that feels like more complexity than the problem warrants for a test helper.
From my side, stack-allocating a ~4 KB buffer is not an acceptable solution.
Does that makes sense?
Update help text and descriptions to use "VOS" instead of "vos" for consistency. Also clarify "vos db" references to use "sys db" where appropriate. Signed-off-by: Cedric Koch-Hofer <cedric.koch-hofer@hpe.com>
Fix reviewers comments. Signed-off-by: Cedric Koch-Hofer <cedric.koch-hofer@hpe.com>
…/daos-18304-patch-001-split
| Name: "close", | ||
| Aliases: nil, | ||
| Help: "Close the currently opened vos pool shard", | ||
| Help: "Close the currently opened VOS pool shard", |
There was a problem hiding this comment.
You may chose to ignore this one. Since it is getting further and further from your core change.
We are opening a file but closing a shard? 🤔
| Help: "Close the currently opened VOS pool shard", | |
| Help: "Close the currently opened VOS pool file", |
There was a problem hiding this comment.
- Fix help
shard->file
| Name: "rm_pool", | ||
| Aliases: nil, | ||
| Help: "Remove a vos pool.", | ||
| Help: "Remove a VOS pool.", |
There was a problem hiding this comment.
A little bit similar here. We are removing a pool but the argument is a file. Confusing.
There was a problem hiding this comment.
- Fix help
pool->file
kjacque
left a comment
There was a problem hiding this comment.
Only reviewed the Go side. LGTM. Comments are minor.
Nasf-Fan
left a comment
There was a problem hiding this comment.
Looks fine for me. Please resolve the merge conflict and re-commit. Thanks!
|
|
||
| assert_success(vos_cont_open(ctx.dc_poh, g_uuids[0], &coh)); | ||
|
|
||
| dvt_vos_insert_2_records_with_dtx(coh); |
There was a problem hiding this comment.
What is the purpose of inserting two records after opening the container during the setup?
There was a problem hiding this comment.
Originally (on master), dvt_vos_insert_2_records_with_dtx was called inside
dump_dtx_cmd_tests itself, since it was the only test that needed DTX records.
This PR adds four new DTX tests (dtx_stat_tests, dtx_commit_entry_tests,
dtx_act_discard_invalid_tests, dtx_abort_entry_tests) that all require those
records to be present. Rather than having dump_dtx_cmd_tests silently act as
setup for the tests that follow it — which would be fragile and order-dependent —
the insertion was moved to dcv_suit_setup so it is part of the explicit, declared
fixture for the entire suite.
I will add a comment to make this intent clear:
- Add comment on
dvt_vos_insert_2_records_with_dtx(coh)
…/daos-18304-patch-001-split
| // DdbAPI defines the set of ddb operations exposed to the command layer. | ||
| // It abstracts the concrete DdbContext implementation to enable dependency | ||
| // injection and unit testing via mock implementations. | ||
| type DdbAPI interface { |
There was a problem hiding this comment.
Hmm. I understand the intent behind this interface, but giant predefined "god" interfaces is a well-known anti-pattern in Go. Per the community review guidelines, interfaces should be defined at the consumer site. This guidance is codified in the "Accept interfaces, return structs" proverb. While superficially the predefined interface may seem like the easier or more convenient path, it's a trap. It means that every implementation has to define all of those methods, leading to maintenance headaches and friction that results in good tests not being written.
At a minimum, just rearchitecting the tests to define interfaces for the subset of DdbContext methods that will be tested should make this much less of a tech debt trap.
For an alternative approach to doing Go/cgo testing, take a look at src/control/lib/daos/api. It's not perfect, but I think it achieves the goal of exposing as much of the Go surface for unit testing as possible. It also demonstrates a better design (IMO) than a single API object thing that has (at current count) 28 methods. I suggest a smaller, focused fix for the db_path problem and a follow up effort to rework this package along similar lines to the libdaos Go API. It's more work in the short term, but will yield long-term benefits.
There was a problem hiding this comment.
Agreed. I think the argument for going this PR's route is entirely as an intermediate step to increase testability in the short term. It is indeed technical debt that will need a thoughtful redesign in the future either way.
There was a problem hiding this comment.
@mjmac thank you for pointing this out with a concrete alternative to follow.
My apologies to @tanabarr and @kjacque who had already pinpointed this issue — I had not fully
internalized how much the god-interface pattern goes against Go best practices. I have to admit
that at the time I did not see how to solve it without the god-interface pattern: still a Go
padawan, am I 😉
Following @mjmac's suggestion, I preferred fixing it now rather than carrying it as technical debt,
as in my experience it rarely gets easier to address later. I reworked the code to adopt the
build-tag CGo stub pattern used in src/control/lib/daos/api. Commit 7310312 replaces the
DdbAPI god interface with that approach, and PR #18086 shows the stubs being exercised in unit
tests.
| int | ||
| dv_pool_open(const char *path, struct vos_file_parts *path_parts, daos_handle_t *poh, | ||
| uint32_t flags, bool write_mode) | ||
| dv_pool_open(const char *path, const char *db_path, struct ddb_ctx *ctx, uint32_t flags) |
There was a problem hiding this comment.
One potential problem with these changes is that it reduces composability by tying the code to the ddb CLI implementation. If there is any desire to repackage the shim later for improved maintainability and testability, this is a change that would need to be undone.
There was a problem hiding this comment.
Thanks for pointing out this issue. Commit 9de3531 addresses it as you suggested.
While refactoring the god-interface pattern, I also identified a related coupling issue on the Go side: the CGo wrapper (commands_wrapper.go) was carrying CLI-specific logic — namely DtxAggr
argument validation and a log proxy field — that had no place in a translation layer between Go
and C.
The commit d683277 fixes this by moving the validation into the command layer and removing the log field from DdbContext.
Replace the opaque struct ddb_ctx * parameter with explicit poh and write_mode parameters so that dv_pool_open is a self-contained VOS helper. The coupling was identified by mjmac during code review of PR #18124. Move the vmd_wa_can_proceed() guard to the two callers (ddb_run_open and ddb_run_feature) using the existing DDB_CAN_PROCEED macro, which is already used at every other call site where a db_path is needed. No behaviour change: callers set ctx->dc_write_mode before calling dv_pool_open, and now also pass it explicitly. Signed-off-by: Cedric Koch-Hofer <cedric.koch-hofer@hpe.com>
Address the god-interface anti-pattern identified by mjmac in PR #18124. Instead of a DdbAPI interface satisfied by a heavyweight DdbContextStub, use the build-tag pattern established in src/control/lib/daos/api: carries the -lddb / -lgurt LDFLAGS so that test builds do not require the ddb shared library. - libddb_stubs.go (test_stubs): per-function RC variables and Go-typed hook functions (ddb_run_ls_Fn, ddb_run_open_Fn, …) that tests set directly — no interface boilerplate needed. - commands_wrapper.go: drop DdbAPI interface and #cgo LDFLAGS; all C.ddb_xxx() calls are replaced by the Go wrapper calls defined in the two files above. - ddb_commands.go / main.go: replace DdbAPI with *DdbContext throughout. Signed-off-by: Cedric Koch-Hofer <cedric.koch-hofer@hpe.com>
The mutual-exclusion check between --cmt_time and --cmt_date belongs in the command layer (ddb_commands.go) rather than in the CGo wrapper. Moving it there removes the only caller of ctx.log, which in turn allows two further simplifications: - Remove the log field from DdbContext (it was only a proxy for passing error messages up from the wrapper layer). - Simplify Init() to Init() (func(), error) — the logger argument is no longer needed at context-initialisation time. The validation now returns a plain fmt.Errorf, consistent with how other command-layer argument errors (e.g. flag parsing) are reported. Signed-off-by: Cedric Koch-Hofer <cedric.koch-hofer@hpe.com>
unknownCmdError.Error() previously emitted a different string than grumbleUnknownCmdErr. Tests using errUnknownCmd (defined as ddbTestErr(grumbleUnknownCmdErr)) failed for the command-line code path because the error text did not contain grumbleUnknownCmdErr. Reuse grumbleUnknownCmdErr as the prefix so that assertions work consistently for both the command-line and command-file execution paths. Signed-off-by: Cedric Koch-Hofer <cedric.koch-hofer@hpe.com>
Apply gofmt alignment to function declarations in libddb.go and libddb_stubs.go to keep the files consistently formatted. Signed-off-by: Cedric Koch-Hofer <cedric.koch-hofer@hpe.com>
Replace the single-line comment with a full Doxygen block. Fix db_path description (NULL/empty derives the DB dir from path), correct the flags example from VOS_POF_RDB to VOS_POF_FOR_FEATURE_FLAG, and document the copy-on-write mechanism used by write_mode=false. Signed-off-by: Cedric Koch-Hofer <cedric.koch-hofer@hpe.com>
…/daos-18304-patch-001-split
Signed-off-by: Cedric Koch-Hofer <cedric.koch-hofer@hpe.com>
Replace \param[in]/\param[out]/\return tags with the @param/@return style used throughout the DAOS codebase (e.g. ddb.h, ddb_tree_path.h). Remove direction tags [in]/[out] as they are not used in DAOS Doxygen. Signed-off-by: Cedric Koch-Hofer <cedric.koch-hofer@hpe.com>
Restore the original 'Unknown command: <cmd>. Run help to see available commands.' message in unknownCmdError.Error() instead of reusing grumble's internal error string. Also fix runFileCmds to detect the grumble unknown-command sentinel and wrap it as an unknownCmdError, so that both the command-line and command-file paths produce a consistent error message. The test sentinel in patch-002 is updated accordingly to match 'Unknown command:' rather than grumble's internal string. Signed-off-by: Cedric Koch-Hofer <cedric.koch-hofer@hpe.com>
|
@daos-stack/daos-gatekeeper please could you lend this PR with the following message please:
|
Introduce the Go test suite for the ddb CLI layer, built on top of the build-tag CGo stub infrastructure landed in #18124: - Add test_helpers.go: newTestContext(t) resets all CGo stubs via resetDdbStubs() and returns a *DdbContext ready for use in tests. Test cases set per-function _Fn hook variables directly. - All test files carry the //go:build test_stubs tag so they only compile when the stub infrastructure is present. - TestCmds: open (default, write_mode, db_path), feature (show, enable, disable), and dtx_aggr (mutual exclusion, cmt_time, cmt_date, path). Adds skipCmdLine field for flags shared between CLI and grumble layers. - TestHelpCmds: unknown-command help flow. - TestParseOpts / TestRun: CLI-level option parsing and run() dispatch, including unknown-command detection for both command-line and command-file paths. - TestNewLogger: 6 sub-cases (default level, explicit debug, invalid level, valid LogDir, non-existent LogDir, LogDir is a file). - TestClosePoolIfOpen: Close not called when already closed, called when open, Close error tolerated. Test-tag: unittest Required-githooks: yes Signed-off-by: Cedric Koch-Hofer <cedric.koch-hofer@hpe.com>
Introduce the Go test suite for the ddb CLI layer, built on top of the build-tag CGo stub infrastructure landed in #18124: - Add test_helpers.go: newTestContext(t) resets all CGo stubs via resetDdbStubs() and returns a *DdbContext ready for use in tests. Test cases set per-function _Fn hook variables directly. - All test files carry the //go:build test_stubs tag so they only compile when the stub infrastructure is present. - TestCmds: open (default, write_mode, db_path), feature (show, enable, disable), and dtx_aggr (mutual exclusion, cmt_time, cmt_date, path). Adds skipCmdLine field for flags shared between CLI and grumble layers. - TestHelpCmds: unknown-command help flow. - TestParseOpts / TestRun: CLI-level option parsing and run() dispatch, including unknown-command detection for both command-line and command-file paths. - TestNewLogger: 6 sub-cases (default level, explicit debug, invalid level, valid LogDir, non-existent LogDir, LogDir is a file). - TestClosePoolIfOpen: Close not called when already closed, called when open, Close error tolerated. Test-tag: unittest Required-githooks: yes Signed-off-by: Cedric Koch-Hofer <cedric.koch-hofer@hpe.com>
Introduce the Go test suite for the ddb CLI layer, built on top of the build-tag CGo stub infrastructure landed in #18124: - Add test_helpers.go: newTestContext(t) resets all CGo stubs via resetDdbStubs() and returns a *DdbContext ready for use in tests. Test cases set per-function _Fn hook variables directly. - All test files carry the //go:build test_stubs tag so they only compile when the stub infrastructure is present. - TestCmds: open (default, write_mode, db_path), feature (show, enable, disable), and dtx_aggr (mutual exclusion, cmt_time, cmt_date, path). Adds skipCmdLine field for flags shared between CLI and grumble layers. - TestHelpCmds: unknown-command help flow. - TestParseOpts / TestRun: CLI-level option parsing and run() dispatch, including unknown-command detection for both command-line and command-file paths. - TestNewLogger: 6 sub-cases (default level, explicit debug, invalid level, valid LogDir, non-existent LogDir, LogDir is a file). - TestClosePoolIfOpen: Close not called when already closed, called when open, Close error tolerated. Test-tag: unittest Required-githooks: yes Signed-off-by: Cedric Koch-Hofer <cedric.koch-hofer@hpe.com>
Introduce the Go test suite for the ddb CLI layer, built on top of the build-tag CGo stub infrastructure landed in #18124: - Add test_helpers.go: newTestContext(t) resets all CGo stubs via resetDdbStubs() and returns a *DdbContext ready for use in tests. Test cases set per-function _Fn hook variables directly. - All test files carry the //go:build test_stubs tag so they only compile when the stub infrastructure is present. - TestCmds: open (default, write_mode, db_path), feature (show, enable, disable), and dtx_aggr (mutual exclusion, cmt_time, cmt_date, path). Adds skipCmdLine field for flags shared between CLI and grumble layers. - TestHelpCmds: unknown-command help flow. - TestParseOpts / TestRun: CLI-level option parsing and run() dispatch, including unknown-command detection for both command-line and command-file paths. - TestNewLogger: 6 sub-cases (default level, explicit debug, invalid level, valid LogDir, non-existent LogDir, LogDir is a file). - TestClosePoolIfOpen: Close not called when already closed, called when open, Close error tolerated. Test-tag: unittest Required-githooks: yes Signed-off-by: Cedric Koch-Hofer <cedric.koch-hofer@hpe.com>
Description
This PR is the second split of #17967, broken out at reviewers' request to make the review process easier. The first split #18125, contains unrelated code fixes found during the creation of the initial PR #17967. The content of this PR cannot be reduced further without breaking the build, as the Go interface and the C
dv_pool_openAPI changes are tightly coupled: the new method signatures in the Go layer directly depend on the updated C function signature, so neither half can land independently.This patch refactors the
ddbGo CLI layer and the underlying C VOS pool open API to improve testability and fix adb_pathpropagation bug.Go layer
DdbAPIinterface incommands_wrapper.gocapturing all ddb operations, decoupling command dispatch from the concreteDdbContextimplementation.ddbXxx(ctx *DdbContext, ...)functions to methods on*DdbContext, makingDdbContextimplementDdbAPI.dbPathexplicitly toOpen,Feature, andRmPoolinstead of storing it globally inctx.ctx.dc_db_path, preventing the value from leaking across commands.C layer
dv_pool_openAPI: callers pass a*ddb_ctxdirectly instead of a pre-parsedvos_file_parts; the function populatesctx->dc_poh.vf_vos_file_pathwithinvos_file_parts, enabling callers to retrieve it without re-parsing.VOS_PATH_SIZEoverflow test and improves assertion coverage inddb_parse_tests.c.Steps for the author:
After all prior steps are complete: