Skip to content
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

Merged
merged 26 commits into from
Jul 25, 2024
Merged

Conversation

burmanm
Copy link
Contributor

@burmanm burmanm commented Apr 24, 2024

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

  • Changes manually tested
  • Automated Tests added/updated
  • Documentation added/updated
  • CHANGELOG.md updated (not required for documentation PRs)
  • CLA Signed: DataStax CLA

@burmanm
Copy link
Contributor Author

burmanm commented Apr 30, 2024

@olim7t

@burmanm burmanm force-pushed the upgrade_process branch 2 times, most recently from 5801d6a to 2881e5f Compare May 13, 2024 10:44
@burmanm burmanm marked this pull request as ready for review May 13, 2024 10:44
@burmanm burmanm requested a review from a team as a code owner May 13, 2024 10:44
@adejanovski
Copy link
Contributor

@burmanm , is this ready for another round of review?

@burmanm
Copy link
Contributor Author

burmanm commented May 22, 2024

Yes

Copy link
Contributor

@olim7t olim7t left a 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.

controllers/control/k8ssandratask_controller.go Outdated Show resolved Hide resolved
controllers/k8ssandra/datacenters.go Outdated Show resolved Hide resolved
controllers/k8ssandra/datacenters.go Show resolved Hide resolved
Copy link
Contributor

@olim7t olim7t left a 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.

Copy link

sonarcloud bot commented Jun 1, 2024

Quality Gate Failed Quality Gate failed

Failed conditions
3.1% Duplication on New Code (required ≤ 3%)

See analysis details on SonarCloud

@olim7t
Copy link
Contributor

olim7t commented Jun 4, 2024

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).

OK, this might be a side-effect of my testing method: I was artificially setting the RequiresUpdate=true condition on the CassandraDatacenter to simulate a cass-operator upgrade, but refreshing the DC did not produce any actual changes to the stateful set. It looks like something in the way the refresh is implemented causes the task to not be marked as completed in that case. This is not something that will happen in real life, so I think we can safely ignore it.

@adejanovski
Copy link
Contributor

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.
Also, and that's probably something we should have done from the beginning, I think we should make this an opt in feature rather than the default behavior.

@adejanovski
Copy link
Contributor

@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.
Thanks!

Copy link
Contributor

@rzvoncek rzvoncek left a 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.
Copy link
Contributor

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

Copy link
Contributor Author

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" ;)

Copy link
Contributor

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.

burmanm and others added 8 commits July 24, 2024 13:24
…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.
olim7t and others added 11 commits July 24, 2024 13:24
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.
Copy link

codecov bot commented Jul 24, 2024

Codecov Report

Attention: Patch coverage is 35.22727% with 57 lines in your changes missing coverage. Please review.

Project coverage is 58.99%. Comparing base (8804770) to head (e2f21d3).
Report is 14 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@            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     
Files Coverage Δ
...pis/k8ssandra/v1alpha1/k8ssandracluster_webhook.go 80.89% <100.00%> (+6.75%) ⬆️
pkg/cassandra/datacenter.go 66.22% <ø> (-0.27%) ⬇️
apis/k8ssandra/v1alpha1/k8ssandracluster_types.go 39.32% <22.22%> (-4.21%) ⬇️
...ntrollers/k8ssandra/k8ssandracluster_controller.go 73.68% <52.63%> (-3.91%) ⬇️
controllers/k8ssandra/datacenters.go 65.49% <24.07%> (-7.82%) ⬇️

... and 6 files with indirect coverage changes

Copy link

sonarcloud bot commented Jul 25, 2024

Quality Gate Failed Quality Gate failed

Failed conditions
18.9% Duplication on New Code (required ≤ 3%)

See analysis details on SonarCloud

@burmanm burmanm merged commit 99a34d5 into k8ssandra:main Jul 25, 2024
61 of 63 checks passed
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.

k8ssandra-operator shouldn't modify the cassdc objects on operator upgrade
4 participants