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.

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

Jump to bottom

DAOS-16312 control: Always use --force for dmg system stop #15811

Open
wants to merge 1 commit into
base: release/2.6
Choose a base branch
from
Open

DAOS-16312 control: Always use --force for dmg system stop #15811

wants to merge 1 commit into from

Conversation

tanabarr
Copy link
Contributor

@tanabarr tanabarr commented Jan 29, 2025 •
edited
Loading

Whenever stopping an engine process from within the control-plane, use
SIGKILL rather than asking nicely (SIGTERM). This has been requested
to try to avoid situations that could result in dataloss.

This change preserves the behaviour where ds_mgmt_drpc_prep_shutdown()
and then ds_pool_disable_exclude() will be called during a controlled
shutdown where dmg system stop is called with new --full argument.

Notable behavior changes with this PR:

  • Always performs SIGKILL on dmg system stop unless --full command
    option is supplied.
  • Will attempt prepare shutdown to disable exclusions across cluster
    during “controlled” shutdown where dmg system stop is called with
    --full option but this should be regarded as experimental and not
    for use in production environments.
  • Force option is a no-op and is retained for backward compatibility
    and future use.

Allow-unstable-test: true
Features: control

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 requested review from a team as code owners January 29, 2025 16:32
@tanabarr tanabarr self-assigned this Jan 29, 2025
@github-actions GitHub Actions
Copy link

Errors are Unable to load ticket data
https://daosio.atlassian.net/browse/DAOS-16312

@tanabarr tanabarr added control-plane work on the management infrastructure of the DAOS Control Plane unclean-cherry-pick Indicates that a cherry-pick had merge conflicts that needed resolving. usability Changes specific to user facing tools or behaviour. labels Jan 29, 2025
@tanabarr tanabarr force-pushed the tanabarr/control-stop-sigkill-rel2_6 branch from 6e9e271 to 047c910 Compare January 29, 2025 17:59
@tanabarr tanabarr requested review from a team as code owners January 29, 2025 17:59
mjmac
mjmac reviewed Jan 29, 2025
View reviewed changes
src/control/cmd/dmg/system.go Outdated
@@ -185,7 +186,8 @@ func (cmd *systemStopCmd) Execute(_ []string) (errOut error) {
if err := cmd.validateHostsRanks(); err != nil {
return err
}
req := &control.SystemStopReq{Force: cmd.Force}
// DAOS-16312: Always use force when stopping ranks.
req := &control.SystemStopReq{Force: true}
Copy link
Contributor

Choose a reason for hiding this comment

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

This is different than the PR for master. Was that intentional? Changing it here means that the behavior will only be modified for dmg users.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

not intentional let me fix

@tanabarr tanabarr force-pushed the tanabarr/control-stop-sigkill-rel2_6 branch from 047c910 to 649a314 Compare January 29, 2025 18:14
@tanabarr tanabarr requested a review from mjmac January 29, 2025 18:16
@tanabarr
Copy link
Contributor Author

@mjmac Hope that's better apologies for the force push

mjmac
mjmac previously approved these changes Jan 29, 2025
View reviewed changes
daltonbohning
daltonbohning reviewed Jan 29, 2025
View reviewed changes
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

@tanabarr tanabarr added clean-cherry-pick Cherry-pick from another branch that did not require additional edits and removed unclean-cherry-pick Indicates that a cherry-pick had merge conflicts that needed resolving. labels Jan 29, 2025
@tanabarr
Copy link
Contributor Author

code wise it's a clean cherry pick there is one Copyright notice difference from the Master PR

@daosbuild1
Copy link
Collaborator

Test stage Functional on EL 8.8 completed with status FAILURE. https://build.hpdd.intel.com//job/daos-stack/job/daos/view/change-requests/job/PR-15811/5/execution/node/1202/log

@daosbuild1
Copy link
Collaborator

Test stage Functional Hardware Medium completed with status UNSTABLE. https://build.hpdd.intel.com/job/daos-stack/job/daos//view/change-requests/job/PR-15811/5/testReport/

@daosbuild1
Copy link
Collaborator

Test stage Functional Hardware Medium Verbs Provider completed with status FAILURE. https://build.hpdd.intel.com//job/daos-stack/job/daos/view/change-requests/job/PR-15811/5/execution/node/1476/log

@tanabarr
Copy link
Contributor Author

Known issues:

kjacque
kjacque previously approved these changes Feb 1, 2025
View reviewed changes
@tanabarr
DAOS-16312 control: Always use --force for dmg system stop
20eb8b9 
Allow-unstable-test: true
Features: control
Signed-off-by: Tom Nabarro <[email protected]>
@tanabarr tanabarr dismissed stale reviews from kjacque and mjmac via 20eb8b9 February 1, 2025 09:55
@tanabarr tanabarr force-pushed the tanabarr/control-stop-sigkill-rel2_6 branch from 649a314 to 20eb8b9 Compare February 1, 2025 09:55
@tanabarr
Copy link
Contributor Author

tanabarr commented Feb 1, 2025

re-pushed at P2
Gatekeeper pleases PR description and title in commit message when landing. TIA

@daosbuild1
Copy link
Collaborator

Test stage Unit Test with memcheck on EL 8.8 completed with status FAILURE. https://build.hpdd.intel.com/job/daos-stack/job/daos/job/PR-15811/7/display/redirect

@daosbuild1
Copy link
Collaborator

Test stage Unit Test bdev on EL 8.8 completed with status FAILURE. https://build.hpdd.intel.com/job/daos-stack/job/daos/job/PR-15811/7/display/redirect

@daosbuild1
Copy link
Collaborator

Test stage Unit Test bdev with memcheck on EL 8.8 completed with status FAILURE. https://build.hpdd.intel.com/job/daos-stack/job/daos/job/PR-15811/7/display/redirect

@daosbuild1
Copy link
Collaborator

Test stage Unit Test on EL 8.8 completed with status FAILURE. https://build.hpdd.intel.com/job/daos-stack/job/daos/job/PR-15811/7/display/redirect

@daosbuild1
Copy link
Collaborator

Test stage NLT on EL 8.8 completed with status FAILURE. https://build.hpdd.intel.com/job/daos-stack/job/daos/job/PR-15811/7/display/redirect

@daosbuild1
Copy link
Collaborator

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@daltonbohning daltonbohning daltonbohning left review comments

@kjacque kjacque kjacque left review comments

@mjmac mjmac mjmac left review comments

At least 1 approving review is required to merge this pull request.

Assignees

@tanabarr tanabarr

Labels
clean-cherry-pick Cherry-pick from another branch that did not require additional edits control-plane work on the management infrastructure of the DAOS Control Plane usability Changes specific to user facing tools or behaviour.
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@tanabarr @daosbuild1 @mjmac @daltonbohning @kjacque