Skip to content

DAOS-17831 control: Add dmg pool rebuild start|stop cmds #16686

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
johannlombardi merged 5 commits into from
Sep 16, 2025
Merged

DAOS-17831 control: Add dmg pool rebuild start|stop cmds #16686

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
37c3802
DAOS-17831 control: Add dmg pool rebuild start|stop cmds
tanabarr Aug 30, 2025
baf8686
address review comments from kjacque and kcain
tanabarr Sep 6, 2025
1ea3ef7
Merge remote-tracking branch 'origin/master' into tanabarr/control-po…
tanabarr Sep 6, 2025
854e060
Merge branch 'master' into tanabarr/control-poolrbld-startstop
tanabarr Sep 10, 2025
becfcc0
Merge remote-tracking branch 'origin/master' into tanabarr/control-po…
tanabarr Sep 12, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions src/control/cmd/dmg/command_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -180,6 +180,12 @@ func (bci *bridgeConnInvoker) InvokeUnaryRPC(ctx context.Context, uReq control.U
resp = control.MockMSResponse("", nil, &mgmtpb.PoolRanksResp{})
case *control.PoolExtendReq:
resp = control.MockMSResponse("", nil, &mgmtpb.PoolExtendResp{})
case *control.PoolRebuildReq:
if req.Op == control.PoolRebuildOpCodeStart {
resp = control.MockMSResponse("", nil, &mgmtpb.DaosResp{})
} else if req.Op == control.PoolRebuildOpCodeStop {
resp = control.MockMSResponse("", nil, &mgmtpb.DaosResp{})
}
case *control.SystemCheckEnableReq:
resp = control.MockMSResponse("", nil, &mgmtpb.DaosResp{})
case *control.SystemCheckDisableReq:
Expand Down
3 changes: 2 additions & 1 deletion src/control/cmd/dmg/json_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,8 @@ func TestDmg_JsonOutput(t *testing.T) {
testArgs = append(testArgs, test.MockUUID())
case "pool create":
testArgs = append(testArgs, "-s", "1TB", "label")
case "pool destroy", "pool evict", "pool query", "pool get-acl", "pool upgrade":
case "pool destroy", "pool evict", "pool query", "pool get-acl", "pool upgrade",
"pool rebuild start", "pool rebuild stop":
testArgs = append(testArgs, test.MockUUID())
case "pool overwrite-acl", "pool update-acl":
testArgs = append(testArgs, test.MockUUID(), "-a", aclPath)
Expand Down
51 changes: 51 additions & 0 deletions src/control/cmd/dmg/pool.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@ type PoolCmd struct {
SetProp poolSetPropCmd `command:"set-prop" description:"Set pool property"`
GetProp poolGetPropCmd `command:"get-prop" description:"Get pool properties"`
Upgrade poolUpgradeCmd `command:"upgrade" description:"Upgrade pool to latest format"`
Rebuild poolRebuildCmd `command:"rebuild" description:"Manage interactive rebuild process for pools"`
}

var (
Expand Down Expand Up @@ -1054,3 +1055,53 @@ func (cmd *poolDeleteACLCmd) Execute(args []string) error {

return nil
}

// poolRebuildCmd represents the pool rebuild subcommand.
type poolRebuildCmd struct {
Start poolRebuildStartCmd `command:"start" description:"Rebuild start request submitted to pool"`
Stop poolRebuildStopCmd `command:"stop" description:"Rebuild stop request submitted to pool"`
Comment on lines +1061 to +1062

@kjacque kjacque Sep 4, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Start poolRebuildStartCmd `command:"start" description:"Rebuild start request submitted to pool"`
Stop poolRebuildStopCmd `command:"stop" description:"Rebuild stop request submitted to pool"`
Start poolRebuildStartCmd `command:"start" description:"Start rebuild for a pool"`
Stop poolRebuildStopCmd `command:"stop" description:"Stop rebuild for a pool"`

@tanabarr tanabarr Sep 6, 2025 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reason I went with the above text was because my understanding is that it is not guaranteed that a rebuild will start. The command just requests one. @kccain should I make this change or not?

@kccain kccain Sep 8, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably best to keep the language as-is. A stop request may arrive when there is nothing to stop. Similarly, even if a rebuild stop was submitted and processed, the pool may have in the meantime resumed rebuilding activity if some other manual or automatic rebuild was triggered in the meantime - which would make a subsequent "rebuild start" command a no-op. These commands are "one shot" and do not persistently change the self_heal / automatic recovery policy of the pool.

@kjacque kjacque Sep 8, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The main thing for me is that the language is passive voice and seems different from the way we describe other actions in the help. All the commands we send are essentially requests that could fail or be no-ops, so I don't really understand why the language should be different here, unless start/stop rebuild is just a suggestion that the server may or may not execute, even if it's not a no-op. I guess I'm thinking of this in terms of what the user wants to do.

To address the no-op case: Is the end result that rebuild is running or not running according to the user's wishes? Might be okay to just consider it successful in that case. Or alternately could we detect that rebuild is already in the desired state and return something like -DER_ALREADY at the engine level or a helpful "rebuild is already running/not running" error at the control plane level?

}

type poolRebuildOpCmd struct {
poolCmd
}

func (cmd *poolRebuildOpCmd) execute(opCode control.PoolRebuildOpCode, force bool) (errOut error) {
req := &control.PoolRebuildReq{
ID: cmd.PoolID().String(),
Op: opCode,
Force: force,
}

err := control.PoolRebuild(cmd.MustLogCtx(), cmd.ctlInvoker, req)
if err != nil || cmd.JSONOutputEnabled() {
if err != nil {
cmd.ctlInvoker.Debug(err.Error())
}

return err
}

msg := fmt.Sprintf("Pool-rebuild %s request succeeded", opCode.String())
cmd.ctlInvoker.Debug(msg)
cmd.Info(msg)

return nil
}

type poolRebuildStartCmd struct {
poolRebuildOpCmd
}

func (cmd *poolRebuildStartCmd) Execute(_ []string) error {
return cmd.execute(control.PoolRebuildOpCodeStart, false)
}

type poolRebuildStopCmd struct {
poolRebuildOpCmd
Force bool `short:"f" long:"force" description:"Forcibly stop rebuild"`
}

func (cmd *poolRebuildStopCmd) Execute(_ []string) error {
return cmd.execute(control.PoolRebuildOpCodeStop, cmd.Force)
}
84 changes: 82 additions & 2 deletions src/control/cmd/dmg/pool_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1173,7 +1173,7 @@ func TestPoolCommands(t *testing.T) {
"Query pool with empty ID",
"pool query \"\"",
"",
fmt.Errorf("invalid label"),
errors.New("invalid label"),
},
{
"Upgrade pool with pool ID",
Expand All @@ -1185,11 +1185,91 @@ func TestPoolCommands(t *testing.T) {
}, " "),
nil,
},
{
"Interactive rebuild with no operation specified",
"pool rebuild",
"",
errors.New("specify one command"),
},
{
"Interactive rebuild with no operation specified but label",
"pool rebuild test_label",
"",
errors.New("specify one command"),
},
{
"Start interactive rebuild on pool with no ID or label",
"pool rebuild start",
"",
errors.New("not provided"),
},
{
"Start interactive rebuild on pool with empty ID or label",
"pool rebuild start \"\"",
"",
errors.New("invalid label"),
},
{
"Start interactive rebuild on pool with ID",
"pool rebuild start 031bcaf8-f0f5-42ef-b3c5-ee048676dceb",
strings.Join([]string{
printRequest(t, &control.PoolRebuildReq{
ID: "031bcaf8-f0f5-42ef-b3c5-ee048676dceb",
Op: control.PoolRebuildOpCodeStart,
}),
}, " "),
nil,
},
{
"Start interactive rebuild on pool with label",
"pool rebuild start test_label",
strings.Join([]string{
printRequest(t, &control.PoolRebuildReq{
ID: "test_label",
Op: control.PoolRebuildOpCodeStart,
}),
}, " "),
nil,
},
{
"Stop interactive rebuild on pool with no ID or label",
"pool rebuild stop",
"",
errors.New("not provided"),
},
{
"Stop interactive rebuild on pool with empty ID or label",
"pool rebuild stop \"\"",
"",
errors.New("invalid label"),
},
{
"Stop interactive rebuild on pool with ID",
"pool rebuild stop 031bcaf8-f0f5-42ef-b3c5-ee048676dceb",
strings.Join([]string{
printRequest(t, &control.PoolRebuildReq{
ID: "031bcaf8-f0f5-42ef-b3c5-ee048676dceb",
Op: control.PoolRebuildOpCodeStop,
}),
}, " "),
nil,
},
{
"Stop interactive rebuild on pool with label",
"pool rebuild stop test_label",
strings.Join([]string{
printRequest(t, &control.PoolRebuildReq{
ID: "test_label",
Op: control.PoolRebuildOpCodeStop,
}),
}, " "),
nil,
},
{
"Nonexistent subcommand",
"pool quack",
"",
fmt.Errorf("Unknown command"),
errors.New("Unknown command"),
},
})
}
Expand Down
20 changes: 20 additions & 0 deletions src/control/common/proto/mgmt/addons.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -161,6 +161,26 @@ func (r *PoolQueryTargetReq) SetUUID(id uuid.UUID) {
r.Id = id.String()
}

// SetUUID sets the request's ID to a UUID.
func (r *PoolRebuildStartReq) SetUUID(id uuid.UUID) {
r.Id = id.String()
}

// SetSvcRanks sets the request's Pool Service Ranks.
func (r *PoolRebuildStartReq) SetSvcRanks(rl []uint32) {
r.SvcRanks = rl
}

// SetUUID sets the request's ID to a UUID.
func (r *PoolRebuildStopReq) SetUUID(id uuid.UUID) {
r.Id = id.String()
}

// SetSvcRanks sets the request's Pool Service Ranks.
func (r *PoolRebuildStopReq) SetSvcRanks(rl []uint32) {
r.SvcRanks = rl
}

// SetSvcRanks sets the request's Pool Service Ranks.
func (r *GetACLReq) SetSvcRanks(rl []uint32) {
r.SvcRanks = rl
Expand Down
Loading
Loading