-
Notifications
You must be signed in to change notification settings - Fork 79
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
ObservedGeneration and update block #1298
Conversation
5801d6a
to
2881e5f
Compare
@burmanm , is this ready for another round of review? |
Yes |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still have to test this manually, but publishing this review early because I think the task naming issue needs attention.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I pushed commits to fix a few remaining issues. This is good to go as far as k8ssandra-operator is concerned.
However I'm noticing an issue with the refresh CassandraTask
: it doesn't seem to ever be marked as finished (and as a consequence neither does the K8ssandraTask
). It doesn't impact the new behavior we're introducing here, the conditions and annotations are correctly reset.
I'll test on cass-operator alone and file an issue if necessary.
Quality Gate failedFailed conditions |
OK, this might be a side-effect of my testing method: I was artificially setting the |
Given we found some bugs (or side effects) in cass-operator's implementation of this feature, I think we should first solve those and re-evaluate how it's done in this PR. |
@burmanm, as discussed together, could you add some docs before we merge this? We'd need to explain users what the new behavior looks like and how to unblock a spec update on operator upgrade. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed the docs update (docs/content/en/install/upgrade/_index.md
).
There's one small improvement I found, but I couldn't get GH to let me propose a commit.
|
||
## Updates after operator upgrade to running Cassandra clusters | ||
|
||
Sometimes the updates to operators might bring new features or improvements to existing running Cassandra clusters. However, starting from release 1.18 we will no longer update them automatically when the operators are upgraded to prevent a rolling restart on inconvenient time. If there are changes to be applied after upgrading, the ``K8ssandraCluster`` instances are marked with a Status Condition ``RequiresUpdate`` set to True. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
on inconvenient time
-> at inconvenient time
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the correct form is "at an inconvenient time" ;)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"at an inconvenient time" sounds like the best wording to me.
…the end of the reconciliation
…also, not just Stargate
…tricted values for AutomatedUpdate, "always" and "once" and verify them in the webhook. Remove tee outputting to stdout in the helm prepare script. UpdateStatus should only delete and reset the ClusterRequiresUpdate if it was allowed to update in the first place. Also, we should listen to changes and reconcile if annotations change. Add additional checks to the GenerationCheck test to ensure we do not touch the CassandraDatacenter unless given permission.
Since ObservedGeneration is being added by this PR, it will be 0 when the operator gets upgraded to this version. We want to interpret that as "the generation didn't change". This was agreed upon previously but got lost in a force-push.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1298 +/- ##
==========================================
+ Coverage 58.65% 58.99% +0.33%
==========================================
Files 122 122
Lines 11910 12059 +149
==========================================
+ Hits 6986 7114 +128
Misses 4315 4315
- Partials 609 630 +21
|
Quality Gate failedFailed conditions |
What this PR does:
Adds new status field: ObservedGeneration and ensure it is updated at the end of the reconciliation
Which issue(s) this PR fixes:
Fixes #1274
Checklist