DAOS-19260 control: Add capability to format storage of specific engine#18604
DAOS-19260 control: Add capability to format storage of specific engine#18604tanabarr wants to merge 1 commit into
Conversation
Features: control Signed-off-by: Tom Nabarro <thomas.nabarro@hpe.com>
|
Ticket title is 'Allow format on specific engine instance' |
|
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-18604/1/testReport/ |
knard38
left a comment
There was a problem hiding this comment.
If the engine-idx default-value issue I raised is confirmed, I think it should be fixed before landing. It would also be good to add unit test coverage for the new --engine-idx feature.
| req := &control.StorageFormatReq{ | ||
| Reformat: cmd.Force, | ||
| Replace: cmd.Replace, | ||
| Rank: rank, | ||
| EngineIdx: cmd.EngineIdx, | ||
| } |
There was a problem hiding this comment.
From my understanding, using cmd.EngineIdx without cmd.Rank defined and cmd.Replace true makes no sense and can even lead to some unexpected behaviour. If I am correct, the same kind of consistency checking done for cmd.Rank should be done for cmd.EngineIdx:
if cmd.EngineIdx != nil && cmd.Rank == nil {
return errors.New("--engine-idx option is only valid when used with --rank")
}| } | ||
|
|
||
| // Filter instances if engine-idx is specified (MaxUint32 means all engines) | ||
| if req.EngineIdx != ^uint32(0) { |
There was a problem hiding this comment.
From my understanding, the MaxUint32 sentinel for "no filter, format all engines" is only ever set in control.StorageFormat() (src/control/lib/control/storage.go), after convert.Types() runs. Any other code path (such as auto-format) that builds a ctlpb.StorageFormatReq directly gets the Go zero value (0) for EngineIdx, which this check reads as "filter to engine 0 only".
From my investigation, latest version of protoc support "optional" field. Sadly, the version used by DAOS seems to not support such feature. Then, one solution could be to add a bool field engine_idx_enabled to the proto message StorageFormatReq and to update the code in the following way:
| if req.EngineIdx != ^uint32(0) { | |
| if req.Replace && req.EngineIdxEnabled { |
We could also keep the sentinel solution but we will have to fix all the code using this protoc struct and setting the replace field to true.
Add optional engine_idx parameter to StorageFormatReq to allow formatting
storage for a specific engine instance rather than all engines on a host.
Features: control
Steps for the author:
After all prior steps are complete: