search

cybozu-go / accurate

Kubernetes controller for multi-tenancy. It propagates resources between namespaces accurately and allows tenant users to create/delete sub-namespaces.
https://cybozu-go.github.io/accurate/
Apache License 2.0
34 stars 5 forks source link

chore: migrate controllers to SSA #112

Closed erikgb closed 5 months ago

erikgb commented 7 months ago

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

erikgb commented 7 months ago

After testing this PR I think it needs a bit more work - especially to avoid endless reconcile loops.

erikgb commented 5 months ago

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?

yamatcha commented 5 months ago

@erikgb

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.

Current test is the scenario we need in our environment, so we don't want to lose some of them in the major refactoring. Would you tell me how you want to refactor this.

erikgb commented 5 months ago

@erikgb

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.

Current test is the scenario we need in our environment, so we don't want to lose some of them in the major refactoring. Would you tell me how you want to refactor this.

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 By construct is a sign of this (but required for these lengthy tests). 😄 No offense, of course. This is just my point of view.