DAOS-19008 control: Erase formatting after failed format --replace by tanabarr · Pull Request #18446 · daos-stack/daos

tanabarr · 2026-06-05T14:11:20Z

Prevent a scenario where an engine may rejoin with an unintended new
rank following a failed dmg storage format --replace operation.

If the join-replace fails, the storage remains formatted and the
engine is left in a state where it is treated as a fresh instance and
will join the system with a new rank ID when restarted. Avoid this
inadvertent creation of a new rank by removing the engine's superblock
immediately after join-replace failure. Then the engine will require
an explicit format command before being able to join the system.

Also simplify and deduplicate server unit test setup code

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

github-actions · 2026-06-05T14:40:52Z

Ticket title is 'Aurora daos_user: PMEM Device should Unmount and revert the --replace operation fully if it fails'
Status is 'In Review'
Labels: 'ALCF'
https://daosio.atlassian.net/browse/DAOS-19008

daosbuild3 · 2026-06-05T14:48:54Z

Test stage NLT completed with status UNSTABLE. https://jenkins-3.daos.hpc.amslabs.hpecorp.net/job/daos-stack/job/daos//view/change-requests/job/PR-18446/1/testReport/

daosbuild3 · 2026-06-05T21:08:30Z

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-18446/1/execution/node/983/log

kjacque · 2026-06-05T21:58:45Z

+	cmd.Debugf("Invoking SystemErase to clean up after failed format operation")
+
+	eraseReq := &control.SystemEraseReq{}
+	eraseResp, err := control.SystemErase(ctx, cmd.ctlInvoker, eraseReq)


I don't think this will work... SystemErase doesn't allow you to choose ranks or nodes.

I think you'll need to handle this from the daos_server that owns the engine. If the engine fails to join, and it's a replace operation, blow the storage away. The failure that triggered this request was happening at the join stage.

If the format itself fails, I don't think there's any risk of the engine coming up. If there's a partial failure, it's not a bad idea to clean up, but I think that would have to happen from the server side, too.

right , this needs a rework

daosbuild3 · 2026-06-09T06:44:51Z

Test stage Functional Hardware Medium MD on SSD completed with status UNSTABLE. https://jenkins-3.daos.hpc.amslabs.hpecorp.net/job/daos-stack/job/daos//view/change-requests/job/PR-18446/4/testReport/

daosbuild3 · 2026-06-09T08:12:37Z

Test stage Functional Hardware Medium Verbs Provider 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-18446/4/execution/node/1394/log

daosbuild3 · 2026-06-09T12:13:00Z

Test stage NLT completed with status UNSTABLE. https://jenkins-3.daos.hpc.amslabs.hpecorp.net/job/daos-stack/job/daos//view/change-requests/job/PR-18446/5/testReport/

daosbuild3 · 2026-06-09T14:34:24Z

Test stage NLT completed with status UNSTABLE. https://jenkins-3.daos.hpc.amslabs.hpecorp.net/job/daos-stack/job/daos//view/change-requests/job/PR-18446/6/testReport/

daosbuild3 · 2026-06-09T17:18:31Z

Test stage Unit Test completed with status UNSTABLE. https://jenkins-3.daos.hpc.amslabs.hpecorp.net/job/daos-stack/job/daos//view/change-requests/job/PR-18446/8/testReport/

Signed-off-by: Tom Nabarro <thomas.nabarro@hpe.com>

tanabarr · 2026-06-15T10:22:06Z

+			}
+			if len(storageCfg.Tiers) != 0 {
+				if err := ei.WriteSuperblock(); err != nil {
+					//storage.FormatControlMetadata([]uint{0}); err != nil {


commented code removed in follow-up PR #18127

removed in this PR because of repush

mjmac

Looks generally fine to me... Only potential blocker is the removal of the guard against allowing an AdminExcluded rank to join. Not sure if that was intentional as a convenience or an oversight, but IMO it should be a requirement for the admin to explicitly unexclude a rank in order to avoid accidents.

mjmac · 2026-06-15T17:04:05Z

+				t.Error("unexpected unmount call")
+			}
+
+			if !tc.expSBCreated {


There is no test case that sets this to be true, so the remainder of the test is dead code. Maybe copy/paste from another test?

removed the test cases that used it

mjmac · 2026-06-15T17:07:30Z

-	if cm.State == MemberStateAdminExcluded {
-		return nil, ErrJoinAdminExcluded(cm.UUID, cm.Rank)
-	}


Was this intentional? An admin-excluded rank should have to be explicitly admin-unexcluded before it can rejoin, no?

This is only allowing during dmg storage format --replace (in joinReplace()), this change is the missing piece from https://daosio.atlassian.net/browse/DAOS-18472 which unfortunately only contained a partial fix because I had the rest (the bit you are referring to) in a working directory which didn't get committed! The change will not affect the normal join case.

Hmm. I read over the various tickets. I understand the intent behind this change, which is to interpret the admin's action of running a storage format --replace as an implicit clearing of the AdminExcluded state on a rank. I agree that usually this will be correct, and that it seems like a nice convenience.

However, I am concerned that this policy change weakens AdminExcluded from being a state with clearly-defined entry and exit criteria to being a bit more wishy-washy. It sets a precedent that the state can be automatically cleared based on criteria that may not be obvious without reading code, and in my mind this erodes the value of having an AdminExcluded state in addition to the programmatically-toggled Excluded state.

The correct response here, IMO, is not to weaken AdminExcluded in order to work around an admin error or UX deficiency, it's to figure out how to make the correct order of operations (admin excludes rank, repairs rank, unexcludes rank, rejoins rank) clearer to the admin.

If there is consensus that there is not strong value in having an explicit AdminExcluded state, then IMO it would be better to just remove it.

I don't agree that this weakens the position of the state but given I'm unlikely to get a consensus I will revert this change. You are correct that this was a workaround for admin error scenarios but in my mind it strengthens the UX rather than weakening it. Check replaced.

mjmac · 2026-06-15T17:10:07Z

+	storageProv := ei.GetStorage()
+
+	// Get SCM config to access mount point and class
+	scmCfg, err := storageProv.GetScmConfig()


scmCfg isn't really used for anything here, as far as I can tell. GetScmConfig() will return a non-nil error if there's no SCM config (or at least it should), so checking for a nil scmCfg seems like an anti pattern.

cruft removed from stale implementation

mjmac · 2026-06-15T17:11:16Z

 	"github.com/daos-stack/daos/src/control/lib/ranklist"
 	"github.com/daos-stack/daos/src/control/logging"
 	"github.com/daos-stack/daos/src/control/provider/system"
+	sysprov "github.com/daos-stack/daos/src/control/provider/system"


This is a duplicate import of the provider/system package.

removed "system" import because of ambiguity with the package of the same name

Features: control Signed-off-by: Tom Nabarro <thomas.nabarro@hpe.com>

…treplace-rank-erase Features: control Signed-off-by: Tom Nabarro <thomas.nabarro@hpe.com>

Features: control Signed-off-by: Tom Nabarro <thomas.nabarro@hpe.com>

mjmac

LGTM. I'm open to the idea of having a discussion about implicit clear of AdminExcluded, or the removal of the state entirely if we don't think it's actually useful. But I think that should be a separate conversation and patch rather than sneaking it in as part of something else.

knard38

From my understanding, the note of the section "Storage Format Replace" of the file docs/admin/administration.md is not correct.
According to the discussion with @mjmac, the note at line 1154 says --replace can be used on AdminExcluded ranks — but checking membership.go:201 the ErrJoinAdminExcluded guard is still there and this PR doesn't change it, so that note appears to be incorrect.

Signed-off-by: Tom Nabarro <thomas.nabarro@hpe.com>

…treplace-rank-erase Signed-off-by: Tom Nabarro <thomas.nabarro@hpe.com>

daosbuild3 · 2026-06-23T01:19:50Z

Test stage Functional Hardware Medium MD on SSD completed with status UNSTABLE. https://jenkins-3.daos.hpc.amslabs.hpecorp.net/job/daos-stack/job/daos//view/change-requests/job/PR-18446/14/testReport/

tanabarr · 2026-06-23T10:01:39Z

CI failed on known issue dfuse test_dfuse_daos_build_wb failure

daltonbohning · 2026-06-23T14:22:44Z

CI failed on known issue dfuse test_dfuse_daos_build_wb failure

This test should already be fixed in master, including in the commit that this PR is based on. It looks to have failed with a timeout on curl. It might just be network flakiness, but can you please re-run this to make sure this PR is not introducing a new issue?

tanabarr · 2026-06-23T15:19:45Z

CI failed on known issue dfuse test_dfuse_daos_build_wb failure

This test should already be fixed in master, including in the commit that this PR is based on. It looks to have failed with a timeout on curl. It might just be network flakiness, but can you please re-run this to make sure this PR is not introducing a new issue?

https://jenkins.daos.hpc.amslabs.hpecorp.net/blue/organizations/jenkins/daos-stack%2Fdaos/detail/PR-18446/15

fyi, same test is also failing here: https://jenkins.daos.hpc.amslabs.hpecorp.net/blue/organizations/jenkins/daos-stack%2Fdaos/detail/PR-18486/8/tests

daosbuild3 · 2026-06-24T15:57:42Z

Test stage Functional Hardware Medium MD on SSD completed with status UNSTABLE. https://jenkins-3.daos.hpc.amslabs.hpecorp.net/job/daos-stack/job/daos//view/change-requests/job/PR-18446/15/testReport/

tanabarr requested review from a team as code owners June 5, 2026 14:11

kjacque reviewed Jun 5, 2026

View reviewed changes

tanabarr force-pushed the tanabarr/control-fmtreplace-rank-erase branch 3 times, most recently from d8cf409 to 6785032 Compare June 8, 2026 20:46

tanabarr force-pushed the tanabarr/control-fmtreplace-rank-erase branch from 6785032 to 309615f Compare June 9, 2026 11:34

tanabarr force-pushed the tanabarr/control-fmtreplace-rank-erase branch from 309615f to a593fcb Compare June 9, 2026 13:55

tanabarr changed the title ~~Erase formatting after failed format --replace~~ DAOS-19008 control: Erase formatting after failed format --replace Jun 9, 2026

tanabarr force-pushed the tanabarr/control-fmtreplace-rank-erase branch 2 times, most recently from 21e8fb7 to 862f9d5 Compare June 9, 2026 15:57

tanabarr added 3 commits June 10, 2026 12:25

DAOS-19008 control: Erase formatting after failed format --replace

e1e035a

Signed-off-by: Tom Nabarro <thomas.nabarro@hpe.com>

consolidate test engine setup logic into helpers

f343e59

Signed-off-by: Tom Nabarro <thomas.nabarro@hpe.com>

add unit tests for cleanup function

550d853

Signed-off-by: Tom Nabarro <thomas.nabarro@hpe.com>

tanabarr force-pushed the tanabarr/control-fmtreplace-rank-erase branch from 862f9d5 to 550d853 Compare June 12, 2026 20:06

tanabarr commented Jun 15, 2026

View reviewed changes

tanabarr requested review from kjacque, knard38 and mjmac June 15, 2026 11:15

mjmac reviewed Jun 15, 2026

View reviewed changes

tanabarr added 2 commits June 16, 2026 14:07

fix unit tests and address review comments

65ff26e

Features: control Signed-off-by: Tom Nabarro <thomas.nabarro@hpe.com>

Merge remote-tracking branch 'origin/master' into tanabarr/control-fm…

d49d274

…treplace-rank-erase Features: control Signed-off-by: Tom Nabarro <thomas.nabarro@hpe.com>

reinstate Adminexcluded check for joinReplace

710d497

Features: control Signed-off-by: Tom Nabarro <thomas.nabarro@hpe.com>

tanabarr requested a review from mjmac June 16, 2026 15:07

mjmac previously approved these changes Jun 16, 2026

View reviewed changes

tanabarr self-assigned this Jun 17, 2026

tanabarr added the control-plane work on the management infrastructure of the DAOS Control Plane label Jun 17, 2026

knard38 reviewed Jun 18, 2026

View reviewed changes

Comment thread src/control/server/instance_storage.go Outdated

Comment thread src/control/server/util_test.go Outdated

Comment thread src/control/server/instance_storage_test.go

tanabarr added 2 commits June 18, 2026 12:35

consistency improvement

4f87d31

Signed-off-by: Tom Nabarro <thomas.nabarro@hpe.com>

address review comments from knard38

8f3ef7f

Signed-off-by: Tom Nabarro <thomas.nabarro@hpe.com>

tanabarr dismissed mjmac’s stale review via 8f3ef7f June 18, 2026 12:55

tanabarr requested a review from a team as a code owner June 18, 2026 12:55

Merge remote-tracking branch 'origin/master' into tanabarr/control-fm…

944dee3

…treplace-rank-erase Signed-off-by: Tom Nabarro <thomas.nabarro@hpe.com>

tanabarr requested review from knard38 and mjmac June 18, 2026 13:01

knard38 approved these changes Jun 18, 2026

View reviewed changes

mjmac approved these changes Jun 18, 2026

View reviewed changes

tanabarr requested a review from a team June 23, 2026 09:58

tanabarr added the forced-landing The PR has known failures or has intentionally reduced testing, but should still be landed. label Jun 23, 2026

daltonbohning removed the request for review from a team June 23, 2026 14:22

tanabarr requested a review from a team June 26, 2026 09:46

daltonbohning removed the forced-landing The PR has known failures or has intentionally reduced testing, but should still be landed. label Jun 26, 2026

daltonbohning merged commit 42cabc9 into master Jun 26, 2026
47 checks passed

daltonbohning deleted the tanabarr/control-fmtreplace-rank-erase branch June 26, 2026 17:59

Uh oh!

Conversation

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

Uh oh!

Steps for the author:

After all prior steps are complete:

Uh oh!

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

Uh oh!

Uh oh!

daosbuild3 commented Jun 5, 2026

Uh oh!

daosbuild3 commented Jun 5, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

daosbuild3 commented Jun 9, 2026

Uh oh!

daosbuild3 commented Jun 9, 2026

Uh oh!

daosbuild3 commented Jun 9, 2026

Uh oh!

daosbuild3 commented Jun 9, 2026

Uh oh!

daosbuild3 commented Jun 9, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mjmac left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tanabarr Jun 16, 2026 • 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!

mjmac 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!

knard38 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

daosbuild3 commented Jun 23, 2026

Uh oh!

tanabarr commented Jun 23, 2026

Uh oh!

daltonbohning commented Jun 23, 2026

Uh oh!

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

Uh oh!

Uh oh!

daosbuild3 commented Jun 24, 2026

Uh oh!

tanabarr commented Jun 5, 2026 •

edited

Loading

github-actions Bot commented Jun 5, 2026 •

edited

Loading

tanabarr Jun 16, 2026 •

edited

Loading

mjmac left a comment •

edited

Loading

tanabarr commented Jun 23, 2026 •

edited

Loading