-
Notifications
You must be signed in to change notification settings - Fork 23
ECOPROJECT-4839 | feat: add collection_id to the duck_db ingestion logic #1304
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -2,6 +2,7 @@ package duckdb_parser | |||||||||||||
|
|
||||||||||||||
| import ( | ||||||||||||||
| "context" | ||||||||||||||
| "crypto/md5" | ||||||||||||||
| "database/sql" | ||||||||||||||
| "fmt" | ||||||||||||||
| "path/filepath" | ||||||||||||||
|
|
@@ -389,3 +390,134 @@ func TestIngestSqlite_PopulateVCluster_SkipsWhenAlreadyPopulated(t *testing.T) { | |||||||||||||
| require.NoError(t, db.QueryRowContext(ctx, `SELECT "Object ID" FROM vcluster WHERE "Name" = 'existing-cluster'`).Scan(&objectID)) | ||||||||||||||
| assert.Equal(t, "sentinel-id", objectID, "pre-existing row must not be overwritten") | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| // addCollectionColumns adds vmmoid and collection_id columns to vinfo so that | ||||||||||||||
| // IngestSqliteWithCollection can write to them. In production these columns are | ||||||||||||||
| // added by agent migration 026; in tests we add them manually. | ||||||||||||||
| func addCollectionColumns(t *testing.T, db *sql.DB) { | ||||||||||||||
| t.Helper() | ||||||||||||||
| ctx := context.Background() | ||||||||||||||
| _, err := db.ExecContext(ctx, `ALTER TABLE vinfo ADD COLUMN IF NOT EXISTS vmmoid VARCHAR`) | ||||||||||||||
| require.NoError(t, err, "adding vmmoid column to vinfo") | ||||||||||||||
| _, err = db.ExecContext(ctx, `ALTER TABLE vinfo ADD COLUMN IF NOT EXISTS collection_id BIGINT`) | ||||||||||||||
| require.NoError(t, err, "adding collection_id column to vinfo") | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| func TestIngestSqliteWithCollection_HashesVMIDsAndSetsVmmoid(t *testing.T) { | ||||||||||||||
| const collectionID = int64(7) | ||||||||||||||
| ctx := context.Background() | ||||||||||||||
|
|
||||||||||||||
| parser, db, cleanup := setupTestParser(t, &testValidator{}) | ||||||||||||||
| defer cleanup() | ||||||||||||||
|
|
||||||||||||||
| addCollectionColumns(t, db) | ||||||||||||||
|
|
||||||||||||||
| clusters := []sqliteCluster{ | ||||||||||||||
| {id: "domain-c1", name: "cluster1", datacenter: "dc1"}, | ||||||||||||||
| } | ||||||||||||||
| vms := []sqliteVM{ | ||||||||||||||
| {id: "vm-001", name: "vm-1", clusterName: "cluster1"}, | ||||||||||||||
| {id: "vm-002", name: "vm-2", clusterName: "cluster1"}, | ||||||||||||||
| } | ||||||||||||||
| sqlitePath := createTestSQLite(t, "vcenter-uuid-001", clusters, vms) | ||||||||||||||
|
|
||||||||||||||
| result, err := parser.IngestSqliteWithCollection(ctx, sqlitePath, collectionID) | ||||||||||||||
| require.NoError(t, err) | ||||||||||||||
| require.True(t, result.IsValid()) | ||||||||||||||
|
|
||||||||||||||
| rows, err := db.QueryContext(ctx, `SELECT "VM ID", vmmoid, collection_id FROM vinfo`) | ||||||||||||||
| require.NoError(t, err) | ||||||||||||||
| defer func() { _ = rows.Close() }() | ||||||||||||||
|
|
||||||||||||||
| rowCount := 0 | ||||||||||||||
| for rows.Next() { | ||||||||||||||
| rowCount++ | ||||||||||||||
| var vmID, vmmoid string | ||||||||||||||
| var colID int64 | ||||||||||||||
| require.NoError(t, rows.Scan(&vmID, &vmmoid, &colID)) | ||||||||||||||
|
|
||||||||||||||
| // "VM ID" must be a 32-char lowercase hex md5 string. | ||||||||||||||
| assert.Len(t, vmID, 32, "VM ID should be a 32-char md5 hex string") | ||||||||||||||
| assert.Regexp(t, `^[0-9a-f]{32}$`, vmID, "VM ID should be a lowercase hex string") | ||||||||||||||
|
|
||||||||||||||
| // vmmoid is the original MOID (non-empty, not itself a 32-char hash). | ||||||||||||||
| assert.NotEmpty(t, vmmoid, "vmmoid should be the original MOID") | ||||||||||||||
| assert.NotEqual(t, 32, len(vmmoid), "vmmoid should not look like an md5 hash (original MOIDs are shorter)") | ||||||||||||||
|
|
||||||||||||||
| // collection_id must be set correctly. | ||||||||||||||
| assert.Equal(t, collectionID, colID, "collection_id should match the supplied collectionID") | ||||||||||||||
|
|
||||||||||||||
| // Hash must be deterministic: md5("{collectionID}_{vmmoid}") == vmID | ||||||||||||||
| expectedHash := fmt.Sprintf("%x", md5.Sum(fmt.Appendf(nil, "%d_%s", collectionID, vmmoid))) | ||||||||||||||
| assert.Equal(t, expectedHash, vmID, "VM ID should equal md5('%d_%s', collectionID, vmmoid)") | ||||||||||||||
|
Comment on lines
+450
to
+452
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win Assertion failure message won't interpolate values.
🐛 Proposed fix- assert.Equal(t, expectedHash, vmID, "VM ID should equal md5('%d_%s', collectionID, vmmoid)")
+ assert.Equal(t, expectedHash, vmID, "VM ID should equal md5(%d_%s)", collectionID, vmmoid)As per path instructions, "Assertions must be specific and provide informative messages on failure." 📝 Committable suggestion
Suggested change
🧰 Tools🪛 ast-grep (0.44.0)[warning] 450-450: Detected use of the 'crypto/md5' package (md5.New / md5.Sum). MD5 is a cryptographically broken, collision-prone hash and must not be used for security purposes such as integrity checks, signatures, or password hashing. Use a strong hash from 'crypto/sha256' (sha256.New / sha256.Sum256) or 'crypto/sha512' instead; for passwords use a dedicated KDF such as golang.org/x/crypto/bcrypt or argon2. (weak-hash-md5-go) 🪛 OpenGrep (1.23.0)[WARNING] 451-451: MD5 and SHA1 are cryptographically broken and should not be used for security purposes. Use SHA-256 or stronger. (coderabbit.crypto.go-weak-hash) 🤖 Prompt for AI AgentsSource: Path instructions |
||||||||||||||
| } | ||||||||||||||
| require.NoError(t, rows.Err()) | ||||||||||||||
| assert.Equal(t, len(vms), rowCount, "vinfo should have one row per non-template VM") | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| func TestIngestSqliteWithCollection_RelationalTablesMatchVinfoIDs(t *testing.T) { | ||||||||||||||
| const collectionID = int64(7) | ||||||||||||||
| ctx := context.Background() | ||||||||||||||
|
|
||||||||||||||
| parser, db, cleanup := setupTestParser(t, &testValidator{}) | ||||||||||||||
| defer cleanup() | ||||||||||||||
|
|
||||||||||||||
| addCollectionColumns(t, db) | ||||||||||||||
|
|
||||||||||||||
| clusters := []sqliteCluster{ | ||||||||||||||
| {id: "domain-c1", name: "cluster1", datacenter: "dc1"}, | ||||||||||||||
| } | ||||||||||||||
| vms := []sqliteVM{ | ||||||||||||||
| {id: "vm-001", name: "vm-1", clusterName: "cluster1"}, | ||||||||||||||
| {id: "vm-002", name: "vm-2", clusterName: "cluster1"}, | ||||||||||||||
| } | ||||||||||||||
| sqlitePath := createTestSQLite(t, "vcenter-uuid-001", clusters, vms) | ||||||||||||||
|
|
||||||||||||||
| _, err := parser.IngestSqliteWithCollection(ctx, sqlitePath, collectionID) | ||||||||||||||
| require.NoError(t, err) | ||||||||||||||
|
|
||||||||||||||
| // Every vcpu "VM ID" must exist in vinfo "VM ID" — no orphaned rows. | ||||||||||||||
| var orphans int | ||||||||||||||
| err = db.QueryRowContext(ctx, ` | ||||||||||||||
| SELECT COUNT(*) FROM vcpu | ||||||||||||||
| WHERE "VM ID" NOT IN (SELECT "VM ID" FROM vinfo) | ||||||||||||||
| `).Scan(&orphans) | ||||||||||||||
| require.NoError(t, err) | ||||||||||||||
| assert.Equal(t, 0, orphans, "all vcpu rows should reference a valid vinfo VM ID") | ||||||||||||||
|
Comment on lines
+458
to
+486
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win Exercise every collection-aware relationship, not only The template changed 🤖 Prompt for AI AgentsSource: Path instructions |
||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| func TestIngestSqliteWithCollection_ZeroCollectionIDMatchesIngestSqlite(t *testing.T) { | ||||||||||||||
| ctx := context.Background() | ||||||||||||||
|
|
||||||||||||||
| clusters := []sqliteCluster{ | ||||||||||||||
| {id: "domain-c1", name: "cluster1", datacenter: "dc1"}, | ||||||||||||||
| } | ||||||||||||||
| vms := []sqliteVM{ | ||||||||||||||
| {id: "vm-001", name: "vm-1", clusterName: "cluster1"}, | ||||||||||||||
| {id: "vm-002", name: "vm-2", clusterName: "cluster1"}, | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| // Parser A uses IngestSqlite (the zero-collection-ID path). | ||||||||||||||
| parserA, dbA, cleanupA := setupTestParser(t, &testValidator{}) | ||||||||||||||
| defer cleanupA() | ||||||||||||||
|
|
||||||||||||||
| // Parser B uses IngestSqliteWithCollection with collectionID=0. | ||||||||||||||
| parserB, dbB, cleanupB := setupTestParser(t, &testValidator{}) | ||||||||||||||
| defer cleanupB() | ||||||||||||||
|
|
||||||||||||||
| sqlitePathA := createTestSQLite(t, "vcenter-uuid-001", clusters, vms) | ||||||||||||||
| sqlitePathB := createTestSQLite(t, "vcenter-uuid-001", clusters, vms) | ||||||||||||||
|
|
||||||||||||||
| resultA, err := parserA.IngestSqlite(ctx, sqlitePathA) | ||||||||||||||
| require.NoError(t, err) | ||||||||||||||
|
|
||||||||||||||
| resultB, err := parserB.IngestSqliteWithCollection(ctx, sqlitePathB, 0) | ||||||||||||||
| require.NoError(t, err) | ||||||||||||||
|
|
||||||||||||||
| assert.Equal(t, resultA.IsValid(), resultB.IsValid(), "validity should match between IngestSqlite and IngestSqliteWithCollection(0)") | ||||||||||||||
|
|
||||||||||||||
| var countA, countB int | ||||||||||||||
| require.NoError(t, dbA.QueryRowContext(ctx, `SELECT COUNT(*) FROM vinfo`).Scan(&countA)) | ||||||||||||||
| require.NoError(t, dbB.QueryRowContext(ctx, `SELECT COUNT(*) FROM vinfo`).Scan(&countB)) | ||||||||||||||
| assert.Equal(t, countA, countB, "VM count should match between IngestSqlite and IngestSqliteWithCollection(0)") | ||||||||||||||
|
Comment on lines
+517
to
+522
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win Compare representative IDs, not just row counts. Backward compatibility for 🤖 Prompt for AI AgentsSource: Path instructions |
||||||||||||||
| } | ||||||||||||||
|
Comment on lines
+406
to
+523
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win Missing test coverage for negative collectionID rejection.
As per path instructions, tests should "Strive for high test coverage on critical logic paths." 🧰 Tools🪛 ast-grep (0.44.0)[warning] 450-450: Detected use of the 'crypto/md5' package (md5.New / md5.Sum). MD5 is a cryptographically broken, collision-prone hash and must not be used for security purposes such as integrity checks, signatures, or password hashing. Use a strong hash from 'crypto/sha256' (sha256.New / sha256.Sum256) or 'crypto/sha512' instead; for passwords use a dedicated KDF such as golang.org/x/crypto/bcrypt or argon2. (weak-hash-md5-go) 🪛 OpenGrep (1.23.0)[WARNING] 451-451: MD5 and SHA1 are cryptographically broken and should not be used for security purposes. Use SHA-256 or stronger. (coderabbit.crypto.go-weak-hash) 🤖 Prompt for AI AgentsSource: Path instructions |
||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -18,7 +18,7 @@ Key transformations: | |
| ATTACH '{{.FilePath}}' AS src (TYPE sqlite); | ||
|
|
||
| INSERT INTO vinfo ( | ||
| "VM ID", "VM", "Folder ID", "Folder", "Host", "SMBIOS UUID", "VM UUID", | ||
| "VM ID", {{if .CollectionID}}"vmmoid", "collection_id", {{end}}"VM", "Folder ID", "Folder", "Host", "SMBIOS UUID", "VM UUID", | ||
| "Firmware", "Powerstate", "Connection state", "FT State", | ||
| "CPUs", "Memory", | ||
| "OS according to the configuration file", "OS according to the VMware Tools", | ||
|
|
@@ -29,7 +29,7 @@ INSERT INTO vinfo ( | |
| "migration_excluded", "labels", "guest_apps" | ||
| ) | ||
| SELECT | ||
| v.ID, | ||
| {{if .CollectionID}}md5(CAST({{.CollectionID}} AS VARCHAR) || '_' || v.ID), v.ID, {{.CollectionID}},{{else}}v.ID,{{end}} | ||
| v.Name, | ||
| v.Folder, | ||
| v.Folder, | ||
|
|
@@ -70,29 +70,29 @@ WHERE v.IsTemplate = 0; | |
|
|
||
| INSERT INTO vcpu ("VM ID", "Hot Add", "Hot Remove", "Sockets", "Cores p/s") | ||
| SELECT | ||
| v.ID, | ||
| {{if .CollectionID}}md5(CAST({{.CollectionID}} AS VARCHAR) || '_' || v.ID){{else}}v.ID{{end}}, | ||
| v.CpuHotAddEnabled = 1, | ||
| v.CpuHotRemoveEnabled = 1, | ||
| v.CpuCount / NULLIF(v.CoresPerSocket, 0), | ||
| v.CoresPerSocket | ||
| FROM src.VM v | ||
| WHERE v.ID IN (SELECT "VM ID" FROM vinfo); | ||
| WHERE v.ID IN (SELECT {{if .CollectionID}}vmmoid{{else}}"VM ID"{{end}} FROM vinfo{{if .CollectionID}} WHERE vmmoid IS NOT NULL{{end}}); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win Scope related-table filters to the current collection. Under collection mode these subqueries match any historical 🔧 Proposed fix-WHERE v.ID IN (SELECT {{if .CollectionID}}vmmoid{{else}}"VM ID"{{end}} FROM vinfo{{if .CollectionID}} WHERE vmmoid IS NOT NULL{{end}});
+WHERE {{if .CollectionID}}EXISTS (
+ SELECT 1 FROM vinfo
+ WHERE vinfo.vmmoid = v.ID
+ AND vinfo.collection_id = {{.CollectionID}}
+){{else}}v.ID IN (SELECT "VM ID" FROM vinfo){{end}};Apply the same Also applies to: 87-87, 112-112, 125-125 🤖 Prompt for AI Agents |
||
|
|
||
| INSERT INTO vmemory ("VM ID", "Hot Add", "Ballooned") | ||
| SELECT | ||
| v.ID, | ||
| {{if .CollectionID}}md5(CAST({{.CollectionID}} AS VARCHAR) || '_' || v.ID){{else}}v.ID{{end}}, | ||
| v.MemoryHotAddEnabled = 1, | ||
| v.BalloonedMemory | ||
| FROM src.VM v | ||
| WHERE v.ID IN (SELECT "VM ID" FROM vinfo); | ||
| WHERE v.ID IN (SELECT {{if .CollectionID}}vmmoid{{else}}"VM ID"{{end}} FROM vinfo{{if .CollectionID}} WHERE vmmoid IS NOT NULL{{end}}); | ||
|
|
||
| INSERT INTO vdisk ( | ||
| "VM ID", "Disk Key", "Unit #", "Path", "Disk Path", "Capacity MiB", | ||
| "Sharing mode", "Raw", "Shared Bus", "Disk Mode", "Disk UUID", | ||
| "Thin", "Controller", "Label", "SCSI Unit #" | ||
| ) | ||
| SELECT | ||
| v.ID, | ||
| {{if .CollectionID}}md5(CAST({{.CollectionID}} AS VARCHAR) || '_' || v.ID){{else}}v.ID{{end}}, | ||
| disk->>'key', | ||
| disk->>'unitNumber', | ||
| disk->>'file', | ||
|
|
@@ -109,11 +109,11 @@ SELECT | |
| disk->>'unitNumber' | ||
| FROM src.VM v, | ||
| LATERAL unnest(from_json(v.Disks, '[{"key":"INTEGER","unitNumber":"INTEGER","file":"VARCHAR","capacity":"BIGINT","shared":"BOOLEAN","rdm":"BOOLEAN","bus":"VARCHAR","mode":"VARCHAR","serial":"VARCHAR"}]')) AS t(disk) | ||
| WHERE v.Disks != '[]' AND v.ID IN (SELECT "VM ID" FROM vinfo); | ||
| WHERE v.Disks != '[]' AND v.ID IN (SELECT {{if .CollectionID}}vmmoid{{else}}"VM ID"{{end}} FROM vinfo{{if .CollectionID}} WHERE vmmoid IS NOT NULL{{end}}); | ||
|
|
||
| WITH nic_flat AS ( | ||
| SELECT | ||
| v.ID AS vm_id, | ||
| {{if .CollectionID}}md5(CAST({{.CollectionID}} AS VARCHAR) || '_' || v.ID){{else}}v.ID{{end}} AS vm_id, | ||
| v.IpAddress, | ||
| v.GuestNetworks, | ||
| nic.mac AS mac_addr, | ||
|
|
@@ -122,7 +122,7 @@ WITH nic_flat AS ( | |
| FROM src.VM v | ||
| LEFT JOIN src.Host h ON v.Host = h.ID, | ||
| LATERAL unnest(from_json(v.NICs, '[{"network":{"kind":"VARCHAR","id":"VARCHAR"},"mac":"VARCHAR","order":"INTEGER","deviceKey":"INTEGER"}]')) AS t(nic) | ||
| WHERE v.NICs != '[]' AND v.ID IN (SELECT "VM ID" FROM vinfo) | ||
| WHERE v.NICs != '[]' AND v.ID IN (SELECT {{if .CollectionID}}vmmoid{{else}}"VM ID"{{end}} FROM vinfo{{if .CollectionID}} WHERE vmmoid IS NOT NULL{{end}}) | ||
| ), | ||
| gn_flat AS ( | ||
| SELECT | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Do not use
VMMOIDas the proxy for RVTools-only leniency.This also makes the SQLite
collectionID=0INSERT INTO vinfonon-critical, because that template path has novmmoidcolumn. A failed SQLitevinfoload can now continue without an ingestion error, which is not the same as preserving existingIngestSqlitebehavior. Scope the relaxed behavior by ingestion source/policy instead of statement contents. As per path instructions, errors must be checked, wrapped with context, and propagated.🤖 Prompt for AI Agents
Source: Path instructions