feat(nova): probe os_type for KVM servers during server sync#886
Merged
Conversation
Integrate liquidapi.OSTypeProber into the nova datasource to automatically determine os_type for KVM-flavored servers at sync time. The prober resolves os_type from Glance image metadata (vmware_ostype property or ostype: tags). - Add osTypeProber to novaAPI, initialized during Init() for servers datasource - Add probeOSTypes/probeOSType methods that iterate KVM servers sequentially (prober is not goroutine-safe due to internal map cache) - Add OSType field to Server and Image types - Add GetAllImages API method with deriveOSType logic for the images datasource - Remove ProbeOSType from NovaAPI interface (now internal to GetAllServers) - Add initOSTypeProber with panic recovery for environments without Glance
umswmayj
requested review from
PhilippMatthes,
SoWieMarkus,
auhlig,
juliusclausnitzer and
mblos
as code owners
|
Warning Review limit reached
More reviews will be available in 45 minutes and 57 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, 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 include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (7)
📝 WalkthroughWalkthroughThis PR adds optional OS type probing for OpenStack Nova KVM servers by introducing an OSType field to the Server model, migrating the database table from v3 to v4, implementing OS type detection during datasource initialization, and updating all dependent SQL queries and usage calculations to reference the new table schema. ChangesOpenStack Nova OS Type Support and Server Table Migration
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/knowledge/datasources/plugins/openstack/nova/nova_api.go (1)
34-35: ⚡ Quick winFix inconsistent concurrency wording for OS type probing.
These comments say probing is concurrent, but
probeOSTypescurrently runs sequentially. Please update wording to avoid misleading behavior expectations.Suggested wording-only diff
- // For KVM flavors, os_type is probed concurrently using the OSTypeProber. + // For KVM flavors, os_type is probed using the OSTypeProber. @@ - // Probe OS type concurrently for KVM servers. + // Probe OS type for KVM servers.Also applies to: 170-171
🤖 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 `@internal/knowledge/datasources/plugins/openstack/nova/nova_api.go` around lines 34 - 35, The comment incorrectly states OS type probing is concurrent; update the comments near GetAllServers and the probeOSTypes implementation to remove or change "concurrently" to reflect the actual sequential behavior (e.g., "OS type is probed using the OSTypeProber" or "probed sequentially by OSTypeProber"). Locate the comments referencing concurrency around the GetAllServers declaration and the comment above probeOSTypes (and the second occurrence around lines ~170-171) and adjust wording so it no longer implies concurrent execution.
🤖 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.
Nitpick comments:
In `@internal/knowledge/datasources/plugins/openstack/nova/nova_api.go`:
- Around line 34-35: The comment incorrectly states OS type probing is
concurrent; update the comments near GetAllServers and the probeOSTypes
implementation to remove or change "concurrently" to reflect the actual
sequential behavior (e.g., "OS type is probed using the OSTypeProber" or "probed
sequentially by OSTypeProber"). Locate the comments referencing concurrency
around the GetAllServers declaration and the comment above probeOSTypes (and the
second occurrence around lines ~170-171) and adjust wording so it no longer
implies concurrent execution.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: af14a324-0059-4251-be0c-ef1ab7d1ceaf
📒 Files selected for processing (9)
internal/knowledge/datasources/plugins/openstack/nova/nova_api.gointernal/knowledge/datasources/plugins/openstack/nova/nova_sync.gointernal/knowledge/datasources/plugins/openstack/nova/nova_types.gointernal/knowledge/extractor/plugins/compute/libvirt_domain_cpu_steal_pct.sqlinternal/knowledge/extractor/plugins/compute/vm_host_residency.sqlinternal/knowledge/extractor/plugins/compute/vm_life_span.sqlinternal/knowledge/extractor/plugins/compute/vrops_hostsystem_resolver.sqlinternal/knowledge/extractor/plugins/compute/vrops_project_noisiness.sqlinternal/scheduling/reservations/commitments/usage.go
umswmayj
force-pushed
the
feat/ostype-prober
branch
from
4531257 to
9024b1d
Compare
Contributor
Test Coverage ReportTest Coverage 📊: 69.6% |
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.
No description provided.