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

Add support for graceful halt via server config #4059

Merged
merged 11 commits into from
Apr 23, 2019

Conversation

alexanderbez
Copy link
Contributor

@alexanderbez alexanderbez commented Apr 5, 2019

Add support for a halt-height in the node's server-config/CLI to gracefully halt and shutdown a node.

closes: #3981


  • Targeted PR against correct branch (see CONTRIBUTING.md)

  • Linked to github-issue with discussion and accepted design OR link to spec that describes this work.

  • Wrote tests

  • Updated relevant documentation (docs/)

  • Added a relevant changelog entry: sdkch add [section] [stanza] [message]

  • rereviewed Files changed in the github PR explorer


For Admin Use:

  • Added appropriate labels to PR (ex. wip, ready-for-review, docs)
  • Reviewers Assigned
  • Squashed all commits, uses message "Merge pull request #XYZ: [title]" (coding standards)

@alexanderbez alexanderbez mentioned this pull request Apr 5, 2019
10 tasks
@alexanderbez
Copy link
Contributor Author

@ebuchman given the BaseApp has knowledge of the height to halt at, where is the most appropriate to check and gracefully exit while respecting the ABCI semantics? Commit?

@codecov
Copy link

codecov bot commented Apr 5, 2019

Codecov Report

Merging #4059 into develop will decrease coverage by 0.03%.
The diff coverage is 20%.

@@             Coverage Diff             @@
##           develop    #4059      +/-   ##
===========================================
- Coverage    60.21%   60.17%   -0.04%     
===========================================
  Files          212      212              
  Lines        15155    15165      +10     
===========================================
  Hits          9126     9126              
- Misses        5405     5414       +9     
- Partials       624      625       +1

@alexanderbez alexanderbez changed the base branch from release/v0.34.0 to develop April 15, 2019 17:00
@alexanderbez alexanderbez changed the title Add support for graceful halt via gaiad config Add support for graceful halt via server config Apr 15, 2019
baseapp/baseapp.go Outdated Show resolved Hide resolved
Copy link
Contributor

@rigelrozanski rigelrozanski left a comment

Choose a reason for hiding this comment

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

Is there a document describing how a validator would change the halting height on a live node? I'm assuming one would just write to the config live somehow

@alexanderbez
Copy link
Contributor Author

@rigelrozanski I'll add a section to the docs somewhere on this new config feature. There currently isn't a way to do it live. You must, set the config and start (or restart) your node for the change to pickup.

@alexanderbez
Copy link
Contributor Author

@rigelrozanski I added a section to the validator-setup.md docs.

@@ -236,6 +236,8 @@ func initTestnet(config *tmconfig.Config, cdc *codec.Codec) error {
return err
}

// TODO: Rename config file to server.toml as it's not particular to Gaia
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

you want your node to shutdown or by passing the `--halt-height` flag to `gaiad`.
The node will shutdown with a zero exit code at that given height after committing
the block.

Copy link
Contributor

@sabau sabau Apr 23, 2019

Choose a reason for hiding this comment

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

This can be set only at boot time right? Do you think will be possible to undo this at runtime, or to set/change this flag at runtime, let's say a governance proposal that proposed to update at height X got downvoted but I started a node with this this --halt-height X

Probably even if it's true it makes sense to do it in a separate PR

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good question @sabau. This value can be set at any time really but the node would have to be restarted for it to take effect. There currently isn't a way to do this at runtime. I think there is some crossover functionality between this and the software upgrade module/process we plan to have. Essentially, to do this at runtime, it would have to be parameter somewhere.

Copy link
Contributor

@sabau sabau left a comment

Choose a reason for hiding this comment

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

I left a question more than a comment, if it makes sense probably should be addressed as a following PR

@alexanderbez
Copy link
Contributor Author

alexanderbez commented Apr 23, 2019

Answered your (good) question @sabau. I think it's safe to merge this PR for now until we have a more established software upgrade process. I'm thinking we should maybe even get this into a v0.34.2 patch release along with some other small bits.

@alexanderbez alexanderbez merged commit c6cb84c into develop Apr 23, 2019
@alexanderbez alexanderbez mentioned this pull request Apr 23, 2019
7 tasks
@alexanderbez alexanderbez deleted the bez/3981-gaiad-cfg-block-halt branch April 24, 2019 16:19
youngjoon-lee pushed a commit to medibloc/cosmos-sdk that referenced this pull request Oct 5, 2020
youngjoon-lee pushed a commit to medibloc/cosmos-sdk that referenced this pull request Oct 14, 2020
youngjoon-lee pushed a commit to medibloc/cosmos-sdk that referenced this pull request Dec 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

add an option on gaiad.toml to stop a node at n block height
5 participants