Revert "Add KVM source (#128)"#135
Conversation
There was a problem hiding this comment.
Pull request overview
Reverts the previously added KVM source support (#128), removing the KVM CRD/controller/client and associated tests/docs, and aligning shared helpers back to a non-KVM baseline.
Changes:
- Remove KVMSource API types, generated controllers, migration controller logic, and KVM source client implementation.
- Remove KVM integration test/setup code and KVM documentation from README.
- Simplify shared helpers (qemu VMDK→RAW conversion, Hardware CPU model field removal) and adjust callers accordingly.
Reviewed changes
Copilot reviewed 23 out of 26 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/setup/setup_kvm.go | Removes KVM test environment setup helpers. |
| tests/integration/suite_test.go | Removes SKIP_VCSIM gating; vcsim is started unconditionally. |
| tests/integration/kvm_test.go | Removes KVM integration specs. |
| pkg/source/vmware/client.go | Switches to qemu.ConvertVMDKtoRAW and tweaks an error message. |
| pkg/source/ova/client.go | Switches to qemu.ConvertVMDKtoRAW; updates NewHardware call signature. |
| pkg/source/openstack/client.go | Uses local raw image filename helper; introduces a typo in an error message; adds helper function. |
| pkg/source/kvm/client_test.go | Removes KVM client unit tests. |
| pkg/source/kvm/client.go | Removes KVM source implementation. |
| pkg/source/helper_test.go | Updates tests for NewHardware signature change. |
| pkg/source/helper.go | Removes CPU model from Hardware + VM spec CPU model usage; removes shared raw image filename helper. |
| pkg/qemu/qemu.go | Replaces generic ConvertToRAW with VMDK-specific helper; adds no-op SysProcAttr assignment. |
| pkg/qemu/gemu_test.go | Updates qemu test to call ConvertVMDKtoRAW. |
| pkg/generated/controllers/migration.harvesterhci.io/v1beta1/kvmsource.go | Removes generated KVMSource controller. |
| pkg/generated/controllers/migration.harvesterhci.io/v1beta1/interface.go | Removes KVMSource controller from factory interface. |
| pkg/crd/crd.go | Removes KVMSource CRD registration. |
| pkg/controllers/migration/virtualmachine.go | Removes KVM from supported source kinds and client/source retrieval. |
| pkg/controllers/migration/kvm.go | Removes KVM source controller reconciliation logic. |
| pkg/controllers/controllers.go | Stops registering the KVM controller and removes KVM from VM import controller wiring. |
| pkg/apis/migration.harvesterhci.io/v1beta1/zz_generated_register.go | Removes KVMSource types from scheme registration. |
| pkg/apis/migration.harvesterhci.io/v1beta1/zz_generated_list_types.go | Removes KVMSource list types. |
| pkg/apis/migration.harvesterhci.io/v1beta1/zz_generated_deepcopy.go | Removes KVMSource-related deepcopy code. |
| pkg/apis/migration.harvesterhci.io/v1beta1/kvm.go | Removes KVMSource API definitions/options. |
| pkg/apis/migration.harvesterhci.io/v1beta1/common.go | Removes KindKVMSource constant. |
| go.mod | Drops KVM-related deps (sftp/libvirt); adjusts x/* versions. |
| go.sum | Removes sums for dropped deps; updates x/* sums accordingly. |
| README.md | Removes KVMSource and KVM VirtualMachineImport documentation and env vars. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| func runCommand(command string, args ...string) error { | ||
| cmd := exec.Command(command, args...) | ||
| cmd.SysProcAttr = &syscall.SysProcAttr{} | ||
| stderr, err := cmd.StderrPipe() | ||
|
|
There was a problem hiding this comment.
cmd.SysProcAttr = &syscall.SysProcAttr{} is a no-op (all zero values) and adds an otherwise-unneeded syscall dependency. Unless you intend to set specific process attributes (e.g., setpgid/credential changes), consider removing this assignment (and the import) to keep the helper minimal and avoid OS-specific baggage.
| pool, err = dockertest.NewPool("") | ||
| Expect(err).NotTo(HaveOccurred()) | ||
| runOpts := &dockertest.RunOptions{ | ||
| Name: "vcsim-integration", | ||
| Repository: "vmware/vcsim", | ||
| Tag: "v0.29.0", | ||
| } | ||
|
|
||
| vcsimMock, err = pool.RunWithOptions(runOpts) | ||
| Expect(err).NotTo(HaveOccurred()) | ||
| vcsimMock, err = pool.RunWithOptions(runOpts) | ||
| Expect(err).NotTo(HaveOccurred()) | ||
|
|
||
| vcsimPort = vcsimMock.GetPort("8989/tcp") | ||
| vcsimPort = vcsimMock.GetPort("8989/tcp") | ||
|
|
||
| time.Sleep(30 * time.Second) | ||
| } | ||
| time.Sleep(30 * time.Second) |
There was a problem hiding this comment.
The integration suite now unconditionally starts the vcsim Docker container in BeforeSuite. This makes the test suite fail in environments without Docker even when USE_EXISTING_CLUSTER is not enabled (in which case the vmware integration specs are skipped anyway). Consider guarding this block behind useExisting and/or restoring an opt-out env var so contributors can run non-vmware integration tests without requiring Docker.
| fw, err := c.getFirmwareSettings(&vmObj.Server) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("error getting firmware settings: %w", err) | ||
| return nil, fmt.Errorf("error getting firware settings: %w", err) |
There was a problem hiding this comment.
Typo in error message: "firware" should be "firmware" to keep logs/errors searchable and professional.
| return nil, fmt.Errorf("error getting firware settings: %w", err) | |
| return nil, fmt.Errorf("error getting firmware settings: %w", err) |
This reverts commit 4bb4e1e. Signed-off-by: Volker Theile <vtheile@suse.com>
votdev
force-pushed
the
revert_pr_128_kvm_source
branch
from
52b61cf to
2241981
Compare
|
We should ignore Copilot suggestions because we want to undo the merged PR to get the origin state before the PR. |
This reverts commit 4bb4e1e (#128) as discussed here: harvester/harvester#9948 (comment).
IMPORTANT: Please do not create a Pull Request without creating an issue first.
Problem:
Solution:
Related Issue:
Test plan: