Update controller-runtime to 0.16 #710
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Zerpet
force-pushed
the
update-controller-runtime
branch
from
7fdb260 to
e4b4ebc
Compare
This version of controller runtime has breaking changes. Refactored the Topology Controller tests because they were flaky. This group of tests was defining their own queue reconciler in the same manager that has all the controllers (included one for Queue types). This caused a "race" between all setup reconcilers, and the test passed when the controller registered in the specific test case was the first to reconcile, which seemed to happen often enough to hide the flake. After the refactor, each test case defines its own manager, which watches a dedicated namespace for the Topology Controller tests, and cancels/stops the manager at the end of each test case. This ensures that only one controller is reconciling the Queue object. The controllers defined in the BeforeSuite now watch only the default namespace. All our tests were using the default namespace anyway, so there are no failures as part of this subtle change. Signed-off-by: Aitor Perez Cedres <[email protected]>
Controllers now are scoped to a dedicated namespace. Controllers now are started and stopped for each test case. This provides isolation from other test suites, ensuring there's no environment pollution. We also bumped Kubernetes to 1.26, because this version had some changes regarding Pod Admission Controllers, and it will be the minimum supported Kubernetes version in the next minor release. Signed-off-by: Aitor Perez Cedres <[email protected]>
Zerpet
force-pushed
the
update-controller-runtime
branch
from
e4b4ebc to
2f6e50e
Compare
Execution time has cut down from 10 minutes to 1 minute and a few seconds (!!!) The solution is to start an API server for each parallel process, listening on different ports. I tried to start only one API server and share the API information among other parallel processes, but it was not possible because the testEnv Environment does not implement any of the marshall interfaces, so it is not possible to binary encode it. Binary encoding is the method that Ginkgo uses to share information between processes in the Synchronized Before/After suites. Signed-off-by: Aitor Perez Cedres <[email protected]>
Zerpet
force-pushed
the
update-controller-runtime
branch
from
2f6e50e to
cad37eb
Compare
|
The SuperStream test suite is still flaking. Looking into it 👀 |
Use UID to verify whether an object has changed is more reliable than a timestamp. A timestamp can be very close to each other if the recreate operation is fast. Signed-off-by: Aitor Perez Cedres <[email protected]>
|
This is ready for review after b8f7820 🥳 |
DanielePalaia
approved these changes
Dec 14, 2023
|
Ah, I removed it locally because it was complaining about my local Go version and not being very useful at all. I'll add it back to the unit-tests target. IMO it's unnecessary to run them at unit and at integration level. |
[skip ci] Signed-off-by: Aitor Perez Cedres <[email protected]>
This was referenced Dec 14, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Note to reviewers: remember to look at the commits in this PR and consider if they can be squashed
Note to contributors: remember to re-generate client set if there are any API changes
Summary Of Changes
Additional Context
This version of controller runtime has breaking changes. Refactored the
Topology Controller tests because they were flaky. This group of tests
was defining their own queue reconciler in the same manager that has all
the controllers (included one for Queue types). This caused a "race"
between all setup reconcilers, and the test passed when the controller
registered in the specific test case was the first to reconcile, which
seemed to happen often enough to hide the flake.
After the refactor, each test case defines its own manager, which
watches a dedicated namespace for the Topology Controller tests, and
cancels/stops the manager at the end of each test case. This ensures
that only one controller is reconciling the Queue object. The
controllers defined in the BeforeSuite now watch only the default
namespace. All our tests were using the default namespace anyway, so
there are no failures as part of this subtle change.