Skip to content

Commit 44ca732

Browse files
ronenavronenav
committed
ECOPROJECT-4328 | fix: require VI SDK UUID and fix inventory snapshot version detection
Signed-off-by: Ronen Avraham <ravraham@redhat.com>
1 parent d476f5c commit 44ca732

File tree

5 files changed

+196
-54
lines changed

5 files changed

+196
-54
lines changed

‎internal/util/util.go‎

Lines changed: 10 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -151,16 +151,19 @@ func BoolPtr(b bool) *bool {
151151
return &b
152152
}
153153

154-
// Unmarshal does not return error when v1 inventory is unmarshal into a v2 struct.
155-
// The only way to differentiate the version is to check the internal structure.
154+
// GetInventoryVersion returns v2 if JSON has top-level "clusters" or "vcenter_id"; else v1.
156155
func GetInventoryVersion(inventory []byte) int {
157-
i := v1alpha1.Inventory{}
158-
_ = json.Unmarshal(inventory, &i)
159-
160-
if i.VcenterId == "" {
156+
var keys map[string]json.RawMessage
157+
if err := json.Unmarshal(inventory, &keys); err != nil {
161158
return model.SnapshotVersionV1
162159
}
163-
return model.SnapshotVersionV2
160+
if _, ok := keys["clusters"]; ok {
161+
return model.SnapshotVersionV2
162+
}
163+
if _, ok := keys["vcenter_id"]; ok {
164+
return model.SnapshotVersionV2
165+
}
166+
return model.SnapshotVersionV1
164167
}
165168

166169
// ErrEmptyInventory indicates the inventory data is empty

‎internal/util/util_test.go‎

Lines changed: 72 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,72 @@
1+
package util
2+
3+
import (
4+
"testing"
5+
6+
"github.com/kubev2v/migration-planner/internal/store/model"
7+
)
8+
9+
func TestGetInventoryVersion(t *testing.T) {
10+
t.Parallel()
11+
tests := []struct {
12+
name string
13+
json string
14+
want int
15+
}{
16+
{
17+
name: "legacy v1 shape without top-level clusters or vcenter_id",
18+
json: `{"vms":{"total":10},"infra":{"totalHosts":5},"vcenter":{"id":"x","name":"n"}}`,
19+
want: model.SnapshotVersionV1,
20+
},
21+
{
22+
name: "v2 with clusters and vcenter_id",
23+
json: `{"vcenter_id":"uuid","vcenter":{"vms":{"total":1},"infra":{}},"clusters":{}}`,
24+
want: model.SnapshotVersionV2,
25+
},
26+
{
27+
name: "v2 with empty vcenter_id and clusters",
28+
json: `{"vcenter_id":"","vcenter":{"vms":{"total":5},"infra":{"totalHosts":2}},"clusters":{"c1":{"vms":{"total":5},"infra":{}}}}`,
29+
want: model.SnapshotVersionV2,
30+
},
31+
{
32+
name: "v2 with vcenter_id only no clusters key",
33+
json: `{"vcenter_id":"test-vcenter","vcenter":{"vms":{"total":10},"infra":{"totalHosts":5}}}`,
34+
want: model.SnapshotVersionV2,
35+
},
36+
{
37+
name: "v2 with clusters only",
38+
json: `{"vcenter":{"vms":{"total":1},"infra":{}},"clusters":{}}`,
39+
want: model.SnapshotVersionV2,
40+
},
41+
{
42+
name: "empty input defaults to v1",
43+
json: "",
44+
want: model.SnapshotVersionV1,
45+
},
46+
{
47+
name: "invalid JSON defaults to v1",
48+
json: `{`,
49+
want: model.SnapshotVersionV1,
50+
},
51+
{
52+
name: "JSON null unmarshals to nil map defaults to v1",
53+
json: `null`,
54+
want: model.SnapshotVersionV1,
55+
},
56+
}
57+
for _, tt := range tests {
58+
t.Run(tt.name, func(t *testing.T) {
59+
t.Parallel()
60+
if got := GetInventoryVersion([]byte(tt.json)); got != tt.want {
61+
t.Fatalf("GetInventoryVersion() = %d, want %d", got, tt.want)
62+
}
63+
})
64+
}
65+
}
66+
67+
func TestGetInventoryVersion_nilSlice(t *testing.T) {
68+
t.Parallel()
69+
if got := GetInventoryVersion(nil); got != model.SnapshotVersionV1 {
70+
t.Fatalf("GetInventoryVersion(nil) = %d, want v1", got)
71+
}
72+
}

‎pkg/duckdb_parser/inventory_builder_test.go‎

Lines changed: 37 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -472,26 +472,47 @@ func TestValidation_ErrorCodes(t *testing.T) {
472472
forbiddenCodes: []string{CodeNoVMs},
473473
},
474474
{
475-
name: "missing VM ID column and empty VM values reports both errors",
476-
customHeaders: []string{"VM", "Host", "CPUs", "Memory", "Powerstate", "Cluster", "Datacenter"},
477-
vms: []map[string]string{{"VM": "", "Host": "esxi-host-1", "CPUs": "4", "Memory": "8192", "Powerstate": "poweredOn", "Cluster": "cluster1", "Datacenter": "dc1"}},
478-
expectedCodes: []string{CodeMissingVMID, CodeMissingVMName},
475+
name: "missing VM ID column and empty VM values reports query error and missing name",
476+
customHeaders: []string{"VM", "VI SDK UUID", "Host", "CPUs", "Memory", "Powerstate", "Cluster", "Datacenter"},
477+
vms: []map[string]string{{"VM": "", "VI SDK UUID": "550e8400-e29b-41d4-a716-446655440000", "Host": "esxi-host-1", "CPUs": "4", "Memory": "8192", "Powerstate": "poweredOn", "Cluster": "cluster1", "Datacenter": "dc1"}},
478+
expectedCodes: []string{CodeColumnValidationFailed, CodeMissingVMName},
479479
forbiddenCodes: []string{CodeNoVMs},
480480
},
481481
{
482482
name: "missing Cluster column reports MISSING_CLUSTER",
483-
customHeaders: []string{"VM", "VM ID", "Host", "CPUs", "Memory", "Powerstate", "Datacenter"},
484-
vms: []map[string]string{{"VM": "vm-1", "VM ID": "vm-001", "Host": "esxi-host-1", "CPUs": "4", "Memory": "8192", "Powerstate": "poweredOn", "Datacenter": "dc1"}},
483+
customHeaders: []string{"VM", "VM ID", "VI SDK UUID", "Host", "CPUs", "Memory", "Powerstate", "Datacenter"},
484+
vms: []map[string]string{{"VM": "vm-1", "VM ID": "vm-001", "VI SDK UUID": "550e8400-e29b-41d4-a716-446655440000", "Host": "esxi-host-1", "CPUs": "4", "Memory": "8192", "Powerstate": "poweredOn", "Datacenter": "dc1"}},
485485
expectedCodes: []string{CodeMissingCluster},
486486
forbiddenCodes: []string{CodeNoVMs, CodeMissingVMID, CodeMissingVMName},
487487
},
488488
{
489489
name: "empty Cluster values reports MISSING_CLUSTER",
490-
customHeaders: []string{"VM", "VM ID", "Host", "CPUs", "Memory", "Powerstate", "Cluster", "Datacenter"},
491-
vms: []map[string]string{{"VM": "vm-1", "VM ID": "vm-001", "Host": "esxi-host-1", "CPUs": "4", "Memory": "8192", "Powerstate": "poweredOn", "Cluster": "", "Datacenter": "dc1"}},
490+
customHeaders: []string{"VM", "VM ID", "VI SDK UUID", "Host", "CPUs", "Memory", "Powerstate", "Cluster", "Datacenter"},
491+
vms: []map[string]string{{"VM": "vm-1", "VM ID": "vm-001", "VI SDK UUID": "550e8400-e29b-41d4-a716-446655440000", "Host": "esxi-host-1", "CPUs": "4", "Memory": "8192", "Powerstate": "poweredOn", "Cluster": "", "Datacenter": "dc1"}},
492492
expectedCodes: []string{CodeMissingCluster},
493493
forbiddenCodes: []string{CodeNoVMs, CodeMissingVMID, CodeMissingVMName},
494494
},
495+
{
496+
name: "missing VI SDK UUID column reports MISSING_VI_SDK_UUID",
497+
customHeaders: []string{"VM", "VM ID", "Host", "CPUs", "Memory", "Powerstate", "Cluster", "Datacenter"},
498+
vms: []map[string]string{{"VM": "vm-1", "VM ID": "vm-001", "Host": "esxi-host-1", "CPUs": "4", "Memory": "8192", "Powerstate": "poweredOn", "Cluster": "cluster1", "Datacenter": "dc1"}},
499+
expectedCodes: []string{CodeMissingVISDKUUID},
500+
forbiddenCodes: []string{CodeNoVMs},
501+
},
502+
{
503+
name: "empty VI SDK UUID values report MISSING_VI_SDK_UUID",
504+
customHeaders: []string{"VM", "VM ID", "VI SDK UUID", "Host", "CPUs", "Memory", "Powerstate", "Cluster", "Datacenter"},
505+
vms: []map[string]string{{"VM": "vm-1", "VM ID": "vm-001", "VI SDK UUID": "", "Host": "esxi-host-1", "CPUs": "4", "Memory": "8192", "Powerstate": "poweredOn", "Cluster": "cluster1", "Datacenter": "dc1"}},
506+
expectedCodes: []string{CodeMissingVISDKUUID},
507+
forbiddenCodes: []string{CodeNoVMs},
508+
},
509+
{
510+
name: "whitespace-only VI SDK UUID values report MISSING_VI_SDK_UUID",
511+
customHeaders: []string{"VM", "VM ID", "VI SDK UUID", "Host", "CPUs", "Memory", "Powerstate", "Cluster", "Datacenter"},
512+
vms: []map[string]string{{"VM": "vm-1", "VM ID": "vm-001", "VI SDK UUID": " ", "Host": "esxi-host-1", "CPUs": "4", "Memory": "8192", "Powerstate": "poweredOn", "Cluster": "cluster1", "Datacenter": "dc1"}},
513+
expectedCodes: []string{CodeMissingVISDKUUID},
514+
forbiddenCodes: []string{CodeNoVMs},
515+
},
495516
}
496517

497518
for _, tt := range tests {
@@ -751,18 +772,18 @@ func TestBuildInventory_MigratableWithWarnings(t *testing.T) {
751772
assert.Equal(t, 2, inv.VCenter.VMs.TotalMigratableWithWarnings, "VMs with warnings should be counted")
752773
}
753774

754-
// TestBuildInventory_MinimalSchema tests that a file with only VM ID, VM, and Cluster
755-
// columns (the minimal required schema) correctly ingests VMs and builds an inventory.
775+
// TestBuildInventory_MinimalSchema tests that a file with only VM ID, VM, Cluster, and VI SDK UUID
776+
// columns (the minimal required schema for RVTools) correctly ingests VMs and builds an inventory.
756777
func TestBuildInventory_MinimalSchema(t *testing.T) {
757778
parser, _, cleanup := setupTestParser(t, &testValidator{})
758779
defer cleanup()
759780

760-
// Create Excel with only the minimal required columns
761-
minimalHeaders := []string{"VM ID", "VM", "Cluster"}
781+
vcUUID := "550e8400-e29b-41d4-a716-446655440000"
782+
minimalHeaders := []string{"VM ID", "VM", "Cluster", "VI SDK UUID"}
762783
vms := []map[string]string{
763-
{"VM ID": "vm-100", "VM": "VM-SRV-0000", "Cluster": "cluster-1"},
764-
{"VM ID": "vm-101", "VM": "VM-SRV-0001", "Cluster": "cluster-1"},
765-
{"VM ID": "vm-102", "VM": "VM-SRV-0002", "Cluster": "cluster-1"},
784+
{"VM ID": "vm-100", "VM": "VM-SRV-0000", "Cluster": "cluster-1", "VI SDK UUID": vcUUID},
785+
{"VM ID": "vm-101", "VM": "VM-SRV-0001", "Cluster": "cluster-1", "VI SDK UUID": vcUUID},
786+
{"VM ID": "vm-102", "VM": "VM-SRV-0002", "Cluster": "cluster-1", "VI SDK UUID": vcUUID},
766787
}
767788

768789
tmpFile := createTestExcelWithCustomHeaders(t, minimalHeaders, vms)
@@ -775,6 +796,7 @@ func TestBuildInventory_MinimalSchema(t *testing.T) {
775796
inv, err := parser.BuildInventory(ctx)
776797
require.NoError(t, err)
777798
assert.Equal(t, 3, inv.VCenter.VMs.Total, "Should have 3 VMs from minimal schema")
799+
assert.Equal(t, vcUUID, inv.VCenterID, "VCenterID should be populated from VI SDK UUID column")
778800
}
779801

780802
// TestBuildInventory_VMsWithSharedDisksCount ingests Excel with vDisk data and asserts

‎pkg/duckdb_parser/queries.go‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,8 @@ package duckdb_parser
22

33
import (
44
"context"
5+
"database/sql"
6+
"errors"
57
"fmt"
68

79
"github.com/georgysavva/scany/v2/sqlscan"
@@ -433,6 +435,9 @@ func (p *Parser) VCenterID(ctx context.Context) (string, error) {
433435
}
434436
var vcenterID string
435437
if err := p.db.QueryRowContext(ctx, q).Scan(&vcenterID); err != nil {
438+
if errors.Is(err, sql.ErrNoRows) {
439+
return "", nil
440+
}
436441
return "", fmt.Errorf("scanning vcenter id: %w", err)
437442
}
438443
return vcenterID, nil

‎pkg/duckdb_parser/schema_validation.go‎

Lines changed: 72 additions & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,12 @@ package duckdb_parser
22

33
import (
44
"context"
5+
"database/sql"
6+
"errors"
57
"fmt"
8+
"strings"
9+
10+
"go.uber.org/zap"
611
)
712

813
// ValidationResult contains errors and warnings from schema validation.
@@ -50,10 +55,12 @@ func (v ValidationResult) Error() error {
5055

5156
// Validation codes for errors
5257
const (
53-
CodeNoVMs = "NO_VMS"
54-
CodeMissingVMID = "MISSING_VM_ID"
55-
CodeMissingVMName = "MISSING_VM_NAME"
56-
CodeMissingCluster = "MISSING_CLUSTER"
58+
CodeNoVMs = "NO_VMS"
59+
CodeMissingVMID = "MISSING_VM_ID"
60+
CodeMissingVMName = "MISSING_VM_NAME"
61+
CodeMissingCluster = "MISSING_CLUSTER"
62+
CodeMissingVISDKUUID = "MISSING_VI_SDK_UUID"
63+
CodeColumnValidationFailed = "COLUMN_VALIDATION_FAILED"
5764
)
5865

5966
// Validation codes for warnings
@@ -100,57 +107,79 @@ func (p *Parser) ValidateSchema(ctx context.Context, table string) ValidationRes
100107
// Reports NO_VMS if table has no data rows, otherwise reports specific column errors.
101108
// The table parameter allows validating against "vinfo_raw" (RVTools) or "vinfo" (SQLite).
102109
func (p *Parser) validateVinfoData(ctx context.Context, result *ValidationResult, table string) {
103-
// First check if we have any rows at all (independent of column existence)
104-
if p.countRows(ctx, fmt.Sprintf(`SELECT COUNT(*) FROM %s`, table)) == 0 {
110+
rowCount, err := p.countRowsErr(ctx, fmt.Sprintf(`SELECT COUNT(*) FROM %s`, table))
111+
if err != nil {
105112
result.Errors = append(result.Errors, ValidationIssue{
106-
Code: CodeNoVMs,
113+
Code: CodeColumnValidationFailed,
107114
Table: table,
108-
Message: "No VMs found in vInfo sheet - cannot build inventory without VM data",
115+
Message: fmt.Sprintf("could not read vInfo table: %v", err),
109116
})
110117
return
111118
}
112-
113-
// Rows exist — check each required column independently.
114-
// hasValidColumnData returns false if column is missing OR has no valid values.
115-
if !p.hasValidColumnData(ctx, table, "VM ID") {
119+
if rowCount == 0 {
116120
result.Errors = append(result.Errors, ValidationIssue{
117-
Code: CodeMissingVMID,
121+
Code: CodeNoVMs,
118122
Table: table,
119-
Column: "VM ID",
120-
Message: "'VM ID' column is missing or empty in the vInfo sheet - VMs cannot be identified",
123+
Message: "No VMs found in vInfo sheet - cannot build inventory without VM data",
121124
})
125+
return
126+
}
127+
128+
p.appendNonEmptyColumnIssue(ctx, result, table, "VM ID", CodeMissingVMID, "'VM ID' column is missing or empty in the vInfo sheet - VMs cannot be identified")
129+
p.appendNonEmptyColumnIssue(ctx, result, table, "VM", CodeMissingVMName, "'VM' column is missing or empty in the vInfo sheet - VMs cannot be identified")
130+
p.appendNonEmptyColumnIssue(ctx, result, table, "Cluster", CodeMissingCluster, "'Cluster' column is missing or empty in the vInfo sheet - cannot determine cluster membership")
131+
132+
if table == "vinfo_raw" {
133+
p.appendNonEmptyColumnIssue(ctx, result, table, "VI SDK UUID", CodeMissingVISDKUUID, "The RVTools export is missing the 'VI SDK UUID' column on the vInfo sheet, or all values are empty. That column is required to identify your vCenter. Re-export the file using RVTools so the vInfo sheet includes a non-empty VI SDK UUID for at least one VM.")
122134
}
135+
}
123136

124-
if !p.hasValidColumnData(ctx, table, "VM") {
137+
// appendNonEmptyColumnIssue appends a user-facing error if the column has no non-empty values,
138+
// or COLUMN_VALIDATION_FAILED if the database query fails (so query errors are not mistaken for bad data).
139+
func (p *Parser) appendNonEmptyColumnIssue(ctx context.Context, result *ValidationResult, table, column, code, emptyMsg string) {
140+
hasData, err := p.hasValidColumnData(ctx, table, column)
141+
if err != nil {
125142
result.Errors = append(result.Errors, ValidationIssue{
126-
Code: CodeMissingVMName,
143+
Code: CodeColumnValidationFailed,
127144
Table: table,
128-
Column: "VM",
129-
Message: "'VM' column is missing or empty in the vInfo sheet - VMs cannot be identified",
145+
Column: column,
146+
Message: fmt.Sprintf("could not validate column %q: %v", column, err),
130147
})
148+
return
131149
}
132-
133-
if !p.hasValidColumnData(ctx, table, "Cluster") {
150+
if !hasData {
134151
result.Errors = append(result.Errors, ValidationIssue{
135-
Code: CodeMissingCluster,
152+
Code: code,
136153
Table: table,
137-
Column: "Cluster",
138-
Message: "'Cluster' column is missing or empty in the vInfo sheet - cannot determine cluster membership",
154+
Column: column,
155+
Message: emptyMsg,
139156
})
140157
}
141158
}
142159

143160
// hasValidColumnData checks if a table has a column with at least one non-empty value.
144-
// Returns false if the column doesn't exist or has no valid values.
145-
func (p *Parser) hasValidColumnData(ctx context.Context, table, column string) bool {
161+
// Returns (false, nil) if the column is missing or empty; (false, err) if the query fails.
162+
func (p *Parser) hasValidColumnData(ctx context.Context, table, column string) (bool, error) {
146163
query := fmt.Sprintf(`SELECT COUNT(*) FROM %s WHERE "%s" IS NOT NULL AND TRIM("%s") != ''`, table, column, column)
147-
return p.countRows(ctx, query) > 0
164+
n, err := p.countRowsErr(ctx, query)
165+
if err != nil {
166+
return false, err
167+
}
168+
return n > 0, nil
148169
}
149170

150171
// validateOptionalTables checks that optional tables have data, producing warnings if empty.
151172
func (p *Parser) validateOptionalTables(ctx context.Context, result *ValidationResult) {
152173
for _, check := range warningChecks {
153-
if p.countRows(ctx, fmt.Sprintf(`SELECT COUNT(*) FROM %s`, check.table)) == 0 {
174+
n, err := p.countRowsErr(ctx, fmt.Sprintf(`SELECT COUNT(*) FROM %s`, check.table))
175+
if err != nil {
176+
// Log unexpected errors (connection, permissions); missing tables are expected
177+
if !isTableMissingError(err) {
178+
zap.S().Named("duckdb_parser").Warnf("Failed to validate optional table %s: %v", check.table, err)
179+
}
180+
continue
181+
}
182+
if n == 0 {
154183
result.Warnings = append(result.Warnings, ValidationIssue{
155184
Code: check.code,
156185
Table: check.table,
@@ -160,11 +189,22 @@ func (p *Parser) validateOptionalTables(ctx context.Context, result *ValidationR
160189
}
161190
}
162191

163-
// countRows executes a COUNT query and returns the result, or 0 on any error.
164-
func (p *Parser) countRows(ctx context.Context, query string) int {
192+
func (p *Parser) countRowsErr(ctx context.Context, query string) (int, error) {
165193
var count int
166194
if err := p.db.QueryRowContext(ctx, query).Scan(&count); err != nil {
167-
return 0
195+
return 0, err
196+
}
197+
return count, nil
198+
}
199+
200+
// isTableMissingError returns true if the error indicates a table doesn't exist.
201+
func isTableMissingError(err error) bool {
202+
if errors.Is(err, sql.ErrNoRows) {
203+
return true
168204
}
169-
return count
205+
errMsg := strings.ToLower(err.Error())
206+
return strings.Contains(errMsg, "table") &&
207+
(strings.Contains(errMsg, "not found") ||
208+
strings.Contains(errMsg, "does not exist") ||
209+
strings.Contains(errMsg, "no such table"))
170210
}

0 commit comments

Comments
 (0)