Skip to content

Check Needs-Restart condition and restart pods if required #1018

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

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

jan-g
Copy link
Contributor

@jan-g jan-g commented Aug 5, 2025

No description provided.

@jan-g jan-g force-pushed the opv1-needs-restart branch 5 times, most recently from 41c1323 to 7ebcc4c Compare August 6, 2025 08:41
@jan-g jan-g force-pushed the opv1-needs-restart branch from 7ebcc4c to ff136fa Compare August 6, 2025 15:49
Copy link
Contributor

@chrisseto chrisseto left a comment

Choose a reason for hiding this comment

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

You're right, I'm not a huge fan of further using the conditions but this is the minimally effective patch!

A test would be fantastic. I think it would be fairly easy to emulate this case in the ginko suite that's using the MockAdminAPI.

@@ -0,0 +1,4 @@
project: operator
kind: Fixed
body: 'The legacy operator is more robust against configuration changes that "need restart": it will sweep over pods and mark those requiring a configuration restart according to the admin API response.'
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: for consistency refer to it as either the vectorized operator or operator v1.

updateForConfigRestart = true
}

if patchResult.IsEmpty() && !updateForConfigRestart {
Copy link
Contributor

Choose a reason for hiding this comment

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

Would you mind adding a log line here as well? Hopefully we won't hit it but it will very likely prevent a bit of hair pulling if we do find ourselves asking "why isn't the operator restarting my Pods?" again.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah.

@jan-g
Copy link
Contributor Author

jan-g commented Aug 7, 2025

This is still not quite right. It'd be good if it were, but the sts is still stripping off the "restarting" condition for some reason, which means that all pods don't get eventually restarted.

@jan-g jan-g marked this pull request as draft August 7, 2025 10:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants