feat: utility to verify sync behaviour correctness - #1066
Conversation
📝 WalkthroughWalkthroughThe PR adds a synchronization consistency utility that generates and validates Application specification and status events. It also updates event deduplication diagnostics, queue logging, Application status handling, outbound processing, and related tests. ChangesApplication event synchronization
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Main
participant ControlPlane
participant ManagedAgent
participant EventState
Main->>ControlPlane: create, update, and delete Applications
Main->>ManagedAgent: patch Application status
ControlPlane->>EventState: record source events
ManagedAgent->>EventState: record watched events
Main->>EventState: validate ordering, deduplication, and propagation
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
461cd58 to
3b6d117
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (2)
hack/event-generator/spec-status-reader-writer.go (2)
330-385: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the shared watch loop.
startManagedAgentSpecWatchLogandstartControlPlaneStatusWatchLogare near-identical. Only three things differ: the namespace, thedirectionvalue, and whether the event carriesrepoURLorstatusValue. Everything else, including thewatch.Errorhandling, the channel-closed handling, and thedefaultcase, is duplicated.Extract one
startApplicationWatchLog(ctx, c, state, namespace string, dir direction, build func(*v1alpha1.Application) applicationEvent)helper and call it twice. This removes about 50 duplicated lines and keeps the two watchers in step when the error handling changes.This is a hack utility, so treat it as optional cleanup rather than a merge blocker.
Also applies to: 389-441
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@hack/event-generator/spec-status-reader-writer.go` around lines 330 - 385, Extract the duplicated watch-loop logic from startManagedAgentSpecWatchLog and startControlPlaneStatusWatchLog into a shared startApplicationWatchLog helper accepting ctx, client, state, namespace, direction, and an application-to-event builder. Preserve the existing watch.Error, closed-channel, unexpected-type, and event-recording behavior, while having each watcher supply only its namespace, direction, and repoURL/statusValue event construction.
609-661: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRestore chronological order before returning
result.
filterSourceEventsNearDeletewalksallSourceEventsReversedand appends toresult, soresultis newest-first. The function never reverses it back.This does not change the verdict:
validateMaxPropagationDelayscans both lists independently of order. It does change the debug output. Line 596 passesnewSourceEventstooutputEventList, which prints the source list newest-first while every other list in this file prints oldest-first. The whole purpose of this utility is failure diagnosis, so the mismatched ordering makes the output harder to read.♻️ Proposed fix
} + // Restore chronological (oldest-first) order, to match every other event list. + slices.Reverse(result) + return result }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@hack/event-generator/spec-status-reader-writer.go` around lines 609 - 661, Reverse result back to chronological order before returning from filterSourceEventsNearDelete, after the delete-only cleanup logic and before the return statement. Preserve the existing filtering and validation behavior while ensuring outputEventList receives result in the same oldest-first order as the other event lists.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@hack/event-generator/go.mod`:
- Around line 99-100: Update the indirect dependency resolutions in the
event-generator module so golang.org/x/crypto, github.com/go-git/go-git/v5, and
github.com/go-git/go-billy/v5 resolve to patched releases, then rerun go mod
tidy in the module to synchronize go.mod and go.sum.
In `@hack/event-generator/README.md`:
- Line 21: Correct the five typographical errors in the user-facing README text,
including “beween” in the eventual-consistency description, and apply the
corresponding spelling fixes at the other referenced documentation lines.
Preserve the existing wording and meaning.
- Line 96: Correct the limitation statement in the README to match run(),
startControlPlaneSpecWriter, and startManagedAgentStatusWriter: spec-writes and
status-writes run concurrently. If retaining a limitation, clarify that only one
goroutine is active per writer type, with no parallel writers of the same type.
In `@hack/event-generator/spec-status-reader-writer.go`:
- Around line 369-376: Guard all Application.Spec.Source dereferences in the
event-generation paths: in hack/event-generator/spec-status-reader-writer.go
lines 369-376, assign RepoURL to a local repoURL only when app.Spec.Source is
non-nil before constructing the watcher event; apply the same nil-safe repoURL
handling in lines 248-255 for the delete event. Preserve empty repoURL behavior
when Source is nil.
In `@hack/event-generator/util.go`:
- Around line 159-205: Bound both startup cleanup wait loops using a
startupDeleteTimeoutSeconds constant alongside the existing tuning constants,
aborting through the established exit() path with a clear timeout error when the
limit is exceeded. Update the managed-agent wait logging to print only
Application names, not full apps.Items objects, while preserving the existing
deletion checks and error handling.
In `@internal/event/event_writer.go`:
- Around line 552-574: The trace logging in the duplicate-event removal block
must not serialize the complete Application payload. Update the
Application-specific handling in the surrounding event writer logic to log only
safe Application identifiers, or explicitly redact and allowlist approved fields
before serialization; remove or replace the app_spec and app_status fields while
preserving the existing event metadata and trace message.
---
Nitpick comments:
In `@hack/event-generator/spec-status-reader-writer.go`:
- Around line 330-385: Extract the duplicated watch-loop logic from
startManagedAgentSpecWatchLog and startControlPlaneStatusWatchLog into a shared
startApplicationWatchLog helper accepting ctx, client, state, namespace,
direction, and an application-to-event builder. Preserve the existing
watch.Error, closed-channel, unexpected-type, and event-recording behavior,
while having each watcher supply only its namespace, direction, and
repoURL/statusValue event construction.
- Around line 609-661: Reverse result back to chronological order before
returning from filterSourceEventsNearDelete, after the delete-only cleanup logic
and before the return statement. Preserve the existing filtering and validation
behavior while ensuring outputEventList receives result in the same oldest-first
order as the other event lists.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 953d0445-5cfe-4df8-ac15-1f743c1bd3f9
⛔ Files ignored due to path filters (1)
hack/event-generator/go.sumis excluded by!**/*.sum
📒 Files selected for processing (11)
agent/outbound.gohack/event-generator/.gitignorehack/event-generator/README.mdhack/event-generator/go.modhack/event-generator/main.gohack/event-generator/spec-status-reader-writer.gohack/event-generator/util.gointernal/event/event_writer.gointernal/event/event_writer_test.goprincipal/event.goprincipal/event_test.go
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1066 +/- ##
==========================================
- Coverage 48.62% 48.59% -0.03%
==========================================
Files 126 126
Lines 19136 19153 +17
==========================================
+ Hits 9304 9307 +3
- Misses 8996 9009 +13
- Partials 836 837 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
3b6d117 to
a9df6d2
Compare
Signed-off-by: Jonathan West <jgwest@gmail.com>
a9df6d2 to
1aee933
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (7)
hack/sync-consistency-util/util.go (3)
223-233: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueOptional: guard
randomKeyFromAppNameMapagainst an empty map.
rand.IntNpanics whenlen(m)is 0. Every current caller checks the length first, but the check and the call are not atomic with respect to the function itself, and a future caller can miss the check. The function already has anexit()path for the unreachable case; use it for the empty case too.♻️ Proposed refactor
func randomKeyFromAppNameMap(m map[string]bool) string { + if len(m) == 0 { + exit("randomKeyFromAppNameMap called with empty map") + return "" + } idx := rand.IntN(len(m))🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@hack/sync-consistency-util/util.go` around lines 223 - 233, Update randomKeyFromAppNameMap to detect an empty map before calling rand.IntN, and route that case through the existing exit() failure path. Preserve the current random key selection behavior for non-empty maps.
125-125: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueOptional: align the Application name prefix with the new utility name.
The module and directory are now
sync-consistency-util, but the startup cleanup filter and the generated names inhack/sync-consistency-util/spec-status-reader-writer.goline 142 still useevent-generator-app. Keeping the old prefix works, but it makes the leftover Applications harder to trace back to this utility. If you rename, change both sites together, otherwise startup cleanup stops matching the generated names.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@hack/sync-consistency-util/util.go` at line 125, Optionally rename the Application name prefix from event-generator-app to the new sync-consistency-util prefix in both the startup cleanup filter and the generated-name logic in spec-status-reader-writer.go. Update both sites together so the cleanup condition continues matching generated Application names; otherwise leave both existing prefixes unchanged.
235-264: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueOptional: return errors instead of calling
exitinsidegetK8sClientByContextName.The function signature returns an
error, but the scheme-registration and kubeconfig failures callexit(), which terminates the process. The caller inrun()already propagates errors. Return the errors for a consistent contract.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@hack/sync-consistency-util/util.go` around lines 235 - 264, The getK8sClientByContextName function should return errors instead of terminating via exit. Replace each exit call for scheme registration and kubeconfig loading with an error return, preserving the existing contextual error messages so run() can propagate them consistently.hack/sync-consistency-util/README.md (1)
27-49: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueOptional: fix the markdown lint findings.
The heading at line 27 jumps from h2 to h4, and the fenced blocks at lines 31, 37, and 49 have no language. Use
###and addshellto the fences.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@hack/sync-consistency-util/README.md` around lines 27 - 49, Update the README headings in the setup and process-start instructions so the heading currently using h4 becomes h3, and label the fenced command blocks around the setup, agent-start, and utility-start examples as shell fences.Source: Linters/SAST tools
hack/sync-consistency-util/main.go (1)
36-40: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueOptional: set
clientBurstto a value that matchesclientQPS.When
Burstis 0, client-go appliesDefaultBurstof 10. WithQPSat 50, the burst budget stays well below the sustained rate, so short bursts of Application writes are throttled harder than the QPS setting suggests. SetclientBurstto at least the QPS value if you want the configured rate to be reachable.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@hack/sync-consistency-util/main.go` around lines 36 - 40, Update the clientBurst configuration alongside clientQPS so its value is at least the configured QPS of 50, rather than relying on client-go’s default burst of 10. Preserve the existing throttle configuration and comments while making the burst budget match the intended sustained rate.hack/sync-consistency-util/spec-status-reader-writer.go (2)
70-96: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueOptional: cancel
readerCtxon shutdown.
readerCtxiscontext.Background(), so the two watch goroutines keep receiving and recording events whilevalidateEventualConsistencyOfEventListruns at line 80. The validation works on a snapshot taken under the mutex at lines 448-454, so the result is not corrupted, and the process exits right after. Cancelling the reader context before validation would still make shutdown deterministic and stop the watchers cleanly.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@hack/sync-consistency-util/spec-status-reader-writer.go` around lines 70 - 96, Make reader shutdown cancellable by creating readerCtx with context.WithCancel and retaining its cancel function alongside the existing writer context. Invoke the reader cancel function immediately after writerCancelFunc and before validateEventualConsistencyOfEventList, while preserving the current watcher setup using readerCtx.
127-140: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAdd a cancellation check and a small delay to the spec writer loop.
Two problems in
startControlPlaneSpecWriter:
- The loop has no
ctx.Done()check. AfterwriterCancelFunc()runs, the create branch records the event and continues instead of returning, because line 172 suppresses the error whenctx.Err() != nil. The goroutine only stops when a later round takes the update or delete branch andGetreturns a cancellation error. The writer keeps issuing failing API calls during the 10-second settle window at line 78, which adds noise to the events that the consistency check evaluates.- A skipped action calls
continuewith no delay. At startupappNameListis empty, so the update and delete branches spin at full CPU until the create branch populates the map. The status writer at line 281 already sleeps 200ms in the equivalent case.♻️ Proposed refactor
for round := 0; ; round++ { + if ctx.Err() != nil { + return nil + } + roll := rand.IntN(100) switch { case roll < specWriterCreatePercent: state.mutex.RLock() if len(state.appNameList) > maxConcurrentApps { state.mutex.RUnlock() + time.Sleep(50 * time.Millisecond) continue } state.mutex.RUnlock()Apply the same
time.Sleepbefore thecontinueat lines 191 and 235.Also applies to: 230-239
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@hack/sync-consistency-util/spec-status-reader-writer.go` around lines 127 - 140, Update startControlPlaneSpecWriter to check ctx.Done() at the top of each loop iteration and return promptly when cancellation is requested. Add the same 200ms delay used by the status writer before the skipped-action continue paths in both the update and delete branches, including the max-concurrency skip, without changing normal create/update/delete behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@hack/sync-consistency-util/go.mod`:
- Line 3: Set the CI workflow’s actions/setup-go configuration to explicitly use
Go 1.26, and document the Go 1.26+ requirement for developers in the appropriate
project documentation. Keep the existing go.mod declarations, including the
hack/ modules, aligned with this minimum version.
In `@hack/sync-consistency-util/README.md`:
- Around line 61-63: Update the startup deletion note to accurately describe
deleteOldApplicationsOnStartup: it deletes only control-plane Applications in
the agent-managed namespace whose names start with event-generator-app, while
managed-agent cleanup only waits for that namespace to become empty. Remove the
claim that all Applications on all vclusters are deleted.
In `@hack/sync-consistency-util/util.go`:
- Around line 346-363: Introduce one shared timeout constant beside the tuning
constants in main.go, then apply it to both affected regions in
hack/sync-consistency-util/util.go: add deadline-based exits to the
argocd-application-controller-0 termination loop at lines 346-363 and the
per-application deletion and managed-agent list loops at lines 159-209. Return
clear timeout errors when deadlines expire, including the remaining Application
names for the latter loops.
---
Nitpick comments:
In `@hack/sync-consistency-util/main.go`:
- Around line 36-40: Update the clientBurst configuration alongside clientQPS so
its value is at least the configured QPS of 50, rather than relying on
client-go’s default burst of 10. Preserve the existing throttle configuration
and comments while making the burst budget match the intended sustained rate.
In `@hack/sync-consistency-util/README.md`:
- Around line 27-49: Update the README headings in the setup and process-start
instructions so the heading currently using h4 becomes h3, and label the fenced
command blocks around the setup, agent-start, and utility-start examples as
shell fences.
In `@hack/sync-consistency-util/spec-status-reader-writer.go`:
- Around line 70-96: Make reader shutdown cancellable by creating readerCtx with
context.WithCancel and retaining its cancel function alongside the existing
writer context. Invoke the reader cancel function immediately after
writerCancelFunc and before validateEventualConsistencyOfEventList, while
preserving the current watcher setup using readerCtx.
- Around line 127-140: Update startControlPlaneSpecWriter to check ctx.Done() at
the top of each loop iteration and return promptly when cancellation is
requested. Add the same 200ms delay used by the status writer before the
skipped-action continue paths in both the update and delete branches, including
the max-concurrency skip, without changing normal create/update/delete behavior.
In `@hack/sync-consistency-util/util.go`:
- Around line 223-233: Update randomKeyFromAppNameMap to detect an empty map
before calling rand.IntN, and route that case through the existing exit()
failure path. Preserve the current random key selection behavior for non-empty
maps.
- Line 125: Optionally rename the Application name prefix from
event-generator-app to the new sync-consistency-util prefix in both the startup
cleanup filter and the generated-name logic in spec-status-reader-writer.go.
Update both sites together so the cleanup condition continues matching generated
Application names; otherwise leave both existing prefixes unchanged.
- Around line 235-264: The getK8sClientByContextName function should return
errors instead of terminating via exit. Replace each exit call for scheme
registration and kubeconfig loading with an error return, preserving the
existing contextual error messages so run() can propagate them consistently.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: a77a350d-79ad-4466-a301-e226aba2cb36
⛔ Files ignored due to path filters (1)
hack/sync-consistency-util/go.sumis excluded by!**/*.sum
📒 Files selected for processing (6)
hack/sync-consistency-util/.gitignorehack/sync-consistency-util/README.mdhack/sync-consistency-util/go.modhack/sync-consistency-util/main.gohack/sync-consistency-util/spec-status-reader-writer.gohack/sync-consistency-util/util.go
3 participants
What does this PR do / why we need it:
The primary goal of this utility is to verify the correctness of Application synchronization behaviour between agent and principal.
This utility simulates Application resource events:
.spec.source.repoURLfield is modified with a unique value, to track.specsynchronization to agent.statusfield of Argo CD Applications on workload cluster.statusupdate is broadcast back to Application on control plane cluster.status.sync.revisionfield is modified with a unique value, allowing us to track.statussynchronization back to principal.specwriter and.statuswriter run concurrently, from a single OS process.specwatcher and.statuswatcher (responsible for observing events before verification) likewise run concurrentlySee README.md within the utility directory for more details.
Checklist
Summary by CodeRabbit
New Features
Bug Fixes