Skip to content

feat: graceful shutdown #1735

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

Merged
merged 14 commits into from
Feb 7, 2023
Merged

feat: graceful shutdown #1735

merged 14 commits into from
Feb 7, 2023

Conversation

csviri
Copy link
Collaborator

@csviri csviri commented Jan 25, 2023

No description provided.

@csviri csviri self-assigned this Jan 25, 2023
@csviri
Copy link
Collaborator Author

csviri commented Jan 25, 2023

This might deserve also an end to end test, to showcase, how SIGTERM and SIGKILL works in pods. WDYT @metacosm ?

@csviri csviri requested a review from metacosm January 25, 2023 15:32
// this is needed for the case when controller stopped, but there is a graceful shutdown
// timeout. that should finish the currently executing reconciliations but not the ones
// which where submitted but not started yet
log.debug("Event processor not running skipping resource processing: {}", resourceID);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Isn't this missing a return statement here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yes, wonder how the test passed, will check

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

fixed the unit test, was not trivial, wonder if we should refactor this in the future.

@@ -121,6 +121,7 @@ public HasMetadata clone(HasMetadata object) {
*
* @return the number of seconds to wait before terminating reconciliation threads
*/
@Deprecated(forRemoval = true)
Copy link
Collaborator

Choose a reason for hiding this comment

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

This also needs an associated @deprecated tag in the javadoc…

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

added

@csviri csviri linked an issue Jan 25, 2023 that may be closed by this pull request
@csviri csviri requested a review from metacosm January 26, 2023 09:50
@csviri csviri force-pushed the graceful-shutdown branch from ef81c22 to 1c62152 Compare January 30, 2023 08:59
Copy link
Collaborator

@metacosm metacosm left a comment

Choose a reason for hiding this comment

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

LGTM apart from question regarding deprecation of termination timeout

@@ -121,8 +121,12 @@ public HasMetadata clone(HasMetadata object) {
* Retrieves the number of seconds the SDK waits for reconciliation threads to terminate before
* shutting down.
*
* @deprecated use {@link io.javaoperatorsdk.operator.Operator#stop(Duration)} instead. Where the
Copy link
Collaborator

Choose a reason for hiding this comment

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

question: why not keep this? It could be useful to be able to configure the default timeout as previously…

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I would prefer to have a single way to configure this, and it is much simpler with the parameters. Keeping this, would be two way to do the same thing.

@csviri csviri force-pushed the graceful-shutdown branch from 52ec2ce to 430b1af Compare February 7, 2023 09:08
@sonarqubecloud
Copy link

sonarqubecloud bot commented Feb 7, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 3 Code Smells

55.6% 55.6% Coverage
0.0% 0.0% Duplication

@csviri csviri requested a review from metacosm February 7, 2023 09:12
@csviri csviri changed the title Graceful shutdown feat: graceful shutdown Feb 7, 2023
@csviri csviri merged commit e0aeaee into next Feb 7, 2023
@csviri csviri deleted the graceful-shutdown branch February 7, 2023 13:50
csviri added a commit that referenced this pull request Feb 8, 2023
csviri added a commit that referenced this pull request Feb 8, 2023
csviri added a commit that referenced this pull request Feb 14, 2023
csviri added a commit that referenced this pull request Feb 23, 2023
csviri added a commit that referenced this pull request Feb 28, 2023
csviri added a commit that referenced this pull request Mar 3, 2023
csviri added a commit that referenced this pull request Mar 10, 2023
csviri added a commit that referenced this pull request Mar 14, 2023
csviri added a commit that referenced this pull request Mar 15, 2023
metacosm pushed a commit that referenced this pull request Mar 22, 2023
metacosm pushed a commit that referenced this pull request Mar 27, 2023
csviri added a commit that referenced this pull request Mar 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support graceful stop with configurable timeout
2 participants