Preserve source guest OS metadata during VM import - #230
Conversation
There was a problem hiding this comment.
Pull request overview
This PR extends VM importers to preserve source guest operating system metadata by detecting the guest OS from VMware, OVA/OVF, and OpenStack sources and applying it to the imported KubeVirt VirtualMachine as the harvesterhci.io/os label. It also improves troubleshooting by logging the source VM specification earlier and cleans up test infrastructure around lingering Docker containers.
Changes:
- Detect guest OS type from VMware VM properties, OVF
OperatingSystemSection, and OpenStack Glance image properties; apply it viaharvesterhci.io/oslabel. - Introduce shared guest OS classification helpers (
GuestOsIdToOsType,GuestOsNameToOsType) with unit tests. - Improve early-stage logging of origin VM specs and clean up test container handling.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/integration/suite_test.go | Attempts to remove a leftover integration test container before starting a new run. |
| pkg/source/vmware/client.go | Retrieves/logs VMware VM properties earlier and applies detected guest OS label during VM generation. |
| pkg/source/vmware/client_test.go | Stabilizes vcsim container naming/cleanup and adds tests for VMware guest OS detection helpers. |
| pkg/source/ova/client.go | Extracts guest OS type from OVF envelope and applies the guest OS label when generating the VM. |
| pkg/source/ova/client_test.go | Expands envelope parsing tests to cover guest OS detection behavior. |
| pkg/source/openstack/client.go | Resolves Glance boot image earlier, derives firmware + guest OS type from image properties, and applies guest OS label. |
| pkg/source/openstack/client_test.go | Adds unit tests for OpenStack guest OS detection from image properties. |
| pkg/source/helper.go | Adds ApplyGuestOsSetting to set harvesterhci.io/os label on created VMs. |
| pkg/source/guestos.go | Adds shared OS-type classification tables and mapping functions for IDs and free-text names. |
| pkg/source/guestos_test.go | Adds unit tests for the shared guest OS ID/name classification logic. |
Comments suppressed due to low confidence (1)
pkg/source/openstack/client.go:753
- getFirmwareSettings uses an unchecked type assertion (firmwareType.(string)). If the Glance image property is present but not a string, this will panic. Using a safe string type assertion avoids a potential crash.
firmwareType, ok := imageInfo.Properties["hw_firmware_type"]
if ok && firmwareType.(string) == "uefi" {
fw.UEFI = true
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
34d47bd to
32db774
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
pkg/source/openstack/client.go:756
- getFirmwareSettings can panic if imageInfo is nil (nil pointer deref) and uses an unchecked type assertion on hw_firmware_type (panic if the property isn't a string). Adding a nil guard and safe string assertion makes this robust against unexpected Glance metadata payloads.
firmwareType, ok := imageInfo.Properties["hw_firmware_type"]
if ok && firmwareType.(string) == "uefi" {
fw.UEFI = true
2528a94 to
55dee95
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
pkg/source/openstack/client.go:735
- The getBootImage loop can end up returning an error even when a valid boot image exists: if an earlier bootable volume has volume_image_metadata.image_id set, a later bootable volume without that metadata will overwrite imageID with "" and trigger the "no bootable volume..." error.
Avoid overwriting imageID with an empty value and break once a non-empty image_id is found.
}
}
if imageID == "" {
return nil, fmt.Errorf("no bootable volume with image metadata found for instance %s", instance.ID)
55dee95 to
30f66db
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (3)
pkg/source/vmware/client.go:231
- vmObj.Properties is called with an empty property list, which causes retrieval of the full VM managed object. This can be expensive on vCenter and produces very large log payloads (especially combined with the subsequent Info-level spec log). Prefer requesting only the properties needed for disk sizing + guest OS detection.
var o mo.VirtualMachine
err = vmObj.Properties(c.ctx, vmObj.Reference(), []string{}, &o)
if err != nil {
return fmt.Errorf("error retrieving VM properties: %w", err)
}
pkg/source/vmware/client.go:238
- Logging the entire mo.VirtualMachine at Info level tends to be extremely verbose and may include large nested structures that bloat controller logs. Consider logging only the relevant sub-objects (config/guest/summary) that help troubleshoot OS detection and hardware mapping.
logrus.WithFields(util.FieldsToJSON(logrus.Fields{
"name": vm.Name,
"namespace": vm.Namespace,
"spec": o,
}, []string{"spec"})).Info("Origin spec of the VM to be imported")
pkg/source/openstack/client.go:731
- The /volumes/{id} response is extracted twice (first into volumes.Volume for Bootable, then again into ExtendedVolume for volume_image_metadata). This is redundant and makes the flow harder to follow; extracting once into a combined struct keeps the logic simpler and avoids repeated parsing.
if err := resp.ExtractIntoStructPtr(&volStatus, "volume"); err != nil {
return nil, fmt.Errorf("error extracting volume status for volume %s: %v", v.ID, err)
}
if value, ok := volStatus.VolumeImageMetadata["image_id"]; ok && value != "" {
imageID = value
- Extract the OS type from the VM specifications and set the label “harvesterhci.io/os”. - Log the VM specification as early as possible. Since this is not done until a later stage in the current implementation, this useful information is unfortunately not available if problems occur before then. This is now being addressed by logging the specification at the earliest possible time. - Remove a leftover container from a previous, interrupted test run. - Improve OpenStack source client unit tests so that tests that do not require a real OpenStack environment can be run. - Overall code cleanup. Related to: harvester/harvester#11072 Signed-off-by: Volker Theile <vtheile@suse.com>
30f66db to
104e276
Compare
Problem:
The vm-import-controller currently imports hardware related configuration from the source VM, (CPU,Mem,firmware,disks,network,etc...), but it does not preserve guest operating system metadata.
As a result, The imported VirtualMachine contains no metadata indicating the original guest operating system and defaulted to
linuxfrom UI form.Solution:
Related Issue:
Related to: harvester/harvester#11072
Test plan:
You can use the following
VirtualMachineImportto import a VM from our test system.Note, the VM is very big and might cause timeouts when using VPN.
Virtual Machinesthat theOS Typedropdown on theAdvanced Optionstab is showingWindows.