chore: migrate controllers to SSA #112
Conversation
erikgb
force-pushed
the
ssa-migration
branch
2 times, most recently
from
87f5663
to
a8dbfaa
Compare
erikgb
changed the title
erikgb
force-pushed
the
ssa-migration
branch
from
a8dbfaa
to
334d224
Compare
erikgb
changed the title
|
After testing this PR I think it needs a bit more work - especially to avoid endless reconcile loops. |
erikgb
changed the title
|
I think this PR is ready for review now. After we hopefully get this in, I am hoping to get a new release soon. 🙏 There are already many interesting unreleased improvements, and fixing #98 (this PR) is a high priority for us. While you review this PR, I will prepare some vanilla release preparation PRs like dumping dependencies etc. Have you ever considered enabling Depandabot/Renovate on this project? |
Current test is the scenario we need in our environment, so we don't want to lose some of them in the major refactoring. |
| @@ -30,7 +30,7 @@ metadata: | |||
| team: foo | |||
| ``` | |||
|
|
|||
| Accurate only propagates labels/annotations that have been configured in that respect via the `labelKeys` and `annotationKeys` parameters in `config.yaml`. This prevents the propagation of labels/annotations that were not meant to do so. Accurate currently does not delete previously propagated labels when deleted from the parent namespace to prevent unintended deletions. Users are expected to manually delete labels/annotations that are no longer needed. | |||
| Accurate only propagates labels/annotations that have been configured in that respect via the `labelKeys` and `annotationKeys` parameters in `config.yaml`. This prevents the propagation of labels/annotations that were not meant to do so. | |||
There was a problem hiding this comment.
This PR includes a change in specifications.
Could you please tell me why you are changing this behavior?
There was a problem hiding this comment.
Exactly! @ymmt2005 is correct that this is the main motivation for this PR. There is yet no 1. class support for SSA in controller-runtime, but I have been involved in migrating several operators to SSA using the same approach as in this PR. So I am pretty sure it works, and it's also properly tested now IMO. The main concern with SSA migration is the risk of getting never-ending reconcile loops, but I have added new tests for this.
We would be extremely happy if this PR could eventually get merged and released. #98 is a semi-blocker for our further rollout of Accurate in our clusters.
There was a problem hiding this comment.
@erikgb
Thank you for the detailed description!
I missed issue refered in PR description.
This was just a suggestion and not a requirement. I have no problems leaving the tests as-is. But if I were a maintainer, I would split things more. I don't see a big benefit in the long scenario-based tests, and think the test code would be easier to read if the scenarios were tested one by one. The extensive use of Ginkgo |
| @@ -462,4 +476,60 @@ var _ = Describe("Namespace controller", func() { | |||
| HaveField("Annotations", HaveKeyWithValue("memo", "tama")), | |||
| )) | |||
| }) | |||
|
|
|||
| Context("templated namespace", func() { | |||
There was a problem hiding this comment.
What do you want this test to confirm?
I think confirming that ns is created from template and that labels and annotations are propagated has already been tested here.
There was a problem hiding this comment.
This test is to ensure consistent reconciles. Reconcile loops is the one thing to fear when migrating to SSA.
There was a problem hiding this comment.
Thanks to you, I understood.
This PR migrates all updates in controllers to server-side apply (SSA), except the update removing the finalizer on SubNamespace - which is changed to a standard patch.
SSA works a bit differently than updates, so some of the optimizations had to go in this PR. I am hoping this change won't increase the load on the api-server significantly, but it's something we should monitor. I have added stable resource version tests for namespaces - which are now SSAed unconditionally.
Namespace controller tests are currently highly scenario-based, which makes them long and hard to read/modify. I have added some additional assertions of modified behavior on migration to SSA, but I suggest refactoring the tests in a follow-up PR to improve the test coverage. I can prepare a PR if you agree.
Fixes #98