Skip to content

Add readinessCondition to stop traffic to pods that will be stopped #530

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
Merged
HoustonPutman merged 12 commits into from
Apr 3, 2023
Merged

Add readinessCondition to stop traffic to pods that will be stopped #530

HoustonPutman merged 12 commits into from
Apr 3, 2023

Conversation

@HoustonPutman
Copy link
Contributor

@HoustonPutman HoustonPutman commented Mar 9, 2023 •
edited
Loading

Resolves #529

This uses the pod readiness gates/conditions to ensure that traffic is stopped to pods that are about to be deleted for various reasons.

This will only work for those that use the managed update feature.

Things to do:

  • Add Readiness Condition to podTemplate
  • Make sure readinessCondition is set to True initially
  • Set readinessCondition to False before pod is evicted of replicas and stopped
  • Upgrade Notes and Changelog entry
  • Integration Tests
  • Unit Tests?

@HoustonPutman HoustonPutman mentioned this pull request Mar 28, 2023
Closed
janhoy
janhoy reviewed Mar 28, 2023
View reviewed changes
Copy link
Contributor

@janhoy janhoy 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 not in a position to grok the entire PR, and I have not tested it either. But have some minor comments.

markrmiller
markrmiller approved these changes Mar 29, 2023
View reviewed changes
Copy link
Member

@markrmiller markrmiller left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Not familiar with Go or these kubernetes readiness gates, but I did little reading, tried putting the changes through a couple tools, did a basic logical read through, looks like a good patch to me and a nice improvement.

gerlowskija
gerlowskija approved these changes Apr 3, 2023
View reviewed changes
Copy link
Contributor

@gerlowskija gerlowskija left a comment

Choose a reason for hiding this comment

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

Had a few minor comments and typo corrections, but otherwise LGTM!

config/rbac/role.yaml
- pods/status
verbs:
- get
- patch
Copy link
Contributor

@gerlowskija gerlowskija Apr 3, 2023

Choose a reason for hiding this comment

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

[0] Just want to make sure I understand how this stuff works:

This is a generated file, right? Or is this one maintained by hand?

And this line was added because of the patch call made here? Really cool that the generator is set up to that!

Copy link
Contributor Author

@HoustonPutman HoustonPutman Apr 3, 2023

Choose a reason for hiding this comment

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

This is a generated file, and it would be really cool if it was automatically added based on what commands you use... But alas its a little less cool.

This is what determines this (each controller has a separate list, that all get combined to make this file): https://github.com/apache/solr-operator/pull/530/files#diff-27cb619f80d28ac158f7885b7925d8582babfa7fe1f824599d65a745302d237cR67

@HoustonPutman HoustonPutman merged commit f73e3b1 into apache:main Apr 3, 2023
@HoustonPutman HoustonPutman deleted the pod-update-readiness branch April 3, 2023 17:08
HoustonPutman added a commit that referenced this pull request Apr 3, 2023
@HoustonPutman
Add some unit tests for pod readinessGates
d12b4b2 
Addition for #530
@HoustonPutman HoustonPutman mentioned this pull request May 17, 2023
Merged
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@janhoy janhoy janhoy left review comments

@markrmiller markrmiller markrmiller approved these changes

@gerlowskija gerlowskija gerlowskija approved these changes

@thelabdude thelabdude Awaiting requested review from thelabdude

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Support disruption free rolling restart

4 participants

@HoustonPutman @janhoy @markrmiller @gerlowskija