Index hypervisor crd fields for placement api shim#723
Merged
Conversation
PhilippMatthes
requested review from
SoWieMarkus,
auhlig,
juliusclausnitzer,
mblos and
umswmayj
as code owners
📝 WalkthroughWalkthroughIntroduces field indexing for placement-related OpenStack Hypervisor objects by adding index constants and an Changes
Sequence DiagramsequenceDiagram
participant SM as SetupWithManager
participant IF as indexFields
participant MC as multicluster.Client
participant Cache as Cache/Indexer
SM->>IF: indexFields(ctx, mcl)
IF->>MC: IndexField(idxHypervisorOpenStackId)
MC->>Cache: Register hypervisorId index
Cache-->>MC: success
IF->>MC: IndexField(idxHypervisorKubernetesId)
MC->>Cache: Register metadata.uid index
Cache-->>MC: success
IF->>MC: IndexField(idxHypervisorName)
MC->>Cache: Register metadata.name index
Cache-->>MC: success
IF->>MC: IndexField(idxStatusAggUUIDs)
MC->>Cache: Register aggregates UUIDs index
Cache-->>MC: success
IF-->>SM: nil (all indexes registered)
SM->>MC: BuildController(...)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 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 |
Contributor
Test Coverage ReportTest Coverage 📊: 69.1% |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
internal/shim/placement/field_index_test.go (1)
151-329: Consider consolidating the extractor tests.These four tests repeat the same index registration and assertion loop. A single table keyed by index field would keep the same coverage while shortening the file and reducing update points when indexes change.
As per coding guidelines,
**/*_test.go: “Test files should be short and contain only necessary test cases”.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/shim/placement/field_index_test.go` around lines 151 - 329, Consolidate the four repetitive tests (TestExtractor_HypervisorOpenStackId, TestExtractor_HypervisorKubernetesId, TestExtractor_HypervisorName, TestExtractor_StatusAggUUIDs) by creating a single table-driven test that maps each index identifier (idxHypervisorOpenStackId, idxHypervisorKubernetesId, idxHypervisorName, idxStatusAggUUIDs) to its set of subtests and expected outputs; call indexFields once per top-level run (via buildClient and indexFields) and obtain the extractor with extractorByField, then iterate the subtests invoking the extractor and asserting with strSliceEqual — this removes duplicated setup and loops while preserving all assertions and still referencing indexFields and extractorByField to locate the relevant code.
🤖 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/field_index.go`:
- Around line 89-93: The current loop in the function that builds the aggregate
UUID slice (iterating hv.Status.Aggregates and appending agg.UUID) must skip
empty UUID values so the index does not include "" entries; update the loop that
constructs uuids to only append agg.UUID when agg.UUID != "" (or len(agg.UUID) >
0), preserving the existing return of uuids so the result mirrors OpenStack
behavior and contains only populated IDs.
- Around line 42-45: The structured log call in the type-guard branches is
malformed: change calls like log.Error(errors.New("unexpected type"), "object",
obj) to supply a proper message string as the second parameter and then
key/value pairs, e.g. log.Error(errors.New("unexpected type"), "unexpected type
in extractor", "object", obj, "expected", "*hv1.Hypervisor"); update the
non-*hv1.Hypervisor branches in the extractors (the branches guarding hv, and
the similar checks at the other extractor locations referenced in the diff) so
each log.Error follows this pattern at the four spots mentioned.
---
Nitpick comments:
In `@internal/shim/placement/field_index_test.go`:
- Around line 151-329: Consolidate the four repetitive tests
(TestExtractor_HypervisorOpenStackId, TestExtractor_HypervisorKubernetesId,
TestExtractor_HypervisorName, TestExtractor_StatusAggUUIDs) by creating a single
table-driven test that maps each index identifier (idxHypervisorOpenStackId,
idxHypervisorKubernetesId, idxHypervisorName, idxStatusAggUUIDs) to its set of
subtests and expected outputs; call indexFields once per top-level run (via
buildClient and indexFields) and obtain the extractor with extractorByField,
then iterate the subtests invoking the extractor and asserting with
strSliceEqual — this removes duplicated setup and loops while preserving all
assertions and still referencing indexFields and extractorByField to locate the
relevant code.
🪄 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: 58f55a5e-eab0-43f4-b355-f3e5411c48d7
📒 Files selected for processing (3)
internal/shim/placement/field_index.gointernal/shim/placement/field_index_test.gointernal/shim/placement/shim.go
Comment on lines
+42
to
+45
| hv, ok := obj.(*hv1.Hypervisor) | ||
| if !ok { | ||
| log.Error(errors.New("unexpected type"), "object", obj) | ||
| return nil |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Find malformed logr Error calls where the object is passed
# as an unmatched structured logging argument.
rg -nP 'log\.Error\(errors\.New\("unexpected type"\),\s*"object",\s*obj\)' --type goRepository: cobaltcore-dev/cortex
Length of output: 468
🏁 Script executed:
head -20 internal/shim/placement/field_index.goRepository: cobaltcore-dev/cortex
Length of output: 668
🏁 Script executed:
sed -n '40,50p' internal/shim/placement/field_index.goRepository: cobaltcore-dev/cortex
Length of output: 382
🏁 Script executed:
sed -n '55,65p' internal/shim/placement/field_index.goRepository: cobaltcore-dev/cortex
Length of output: 468
🏁 Script executed:
sed -n '70,80p' internal/shim/placement/field_index.goRepository: cobaltcore-dev/cortex
Length of output: 402
🏁 Script executed:
sed -n '82,92p' internal/shim/placement/field_index.goRepository: cobaltcore-dev/cortex
Length of output: 384
Fix malformed structured logging in the type-guard branches.
log.Error(err, msg, keysAndValues...) currently treats "object" as the message and leaves obj as an unmatched key/value argument. Use a real message plus a key/value pair.
Proposed fix
- log.Error(errors.New("unexpected type"), "object", obj)
+ log.Error(errors.New("unexpected type"), "unexpected object type", "object", obj)Apply the same change to each extractor's non-*hv1.Hypervisor branch at lines 44, 60, 73, and 86.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| hv, ok := obj.(*hv1.Hypervisor) | |
| if !ok { | |
| log.Error(errors.New("unexpected type"), "object", obj) | |
| return nil | |
| hv, ok := obj.(*hv1.Hypervisor) | |
| if !ok { | |
| log.Error(errors.New("unexpected type"), "unexpected object type", "object", obj) | |
| return nil |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/shim/placement/field_index.go` around lines 42 - 45, The structured
log call in the type-guard branches is malformed: change calls like
log.Error(errors.New("unexpected type"), "object", obj) to supply a proper
message string as the second parameter and then key/value pairs, e.g.
log.Error(errors.New("unexpected type"), "unexpected type in extractor",
"object", obj, "expected", "*hv1.Hypervisor"); update the non-*hv1.Hypervisor
branches in the extractors (the branches guarding hv, and the similar checks at
the other extractor locations referenced in the diff) so each log.Error follows
this pattern at the four spots mentioned.
Comment on lines
+89
to
+93
| var uuids []string | ||
| for _, agg := range hv.Status.Aggregates { | ||
| uuids = append(uuids, agg.UUID) | ||
| } | ||
| return uuids |
There was a problem hiding this comment.
Skip empty aggregate UUIDs before indexing.
An empty status.aggregates[*].uuid is not a useful lookup key and can pollute the cache index under ""; this index should mirror the OpenStack ID behavior and only register populated IDs.
Proposed fix
- var uuids []string
+ uuids := make([]string, 0, len(hv.Status.Aggregates))
for _, agg := range hv.Status.Aggregates {
+ if agg.UUID == "" {
+ continue
+ }
uuids = append(uuids, agg.UUID)
}
return uuids📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| var uuids []string | |
| for _, agg := range hv.Status.Aggregates { | |
| uuids = append(uuids, agg.UUID) | |
| } | |
| return uuids | |
| uuids := make([]string, 0, len(hv.Status.Aggregates)) | |
| for _, agg := range hv.Status.Aggregates { | |
| if agg.UUID == "" { | |
| continue | |
| } | |
| uuids = append(uuids, agg.UUID) | |
| } | |
| return uuids |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/shim/placement/field_index.go` around lines 89 - 93, The current
loop in the function that builds the aggregate UUID slice (iterating
hv.Status.Aggregates and appending agg.UUID) must skip empty UUID values so the
index does not include "" entries; update the loop that constructs uuids to only
append agg.UUID when agg.UUID != "" (or len(agg.UUID) > 0), preserving the
existing return of uuids so the result mirrors OpenStack behavior and contains
only populated IDs.
2 tasks
3 tasks
PhilippMatthes
pushed a commit
that referenced
this pull request
Apr 24, 2026
## Summary - Updated the Placement API Shim section in docs/architecture.md to reflect the current implementation after PRs #723, #724, #725, and #729 - Added documentation for feature flags (enableResourceProviders, enableRoot, enableTraits) that gate each capability - Added CRD-Backed Resource Providers subsection covering the full CRUD surface, merge logic with upstream, collision detection, and field indexing - Added Traits subsection documenting the dual ConfigMap approach (static Helm-managed + dynamic shim-managed) and Lease-backed distributed locking - Added Authentication subsection covering Keystone token validation middleware, first-match policy evaluation, project-scoped roles, and token caching with singleflight deduplication - Updated the architecture diagram to include the auth middleware, Keystone, and ConfigMap data stores - Documented non-fatal upstream connectivity (standalone CRD-backed boot mode) ## Motivation The existing docs described only the passthrough concept but none of the CRD-backed endpoints, authentication layer, traits store, or feature flags that were implemented this week. A developer reading the architecture doc would get a misleading picture of the shim's capabilities. Co-authored-by: Claude <claude@anthropic.com> Co-authored-by: Claude Opus 4.7 <noreply@anthropic.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
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.
This adds kubernetes indexes to the placement api shim which will be needed by the endpoints to make quick hypervisor lookups.