Fix empty HypervisorID leak in resource provider listing#755
Closed
sapcc-bot wants to merge 2 commits into
Closed
Conversation
…, auth, and locking Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…rvisorID Hypervisors that have not yet been registered with OpenStack have an empty Status.HypervisorID. HandleListResourceProviders fetches all hypervisors via s.List() without filtering these out, causing them to appear in the merged resource provider list with an empty UUID. This violates the Placement API contract and can confuse downstream clients. Other handlers (Show, Update, Delete, Create) use field-index lookups that naturally skip empty-ID hypervisors, so they are not affected. Filter out hypervisors with empty HypervisorID immediately after the List call, before any query-parameter filtering or merging. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
sapcc-bot
requested review from
PhilippMatthes,
SoWieMarkus,
auhlig,
juliusclausnitzer,
mblos and
umswmayj
as code owners
Contributor
Test Coverage ReportTest Coverage 📊: 70.0% |
|
Caution Review failedPull request was closed or merged during review 📝 WalkthroughWalkthroughThe pull request updates placement API shim documentation with expanded coverage of authentication, authorization, distributed locking, and feature flags. Concurrently, the list resource provider handler filters out Kubernetes hypervisors lacking a non-empty Status.HypervisorID before merging results with upstream data. A test validates this filtering behavior. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
Member
|
Test PR for claude workflow. |
PhilippMatthes
deleted the
claude/fix-empty-hypervisor-id-in-list-resource-providers
branch
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
4 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.
Summary
What the bug is
Hypervisors that have not yet been registered with OpenStack have an empty Status.HypervisorID. When HandleListResourceProviders merges Kubernetes hypervisors with upstream placement results, it includes these unregistered hypervisors, producing resource provider entries with empty UUIDs in the API response.
How it was found
Weekly review of recent changes (PR #724 introducing CRD-backed resource provider APIs).
What the fix does
Filters out hypervisors with empty HypervisorID immediately after the List call, before query-parameter filtering or merging with upstream results. Adds a test case covering this scenario.
Test plan
🤖 Generated with Claude Code