set ForceSynchronization to true by default #1210
Conversation
WalkthroughChanged the default of MetricsConfig.ForceSynchronization() to true when the flag is unset; added test helpers to initialize counters with an explicit ToolchainConfig or with metrics synchronization disabled; updated many tests and test helper signatures to use the new helpers and to pass ToolchainConfig where needed. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant App
participant ToolchainConfig
participant MetricsConfig
App->>ToolchainConfig: Load ToolchainConfig (may be absent)
ToolchainConfig->>MetricsConfig: Query ForceSynchronization()
alt Flag explicitly set
MetricsConfig-->>ToolchainConfig: Return configured value
else Flag unset
note right of MetricsConfig `#D8F3DC`: New default → true
MetricsConfig-->>ToolchainConfig: Return true
end
ToolchainConfig-->>App: Provide effective Metrics configuration
sequenceDiagram
autonumber
participant TestRunner
participant InitHelper
participant FakeClient
participant Counters
TestRunner->>InitHelper: Call InitializeCountersWithToolchainConfig(...) or InitializeCountersWithMetricsSyncDisabled(...)
alt With ToolchainConfig
InitHelper->>FakeClient: Build FakeClient including ToolchainConfig
else Metrics sync disabled
InitHelper->>FakeClient: Build FakeClient without metrics sync init
end
InitHelper->>Counters: Initialize counters using FakeClient
Counters-->>TestRunner: Test state ready
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used🧬 Code graph analysis (1)controllers/spacecompletion/space_completion_controller_test.go (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
🔇 Additional comments (4)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
controllers/toolchainconfig/configuration.go(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Build & push operator bundles for e2e tests
- GitHub Check: GolangCI Lint
- GitHub Check: Test with Coverage
| func (d MetricsConfig) ForceSynchronization() bool { | ||
| return commonconfig.GetBool(d.metrics.ForceSynchronization, false) | ||
| return commonconfig.GetBool(d.metrics.ForceSynchronization, true) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Verify that forced synchronization resolves the flaky test and doesn't degrade performance.
The default change from false to true aligns with the PR objective to fix flaky space count verification failures. However, please confirm:
- Has this change been tested with the paired toolchain-e2e PR update .govulncheck.yaml #1212 to verify it resolves the context deadline exceeded errors?
- What is the performance impact of forcing synchronization by default? If synchronization is expensive, this could introduce latency in the operator's reconciliation loops.
- Are there any existing deployments or configurations that explicitly rely on the old default of
false? If so, they should be documented in migration notes.
Run the following script to check how ForceSynchronization() is used throughout the codebase to assess potential impact:
🏁 Script executed:
#!/bin/bash
# Description: Find all usages of ForceSynchronization to understand the impact of changing the default
# Search for direct calls to ForceSynchronization method
echo "=== Direct calls to ForceSynchronization() ==="
rg -n -C3 'ForceSynchronization\(\)' --type=go
echo -e "\n=== References to metrics config that might use ForceSynchronization ==="
rg -n -C3 '\.Metrics\(\)' --type=go
echo -e "\n=== Check for any documentation mentioning ForceSynchronization ==="
rg -n 'ForceSynchronization' --type=mdLength of output: 5123
Fix default in configuration tests and document the change
- The default for
ForceSynchronization()is nowtrue, but the test at controllers/toolchainconfig/configuration_test.go:335 still assertsfalse; update it toassert.True(...). - Update any documentation or migration notes to reflect the new default and evaluate potential performance impacts of always forcing synchronization.
🤖 Prompt for AI Agents
In controllers/toolchainconfig/configuration.go around lines 201 to 203, the
ForceSynchronization() default was changed to true; update the failing unit test
at controllers/toolchainconfig/configuration_test.go line ~335 to
assert.True(...) instead of assert.False(...), and update any related
documentation or migration notes to indicate the new default is true and call
out potential performance implications of always forcing synchronization so
reviewers/operators are aware of the change.
alexeykazakov
left a comment
alexeykazakov
left a comment
There was a problem hiding this comment.
@xcoulon are there any potential side effects if always force synch? If so then I would make sure we set it to false in production configuration.
|
@alexeykazakov We already force do the force sync in production: https://github.com/codeready-toolchain/sandbox-sre/blob/dc7005f467bb0c8651b3d538cf2c03c87f183db7/components/sandbox/devsandbox-production/host-cluster/toolchain-config.yaml#L13-L14 When this is executed, then the client cache is already initialized so there shouldn't be any extra load on the API as this will use only the client cache. This is also the reason why I suggested enabling the force sync by default. |
MatousJobanek
left a comment
MatousJobanek
left a comment
There was a problem hiding this comment.
Looks good, but let's try to simplify the code-duplication.
| toolchainConfig := commonconfig.NewToolchainConfigObjWithReset(t, testconfig.Metrics().ForceSynchronization(false)) | ||
| InitializeCounters(t, toolchainStatus, toolchainConfig) |
There was a problem hiding this comment.
it looks like that most of the cases use the disabled synchronization, so maybe worth adjusting the InitializeCounters logic and/or create another one called InitializeCounterWithConfig, or something similar.
| t.Cleanup(counter.Reset) | ||
| initializeCounters(t, commontest.NewFakeClient(t), toolchainStatus) | ||
|
|
||
| toolchainConfig := commonconfig.NewToolchainConfigObjWithReset(t, testconfig.Metrics().ForceSynchronization(false)) |
There was a problem hiding this comment.
Yeah, we could do something similar also in the other function. TBH, I'm wondering how many test cases we have where we don't sync the counters from the ToolchainStatus, but by syncing from the resources. There is maybe some room for additional simplification.
There was a problem hiding this comment.
Thanks for the suggestions!
-
I kept the
InitializeCountersfor the cases we wantForceSynchronizationto true (which is now our default). -
I created
InitializeCountersWithMetricsSyncDisabledto initialize the counters with the metrics synchronization disabled (covers the majority of the test cases) -
I also created
InitializeCountersWithToolchainConfigto initialize the counters with the toolchain configuration given (because some tests require synchronization disabled plus other specific config like automatic approval)
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
controllers/usersignup/usersignup_controller_test.go (2)
2403-2426: Fix variable mix-up: use userSignup2 instead of userSignup in this subtest.Assertions reference the wrong variable, so the test may pass/fail for the wrong object.
Apply this diff:
- assert.Equal(t, "deactivated", userSignup.Labels[toolchainv1alpha1.UserSignupStateLabelKey]) + assert.Equal(t, "deactivated", userSignup2.Labels[toolchainv1alpha1.UserSignupStateLabelKey]) - test.AssertConditionsMatch(t, userSignup.Status.Conditions, + test.AssertConditionsMatch(t, userSignup2.Status.Conditions, toolchainv1alpha1.Condition{ Type: toolchainv1alpha1.UserSignupApproved, Status: corev1.ConditionFalse, Reason: "Deactivated", }, toolchainv1alpha1.Condition{ Type: toolchainv1alpha1.UserSignupComplete, Status: corev1.ConditionTrue, Reason: "Deactivated", }, toolchainv1alpha1.Condition{ Type: toolchainv1alpha1.UserSignupUserDeactivatedNotificationCreated, Status: corev1.ConditionTrue, Reason: "NotificationCRCreated", })
2149-2156: Don’t recreate the fake client; update the existing object instead.Re-invoking prepareReconcile drops the previously created MUR, weakening the intent (“synchronize modified claims to existing MUR”). Update the existing UserSignup and reconcile again.
Apply this diff:
- // Reconcile the UserSignup again - r, req, _ = prepareReconcile(t, userSignup.Name, nil, spc1, userSignup, deactivate30Tier) - res, err = r.Reconcile(context.TODO(), req) + // Reconcile the UserSignup again (same client, preserves existing MUR) + require.NoError(t, r.Client.Update(context.TODO(), userSignup)) + res, err = r.Reconcile(context.TODO(), req)
🧹 Nitpick comments (5)
controllers/spacecompletion/space_completion_controller_test.go (1)
215-229: Consider clearer naming for the boolean parameter.The
skipToolchainConfigparameter creates somewhat inverted logic:falsemeans "create a ToolchainConfig with metrics sync disabled", whiletruemeans "don't create one" (allowing the default ForceSynchronization=true behavior). This aligns with the past review comment suggesting a clearer helper approach.Consider either:
- Renaming to
enableMetricsSyncfor more intuitive semantics- Creating a dedicated helper like
prepareReconcileWithMetricsSync(enable bool)as suggested in the past reviewCurrent implementation works correctly, but the semantics could be more intuitive.
Based on learnings.
controllers/usersignup/usersignup_controller_test.go (4)
4109-4135: Avoid double counter initialization and config divergence in tests.prepareReconcile initializes counters with a default ToolchainConfig (and a default ToolchainStatus), but many tests reinitialize counters immediately after with custom ToolchainStatus. This causes:
- Redundant resets/work.
- Potential divergence between the ToolchainStatus stored in r.Client and the one used to seed in-memory counters.
Consider one of:
- Accept a ToolchainStatus parameter (or an option) in prepareReconcile and initialize counters only once with the caller-provided status.
- Or add a flag/option to skip internal counter init when tests will call InitializeCounters* themselves.
116-119: Reduce boilerplate for test configs that just disable metrics sync.You repeatedly build ToolchainConfig with Metrics().ForceSynchronization(false). Add a tiny helper, eg newTestConfig(t, opts...) that always includes ForceSynchronization(false) and merges extra options (AutomaticApproval, Tiers, Captcha, etc.). This will simplify tests and prevent omissions.
Also applies to: 239-240, 428-431, 572-576, 623-626, 667-673, 716-719, 759-766, 962-965, 1025-1026, 1088-1091, 1219-1229
115-115: Remove redundant counter.Reset() defers when using InitializeCounters helpers.*InitializeCountersWithToolchainConfig/InitializeCountersWithMetricsSyncDisabled already call counter.Reset() and register t.Cleanup(counter.Reset). The extra defers are unnecessary.
Also applies to: 229-229, 334-334
1660-1669: Ensure InitializeCounters reads the same ToolchainConfig used by the reconciler.*When prepareReconcile is called with toolchainConfig == nil, it creates and injects a default config into r.Client, while InitializeCountersWithMetricsSyncDisabled creates a separate ToolchainConfig instance inside its own fake client. Prefer passing the same ToolchainConfig object to both paths (eg, create it once and pass into prepareReconcile; or provide an InitializeCountersFromClient(fakeClient) helper) to avoid subtle divergence.
Also applies to: 1723-1731, 1769-1777
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
controllers/masteruserrecord/masteruserrecord_controller_test.go(28 hunks)controllers/space/space_controller_test.go(39 hunks)controllers/spacecompletion/space_completion_controller_test.go(12 hunks)controllers/toolchainconfig/configuration_test.go(1 hunks)controllers/toolchainstatus/toolchainstatus_controller_test.go(14 hunks)controllers/usersignup/usersignup_controller_test.go(66 hunks)pkg/counter/cache_test.go(11 hunks)test/counter.go(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (7)
controllers/toolchainstatus/toolchainstatus_controller_test.go (1)
test/counter.go (1)
InitializeCountersWithMetricsSyncDisabled(127-137)
controllers/space/space_controller_test.go (2)
test/counter.go (1)
InitializeCountersWithMetricsSyncDisabled(127-137)test/toolchainstatus.go (4)
NewToolchainStatus(15-26)WithEmptyMetrics(125-135)WithMember(76-87)WithSpaceCount(91-95)
pkg/counter/cache_test.go (2)
test/counter.go (1)
InitializeCountersWithMetricsSyncDisabled(127-137)test/toolchainstatus.go (1)
NewToolchainStatus(15-26)
test/counter.go (2)
controllers/toolchainconfig/configuration.go (1)
ToolchainConfig(33-36)pkg/counter/cache.go (1)
Reset(53-57)
controllers/spacecompletion/space_completion_controller_test.go (1)
test/counter.go (2)
InitializeCounters(106-112)InitializeCountersWithMetricsSyncDisabled(127-137)
controllers/usersignup/usersignup_controller_test.go (2)
test/counter.go (2)
InitializeCountersWithToolchainConfig(115-124)InitializeCountersWithMetricsSyncDisabled(127-137)test/toolchainstatus.go (1)
NewToolchainStatus(15-26)
controllers/masteruserrecord/masteruserrecord_controller_test.go (2)
test/counter.go (1)
InitializeCountersWithMetricsSyncDisabled(127-137)test/toolchainstatus.go (1)
NewToolchainStatus(15-26)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Test with Coverage
- GitHub Check: Build & push operator bundles for e2e tests
- GitHub Check: Build & push operator bundles for e2e tests
- GitHub Check: Test with Coverage
🔇 Additional comments (11)
controllers/toolchainconfig/configuration_test.go (1)
330-343: LGTM: Test correctly reflects the inverted default for ForceSynchronization.The test expectations have been properly updated to reflect the new default behavior where
ForceSynchronization()returnstruewhen the flag is unset, andfalsewhen explicitly configured.controllers/masteruserrecord/masteruserrecord_controller_test.go (1)
57-57: LGTM: Tests updated to use explicit metrics sync disabled initialization.All test initializations in this file have been consistently updated to use
InitializeCountersWithMetricsSyncDisabled, ensuring deterministic test behavior that's independent of the new ForceSynchronization default.controllers/space/space_controller_test.go (1)
70-70: LGTM: Consistent test initialization with metrics sync disabled.All space controller tests have been systematically updated to explicitly disable metrics synchronization during initialization, ensuring consistent and predictable test behavior.
pkg/counter/cache_test.go (1)
28-411: LGTM: Counter tests appropriately distinguish sync behavior.Most tests now explicitly use
InitializeCountersWithMetricsSyncDisabledfor deterministic behavior. Notably,TestForceInitializeCounterByLoadingExistingResources(line 373) intentionally continues usingInitializeCountersto specifically test the force synchronization path where counters are recalculated from actual resources. This distinction is correct and aligns with the test's intent.controllers/toolchainstatus/toolchainstatus_controller_test.go (3)
513-532: LGTM! Test correctly configures synchronization behavior.The test appropriately creates a
ToolchainConfigwithForceSynchronization(false)to test counter initialization fromToolchainStatuswithout interference from cluster resource synchronization. This allows verification of incremental counter updates as intended by lines 1535-1538.
705-705: LGTM! Consistent use of metrics sync disabled helper.The consistent use of
InitializeCountersWithMetricsSyncDisabledacross these test cases is appropriate. These tests verify specific counter states and member cluster scenarios where deterministic counter values fromToolchainStatusare required without interference from force synchronization with cluster resources.Also applies to: 742-742, 778-778, 806-806, 827-827, 852-852, 905-905, 927-927, 958-958, 985-985, 1017-1017
1569-1571: LGTM! Counter assertions are correct.The space count assertions correctly verify:
member-1: 7 spaces (6 from initial ToolchainStatus + 1 from increment at line 1536)member-2: 2 spaces (from initial ToolchainStatus, unchanged)The inline comments accurately document the counter sources, and the values match the expected behavior with metrics synchronization disabled.
test/counter.go (4)
114-124: LGTM! Well-designed helper function.
InitializeCountersWithToolchainConfigprovides appropriate flexibility for tests that need explicit control over theToolchainConfig. The implementation follows established patterns, properly handles cleanup, and includes clear documentation.
126-137: LGTM! Useful convenience helper.
InitializeCountersWithMetricsSyncDisabledappropriately encapsulates the pattern of disabling metrics synchronization for tests that require deterministic counter initialization fromToolchainStatus. The clear naming and documentation make the intent obvious.
139-147: LGTM! Appropriate explicit configuration.Both
InitializeCountersWithoutResetandInitializeCountersWithcorrectly maintain their previous behavior by explicitly settingForceSynchronization(false). These functions are designed for specific test scenarios where counter initialization fromToolchainStatuswithout cluster resource synchronization is required. The inline comment at lines 155-158 provides helpful context for this behavior.Also applies to: 169-173
106-112: Update: Verify thatcontrollers/usersignup/approval_test.gotests don't depend on the changed default behavior.The codebase shows a deliberate pattern:
cache_test.goswitched toInitializeCountersWithMetricsSyncDisabled, andspace_completion_controller_test.gouses conditional logic to handle both cases. However,approval_test.gohas 13 calls toInitializeCountersthat remain unchanged.
InitializeCountersnow uses the defaultForceSynchronization=true(set incontrollers/toolchainconfig/configuration.go:202), whereasInitializeCountersWithMetricsSyncDisabledexplicitly disables it. The approval tests don't assert counter/metrics behavior directly, but the counter initialization could still affect test behavior. The inconsistency suggests these tests should be reviewed to ensure they handle the new default correctly.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
test/counter.go (2)
138-146: Consider documenting the implicit metrics synchronization behavior.This function now creates a ToolchainConfig with
ForceSynchronization(false), which is an implicit behavior not reflected in the function name or documentation. While the "WithoutReset" name accurately describes the counter reset behavior, tests using this function may not expect the metrics synchronization to be explicitly disabled.Consider adding a documentation comment similar to:
// InitializeCountersWithoutReset initializes the counters without resetting the counter cache first. // Metrics synchronization is disabled to preserve backward compatibility with existing tests.
148-172: Consider documenting the metrics synchronization behavior and exploring refactoring opportunities.This function now creates a ToolchainConfig with
ForceSynchronization(false), which represents a behavior change that isn't documented. Additionally, there's notable code duplication across the counter initialization functions (the pattern of setting WATCH_NAMESPACE, calling counter.Reset(), and registering cleanup repeats in multiple functions).Based on learnings:
- The past review comment at line 142 already identified potential for simplification
- Consider extracting the common setup pattern into a private helper function
- Consider adding a comment noting that metrics synchronization is explicitly disabled
Example documentation addition:
// InitializeCountersWith initializes the count cache from the counts parameter. // Metrics synchronization is disabled to ensure initialization from ToolchainStatus counts.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
test/counter.go(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
test/counter.go (2)
controllers/toolchainconfig/configuration.go (1)
ToolchainConfig(33-36)pkg/counter/cache.go (1)
Reset(53-57)
🔇 Additional comments (3)
test/counter.go (3)
14-14: LGTM! Necessary imports for the new configuration helpers.The new imports support creating ToolchainConfig instances with specific metrics synchronization settings, which is essential for the new helper functions.
Also applies to: 16-16
113-123: LGTM! Well-structured helper for custom ToolchainConfig.This function follows the established pattern and provides a clean way for tests to specify custom ToolchainConfig settings while maintaining proper cleanup semantics.
125-136: LGTM! Clear helper for backward-compatible test behavior.This function provides an explicit way for tests to disable metrics synchronization, which is useful for maintaining backward compatibility with tests that were written when ForceSynchronization defaulted to false.
|
/retest infra issue |
metlos
left a comment
metlos
left a comment
There was a problem hiding this comment.
Nice simplifications! The only comment I have is to reuse the logic as much as possible so that we don't forget to update multiple places in the future if we need to make changes in the parts of the logic that is common to all the Initialize* functions.
mfrancisc
left a comment
mfrancisc
left a comment
There was a problem hiding this comment.
Looks good 👍
I have only few minor comments
| space := spacetest.NewSpace(test.HostOperatorNs, "oddity", | ||
| spacetest.WithTierName("")) | ||
| r, req, cl := prepareReconcile(t, space, nil) | ||
| r, req, cl := prepareReconcile(t, space, nil, true) |
There was a problem hiding this comment.
just curious - why does it needs to synch the metrics here ? I'm not seeing anything different in this test but maybe I've missed it.
There was a problem hiding this comment.
Just to prevent ToolchainConfig from being cached during counter initialization. With false, a ToolchainConfig is created and cached, so when Reconcile calls GetToolchainConfig it returns from cache and bypasses our mock. With true, no config is cached, so the mock actually gets called and returns the error we're testing for. I will add a comment
controllers/spacecompletion/space_completion_controller_test.go
Outdated
Show resolved
Hide resolved
mfrancisc
left a comment
mfrancisc
left a comment
There was a problem hiding this comment.
Nice 👍
Thanks for addressing my comments.
|
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: alexeykazakov, MatousJobanek, metlos, mfrancisc, rajivnathan, rsoaresd The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Codecov Report❌ Patch coverage is
❌ Your changes status has failed because you have indirect coverage changes. Learn more about Unexpected Coverage Changes and reasons for indirect coverage changes. Additional details and impacted files@@ Coverage Diff @@
## master #1210 +/- ##
==========================================
+ Coverage 84.08% 84.21% +0.13%
==========================================
Files 84 84
Lines 6561 6571 +10
==========================================
+ Hits 5517 5534 +17
+ Misses 827 820 -7
Partials 217 217
🚀 New features to boost your workflow:
|
6 participants
Description
Lately, we are often hitting this flaky test:
The space count mismatch seems to happen between "setup migration" and "verify migration" steps. It can be happening for two reasons:
Premature Operator Shutdown
we simply kill the operators (at the end of the migration setup) too early, so it either doesn't properly decreased the counter or it doesn't store the decreased value to the ToolchainStatus
No Counter Reconciliation on Startup
we don't recount the Spaces at the start of the host operator in e2e tests
Slack thread
https://redhat-internal.slack.com/archives/CHK0J6HT6/p1759830691610259
Paired PR
codeready-toolchain/toolchain-e2e#1212
Issue ticket number and link
SANDBOX-1437
Assisted-by: Cursor
Summary by CodeRabbit
New Features
Bug Fixes / Behavior Changes
Tests