Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

staking-miner: Add handling of SIGTERM, SIGKILL, SIGQUIT and SIGINT #5780

Merged
merged 4 commits into from
Jul 19, 2022

Conversation

chevdor
Copy link
Contributor

@chevdor chevdor commented Jul 13, 2022

This PR adds handling of:

  • SIGQUIT | SIGINT (keyboard signals)
  • SIGTERM | SIGKILL
    to the staking miner.

The doc about those signals can be found here.

The keyboard signals require the user to confirm twice (ie CTRL+C then CTRL+C again) and a confirmation warning is shown after the first signal is received.

Other signals will exit immediately so Docker / K8S should issue SIGTERM or SIGKILL.

fix #5649

@chevdor chevdor force-pushed the wk-220713-fix-5649-staking-miner-sigterm branch from 8d319ce to b4e4c27 Compare July 13, 2022 14:11
@chevdor chevdor force-pushed the wk-220713-fix-5649-staking-miner-sigterm branch from b4e4c27 to da0a934 Compare July 13, 2022 14:13
@chevdor chevdor added A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. labels Jul 13, 2022
@chevdor chevdor requested review from kianenigma and ggwpez July 13, 2022 14:36
@chevdor chevdor marked this pull request as ready for review July 13, 2022 14:36
@chevdor chevdor changed the title staking-miner: Add handling of SIGTERM, SIGKILL and SIGINT staking-miner: Add handling of SIGTERM, SIGKILL, SIGQUIT and SIGINT Jul 13, 2022
Copy link
Member

@niklasad1 niklasad1 left a comment

Choose a reason for hiding this comment

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

Nice attempt but this will not work.

You need to check whether the monitor command or signal flag has been fired.

As the monitor command can run "indefinitely" the future from signal may never be polled.
So it would be better to do select(monitor, signal) or something similar.

Have a look at paritytech/polkadot-staking-miner#35

@niklasad1 niklasad1 dismissed their stale review July 16, 2022 10:41

I was wrong

Copy link
Member

@niklasad1 niklasad1 left a comment

Choose a reason for hiding this comment

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

nice, missed that you used controlled_exit to cancel

Dropping the monitor futures and the tokio rt should close down everything.

@chevdor chevdor merged commit f75ed5e into master Jul 19, 2022
@chevdor chevdor deleted the wk-220713-fix-5649-staking-miner-sigterm branch July 19, 2022 07:32
ordian added a commit that referenced this pull request Jul 22, 2022
* master:
  westend xcm: collectives parachain is trusted teleporter (#5798)
  Cleanup light client leftovers (#5794)
  Fix benchmarking tests (#5791)
  allow re-use and avoid compiling kusama parachain code (#5792)
  Introduce async runtime calling trait for runtime-api subsystem (#5782)
  add `Extrinsic Ordering` check that runs against a local reference node (#5790)
  Co #11456: Expose `benchmark extrinsic` command (#5620)
  `staking-miner`:  Add handling of `SIGTERM`, `SIGKILL`, `SIGQUIT` and `SIGINT` (#5780)
  Zombienet: paritydb test (#5310)
  Fix Typo (#5766)
  Fix Core Version display in the release notes (#5781)
  companion for new pools reward scheme (#5757)
  fix disable-runtime-api feature flag (#5773)
  split NetworkBridge into two subsystems (#5616)
  Implement prune only stagnant check mode (#5761)
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

staking-miner: SIGTERM handling
3 participants