Guard multicluster Create against cross-cluster name collisions - #1097
Merged
Conversation
Signed-off-by: Philipp Matthes <p.matthes@sap.com>
PhilippMatthes
requested review from
SoWieMarkus,
auhlig,
juliusclausnitzer,
mblos and
umswmayj
as code owners
July 28, 2026 12:50
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughAdds Prometheus monitoring for multicluster name conflicts, instruments client operations, registers monitors during manager and shim startup, and adds corresponding Nova and placement-shim alert rules. ChangesMulticluster conflict monitoring
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant ManagerOrShim
participant MulticlusterClient
participant RemoteCluster
participant Monitor
participant Prometheus
ManagerOrShim->>Monitor: Create monitor with cortex_ prefix
ManagerOrShim->>MulticlusterClient: Inject Monitor
ManagerOrShim->>Prometheus: Register Monitor
MulticlusterClient->>RemoteCluster: Probe or read resource
RemoteCluster-->>MulticlusterClient: Existing name or NotFound
MulticlusterClient->>Monitor: Increment conflict counter
Prometheus-->>Prometheus: Evaluate multicluster alert
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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 |
PhilippMatthes
changed the title
![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
🤖 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 `@helm/bundles/cortex-nova/templates/alerts.yaml`:
- Around line 673-676: Update the CortexNovaMulticlusterNameConflicts alert so a
single conflict can fire reliably: remove the 5m for duration or change the
increase lookback to exceed the pending period, while preserving the existing
metric, grouping, and threshold.
🪄 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 Plus
Run ID: cacd078a-08f3-46f2-87e0-9f4d5cbaf360
📒 Files selected for processing (6)
cmd/manager/main.gohelm/bundles/cortex-nova/templates/alerts.yamlpkg/multicluster/client.gopkg/multicluster/client_test.gopkg/multicluster/monitor.gopkg/multicluster/monitor_test.go
…lacement shim Signed-off-by: Philipp Matthes <p.matthes@sap.com>
…onflict Signed-off-by: Philipp Matthes <p.matthes@sap.com>
umswmayj
previously approved these changes
Jul 28, 2026
Member
Author
|
Waiting for pauls review |
PaulPickhardt
requested changes
Jul 28, 2026
PaulPickhardt
left a comment
PaulPickhardt
left a comment
There was a problem hiding this comment.
Other then that looks really nice! Thanks for your effort!
|
One could also argue about having an option for hard constraining not to create CROs when there is an error other then the duplicate one. But I guess as you already wrote "best effort" it is a tradeoff between one cluster/namespace could block everything and having a duplicate because of one cluster having an issue. So I guess this is fine. |
…tric prefix Signed-off-by: Philipp Matthes <p.matthes@sap.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
pkg/multicluster/monitor.go (1)
19-26: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMake the exported monitor contract implementable.
Monitoris exported and returned byNewMonitor, but its unexportedrecordCrossClusterNameConflictmethod prevents downstream packages from directly implementing a custom monitor or collector. Go recommends this sealing pattern only when external implementations are intentionally disallowed. (go.dev)Either export the recording method, or keep the interface internal/concrete if custom implementations are not supported.
🤖 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 `@pkg/multicluster/monitor.go` around lines 19 - 26, Update the exported Monitor interface and its implementations so the cross-cluster conflict recording method is externally implementable: rename recordCrossClusterNameConflict to an exported method consistently, including NewMonitor’s returned implementation and all call sites, or make Monitor non-exported/concrete if custom implementations are intentionally unsupported.
🤖 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 `@helm/bundles/cortex-placement-shim/templates/alerts.yaml`:
- Around line 197-203: Add the missing playbook label to the alert metadata
alongside support_group in the affected rule, using the bundle’s established
runbook path convention and matching the appropriate operational guidance for
this alert.
---
Nitpick comments:
In `@pkg/multicluster/monitor.go`:
- Around line 19-26: Update the exported Monitor interface and its
implementations so the cross-cluster conflict recording method is externally
implementable: rename recordCrossClusterNameConflict to an exported method
consistently, including NewMonitor’s returned implementation and all call sites,
or make Monitor non-exported/concrete if custom implementations are
intentionally unsupported.
🪄 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 Plus
Run ID: cbef4e90-4303-4e5d-bd6d-922931c6ad6a
📒 Files selected for processing (8)
cmd/manager/main.gocmd/shim/main.gohelm/bundles/cortex-nova/templates/alerts.yamlhelm/bundles/cortex-placement-shim/templates/alerts.yamlpkg/multicluster/client.gopkg/multicluster/client_test.gopkg/multicluster/monitor.gopkg/multicluster/monitor_test.go
🚧 Files skipped from review as they are similar to previous changes (4)
- helm/bundles/cortex-nova/templates/alerts.yaml
- cmd/manager/main.go
- pkg/multicluster/client_test.go
- pkg/multicluster/client.go
Signed-off-by: Philipp Matthes <p.matthes@sap.com>
PaulPickhardt
approved these changes
Jul 29, 2026
Contributor
Test Coverage ReportTest Coverage 📊: 70.3% |
SoWieMarkus
approved these changes
Jul 29, 2026
umswmayj
approved these changes
Jul 29, 2026
This was referenced Aug 3, 2026
Merged
Merged
mblos
added a commit
that referenced
this pull request
Aug 3, 2026
## Summary - Adds changelog entry for cortex v0.3.5 - Bumps helm chart versions across all bundles and library charts - Release prep for #1114: merge this before merging #1114 ## Chart Version Bumps | Chart | Old Version | New Version | |-------|------------|-------------| | cortex (library) | 0.3.4 | 0.3.5 | | cortex-shim (library) | 0.1.10 | 0.1.11 | | cortex-nova | 0.0.84 | 0.0.85 | | cortex-cinder | 0.0.84 | 0.0.85 | | cortex-manila | 0.0.84 | 0.0.85 | | cortex-pods | 0.0.84 | 0.0.85 | | cortex-ironcore | 0.0.84 | 0.0.85 | | cortex-crds | 0.0.84 | 0.0.85 | | cortex-placement-shim | 0.1.10 | 0.1.11 | ## What's in this release ### cortex v0.3.5 (sha-684e0b07) Non-breaking changes: - Support comma-separated and prefixed tenant ids in aggregate filter (#1103) - Return more information on 500 in external scheduler call (#1104) - Send image type in failover requests (#1105) - Add az to failover reservation CRD name (#1106) - Guard multicluster Create against cross-cluster name collisions (#1097) - Fix incorrect pipeline names and default interval in failover doc (#1064) - Update `github.com/sapcc/go-bits` (#1101, #1107) - Update `github.com/mattn/go-sqlite3` to v1.14.49 (#1102) - Update `kube-prometheus-stack` to v87.21.0 (#1096, #1099) - Update `actions/stale` to v11 (#1100) ### cortex-shim v0.1.11 (sha-c325b29e) Includes updated image sha-c325b29e with dependency updates. ## Test plan - [ ] Verify changelog entry is correct and complete - [ ] Verify helm chart version bumps are consistent across all charts - [ ] CI passes 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: cortex-ai-agents[bot] <279748396+cortex-ai-agents[bot]@users.noreply.github.com> Co-authored-by: Marcel <mblos@users.noreply.github.com> Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com>
mblos
added a commit
that referenced
this pull request
Aug 3, 2026
## Release cortex v0.3.5 **Release PR**: merges `main` into `release` branch. --- ### cortex v0.3.5 (sha-684e0b07) Non-breaking changes: - Support comma-separated and prefixed tenant ids in aggregate filter — the `filter_aggregate_metadata` filter now splits `filter_tenant_id` values on commas and matches any metadata key with the `filter_tenant_id` prefix (e.g. `filter_tenant_id1`, `filter_tenant_id2`), unioning all values into the allowed project set ([#1103](#1103)) - Return more information on 500 in external scheduler call — improves error reporting when external scheduler API calls fail with HTTP 500, now including the response body in the error message ([#1104](#1104)) - Send image type in failover requests — failover reservation scheduling now includes the image type in scheduling requests ([#1105](#1105)) - Add az to failover reservation CRD name — failover reservation names now include the availability zone for uniqueness ([#1106](#1106)) - Guard multicluster Create against cross-cluster name collisions — `Create` now performs a best-effort cross-cluster existence check before writing and returns a duplicate error instead of creating a colliding object. Adds a `cortex_multicluster_cross_cluster_name_conflicts_total` Prometheus counter and corresponding alerts ([#1097](#1097)) - Fix incorrect pipeline names and default interval in failover doc ([#1064](#1064)) - Update `github.com/sapcc/go-bits` ([#1101](#1101), [#1107](#1107)) - Update `github.com/mattn/go-sqlite3` to v1.14.49 ([#1102](#1102)) - Update `kube-prometheus-stack` to v87.21.0 ([#1096](#1096), [#1099](#1099)) - Update `actions/stale` to v11 ([#1100](#1100)) ### cortex-shim v0.1.11 (sha-c325b29e) Includes updated image sha-c325b29e with dependency updates. ### Chart Versions | Chart | Version | |-------|---------| | cortex | 0.3.5 | | cortex-shim | 0.1.11 | | cortex-nova | 0.0.85 | | cortex-cinder | 0.0.85 | | cortex-manila | 0.0.85 | | cortex-pods | 0.0.85 | | cortex-ironcore | 0.0.85 | | cortex-crds | 0.0.85 | | cortex-placement-shim | 0.1.11 |
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
5 participants
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.
Writes routed to a single cluster via clusterForWrite did not check whether the resulting object name would collide with an existing object on another cluster serving the same GVK. This produced errors, observed in practice for failover reservations. This change makes Create do a best-effort cross-cluster existence check before writing and return a duplicate error (checkable via IsDuplicateError) instead of creating a colliding object. A Prometheus counter cortex_multicluster_cross_cluster_name_conflicts_total, labeled by method and gvk, is incremented from all duplicate-detection paths. The nil-safe Monitor is wired into the nova multicluster client and registered with the metrics registry, and a CortexNovaMulticlusterNameConflicts alert fires on the counter. The placement shim now also constructs and attaches the monitor to its multicluster client in the production setup path, registering it with the wrapped metrics registry, and ships a matching CortexPlacementShimMulticlusterNameConflicts alert scoped to its metrics service. The e2e path uses a metrics-disabled manager and is unchanged.
Assisted-by: Claude Code:claude-opus-latest [Bash] [Read]