Skip to content

feat: graceful(er) server shutdown #568

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
Closed
wants to merge 20 commits into from
Closed

feat: graceful(er) server shutdown #568

wants to merge 20 commits into from

Conversation

razvan
Copy link
Member

@razvan razvan commented Sep 23, 2024 •
edited
Loading

Description

part of #508

Region servers now optionally move regions away before shutting down.

Definition of Done Checklist

  • Not all of these items are applicable to all PRs, the author should update this template to only leave the boxes in that are relevant
  • Please make sure all these things are done and tick the boxes
# Author
- [x] Changes are OpenShift compatible
- [x] CRD changes approved
- [x] CRD documentation for all fields, following the [style guide](https://docs.stackable.tech/home/nightly/contributor/docs/style-guide).
- [x] Helm chart can be installed and deployed operator works
- [ ] Integration tests passed (for non trivial changes)
- [x] Changes need to be "offline" compatible

# Reviewer
- [ ] Code contains useful comments
- [ ] Code contains useful logging statements
- [ ] (Integration-)Test cases added
- [ ] Documentation added or updated. Follows the [style guide](https://docs.stackable.tech/home/nightly/contributor/docs/style-guide).
- [ ] Changelog updated
- [ ] Cargo.toml only contains references to git tags (not specific commits or branches)

# Acceptance
- [ ] Feature Tracker has been updated
- [ ] Proper release label has been added
- [ ] [Roadmap](https://github.com/orgs/stackabletech/projects/25/views/1) has been updated

@razvan
Copy link
Member Author

razvan commented Sep 26, 2024

CI: https://testing.stackable.tech/view/02%20Operator%20Tests%20(custom)/job/hbase-operator-it-custom/19/

@razvan
Copy link
Member Author

razvan commented Sep 26, 2024

CI: https://testing.stackable.tech/view/02%20Operator%20Tests%20(custom)/job/hbase-operator-it-custom/20/

@razvan razvan marked this pull request as ready for review September 26, 2024 08:59
@razvan razvan self-assigned this Sep 26, 2024
@razvan razvan requested a review from a team September 26, 2024 08:59
@razvan
Copy link
Member Author

razvan commented Sep 26, 2024

CI https://testing.stackable.tech/job/hbase-operator-it-custom/21/

fhennig and others added 4 commits September 30, 2024 13:39
@fhennig @razvan
docs: pod overrides, better install instructions, improved wording (#569
2d2d564 
)

* ~

* Update docs/modules/hbase/pages/getting_started/installation.adoc

* Update docs/modules/hbase/pages/usage-guide/operations/graceful-shutdown.adoc

* Update docs/modules/hbase/pages/usage-guide/operations/pod-placement.adoc

* Update docs/modules/hbase/pages/usage-guide/security.adoc

* fix: spelling

---------

Co-authored-by: Razvan-Daniel Mihai <[email protected]>
@razvan
revert back to starting hbase directly
9cc7f62
@razvan
make region moves on shutdown configurable (default off)
51f8487
@razvan
main merge
e4771d5
@razvan
Copy link
Member Author

razvan commented Sep 30, 2024

CI https://testing.stackable.tech/view/02%20Operator%20Tests%20(custom)/job/hbase-operator-it-custom/22/

sbernauer
sbernauer reviewed Oct 1, 2024
View reviewed changes
docs/modules/hbase/pages/usage-guide/operations/graceful-shutdown.adoc

This is equivalent to executing the `bin/hbase-daemon.sh stop master` command, which internally executes `kill <master-pid>` (https://github.com/apache/hbase/blob/8382f55b15be6ae190f8d202a5e6a40af177ec76/bin/hbase-daemon.sh#L338[code]), waits for a configurable period of time (defaults to 20 minutes), and finally executes `kill -9 <master-pid>` to `SIGKILL` the master (https://github.com/apache/hbase/blob/8382f55b15be6ae190f8d202a5e6a40af177ec76/bin/hbase-common.sh#L20-L41[code]).

However, there is no message in the log acknowledging the graceful shutdown.
Copy link
Member

Choose a reason for hiding this comment

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

Is there a message now? If so can you please paste the relevant lines here, similar to the example at the regionservers?

docs/modules/hbase/pages/usage-guide/operations/graceful-shutdown.adoc
2023-10-11 12:40:42,316 INFO [main] RESTServer: ***** STOPPING service 'RESTServer' *****
----

There are two environment variables that you can add to the regionserver roles (or groups) that allow you more control over the shotdonw process: `MOVE_REGIONS_ON_SHUTDOWN` and `REGION_MOVER_OPTS`.
Copy link
Member

Choose a reason for hiding this comment

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

I think it would be awesome to have this as as CR flag. Uses can still envOverride when they are not happy with our CRD.
The CRD could look something like

spec:
  regionServers:
    config:
      gracefulShutdownTimeout: 1h
      gracefulShutdownRegionMover: # mandatory, defaults to the values below (so disabled by default)
        enabled: false # mandatory, defaults to false
        maxThreads: 1 # mandatory, defaults to 1
        ack: true # mandatory, defaults to true

Copy link
Member Author

Choose a reason for hiding this comment

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

I agree except I wouldn't want it to be mandatory to avoid a breaking change.

Copy link
Member Author

@razvan razvan Oct 1, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Having it optional also implies it's enabled (when defined), no need for the redundant field.

Copy link
Member

Choose a reason for hiding this comment

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

Mandatory with a default is de-facto optional ;)
But don't take it too literally, as all of this will go throw the Fragment and Merge macros and well.. it's complicated. Long story short: It feels optional to the uesr

@razvan
Copy link
Member Author

razvan commented Oct 15, 2024

Closing this in favor of #570

@razvan razvan closed this Oct 15, 2024
@lfrancke lfrancke deleted the feat/rs-graceful-stop branch June 19, 2025 12:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@sbernauer sbernauer sbernauer left review comments

@lfrancke lfrancke Awaiting requested review from lfrancke

Assignees

@razvan razvan

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@razvan @sbernauer @fhennig