Refactor /traits API to single-ConfigMap model with Syncer interface#771
Merged
Merged
Conversation
PhilippMatthes
requested review from
SoWieMarkus,
auhlig,
juliusclausnitzer,
mblos and
umswmayj
as code owners
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThis PR refactors the cortex-placement-shim's trait handling system, consolidating from a two-ConfigMap architecture into a single ConfigMap while introducing a pluggable Changes
Sequence Diagram(s)sequenceDiagram
actor Shim as Shim Startup
participant Syncer as TraitSyncer
participant CM as Traits ConfigMap
participant API as Upstream Placement API
Shim->>Syncer: Init(ctx)
Syncer->>CM: Check if ConfigMap exists
alt ConfigMap Missing
Syncer->>CM: Create with data[traits]="[]"
else ConfigMap Exists
Syncer->>Syncer: Continue
end
Syncer-->>Shim: Return error or nil
Shim->>Syncer: Run(ctx)
activate Syncer
Syncer->>Syncer: Wait jitter (0-30s)
loop Every 60s until context cancelled
Syncer->>API: GET /traits (with version header)
API-->>Syncer: JSON traits list
Syncer->>Syncer: Parse & convert to set
Syncer->>CM: Read current ConfigMap
Syncer->>Syncer: Serialize traits to JSON
Syncer->>CM: Update ConfigMap data[traits]
CM-->>Syncer: Update result
end
deactivate Syncer
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (3 warnings)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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. Review rate limit: 0/1 reviews remaining, refill in 47 minutes and 59 seconds.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: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/shim/placement/handle_traits.go`:
- Around line 205-219: The hook handler inside s.forwardWithHook currently
copies headers and status but never copies resp.Body, so error payloads are
lost; update the anonymous callback passed to forwardWithHook (the func(w
http.ResponseWriter, resp *http.Response) closure) to stream the upstream
response body to the client as well: after writing headers and calling
w.WriteHeader(resp.StatusCode), if resp.Body != nil use io.Copy(w, resp.Body)
(handle/return any io errors appropriately and ensure resp.Body.Close() is
called), or alternatively detect non-2xx statuses and delegate to
forwardWithHook's normal proxy behavior instead; apply the same fix to the other
identical hook instance mentioned (the handler around addTraitToConfigMap at the
other location).
In `@internal/shim/placement/shim.go`:
- Around line 439-454: The TraitSyncer should always be created and Init(ctx)
invoked so the ConfigMap exists, but its background Run(ctx) must only be
started when traits are in hybrid mode; update the code that appends
NewTraitSyncer(...) and the goroutine launch loop so that for the created
TraitSyncer you always call syncer.Init(ctx) but only call go syncer.Run(ctx)
when the feature flag indicates hybrid (e.g. check s.config.Features.Traits ==
"hybrid" or equivalent); alternatively make TraitSyncer mode-aware and pass the
mode into NewTraitSyncer and gate Run on that mode. Ensure references to
NewTraitSyncer, TraitSyncer.Run, and Init(ctx) are adjusted accordingly.
In `@internal/shim/placement/syncer_traits.go`:
- Around line 112-154: The sync path in TraitSyncer.sync currently reads and
updates the same ConfigMap that addTraitToConfigMap/removeTraitFromConfigMap
mutate but bypasses resourceLocker; wrap the read-modify-write in the same
single-writer lock used by trait CRUD (use resourceLocker with the same
key/lease used by addTraitToConfigMap/removeTraitFromConfigMap) so the sequence
of Get, writeTraitsToConfigMap, and Update runs under the lock, ensure the lock
is released on all error paths, and log/return on lock acquisition failure to
avoid races overwriting newer local changes.
🪄 Autofix (Beta)
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7d29528a-0f57-4920-a60c-4a8bbe255ce4
📒 Files selected for processing (9)
helm/bundles/cortex-placement-shim/templates/configmap-traits.yamlhelm/bundles/cortex-placement-shim/values.yamlinternal/shim/placement/handle_traits.gointernal/shim/placement/handle_traits_e2e.gointernal/shim/placement/handle_traits_test.gointernal/shim/placement/shim.gointernal/shim/placement/syncer.gointernal/shim/placement/syncer_traits.gointernal/shim/placement/syncer_traits_test.go
💤 Files with no reviewable changes (2)
- helm/bundles/cortex-placement-shim/values.yaml
- helm/bundles/cortex-placement-shim/templates/configmap-traits.yaml
- Copy upstream response body in hybrid mode hooks so error payloads are preserved for the caller. - Only start TraitSyncer.Run in hybrid/passthrough modes to prevent the background sync from overwriting local CRD-mode state. - Acquire the same resource lock in TraitSyncer.sync to serialize with concurrent CRUD operations on the ConfigMap.
Contributor
Test Coverage ReportTest Coverage 📊: 69.3% |
Member
Author
|
E2E tested in tilt setup |
PhilippMatthes
added a commit
that referenced
this pull request
Apr 29, 2026
Implements three-mode support for the /resource_classes placement API endpoints, following the same pattern established in the traits refactor (PR #771). Previously these endpoints only forwarded to upstream placement (returning 501 for hybrid/crd modes). Now passthrough forwards to upstream, hybrid forwards and mirrors locally, and crd serves entirely from a shim-owned ConfigMap. A ResourceClassSyncer periodically fetches upstream state into the ConfigMap so that crd mode can serve without depending on live upstream availability. This is part of the phased placement API migration. Assisted-by: claude-code:claude-opus-latest [Bash] [Read]
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
1 participant
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.
Refactors the placement shim's /traits API from a two-ConfigMap model (Helm-managed static + shim-managed dynamic) to a single shim-owned ConfigMap with a reusable Syncer interface pattern.
Changes:
This establishes the Syncer pattern for future resource types (e.g. /resource_classes).
Assisted-by: Claude (claude-code)