Use cluster endpoint at startup#90
Open
piceri wants to merge 11 commits into
Open
Conversation
Signed-off-by: Eric Pickard <piceri@github.com>
Signed-off-by: Eric Pickard <piceri@github.com>
Signed-off-by: Eric Pickard <piceri@github.com>
Signed-off-by: Eric Pickard <piceri@github.com>
Signed-off-by: Eric Pickard <piceri@github.com>
Signed-off-by: Eric Pickard <piceri@github.com>
Signed-off-by: Eric Pickard <piceri@github.com>
Signed-off-by: Eric Pickard <piceri@github.com>
ajbeattie
reviewed
Jun 1, 2026
ajbeattie
left a comment
Contributor
There was a problem hiding this comment.
Nice 💯, looking great on initial pass. Still working through some of the changes but wanted to go ahead and raise the Job/CronJob suggestion ⬇️
Signed-off-by: Eric Pickard <piceri@github.com>
Signed-off-by: Eric Pickard <piceri@github.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR adds a startup “cluster sync” path so deployment-tracker can post the current cluster state in a single request to the deployment-record cluster endpoint, reducing startup load versus posting one container at a time.
Changes:
- Introduces new deployment record data models to support the cluster endpoint request/response payloads.
- Adds
PostCluster(and refactorsPostOneto share retry logic) in the deployment record client, with accompanying unit tests. - Updates the controller startup flow to (a) suppress enqueueing during informer sync, (b) list current pods, (c) post cluster state, then (d) seed observed/unknown caches before starting workers.
Show a summary per file
| File | Description |
|---|---|
| pkg/deploymentrecord/record.go | Refactors record types into base/request/response structs for cluster endpoint support. |
| pkg/deploymentrecord/client.go | Adds cluster endpoint posting and shared retry helper; updates request body builders. |
| pkg/deploymentrecord/client_test.go | Adds PostCluster and URL-escaping tests; updates record test helpers. |
| internal/controller/reporting.go | Implements building sync records, posting cluster sync, and filling caches; refactors record building. |
| internal/controller/reporting_test.go | Adds tests for sync processing, dedupe, and cache fill behavior; updates poster call assertions. |
| internal/controller/controller.go | Adds startup sync gating via atomic.Bool and calls sync processing after informer sync. |
| internal/controller/controller_test.go | Extends mock poster to support PostCluster; adds mock metadata aggregator for new code paths. |
| internal/controller/controller_integration_test.go | Updates integration test mocks/types for renamed record type and new interface method. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 8/8 changed files
- Comments generated: 4
Comment on lines
158
to
+162
| AddFunc: func(obj any) { | ||
| // Skip adding sync events | ||
| if cntrl.syncing.Load() { | ||
| return | ||
| } |
Contributor
Author
There was a problem hiding this comment.
I think these events should still be captured. I will adjust the PR description.
Comment on lines
+142
to
150
| var deploymentRecords deploymentrecord.RecordsClusterResp | ||
| err = json.Unmarshal(respBody, &deploymentRecords) | ||
| if err != nil { | ||
| slog.Error("Failed to unmarshall response", | ||
| "error", err, | ||
| "record_count", len(syncRecords), | ||
| ) | ||
| return nil | ||
| } |
Contributor
Author
There was a problem hiding this comment.
I believe this would only happen for a 2xx response, so this scenario would only mean we miss out on filling the local caches. I think this would b okay to continue without failure.
Comment on lines
+346
to
348
| // Drain and close response body to enable connection reuse | ||
| respBody, _ := io.ReadAll(resp.Body) | ||
| _ = resp.Body.Close() |
Signed-off-by: Eric Pickard <piceri@github.com>
This file contains hidden or 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
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.
This change allows deployment tracker to use the cluster endpoint at startup to send the current state of the cluster. This will reduce the load cause at startup when deployment tracker sends the state of the cluster one container at a time.
Change details: