Repository navigation
DAOS-19513 ddb: extend SPDK re-init guard to all NVMe devices - #18962
Conversation
|
Ticket title is 'ddb: SPDK re-initialization fails for standard (non-VMD) NVMe devices within a single ddb process' |
397f5c4 to
d844603
Compare
|
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-18962/2/execution/node/1781/log |
d844603 to
2514ce0
Compare
DAOS-18597 (PR #17838)'s guard against a crash-prone second in-process SPDK/VOS init only applied when VMD was involved, but the same SPDK re-init limitation affects any NVMe-backed pool. Generalize the guard (ddb_vmd_wa.c/vmd_wa_can_proceed() -> ddb_spdk_reinit_wa.c/ dwa_can_proceed()) to cover all NVMe-backed pools by checking for <db_path>/daos_nvme.conf; pools with no NVMe config stay safe to reuse indefinitely. dwa_can_proceed(ctx, nvme_conf_dir, can_proceed) is now the single entry point for all 7 affected commands, called from ddb_vos.c next to each vos_self_init() it protects (fixing a bug where dv_pool_open() checked the wrong, unresolved db_path). NULL nvme_conf_dir means "always configured" (smd_sync's SPDK config is a separate nvme_conf argument, independent of db_path); the int return propagates a real internal check error separately from can_proceed's actual allow/refuse decision. Add a diagnostic-only DAOS_DDB_ALLOW_SPDK_REINIT environment variable (compiled out of release builds) to bypass the guard when validating a newer SPDK/DPDK version against real hardware. Document the limitation in ddb's README, --help, and man page. Expand ddb_ut coverage: a dedicated ddb_spdk_reinit_wa_ut.c suite (sequencing, the diagnostic override, NULL-argument asserts), and a gurt-style __wrap_dwa_can_proceed mock (ddb_mocks.c, shared by ddb_commands_ut.c) covering every call site's rc/can_proceed propagation, including feature_uses_derived_db_path_test, which proves ddb_run_feature() checks the guard against the resolved db_path the same way ddb_run_open() does. Features: recovery Signed-off-by: Cedric Koch-Hofer <cedric.koch-hofer@hpe.com>
2514ce0 to
03ef3e0
Compare
…/daos-19513/patch-001 Features: recovery
|
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-18962/6/execution/node/1813/log |
…/daos-19513/patch-001
Moves the dwa_can_proceed() guard check in dv_sync_smd() before the real vos_self_init_ext()/smd_init() calls it protects, simplifying its error handling and removing the VOS/SMD test mocks that guard ordering had required. Relocates the guard's unit tests from ddb_commands_ut.c into ddb_vos_ut.c, exercising the dv_*() VOS API directly instead of through the ddb_run_*() command wrappers, and extends coverage to dev_list, dev_replace, prov_mem, and smd_sync. Also centralizes the DAOS_DDB_ALLOW_SPDK_REINIT env var name in ddb_spdk_reinit_wa.h, tightens the new README.md/man page wording, fixes a stray non-ASCII hyphen, and normalizes comment formatting across the changed files. Features: recovery Signed-off-by: Cedric Koch-Hofer <cedric.koch-hofer@hpe.com>
janekmi
left a comment
There was a problem hiding this comment.
Please note my reply in this thread as well:
|
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-18962/7/execution/node/1770/log |
| void | ||
| mock_dwa_can_proceed_set(dwa_can_proceed_fn_t mock_fn) | ||
| { | ||
| mock_dwa_can_proceed = __real_dwa_can_proceed; |
There was a problem hiding this comment.
Why need to overwrite the old one before re-assign it as new fn?
There was a problem hiding this comment.
if mock_fn is NULL, the default value should be equal to the real dwa_can_proceed() function.
| #ifndef DAOS_BUILD_RELEASE | ||
| if (reinit_diagnostic_override()) { | ||
| *can_proceed = true; | ||
| return -DER_SUCCESS; |
There was a problem hiding this comment.
Why someone return -DER_SUCCESS, but some others return -DER_SUCCESS, such as L45? Can we unify them?
There was a problem hiding this comment.
Oups error from my side
- Replace DER_SUCCESS with -DER_SUCCESS
…/daos-19513/patch-001 Features: recovery
|
Test stage Test RPMs on Leap 15 completed with status FAILURE. https://jenkins-3.daos.hpc.amslabs.hpecorp.net//job/daos-stack/job/daos/view/change-requests/job/PR-18962/9/execution/node/1251/log |
tanabarr
left a comment
There was a problem hiding this comment.
This looks like a good improvement, maybe @NiuYawei could review the src/utils/ddb/ddb_spdk_reinit_wa.c and following changes as listed in the github UI review page to check that this seems sound as it's an important change in terms of data integrity.
Is there a specific reason we initialize SPDK per ddb command? Initializing it once on startup (like the DAOS engine) seems more efficient. |
It depends of the commands.
This patch is thus dedicated to manage this second type of commands. Doest it makes sense ? |
Why the second type of commands need to reinitialize SPDK? DAOS engine also offers this type of function (open, close, pool destroy, etc.) without reinitializing SPDK, right? |
TBH, I'm not the original author of this part, but the reasoning seems to be architectural: unlike daos_engine, which is started once against one fixed set of NVMe/SCM targets for its whole lifetime, a single ddb process isn't tied to one storage config. In an interactive or So each command claims the SPDK/NVMe environment its own target needs, then releases it, instead of assuming the whole session only ever touches the config initialized first. |
Thanks for clarifying. Implementing ddb to re-initialize SPDK per command appears to be a design oversight, unless we explicitly need to switch daos_nvme.conf configurations dynamically during a single ddb process run. |
That's a fair point. Whether ddb genuinely needs to switch I'll open a new JIRA ticket to track that investigation, since it's out of scope for this PR. Thanks for pushing as it could noticeably help improve the ddb user experience. |
Created followup ticket DAOS-19628 |
…/daos-19513/patch-001
Simplify the ddb_ut mocks for dwa_can_proceed(), ddb_parse_vos_file_parts(), and d_asprintf2(): parameters whose expected value is a fixed constant or otherwise unconstrained (vos_path, db_path, vos_file_parts, fmt, can_proceed) now use a direct assert_ptr_equal()/assert_non_null() in the mock body instead of cmocka's check_expected_ptr()/expect_*() bookkeeping, and the now- redundant expect_*() calls are dropped from every call site across the file. dwa_can_proceed_mock()'s ctx check is strengthened from expect_any() to expect_value() so tests verify the exact context pointer is forwarded. Rename wrap_vos_dtx_discard_invalid_mock() to vos_dtx_discard_invalid_mock(), matching the naming of the file's other wrap-mocks. Signed-off-by: Cedric Koch-Hofer <cedric.koch-hofer@hpe.com>
|
@daos-stack/daos-gatekeeper could you lend this PR with the following message:
|
Extends DAOS-18597's guard (introduced in #17838) against ddb's SPDK re-initialization crash so that it covers every NVMe-backed pool, not just pools using the VMD subsystem.
Problem
Unlike
daos_engine, which initializes SPDK only once during the lifetime of the process,ddballows several pool-lifecycle commands (open,rm_pool,feature,dev_list,dev_replace,prov_mem,smd_sync) to run within the same interactive or-fcommand-file session, each potentially performing its ownvos_self_init()/SPDK initialization. SPDK does not fully support being re-initialized within the same process: its re-init path only rescans the PCI bus and never rebuilds the DPDK memory/address-translation tables torn down by the previousvos_self_fini(). This is unsafe for any pool backed by an NVMe device (i.e. whosedb_pathhas adaos_nvme.conf), whether or not VMD is involved, but the existing guard only checked for VMD, so a second in-process init against a plain (non-VMD) NVMe device could still crash ddb.Why extend the guard rather than fix SPDK or add a CLI-level guard
A real fix (making SPDK's
spdk_env_init()reinit path rebuildmem_map/vtophysinstead of assuming they're still valid) would be a new SPDK capability that doesn't exist today — the pieces it needs are internal-only APIs, so it would have to be designed and accepted upstream with no committed timeline. A blanket CLI-level "one lifecycle command per process" counter was also considered, but it's less precise than a per-pool-init guard: for examplefeaturecan safely reuse an already-open pool without a new init, something a command counter can't distinguish from a genuine second init. Generalizing the existing per-process "used once" guard to all NVMe-backed pools is the minimal, low-risk option with no upstream dependency.Bugs found and fixed while generalizing the guard
Moving the guard's call sites next to each
vos_self_init()surfaced two pre-existing bugs in how the old, VMD-only guard was invoked. Both are fixed by this PR:open(andfeature) checked the wrong, unresolveddb_path. Both commands take an optional-p/--db_paththat, when omitted — the normal way to invoke either command — defaults to an empty string and is only derived from the poolpathargument insidedv_pool_open()/parse_vos_file_parts(). The old guard instead ran inddb_commands.cbefore that derivation, against the rawopt->db_pathdirectly. With no--db_pathgiven, that'sNULL/empty, sovmd_subsystem_required()'s<dir>/daos_nvme.conflookup built an unusable path (e.g."(null)/daos_nvme.conf") that can never exist, andaccess()always failed — the guard silently assumed "no VMD/NVMe" regardless of the pool's actual, derived configuration. In other words, the guard was effectively bypassed wheneveropen/featurewere used the normal way (no explicit--db_pathoverride). Fixed by moving the check insidedv_pool_open(), after path resolution, so it always evaluates the real, resolvedvf_db_path.Example: run
open <pool-a>/vos-0then, later in the same interactive/-fsession,open <pool-b>/vos-0— two different NVMe-backed pools, neitheropencall passing--db_path. Both calls hit the bogus, nonexistent<dir>/daos_nvme.confpath described above, so both are reported as "no VMD/NVMe" and allowed unconditionally.vos_self_init()therefore runs a second time, in the same process, for a second NVMe-backed pool — exactly the unsafe re-init this guard exists to prevent — and ddb crashes. This is hit through completely ordinary, default-argument use ofopen, not some obscure edge case.Introduced by DAOS-18304 ddb: Introduce DdbAPI interface and fix db_path propagation #18124.
Test coverage:
tests/ddb_commands_ut.c::open_uses_derived_db_path_testand the newfeature_uses_derived_db_path_testlock this in foropen/featurerespectively — both leaveopt.db_pathNULLand assert, via the__wrap_dwa_can_proceedmock, that the guard is still called with the path derived fromopt.path("/mnt/daos2"), not the omitted raw value.rm_pool_uses_derived_db_path_testproves the same property fordv_pool_destroy()/rm_pool(not itself buggy pre-fix, but sharing the same derivation code).feature_uses_derived_db_path_testspecifically targets the case wherefeatureopens the pool itself (ctx.dc_pohnot already valid, i.e.ddb_pool_is_open(ctx)is false); when a pool is already open in the session (e.g. after a precedingopen),ddb_run_featurereuses it via agoto skipshortcut and never reachesdv_pool_open()/the guard at all, so that reuse path isn't (and can't usefully be) exercised by this kind of test.smd_syncchecked the wrong file entirely. The old guard also probed<db_path>/daos_nvme.confforsmd_sync, butsmd_sync's SPDK/NVMe configuration always comes from its separatenvme_confargument — it has no relationship todb_path(used only for the unrelated SMD/VOS system DB). So the old guard's VMD/NVMe determination forsmd_syncwas based on the wrong file and could come out wrong in either direction. Fixed by havingsmd_synccalldwa_can_proceed(ctx, NULL, ...), which unconditionally treats it as NVMe/SPDK-backed instead of probingdb_path.Example: run
smd_sync --nvme_conf /path/to/nvme.conf --db_path /mnt/daos0where/mnt/daos0has nodaos_nvme.confof its own (or one that doesn't match what/path/to/nvme.confactually configures), thenopen <nvme-backed-pool>/vos-0in the same session. The old guard checks/mnt/daos0/daos_nvme.conffor thesmd_synccall, finds nothing relevant, and reports "no NVMe here" even thoughsmd_syncreally does initialize SPDK from/path/to/nvme.conf. The followingopenthen sees an NVMe-backed pool for what it thinks is "the first time" and proceeds — a real second SPDK init in the same process that the guard should have refused.Introduced by DAOS-18597 ddb: enforce SPDK re-init rules #17838.
Test coverage:
tests/ddb_commands_ut.c::smd_sync_checks_guard_unconditionally_testdeliberately sets mismatchedopt.nvme_conf = "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/configs/engine.json"andopt.db_path = "/mnt/daos_without_a_daos_nvme_conf", then asserts via the mock (expect_value(dwa_can_proceed_mock, nvme_conf_dir, 0)) that the guard is still called withnvme_conf_dir == NULL— i.e. that it's checked unconditionally rather than against the (irrelevant)db_path.vos_self_init_ext()/smd_init()are mocked too, since the guard sits after they succeed.Changes
ddb_spdk_reinit_wa.c/.h(replacesddb_vmd_wa.c)vmd_wa_can_proceed()is replaced bydwa_can_proceed(ctx, nvme_conf_dir, can_proceed), the single entry point for the guard:nvme_conf_diris non-NULL and<nvme_conf_dir>/daos_nvme.confdoesn't exist, the operation is PMEM-only and always allowed.nvme_conf_dir == NULL, meaning "always treat as NVMe-backed") a process-lifetimenvme_used_onceflag gates the decision: the first NVMe-backed operation is allowed, later ones are refused with an actionable "restart ddb" message reported viactx.intreturn is a real internal error (e.g.-DER_NOMEMfrom thedaos_nvme.confcheck), kept separate fromcan_proceed's allow/refuse decision so callers don't conflate the two.DAOS_DDB_ALLOW_SPDK_REINITenvironment variable unconditionally bypasses the guard. It's compiled out of release builds (DAOS_BUILD_RELEASE) and is meant only for developers re-validating a newer SPDK/DPDK version against real hardware — not a supported workflow.ddb_vos.cThe guard call moves next to each
vos_self_init()/vos_self_init_ext()it protects, instead of living in the caller:dv_pool_open()(open,feature) anddv_pool_destroy()(rm_pool) check the resolvedvf_db_pathfromcreate_vos_file_parts()— this is bug fix Adding a .travis.yml file to trigger CI builds. #1 above.dv_sync_smd()(smd_sync) calls the guard withnvme_conf_dir == NULL— this is bug fix Update README.md #2 above.dv_dev_list()(dev_list),dv_dev_replace()(dev_replace), anddv_run_prov_mem()(prov_mem) each gain a newstruct ddb_ctx *ctxparameter and checkdb_pathdirectly.ddb_commands.cRemoves the old
DDB_CAN_PROCEED/vmd_wa_can_proceed()forward-declaration and macro. Call sites now simply forwardctxinto thedv_*VOS API instead of checking before calling it.Documentation
Documents the limitation in three places: a new "Limitations" section in
src/utils/ddb/README.md, a short paragraph in ddb's--help/interactive help text (main.go), and a new "SPDK RE-INITIALIZATION" section in the generated man page (manpage.go), including the special-casedsmd_syncbehavior.Tests
tests/ddb_spdk_reinit_wa_ut.csuite covering the guard's state machine (sequencing of allow/refuse decisions), the diagnostic override, and NULL-argument asserts.tests/ddb_mocks.c/.hwith a gurt-style__wrap_dwa_can_proceedmock, shared byddb_commands_ut.c, covering every call site'src/can_proceedpropagation, including the newfeature_uses_derived_db_path_test(see bug Adding a .travis.yml file to trigger CI builds. #1 above).tests/ddb_vos_tests.candtests/ddb_commands_tests.cupdated for the newctxparameter on thedv_*API.ddb_commands_test.goextends the Go man-page ordered-content assertion to include the new SPDK re-init section.Steps for the author:
After all prior steps are complete: