Skip to content

Graceful shutdown timeout #4571

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

Conversation

elliotfehr
Copy link
Contributor

@elliotfehr elliotfehr commented Feb 23, 2021

Description of the change:
Adds a --drain-time to ansible based operators

Motivation for the change:
This is configurable in the manager but not possible to override using environment variables or CLI flags for roles that might take more than the 30 second default to complete.

Checklist

If the pull request includes user-facing changes, extra documentation is required:

@elliotfehr elliotfehr force-pushed the graceful-shutdown-timeout branch from 6a765eb to 33181cf Compare February 23, 2021 21:34
Signed-off-by: Elliot Fehr <[email protected]>
@elliotfehr elliotfehr force-pushed the graceful-shutdown-timeout branch from de728cd to d7de15c Compare February 24, 2021 01:23
Copy link
Member

@estroz estroz left a comment

Choose a reason for hiding this comment

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

In general I'm ok with allowing flag overrides of operator values. I will say, now that controller-runtime supports a config, flags should be added in controller-runtime using component-base's cli/flag lib to get these flags for free. I'm ok adding a one-off flag like this now as long as the name aligns with what would be added in controller-runtime so config flags can be switched in later without a breaking change.

@@ -0,0 +1,5 @@
entries:
- description: >
For Ansible-based operators, allow passing a --drain-time to configure the duration the manager should wait before stopping.
Copy link
Member

Choose a reason for hiding this comment

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

Why call this --drain-time? Does this terminology come from somewhere?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No real reason or preference here. Any suggestions for what to name it?

Copy link
Member

Choose a reason for hiding this comment

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

--graceful-shutdown-timeout

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated all references!

@elliotfehr elliotfehr requested a review from estroz February 25, 2021 00:58
Copy link
Member

@estroz estroz left a comment

Choose a reason for hiding this comment

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

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Feb 25, 2021
@estroz estroz merged commit 9b0afe9 into operator-framework:master Feb 25, 2021
reinvantveer pushed a commit to reinvantveer/operator-sdk that referenced this pull request Feb 28, 2021
reinvantveer pushed a commit to reinvantveer/operator-sdk that referenced this pull request Feb 28, 2021
reinvantveer pushed a commit to reinvantveer/operator-sdk that referenced this pull request Feb 28, 2021
@elliotfehr elliotfehr mentioned this pull request Jun 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants