design-proposals: VM import from VMware vSphere

[Andrei Kvapil (kvaps)]

Design proposal for migrating VMware vSphere virtual machines into Cozystack, on the Konveyor Forklift engine that cozystack/cozystack#1982 already vendors.

Opened as a draft: the shape is settled, the load-bearing mechanics are verified (see below), but the upstream Forklift seccomp knob described in Design §4 is not yet submitted, and the full flow has not been run against a real vCenter yet.

Why this is here rather than in the implementation PR

cozystack/cozystack#1982 works and has been validated by its author on a live cluster, but its review surfaced questions that are design questions rather than defects: whether a VMware import is a tenant application at all, how a tenant supplies credentials when tenants cannot create Secrets, and whether an import produces managed Cozystack objects or leaves raw Forklift PVCs behind. Settling those in an 81-file PR is more expensive than settling them here.

What it proposes

The tenant API is a pair of CRDs in a new migration.cozystack.io group, reconciled by a controller in core — the shape of Cozystack's own Backup API (BackupJob/RestoreJob), not a Helm chart in the catalog. A VMImportSource registers a long-lived connection to a provider: type, endpoint, credentials entered on the spec, materialized into a Secret by the controller, validated against the live endpoint before anything is created. A VMImportTask runs the one-shot operation: reference a source, name the VMs, get VMDisks and VMInstances. The outputs carry no ownerReference to the Task, so deleting a Task removes the migration machinery and never the imported objects; everything Forklift-internal is owned and garbage-collected.

This follows the shape proposed in the implementation PR's review. An earlier revision of this proposal kept a catalog app plus a VMDisk source extension; it lost to the Backup precedent, which already answers one-shot lifecycle, tenant RBAC (aggregation labels shipped by the owning package), dashboard rendering (schema-generated forms and option pickers on plain CRDs), and controller-side credential materialization. The Alternatives section records the comparison.

A fulfillment controller drives one Forklift Plan per source VM and hands each transferred volume into its VMDisk without a second copy, using cdi.kubevirt.io/storage.populatedFor — the same primitive Cozystack's Velero VM-restore path already depends on in production. That closes both the raw-PVC gap raised in review and the duplicate-copy waste tracked in cozystack/cozystack#3924.

The proprietary VDDK image never enters the tree: it is an optional platform value the operator sets, delivered to the migration controller, and no image field appears on any tenant API.

The privilege story, verified live

On the raw-copy path no Forklift pod moves the bytes — CDI's importer does, through SetRestrictedSecurityContext. Verified in source against Forklift v2.11.5 / CDI v1.64.0 and empirically on a running Cozystack v1.6.2 cluster: the import was admitted into a restricted-enforcing namespace with no PodSecurity denial and ran to completion. Tenant self-service is structurally possible today.

Guest conversion is the constrained path, and its requirement is four syscalls (unshare, mount, umount2, pivot_root), not privilege: libguestfs starts passt, which sandboxes itself into fresh namespaces. baseline and restricted both accept seccompProfile.type: Localhost, and Forklift already selects a Localhost profile — but only when it detects OpenShift (pkg/controller/conversion/builder.go on current main). kubev2v/forklift#4491 is open on exactly this. The proposal pairs a node-side profile shipped through Talos machine.seccompProfiles with a small upstream setting in the VIRT_V2V_* family; a patch in that shape is already built and unit-tested. The privileged-namespace-plus-Unconfined-webhook shape is explicitly rejected.

Status of the claims

Everything load-bearing is cited to file:line. Two claims were additionally verified on a live cluster: restricted-PSS admission of the transfer pod, and the copy-free populatedFor handoff (same PV throughout, no importer pod, data byte-identical) — which also surfaced two constraints now folded into §5 (volumeMode/accessModes must be copied from the PV; the owning DataVolume must be deleted before re-pointing). Nothing has been measured against a real vCenter — the timings quoted are the implementation author's. The ## Open questions section carries a recommended default for each unsettled point.

Requesting review from the reviewers of cozystack/cozystack#1982, whose findings shaped this, and from the contributor whose implementation it builds on.

[coderabbitai]

Important

• 🔍 Trigger review

This repository does not receive automatic reviews because it has fewer than 10 stars.

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 1efd5933-37fd-44a4-a8fc-0e19c0a75bf4

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[Timofei Larkin (lllamnyp)]

I read this proposal against the implementation branch (cozystack/cozystack#3931) and against the Konveyor Forklift v2.11.5 sources that branch pins, since several load-bearing statements here are claims about Forklift's behavior that can be checked at a file and line. The core of the design holds up well: the two-CRD split by lifecycle, credentials on the Source spec projected into a guarded Secret, outputs carrying no owner reference back to the Task, and the copy-free handoff contract are all sound — the checkable constraints in §5 (populatedFor as pure data, CDI's controller ownerRef on adopted claims, volumeMode/accessModes copied from the PV, the owning DataVolume deleted before re-pointing) match what the upstream code actually does, and the tenant surface closes every isolation gap that sank #1982: no image field, no Secret reference, no namespace field, no raw primitive anywhere a tenant can reach. I want this to land. Before it does, one naming change and four amendments of substance.

1. Rename the group: migration.cozystack.io collides with an existing Cozystack concept, and forklift.cozystack.io is the honest name.

Cozystack already has migrations: the platform's own schema migrations, driven by the top-level migrations platform value and the numbered migration scripts under packages/core/platform/images/migrations/. Same word, unrelated mechanism, both operator-facing. The implementation branch already had to dodge this collision once — it ships the platform key as vmImport.vddkImage rather than the migration.vddkImage this document specifies in §3, with a comment explaining that two adjacent keys differing by one letter is a configuration mistake waiting to happen. That reasoning is correct, and it applies with more force to an API group, which is near-permanent where a values key is cheap to change.

I would name the group forklift.cozystack.io. The proposal's own design philosophy argues for it: the controller ships no provider client, mirrors Forklift's verdicts verbatim so "a tenant sees Forklift's own words", and delegates validation, inventory and transfer wholesale — the API's observable contract is deliberately Forklift-shaped, so the group name may as well say so. The stated reason for the operation-name ("oVirt/OpenStack/OVA sources join without a rename") does not distinguish the two candidates: those are all Forklift providers and arrive through the same engine, so forklift.cozystack.io forecloses none of them. The Backup API precedent does not transfer either: backups.cozystack.io earns its engine-neutral name through a strategy abstraction that keeps Velero behind an engine-agnostic BackupJob contract, and this API has no such layer by design. The one scenario where forklift rots — Cozystack swapping the engine — would be a semantic break deserving a new group regardless, and the group is v1alpha1 today, which makes this the cheapest moment the rename will ever have. (If naming a group after the engine reads as a step too far, vmimport.cozystack.io at least avoids the collision and matches both the kind names and the platform key — but I prefer the honest name.) Whichever way this lands, §3's migration.vddkImage should be synced to the vmImport.vddkImage key the branch actually ships.

2. Make the inventory query normative in §5 — the tempting shortcut is disproven by the Forklift sources.

§5 already says the controller "enumerates the datastores of the named VMs through inventory". Keep that, and make it explicit and binding, because the shortcut the implementation branch currently takes — learning the topology from the Plan's VMNetworksNotMapped/VMStorageNotMapped validation conditions instead — cannot work: those conditions carry the VM references in items, not the offending network or datastore IDs. At v2.11.5, pkg/controller/plan/validation.go:940,960 appends ref.String() of the VM, rendered as id:vm-1234 name:'web-01' (pkg/apis/forklift/v1beta1/ref/ref.go:29); the IDs a map entry needs appear on no Forklift custom resource at all, and map resolution is an exact-ID lookup with no wildcard (pkg/controller/plan/adapter/vsphere/validator.go:80,116). So the design should name the mechanism: an authenticated query against Forklift's inventory REST service (the operand runs with feature_auth_required: true) for the named VMs' network and datastore refs, from which the controller writes both maps. This stays inside the "no second vSphere client" boundary — it is still Forklift's view of the source — but it is a real HTTP client with auth, and the document should own that rather than imply the Kubernetes objects suffice.

3. §5's "discarded unstarted" needs targetPowerState: off on every rendered Plan.

Forklift does not guarantee the created VirtualMachine stays off. With no targetPowerState, v2.11.5 sets the created VM's run strategy to match the source's pre-migration power state (pkg/controller/plan/kubevirt.go:3649), so a cold migration of a running source — the normal production case — produces a VM that boots the migrated guest the moment the transfer completes, before any handoff can run, with exactly the duplicate-machine-on-the-network consequence the proposal's Halted-by-default rationale exists to prevent, and with the handoff then re-pointing a volume out from under a running guest. The design should require targetPowerState: off (pkg/apis/forklift/v1beta1/plan/vm.go:27) on every Plan and state the assumption it protects.

4. The collision and restart promises need an output-identity mechanism.

The failure table promises both "output name collision → per-VM Failed, the controller never overwrites tenant objects" and "controller restarts mid-transfer → the handoff is idempotent". Together those require something the document does not yet name: a way to distinguish the controller's own prior outputs from a pre-existing tenant object of the same name. Without it every resume is a guess, and each wrong guess is bad — treating a tenant's object as its own silently attaches stale data while the fresh transfer is garbage-collected with the scaffolding VM; treating its own output as a collision terminally fails an import that actually succeeded. Suggested resolution: stamp created VMDisks, VMInstances and handoff PVCs with a label carrying the Task UID and source VM ID, and define collision as "exists without my marker". A label creates no garbage-collection linkage, so the outputs-outlive-the-task property in §1 is untouched.

5. State the EFI interim behavior explicitly.

§Scope correctly makes cozystack/cozystack#3002 a merge-order dependency so UEFI guests do not land as BIOS. Add the interim semantics: until the firmware field exists and is written through, a source VM with EFI firmware — visible on the Forklift-built VirtualMachine the controller already reads — fails its import per-VM with a message saying why. Silently importing a guest that cannot boot is the class of failure this proposal's own validation philosophy ("surface it on status before anything is created") exists to prevent.

With the rename and these four amendments, this is the design I want for the feature: it fixes what #1982 was rejected for, and the lifecycle semantics it adds — terminal one-shot operation, outputs orphaned by design, a handoff that copies nothing — are things no chart could express.