Skip to content

feat: support for graceful shutdown based on configuration #2479

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 4 commits into from
Aug 6, 2024
Merged

feat: support for graceful shutdown based on configuration #2479

merged 4 commits into from
Aug 6, 2024

Conversation

10000-ki
Copy link
Contributor

@10000-ki 10000-ki commented Aug 4, 2024

resolved: #2476

What i am working on in this PR

  • Allows configuration of graceful shutdown settings in the operator configuration.
  • If a graceful shutdown timeout value is set, it will be used within the stop method.
  • I have enhanced the documentation related to graceful shutdown.

@openshift-ci openshift-ci bot requested review from adam-sandor and csviri August 4, 2024 15:46
.withReconciliationTerminationTimeout(Duration.ofSeconds(5));

final var operator = new Operator(overridden);
```
Copy link
Contributor Author

@10000-ki 10000-ki Aug 4, 2024

Choose a reason for hiding this comment

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

In my opinion, it is beneficial for users to be aware of this feature, so I have documented it.

However, in most users' cases, the graceful shutdown feature may not be necessary.
If you think this documentation is unnecessary, please let me know and I will remove it.

* @return The duration of time to wait before terminating the reconciliation threads
*/
default Duration reconciliationTerminationTimeout() {
return Duration.ZERO;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The default is immediate shutdown

@@ -28,7 +28,7 @@ public class GracefulStopIT {
.build();

@Test
void stopsGracefullyWIthTimeout() {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I also corrected a typo in the method name while I was at it.

LocallyRunOperatorExtension operator =
LocallyRunOperatorExtension.builder()
.withConfigurationService(o -> o.withCloseClientOnStop(false)
.withReconciliationTerminationTimeout(Duration.ofSeconds(RECONCILER_SLEEP)))
Copy link
Contributor Author

@10000-ki 10000-ki Aug 4, 2024

Choose a reason for hiding this comment

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

.withReconciliationTerminationTimeout(Duration.ofMillis(RECONCILER_SLEEP)))

User can configure the graceful shutdown time in the Operator settings.

@10000-ki 10000-ki changed the title Support for graceful shutdown based on configuration feat: support for graceful shutdown based on configuration Aug 5, 2024
Support for graceful shutdown based on configuration

Signed-off-by: 10000-ki <[email protected]>

Fix naming

Signed-off-by: 10000-ki <[email protected]>

Fix lint error

Signed-off-by: 10000-ki <[email protected]>

Fix lint error

Signed-off-by: 10000-ki <[email protected]>

Fix lint error

Signed-off-by: 10000-ki <[email protected]>

Fix test duration

Signed-off-by: 10000-ki <[email protected]>
@csviri
Copy link
Collaborator

csviri commented Aug 6, 2024

I was wondering if we should keep the stop(timeout) variant at all. What do you think? @10000-ki @metacosm

@10000-ki
Copy link
Contributor Author

10000-ki commented Aug 6, 2024

I was wondering if we should keep the stop(timeout) variant at all. What do you think?

@csviri
In my opinion

Configuration-based graceful shutdown can be difficult to modify at runtime, so supporting features like stop(timeout) might be useful in test code.

However, from an actual user's perspective, it doesn't seem very beneficial. It might be clearer to specify the graceful shutdown timeout value when setting up the Operator Configuration

Personally, I think it's fine to no longer support it, but there may be users using it in previous versions. Therefore, I didn't make any changes to it in this PR.

Should we mark it as deprecated??

@metacosm
Copy link
Collaborator

metacosm commented Aug 6, 2024

I was wondering if we should keep the stop(timeout) variant at all. What do you think? @10000-ki @metacosm

I think it's fine to remove it and use whatever value is configured.

@csviri
Copy link
Collaborator

csviri commented Aug 6, 2024

I was wondering if we should keep the stop(timeout) variant at all. What do you think? @10000-ki @metacosm

I think it's fine to remove it and use whatever value is configured.

ok, agreed, @10000-ki could you update the PR pls?

@10000-ki
Copy link
Contributor Author

10000-ki commented Aug 6, 2024

ok, agreed, @10000-ki could you update the PR pls?

@csviri @metacosm

All changes have been applied to the current PR
Thank you for the review.

@metacosm metacosm merged commit e1f2da0 into operator-framework:next Aug 6, 2024
20 checks passed
@metacosm
Copy link
Collaborator

metacosm commented Aug 6, 2024

Thank you!

@csviri
Copy link
Collaborator

csviri commented Aug 6, 2024

Awesome, thank you!

csviri pushed a commit that referenced this pull request Aug 8, 2024
csviri pushed a commit that referenced this pull request Aug 15, 2024
metacosm pushed a commit that referenced this pull request Aug 29, 2024
csviri pushed a commit that referenced this pull request Sep 20, 2024
metacosm pushed a commit that referenced this pull request Oct 10, 2024
metacosm pushed a commit that referenced this pull request Nov 5, 2024
metacosm pushed a commit that referenced this pull request Nov 6, 2024
csviri pushed a commit that referenced this pull request Nov 13, 2024
metacosm pushed a commit that referenced this pull request Nov 19, 2024
metacosm pushed a commit that referenced this pull request Nov 20, 2024
metacosm pushed a commit that referenced this pull request Nov 27, 2024
csviri pushed a commit that referenced this pull request Dec 6, 2024
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.

3 participants