Skip to content

Make TerminationGracePeriodSeconds configurable #424

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

Closed
wants to merge 2 commits into from

Conversation

ChunyiLyu
Copy link
Contributor

@ChunyiLyu ChunyiLyu commented Oct 27, 2020

This closes #419

Note to reviewers: remember to look at the commits in this PR and consider if they can be squashed

Summary Of Changes

  • make TerminationGracePeriodSeconds configurable
  • refactor to use "k8s.io/utils/pointer" library everywhere

Additional Context

Local Testing

Have run unit, integration, and system tests

@ansd
Copy link
Member

ansd commented Oct 28, 2020

@ChunyiLyu could you add more details to the PR or issue as to why this value is made configurable?

@ChunyiLyu
Copy link
Contributor Author

@ansd added more context in the issue.

@Zerpet Zerpet self-requested a review October 29, 2020 11:22
Copy link
Member

@Zerpet Zerpet left a comment

Choose a reason for hiding this comment

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

I'm thinking that we should adapt the timeout in the preStop hooks to match the termination grace period i.e. rabbitmq-upgrade await_online_quorum_plus_one -t 604800. And perhaps remove the preStop hook altogether if the grace period is set to 0.

What do you think?

// For more information, see: https://github.com/rabbitmq/cluster-operator/blob/main/docs/design/20200520-graceful-pod-termination.md
// +kubebuilder:validation:Minimum:=0
// +kubebuilder:default:=604800
TerminationGracePeriodSeconds *int64 `json:"terminationGracePeriodSeconds,omitempty"`
Copy link
Member

Choose a reason for hiding this comment

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

Does omitempty have any effect? Given that we set a default and value 0 is also an acceptable value, I think this field will never be empty.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Without omitempty, defaulting won't work here. See commit

@ChunyiLyu
Copy link
Contributor Author

Merge conflicts in this branch is hard to resolve. Moved to PR #430

Closing

@ChunyiLyu ChunyiLyu closed this Oct 29, 2020
@ChunyiLyu ChunyiLyu deleted the terminationGracePeriodSeconds branch October 29, 2020 14:23
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.

Make terminationGracePeriodSeconds configuration at rabbitmqcluster.spec
3 participants