ECOPROJECT-4285 | feat: add datastore storage io information into the api#1184
ECOPROJECT-4285 | feat: add datastore storage io information into the api#1184itroyano wants to merge 1 commit into
Conversation
… api Signed-off-by: Igor Troyanovsky <itroyano@redhat.com> rh-pre-commit.version: 2.3.2 rh-pre-commit.check-secrets: ENABLED
itroyano
requested review from
amalimov and
nirarg
and removed request for
a team
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
📝 WalkthroughWalkthroughThis PR adds Storage I/O Control (SIOC) configuration support to the migration planner. It extends the API schema and data models, updates database ingestion pipelines to parse SIOC properties from source systems, implements queries with sensible defaults, constructs inventory objects, and validates the end-to-end flow with comprehensive tests. ChangesStorage I/O Control Configuration Support
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/duckdb_parser/templates/datastore_query.go.tmpl (1)
46-63:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFix non-deterministic HBA type derivation using proper aggregation.
Line 56 uses
FIRST(hba."Type") OVER (PARTITION BY w."Name")without an ORDER BY clause. DuckDB'sFIRST()function is non-deterministic without explicit ordering, producing unstable or NULL values that incorrectly classifyprotocolTypein the final query (line 74). Replace the window function with deterministic aggregation using GROUP BY.Suggested fix
with_hba AS ( - SELECT DISTINCT + SELECT w."Cluster", w."Address", w."Name", w."Free MiB", w."MHA", w."Capacity MiB", w."Type", w.ip, w."Object ID", - FIRST(hba."Type") OVER (PARTITION BY w."Name") as hba_type, + MAX(hba."Type") FILTER (WHERE hba."Type" IS NOT NULL) as hba_type, w."SIOC Enabled", w."SIOC Congestion Threshold", w."SIOC Congestion Threshold Mode", w."SIOC Percent Of Peak Throughput" FROM with_host w LEFT JOIN vhba hba ON hba."Device" = w.hba_device + GROUP BY + w."Cluster", w."Address", w."Name", w."Free MiB", w."MHA", w."Capacity MiB", + w."Type", w.ip, w."Object ID", + w."SIOC Enabled", w."SIOC Congestion Threshold", + w."SIOC Congestion Threshold Mode", w."SIOC Percent Of Peak Throughput" )🤖 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/duckdb_parser/templates/datastore_query.go.tmpl` around lines 46 - 63, The SELECT uses FIRST(hba."Type") OVER (PARTITION BY w."Name") (aliased as hba_type) which is non-deterministic without ORDER BY; replace this windowed FIRST with a deterministic aggregate and GROUP BY: remove the window function, add hba_type as an aggregate (e.g., MAX(hba."Type") or MIN(...) depending on desired precedence) and GROUP BY all non-aggregated w.* columns from the with_host row (w."Cluster", w."Address", w."Name", w."Free MiB", w."MHA", w."Capacity MiB", w."Type", w.ip, w."Object ID", and the SIOC fields) so that hba_type is computed deterministically from the LEFT JOIN to vhba.
🤖 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 `@pkg/duckdb_parser/templates/ingest_rvtools.go.tmpl`:
- Line 247: The template currently passes any non-empty "SIOC Congestion
Threshold Mode" through, which can produce values outside the API enum; replace
the COALESCE(NULLIF(TRIM("SIOC Congestion Threshold Mode"), ''), 'automatic')
expression with a normalization that lowercases and constrains values to either
'manual' or 'automatic' (e.g., use a CASE on LOWER(TRIM("SIOC Congestion
Threshold Mode")) returning 'manual' when it equals 'manual' and 'automatic'
otherwise) so the generated column always yields one of the API enum values.
In `@pkg/inventory/converters/to_api.go`:
- Around line 205-213: The StorageIoConfiguration block creates pointers to
fields of d.StorageIoConfiguration directly which is unsafe; extract each value
into local variables first (e.g., enabled := d.StorageIoConfiguration.Enabled,
congestion := d.StorageIoConfiguration.CongestionThreshold, percent :=
d.StorageIoConfiguration.PercentOfPeakThroughput and mode :=
api.StorageIoConfigurationCongestionThresholdMode(d.StorageIoConfiguration.CongestionThresholdMode))
and then assign &enabled, &congestion, &mode, &percent when building
ds.StorageIoConfiguration so pointers reference stable local variables instead
of fields directly.
---
Outside diff comments:
In `@pkg/duckdb_parser/templates/datastore_query.go.tmpl`:
- Around line 46-63: The SELECT uses FIRST(hba."Type") OVER (PARTITION BY
w."Name") (aliased as hba_type) which is non-deterministic without ORDER BY;
replace this windowed FIRST with a deterministic aggregate and GROUP BY: remove
the window function, add hba_type as an aggregate (e.g., MAX(hba."Type") or
MIN(...) depending on desired precedence) and GROUP BY all non-aggregated w.*
columns from the with_host row (w."Cluster", w."Address", w."Name", w."Free
MiB", w."MHA", w."Capacity MiB", w."Type", w.ip, w."Object ID", and the SIOC
fields) so that hba_type is computed deterministically from the LEFT JOIN to
vhba.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: 99f470f9-6f9c-40ab-bc6e-7955923362e9
📒 Files selected for processing (13)
api/v1alpha1/agent/spec.gen.goapi/v1alpha1/openapi.yamlapi/v1alpha1/spec.gen.goapi/v1alpha1/types.gen.gopkg/duckdb_parser/inventory_builder.gopkg/duckdb_parser/inventory_builder_test.gopkg/duckdb_parser/models/inventory.gopkg/duckdb_parser/templates/create_schema.go.tmplpkg/duckdb_parser/templates/datastore_query.go.tmplpkg/duckdb_parser/templates/ingest_rvtools.go.tmplpkg/duckdb_parser/templates/ingest_sqlite.go.tmplpkg/inventory/converters/to_api.gopkg/inventory/model.go
| "Type", | ||
| CASE WHEN LOWER(TRIM(COALESCE("SIOC Enabled", ''))) IN ('true', '1', 'yes') THEN true ELSE false END, | ||
| COALESCE(TRY_CAST("SIOC Congestion Threshold" AS INTEGER), 30), | ||
| COALESCE(NULLIF(TRIM("SIOC Congestion Threshold Mode"), ''), 'automatic'), |
There was a problem hiding this comment.
Normalize and constrain SIOC mode to the API enum values.
Line 247 passes any non-empty value through. That can emit values outside automatic|manual (including case variants), breaking the API contract.
Suggested fix
- COALESCE(NULLIF(TRIM("SIOC Congestion Threshold Mode"), ''), 'automatic'),
+ CASE
+ WHEN LOWER(TRIM(COALESCE("SIOC Congestion Threshold Mode", ''))) IN ('automatic', 'manual')
+ THEN LOWER(TRIM("SIOC Congestion Threshold Mode"))
+ ELSE 'automatic'
+ END,📝 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.
| COALESCE(NULLIF(TRIM("SIOC Congestion Threshold Mode"), ''), 'automatic'), | |
| CASE | |
| WHEN LOWER(TRIM(COALESCE("SIOC Congestion Threshold Mode", ''))) IN ('automatic', 'manual') | |
| THEN LOWER(TRIM("SIOC Congestion Threshold Mode")) | |
| ELSE 'automatic' | |
| END, |
🤖 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/duckdb_parser/templates/ingest_rvtools.go.tmpl` at line 247, The template
currently passes any non-empty "SIOC Congestion Threshold Mode" through, which
can produce values outside the API enum; replace the COALESCE(NULLIF(TRIM("SIOC
Congestion Threshold Mode"), ''), 'automatic') expression with a normalization
that lowercases and constrains values to either 'manual' or 'automatic' (e.g.,
use a CASE on LOWER(TRIM("SIOC Congestion Threshold Mode")) returning 'manual'
when it equals 'manual' and 'automatic' otherwise) so the generated column
always yields one of the API enum values.
| if d.StorageIoConfiguration != nil { | ||
| mode := api.StorageIoConfigurationCongestionThresholdMode(d.StorageIoConfiguration.CongestionThresholdMode) | ||
| ds.StorageIoConfiguration = &api.StorageIoConfiguration{ | ||
| Enabled: &d.StorageIoConfiguration.Enabled, | ||
| CongestionThreshold: &d.StorageIoConfiguration.CongestionThreshold, | ||
| CongestionThresholdMode: &mode, | ||
| PercentOfPeakThroughput: &d.StorageIoConfiguration.PercentOfPeakThroughput, | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify pointer aliasing pattern in converter mapping.
rg -n '(&d\.StorageIoConfiguration\.)|(StorageIoConfiguration:\s*&api\.StorageIoConfiguration)' pkg/inventory/converters/to_api.goRepository: kubev2v/migration-planner
Length of output: 301
🏁 Script executed:
# Get the full function context around lines 205-213
sed -n '190,220p' pkg/inventory/converters/to_api.goRepository: kubev2v/migration-planner
Length of output: 1223
🏁 Script executed:
# Check the function signature and parameter types
head -n 250 pkg/inventory/converters/to_api.go | tail -n 100Repository: kubev2v/migration-planner
Length of output: 2908
🏁 Script executed:
# Search for similar aliasing patterns in the same converter file
rg -n '&d\.' pkg/inventory/converters/to_api.go | head -20Repository: kubev2v/migration-planner
Length of output: 365
🏁 Script executed:
# Look for related tests for this converter function
find . -name '*to_api*test*.go' -o -name '*converter*test*.go' | grep -E 'inventory|converter'Repository: kubev2v/migration-planner
Length of output: 111
🏁 Script executed:
# View the test file to understand coverage and patterns
head -n 100 pkg/inventory/converters/to_api_test.goRepository: kubev2v/migration-planner
Length of output: 2642
🏁 Script executed:
# Find type definitions for StorageIoConfiguration in inventory package
find pkg/inventory -name '*.go' | xargs grep -l 'type StorageIoConfiguration' | head -5Repository: kubev2v/migration-planner
Length of output: 92
🏁 Script executed:
# Look for the complete type definition
rg -A 10 'type StorageIoConfiguration struct' pkg/inventoryRepository: kubev2v/migration-planner
Length of output: 613
🏁 Script executed:
# Search for usage of the toAPIInfra function result
rg -B 3 -A 3 'toAPIInfra' pkg/inventory/converters/to_api.goRepository: kubev2v/migration-planner
Length of output: 386
🏁 Script executed:
# Check the return type and how it's propagated
rg -A 5 'func ToAPI' pkg/inventory/converters/to_api.go | head -20Repository: kubev2v/migration-planner
Length of output: 270
🏁 Script executed:
# Search for concurrent access patterns or channel usage with API structs
rg -n 'go ' pkg/inventory/converters/to_api.goRepository: kubev2v/migration-planner
Length of output: 51
🏁 Script executed:
# Check if the Datastore or API objects are modified after conversion
rg -n 'StorageIoConfiguration' pkg/inventory/converters/ --type goRepository: kubev2v/migration-planner
Length of output: 733
Extract values to local variables before taking addresses for StorageIoConfiguration fields.
This block directly addresses fields from the source struct, inconsistent with the safe pattern used elsewhere in this same function (e.g., lines 212–214 for HostId, lines 217–225 for host CPU/memory fields). Extract the values to locals first, then take addresses of the locals.
Suggested fix
if d.StorageIoConfiguration != nil {
+ enabled := d.StorageIoConfiguration.Enabled
+ congestionThreshold := d.StorageIoConfiguration.CongestionThreshold
mode := api.StorageIoConfigurationCongestionThresholdMode(d.StorageIoConfiguration.CongestionThresholdMode)
+ percentOfPeakThroughput := d.StorageIoConfiguration.PercentOfPeakThroughput
ds.StorageIoConfiguration = &api.StorageIoConfiguration{
- Enabled: &d.StorageIoConfiguration.Enabled,
- CongestionThreshold: &d.StorageIoConfiguration.CongestionThreshold,
+ Enabled: &enabled,
+ CongestionThreshold: &congestionThreshold,
CongestionThresholdMode: &mode,
- PercentOfPeakThroughput: &d.StorageIoConfiguration.PercentOfPeakThroughput,
+ PercentOfPeakThroughput: &percentOfPeakThroughput,
}
}📝 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.
| if d.StorageIoConfiguration != nil { | |
| mode := api.StorageIoConfigurationCongestionThresholdMode(d.StorageIoConfiguration.CongestionThresholdMode) | |
| ds.StorageIoConfiguration = &api.StorageIoConfiguration{ | |
| Enabled: &d.StorageIoConfiguration.Enabled, | |
| CongestionThreshold: &d.StorageIoConfiguration.CongestionThreshold, | |
| CongestionThresholdMode: &mode, | |
| PercentOfPeakThroughput: &d.StorageIoConfiguration.PercentOfPeakThroughput, | |
| } | |
| } | |
| if d.StorageIoConfiguration != nil { | |
| enabled := d.StorageIoConfiguration.Enabled | |
| congestionThreshold := d.StorageIoConfiguration.CongestionThreshold | |
| mode := api.StorageIoConfigurationCongestionThresholdMode(d.StorageIoConfiguration.CongestionThresholdMode) | |
| percentOfPeakThroughput := d.StorageIoConfiguration.PercentOfPeakThroughput | |
| ds.StorageIoConfiguration = &api.StorageIoConfiguration{ | |
| Enabled: &enabled, | |
| CongestionThreshold: &congestionThreshold, | |
| CongestionThresholdMode: &mode, | |
| PercentOfPeakThroughput: &percentOfPeakThroughput, | |
| } | |
| } |
🤖 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/inventory/converters/to_api.go` around lines 205 - 213, The
StorageIoConfiguration block creates pointers to fields of
d.StorageIoConfiguration directly which is unsafe; extract each value into local
variables first (e.g., enabled := d.StorageIoConfiguration.Enabled, congestion
:= d.StorageIoConfiguration.CongestionThreshold, percent :=
d.StorageIoConfiguration.PercentOfPeakThroughput and mode :=
api.StorageIoConfigurationCongestionThresholdMode(d.StorageIoConfiguration.CongestionThresholdMode))
and then assign &enabled, &congestion, &mode, &percent when building
ds.StorageIoConfiguration so pointers reference stable local variables instead
of fields directly.
1 participant
Signed-off-by: Igor Troyanovsky itroyano@redhat.com
rh-pre-commit.version: 2.3.2
rh-pre-commit.check-secrets: ENABLED
Assisted-by: Claude Opus 4.6
Summary by CodeRabbit
Release Notes
New Features
Tests