Fix/vmware vm discovery#1983
Conversation
🚨 Security Vulnerability SummarySecurity posture degraded 📊 Overall Changes
🔍 Detailed Breakdown📦 Gosec (Static Analysis)
📦 Trivy (Dependency Scan)
📋 Baseline Methods
🚨 Added VulnerabilitiesTrivy (Dependencies) - 3 AddedTarget: Target: Target: Only HIGH and CRITICAL severity vulnerabilities are tracked |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| for i := range moVMs { | ||
| vm := object.NewVirtualMachine(c, moVMs[i].Self) | ||
| vmToDatacenter[moVMs[i].Self.Value] = dcName |
There was a problem hiding this comment.
🔴 vm.Name() returns "." for all VMs because InventoryPath is not set on objects created via object.NewVirtualMachine
The new collectVMsFromDatacenters function creates VM objects via object.NewVirtualMachine(c, moVMs[i].Self) at line 640, which sets the managed object reference but leaves InventoryPath empty. In govmomi, object.Common.Name() returns path.Base(o.InventoryPath), and Go's path.Base("") returns ".". The downstream processSingleVM function calls vm.Name() extensively — most critically in RDM disk ownership tracking at k8s/migration/pkg/utils/credutils.go:1881 (rdmInfo.Spec.OwnerVMs = AppendUnique(rdmInfo.Spec.OwnerVMs, vm.Name())) and inside processVMDisk at line 982 (rdmDisk.Spec.OwnerVMs = []string{vmName}). This corrupts the OwnerVMs field stored in Kubernetes RDMDisk CRDs — all VMs will be recorded as "." instead of their actual names. The old code used finder.VirtualMachineList(ctx, "*") which returned VM objects with InventoryPath populated.
Prompt for agents
The bug is in collectVMsFromDatacenters (credutils.go around line 640). When creating VMs via object.NewVirtualMachine(c, moVMs[i].Self), the InventoryPath is not set on the resulting object. This means vm.Name() (which calls path.Base(o.InventoryPath)) returns "." for all VMs.
The fix needs to set the VM name on the object. Options:
1. After creating the VM object, fetch its name from the moVMs properties. Change the Retrieve call to also fetch the "name" property (not just "self"), then use vm.SetInventoryPath() or store the name separately.
2. Alternatively, retrieve ["self", "name"] in the ContainerView Retrieve call, and then set a path on the object: e.g. object.NewVirtualMachine returns a *VirtualMachine that embeds Common — you could set vm.InventoryPath to something that ends with the VM name.
3. Or pass vmProps.Config.Name (which IS correctly fetched later in processSingleVM) to processVMDisk instead of vm.Name(), and fix the other vm.Name() usages in processSingleVM to use vmProps.Config.Name.
Option 3 is probably simplest: in processSingleVM, replace all uses of vm.Name() with vmProps.Config.Name (which is populated from VM properties and is guaranteed correct). This requires moving the vm.Name() calls that happen before vmProps.Config is available (lines 1822, 1827) to use vm.Reference().Value or a fallback.
Was this helpful? React with 👍 or 👎 to provide feedback.
OmkarDeshpande7
force-pushed
the
fix/vmware-vm-discovery
branch
from
e0138eb to
463686a
Compare
✅ Security Vulnerability SummaryNo change in security posture 📊 Overall Changes
🔍 Detailed Breakdown📦 Gosec (Static Analysis)
📦 Trivy (Dependency Scan)
📋 Baseline Methods
Only HIGH and CRITICAL severity vulnerabilities are tracked |
OmkarDeshpande7
force-pushed
the
fix/vmware-vm-discovery
branch
from
463686a to
7dff3db
Compare
✅ Security Vulnerability SummaryNo change in security posture 📊 Overall Changes
🔍 Detailed Breakdown📦 Gosec (Static Analysis)
📦 Trivy (Dependency Scan)
📋 Baseline Methods
Only HIGH and CRITICAL severity vulnerabilities are tracked |
Replace finder.VirtualMachineList("*") with a ContainerView-based
recursive enumeration so VMs inside cluster and resource-pool
sub-folders are discovered. "*" only searched the root VM folder,
causing "vm '*' not found" errors when all VMs lived in sub-folders.
Extract the datacenter-scanning loop into collectVMsFromDatacenters
to make the logic independently testable.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
TestCollectVMsFromDatacenters_WithVMs: verifies VMs in cluster sub-folders are found and mapped to the correct datacenter. TestCollectVMsFromDatacenters_EmptyDatacenter: verifies an empty datacenter returns an empty list without error, covering the notFoundError handling that previously caused spurious ERROR logs and skipped valid datacenters. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Require tests for every touched Go file (not just new code), and add govmomi simulator + fake k8s client guidance for VMware/k8s tests. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
… of "self"
vSphere PropertyCollector does not recognise "self" as a property name;
govmomi auto-populates mo.Self from the object reference when nil is
passed. Passing []string{"self"} caused ServerFaultCode: InvalidProperty
and silently skipped every datacenter, so no VMwareMachine objects were
created.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
meghansh-pf9
force-pushed
the
fix/vmware-vm-discovery
branch
from
7dff3db to
65ff593
Compare
✅ Security Vulnerability SummaryNo change in security posture 📊 Overall Changes
🔍 Detailed Breakdown📦 Gosec (Static Analysis)
📦 Trivy (Dependency Scan)
📋 Baseline Methods
Only HIGH and CRITICAL severity vulnerabilities are tracked |
2 participants
What this PR does / why we need it
Problem
VMwareMachine objects were not being created for VMs in VMware environments. Controller logs showed "vm '*' not found" errors per datacenter.
Root cause: finder.VirtualMachineList(ctx, "*") only searches the root VM folder of a datacenter. VMs placed inside cluster or resource-pool sub-folders are invisible to this call — govmomi returns notFoundError, the code silently skipped the datacenter, and no VMs were discovered.
Changes
Recursive VM enumeration (k8s/migration/pkg/utils/credutils.go)
Extracted collectVMsFromDatacenters helper
Replaced finder.VirtualMachineList("*") with view.ContainerView (recursive=true) — the correct govmomi idiom for enumerating all VMs across nested folders
PropertyCollector fix (same file)
v.Retrieve(..., []string{"self"}, ...) caused ServerFaultCode: InvalidProperty — "self" is not a vSphere property name; govmomi auto-populates mo.Self from the object reference
Fixed by passing nil properties instead
Which issue(s) this PR fixes
(optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)format, will close the issue(s) when PR gets merged)fixes #1985
Special notes for your reviewer
Testing done
please add testing details (logs, screenshots, etc.)