Skip to content
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.

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

DAOS-6611 control: Enable dmg pool exclude,drain,reint on multiple ranks #15731

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

tanabarr
Copy link
Contributor

@tanabarr tanabarr commented Jan 14, 2025

Enable --ranks option on excluded drain and reintegrate dmg pool cmds.
Uses common request and response type for pool-ranks operations where
possible. Reuse common display format for exclude/drain/reint system
and pool cmds. Improvement of workflow and unit test coverage.

Before requesting gatekeeper:

  • Two review approvals and any prior change requests have been resolved.
  • Testing is complete and all tests passed or there is a reason documented in the PR why it should be force landed and forced-landing tag is set.
  • Features: (or Test-tag*) commit pragma was used or there is a reason documented that there are no appropriate tags for this PR.
  • Commit messages follows the guidelines outlined here.
  • Any tests skipped by the ticket being addressed have been run and passed in the PR.

Gatekeeper:

  • You are the appropriate gatekeeper to be landing the patch.
  • The PR has 2 reviews by people familiar with the code, including appropriate owners.
  • Githooks were used. If not, request that user install them and check copyright dates.
  • Checkpatch issues are resolved. Pay particular attention to ones that will show up on future PRs.
  • All builds have passed. Check non-required builds for any new compiler warnings.
  • Sufficient testing is done. Check feature pragmas and test tags and that tests skipped for the ticket are run and now pass with the changes.
  • If applicable, the PR has addressed any potential version compatibility issues.
  • Check the target branch. If it is master branch, should the PR go to a feature branch? If it is a release branch, does it have merge approval in the JIRA ticket.
  • Extra checks if forced landing is requested
    • Review comments are sufficiently resolved, particularly by prior reviewers that requested changes.
    • No new NLT or valgrind warnings. Check the classic view.
    • Quick-build or Quick-functional is not used.
  • Fix the commit message upon landing. Check the standard here. Edit it to create a single commit. If necessary, ask submitter for a new summary.

@tanabarr tanabarr self-assigned this Jan 14, 2025
Copy link

Ticket title is 'Add support to reintegration, drain (dmg) commands for multiple ranks on command line.'
Status is 'In Progress'
Labels: 'Drain,Offline_Reintegration,triaged'
https://daosio.atlassian.net/browse/DAOS-6611

@daosbuild1
Copy link
Collaborator

@daosbuild1
Copy link
Collaborator

Test stage Build on Leap 15.5 with Intel-C and TARGET_PREFIX completed with status FAILURE. https://build.hpdd.intel.com//job/daos-stack/job/daos/view/change-requests/job/PR-15731/1/execution/node/375/log

@daosbuild1
Copy link
Collaborator

Test stage Build RPM on EL 9 completed with status FAILURE. https://build.hpdd.intel.com//job/daos-stack/job/daos/view/change-requests/job/PR-15731/1/execution/node/353/log

@daosbuild1
Copy link
Collaborator

Test stage Build RPM on Leap 15.5 completed with status FAILURE. https://build.hpdd.intel.com//job/daos-stack/job/daos/view/change-requests/job/PR-15731/1/execution/node/261/log

@daosbuild1
Copy link
Collaborator

Test stage Build RPM on EL 8 completed with status FAILURE. https://build.hpdd.intel.com//job/daos-stack/job/daos/view/change-requests/job/PR-15731/1/execution/node/354/log

@daosbuild1
Copy link
Collaborator

Test stage Build DEB on Ubuntu 20.04 completed with status FAILURE. https://build.hpdd.intel.com//job/daos-stack/job/daos/view/change-requests/job/PR-15731/1/execution/node/301/log

Base automatically changed from tanabarr/control-reintpools-pernode to master January 14, 2025 15:56
@daosbuild1
Copy link
Collaborator

Test stage Build on Leap 15.5 with Intel-C and TARGET_PREFIX completed with status FAILURE. https://build.hpdd.intel.com//job/daos-stack/job/daos/view/change-requests/job/PR-15731/2/execution/node/364/log

@daosbuild1
Copy link
Collaborator

@daosbuild1
Copy link
Collaborator

Test stage Build RPM on EL 9 completed with status FAILURE. https://build.hpdd.intel.com//job/daos-stack/job/daos/view/change-requests/job/PR-15731/2/execution/node/344/log

@daosbuild1
Copy link
Collaborator

Test stage Build RPM on EL 8 completed with status FAILURE. https://build.hpdd.intel.com//job/daos-stack/job/daos/view/change-requests/job/PR-15731/2/execution/node/345/log

@daosbuild1
Copy link
Collaborator

Test stage Build RPM on Leap 15.5 completed with status FAILURE. https://build.hpdd.intel.com//job/daos-stack/job/daos/view/change-requests/job/PR-15731/2/execution/node/319/log

@daosbuild1
Copy link
Collaborator

Test stage Build DEB on Ubuntu 20.04 completed with status FAILURE. https://build.hpdd.intel.com//job/daos-stack/job/daos/view/change-requests/job/PR-15731/2/execution/node/314/log

@tanabarr tanabarr force-pushed the tanabarr/control-drainpool-multirank branch from d47692a to 4509d9b Compare January 14, 2025 16:51
@daosbuild1
Copy link
Collaborator

@daosbuild1
Copy link
Collaborator

Test stage Build on Leap 15.5 with Intel-C and TARGET_PREFIX completed with status FAILURE. https://build.hpdd.intel.com//job/daos-stack/job/daos/view/change-requests/job/PR-15731/3/execution/node/349/log

@daosbuild1
Copy link
Collaborator

Test stage Build RPM on EL 9 completed with status FAILURE. https://build.hpdd.intel.com//job/daos-stack/job/daos/view/change-requests/job/PR-15731/3/execution/node/334/log

@daosbuild1
Copy link
Collaborator

Test stage Build RPM on EL 8 completed with status FAILURE. https://build.hpdd.intel.com//job/daos-stack/job/daos/view/change-requests/job/PR-15731/3/execution/node/331/log

@daosbuild1
Copy link
Collaborator

Test stage Build RPM on Leap 15.5 completed with status FAILURE. https://build.hpdd.intel.com//job/daos-stack/job/daos/view/change-requests/job/PR-15731/3/execution/node/401/log

@daosbuild1
Copy link
Collaborator

Test stage Build DEB on Ubuntu 20.04 completed with status FAILURE. https://build.hpdd.intel.com//job/daos-stack/job/daos/view/change-requests/job/PR-15731/3/execution/node/388/log

@tanabarr tanabarr force-pushed the tanabarr/control-drainpool-multirank branch from 4509d9b to 0041dcb Compare January 20, 2025 12:30
@daosbuild1
Copy link
Collaborator

Test stage Build on Leap 15.5 with Intel-C and TARGET_PREFIX completed with status FAILURE. https://build.hpdd.intel.com//job/daos-stack/job/daos/view/change-requests/job/PR-15731/4/execution/node/334/log

@daosbuild1
Copy link
Collaborator

Test stage Build RPM on EL 9 completed with status FAILURE. https://build.hpdd.intel.com//job/daos-stack/job/daos/view/change-requests/job/PR-15731/4/execution/node/327/log

@daosbuild1
Copy link
Collaborator

Test stage Build RPM on EL 8 completed with status FAILURE. https://build.hpdd.intel.com//job/daos-stack/job/daos/view/change-requests/job/PR-15731/4/execution/node/326/log

@daosbuild1
Copy link
Collaborator

Test stage Build RPM on Leap 15.5 completed with status FAILURE. https://build.hpdd.intel.com//job/daos-stack/job/daos/view/change-requests/job/PR-15731/4/execution/node/386/log

@daosbuild1
Copy link
Collaborator

Test stage Build DEB on Ubuntu 20.04 completed with status FAILURE. https://build.hpdd.intel.com//job/daos-stack/job/daos/view/change-requests/job/PR-15731/4/execution/node/373/log

@daosbuild1
Copy link
Collaborator

@tanabarr tanabarr force-pushed the tanabarr/control-drainpool-multirank branch from 0041dcb to 9352123 Compare January 22, 2025 23:32
@tanabarr tanabarr requested review from knard38, mjmac and kjacque February 3, 2025 09:35
…ainpool-multirank

Features: pool
Allow-unstable-test: true
Signed-off-by: Tom Nabarro <tom.nabarrointel.com>
@daosbuild1
Copy link
Collaborator

Test stage Unit Test on EL 8.8 completed with status UNSTABLE. https://build.hpdd.intel.com/job/daos-stack/job/daos//view/change-requests/job/PR-15731/8/testReport/

Copy link
Contributor

@daltonbohning daltonbohning left a comment

Choose a reason for hiding this comment

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

ftest LGTM

@daosbuild1
Copy link
Collaborator

Test stage Unit Test with memcheck on EL 8.8 completed with status UNSTABLE. https://build.hpdd.intel.com/job/daos-stack/job/daos//view/change-requests/job/PR-15731/8/testReport/

@daosbuild1
Copy link
Collaborator

Test stage Build on Leap 15.5 with Intel-C and TARGET_PREFIX completed with status FAILURE. https://build.hpdd.intel.com//job/daos-stack/job/daos/view/change-requests/job/PR-15731/9/execution/node/371/log

@daosbuild1
Copy link
Collaborator

@daosbuild1
Copy link
Collaborator

Test stage Build RPM on EL 9 completed with status FAILURE. https://build.hpdd.intel.com//job/daos-stack/job/daos/view/change-requests/job/PR-15731/9/execution/node/346/log

@daosbuild1
Copy link
Collaborator

Test stage Build RPM on EL 8 completed with status FAILURE. https://build.hpdd.intel.com//job/daos-stack/job/daos/view/change-requests/job/PR-15731/9/execution/node/345/log

@daosbuild1
Copy link
Collaborator

Test stage Build RPM on Leap 15.5 completed with status FAILURE. https://build.hpdd.intel.com//job/daos-stack/job/daos/view/change-requests/job/PR-15731/9/execution/node/365/log

@daosbuild1
Copy link
Collaborator

Test stage Build DEB on Ubuntu 20.04 completed with status FAILURE. https://build.hpdd.intel.com//job/daos-stack/job/daos/view/change-requests/job/PR-15731/9/execution/node/349/log

Features: pool
Allow-unstable-test: true
Signed-off-by: Tom Nabarro <[email protected]>
@tanabarr tanabarr force-pushed the tanabarr/control-drainpool-multirank branch from 1810850 to 79da39d Compare February 3, 2025 22:07
@daosbuild1
Copy link
Collaborator

Test stage Unit Test on EL 8.8 completed with status UNSTABLE. https://build.hpdd.intel.com/job/daos-stack/job/daos//view/change-requests/job/PR-15731/10/testReport/

@daosbuild1
Copy link
Collaborator

Test stage Unit Test with memcheck on EL 8.8 completed with status UNSTABLE. https://build.hpdd.intel.com/job/daos-stack/job/daos//view/change-requests/job/PR-15731/10/testReport/

Features: pool
Allow-unstable-test: true
Signed-off-by: Tom Nabarro <[email protected]>
@daosbuild1
Copy link
Collaborator

Test stage Unit Test on EL 8.8 completed with status UNSTABLE. https://build.hpdd.intel.com/job/daos-stack/job/daos//view/change-requests/job/PR-15731/11/testReport/

@daosbuild1
Copy link
Collaborator

Test stage Unit Test with memcheck on EL 8.8 completed with status UNSTABLE. https://build.hpdd.intel.com/job/daos-stack/job/daos//view/change-requests/job/PR-15731/11/testReport/

Features: pool
Allow-unstable-test: true
Signed-off-by: Tom Nabarro <[email protected]>
@daosbuild1
Copy link
Collaborator

Test stage Unit Test with memcheck on EL 8.8 completed with status UNSTABLE. https://build.hpdd.intel.com/job/daos-stack/job/daos//view/change-requests/job/PR-15731/12/testReport/

@daosbuild1
Copy link
Collaborator

Test stage Functional on EL 8.8 completed with status UNSTABLE. https://build.hpdd.intel.com/job/daos-stack/job/daos//view/change-requests/job/PR-15731/12/testReport/

Copy link
Contributor

@knard38 knard38 left a comment

Choose a reason for hiding this comment

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

Is it planned to have one or more functional tests using these new features ?
Do we have some protocol version information to update as this new feature will change the RPC content and not just extend it ?

type PoolRanksResp struct {
Status int32 `json:"status"`
ID string `json:"id"`
FailedRank ranklist.Rank `json:"failed_rank"`
Copy link
Contributor

Choose a reason for hiding this comment

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

I am probably missing something, but do not see obvious reasons why no more than one rank could fail.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the design chosen is to iterate through ranklist in the engine dRPC handler to update each rank's state. the first failure will break out of the loop and return

Copy link
Contributor

Choose a reason for hiding this comment

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

It seems like it would be more future-proof to make it a list. Even if the current implementation can only ever return a single rank, if the implementation is changed to perform some of these operations in parallel, then it won't require updates to the messages, UI, etc.

@@ -831,9 +927,9 @@ type PoolExtendReq struct {
// This should automatically start the rebalance process.
// Returns an error (including any DER code from DAOS).
func PoolExtend(ctx context.Context, rpcClient UnaryInvoker, req *PoolExtendReq) error {
Copy link
Contributor

@knard38 knard38 Feb 6, 2025

Choose a reason for hiding this comment

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

Out of curiosity, even if PoolExtend is already using a list of ranks, why do not generalise the usage of PoolRanksReq and remove PoolExtendReq ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I started to do this but PoolExtendResp contains an extra field tier_bytes and I didn't want to add this field to the common PoolRanksResp just for the extend specific case.

Features: pool
Allow-unstable-test: true
Signed-off-by: Tom Nabarro <[email protected]>
@tanabarr
Copy link
Contributor Author

tanabarr commented Feb 6, 2025

Is it planned to have one or more functional tests using these new features ? Do we have some protocol version information to update as this new feature will change the RPC content and not just extend it ?

I imagine there will be more than one functional test added, I think @rpadma2 will be adding these. The protocol changes are dmg <-> daos_server <-> engine which at the moment all have to be at the same version IIUC, am I understanding that correct @mjmac @kjacque @NiuYawei @liw or is there some compatibility related work to be done?

@daosbuild1
Copy link
Collaborator

Test stage Unit Test with memcheck on EL 8.8 completed with status UNSTABLE. https://build.hpdd.intel.com/job/daos-stack/job/daos//view/change-requests/job/PR-15731/13/testReport/

type PoolRanksResp struct {
Status int32 `json:"status"`
ID string `json:"id"`
FailedRank ranklist.Rank `json:"failed_rank"`
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems like it would be more future-proof to make it a list. Even if the current implementation can only ever return a single rank, if the implementation is changed to perform some of these operations in parallel, then it won't require updates to the messages, UI, etc.

return cmd.OutputJSON(resp, err)
}

cmd.Debugf("%T: %+v, %T: %+v", req, req, resp, resp)
Copy link
Contributor

Choose a reason for hiding this comment

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

Are these debug lines necessary? I think you should already have debug printing from the control library.

@mjmac
Copy link
Contributor

mjmac commented Feb 7, 2025

The protocol changes are dmg <-> daos_server <-> engine which at the moment all have to be at the same version IIUC, am I understanding that correct @mjmac @kjacque @NiuYawei @liw or is there some compatibility related work to be done?

That's correct. There's no interoperability concern here because all of those components must be the same version. At some point, when rolling upgrades are supported, we'll need to ensure that dmg can always talk to the -1/+1 versions of daos_server, though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

5 participants