feat(vm-import): add VMware VM import via Forklift - #1982
feat(vm-import): add VMware VM import via Forklift#1982Matthieu ROBIN (matthieu-robin) wants to merge 63 commits into
Conversation
dosubot
Bot
added
size/XL
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a vm-import application and vm-adoption-controller plus Forklift integration, Helm charts, RD registration, docs, examples, E2E tests, and minor vm-instance template/schema updates to support DataVolume naming for VMware→KubeVirt imports and automatic adoption into Cozystack. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant VMware as vSphere
participant Forklift as Forklift<br/>Operator
participant KubeVirt as KubeVirt
participant Adoption as VM<br/>AdoptionController
participant Cozystack as Cozystack<br/>Dashboard
User->>KubeVirt: Create Secret (vCenter creds)
User->>KubeVirt: Apply VMImport CR
KubeVirt->>Forklift: Create Provider / Plan / Migration
Forklift->>VMware: Connect & transfer VM disks
Forklift->>KubeVirt: Create VirtualMachines & DataVolumes
Forklift->>KubeVirt: Label VMs (plan reference)
Adoption->>KubeVirt: Watch labeled VMs
Adoption->>KubeVirt: Get Plan (check adoption enabled)
alt adoption enabled
Adoption->>KubeVirt: Create VMInstance CR
Adoption->>KubeVirt: Label VM as adopted
KubeVirt->>Cozystack: VMInstance sync -> HelmRelease
Cozystack->>User: Show VM in dashboard
else adoption disabled
Adoption->>Adoption: Skip VM
end
sequenceDiagram
participant Forklift as Forklift
participant Controller as VM Adoption<br/>Controller
participant API as Kubernetes API
participant VM as VirtualMachine
Forklift->>VM: Create with forklift labels
Forklift->>API: Annotate Plan (adoption enabled/disabled)
loop reconcile (interval)
Controller->>API: List VMs with plan label
Controller->>API: Get Plan resource
alt adoption enabled
Controller->>API: Create VMInstance from VM spec
rect rgba(76, 175, 80, 0.5)
Note over Controller,API: extract state, disks, networks, SSH keys
end
Controller->>API: Patch VM with adopted labels
else
Note over Controller: skip adoption
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
Summary of ChangesHello Matthieu ROBIN (@matthieu-robin), I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the platform's virtualization capabilities by introducing a robust solution for migrating existing VMware virtual machines to KubeVirt. It provides a comprehensive application for users to manage the entire migration lifecycle, from source configuration to network and storage mapping, supporting various migration strategies. This integration streamlines the process of moving workloads from traditional VMware environments to a Kubernetes-native virtualization platform. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on Gemini (@gemini-code-assist) comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces the VMware VM import feature via Forklift, integrating it seamlessly into the existing CozyStack platform. The new packages for forklift-operator, forklift, vm-import, and vm-import-rd are well-structured and follow Helm best practices. The Makefile for forklift-operator includes robust logic for namespace handling and removal of kind: Namespace definitions, which is a good practice for Helm chart generation from upstream Kustomize. The README.md provides clear and comprehensive instructions for users, and the values.schema.json accurately defines the configuration parameters. Overall, the changes are well-implemented and align with the stated objectives of the pull request.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@packages/apps/vm-import/Makefile`:
- Around line 1-5: Add a .PHONY declaration for the custom Makefile target to
match other packages: declare ".PHONY: generate" in the vm-import Makefile so
the "generate" target (the cozyvalues-gen and update-crd invocation) is treated
as phony; do not add duplicates for targets already provided by hack/package.mk
(e.g., all, clean, test).
In `@packages/apps/vm-import/README.md`:
- Around line 7-8: The README uses the incorrect capitalization "Github:";
update the text to the proper brand capitalization "GitHub:" by replacing the
string "Github:" with "GitHub:" in the README entry (the line containing the
link label currently showing "Github: <https://github.com/kubev2v/forklift>").
In `@packages/apps/vm-import/templates/networkmap.yaml`:
- Around line 14-22: The template currently treats any non-"pod" value as multus
and may render an empty destinationName; change the conditional to explicitly
check for "multus" (use else if eq .destinationType "multus") and only render
the multus block when that is true, and inside that block assert/require a
non-empty .destinationName (e.g., wrap name: {{ .destinationName | quote }} with
an if .destinationName check or use the template's required helper to fail
fast), leaving a safe default or no output for other destinationType values;
reference the symbols destinationType and destinationName in networkmap.yaml and
update the pod/multus conditional accordingly.
In `@packages/apps/vm-import/templates/plan.yaml`:
- Around line 17-25: The template currently gates the entire map block on
.Values.networkMap causing storage-only configs to omit map.storage; change the
outer condition to render map when either .Values.networkMap or
.Values.storageMap is set (use Helm's or: {{- if or .Values.networkMap
.Values.storageMap }}), then inside that block render network and storage
subsections independently using {{- if .Values.networkMap }} ... {{- end }} and
{{- if .Values.storageMap }} ... {{- end }}, keeping the existing fields (name:
{{ .Release.Name }}, namespace: {{ .Release.Namespace }}) for each subsection.
🧹 Nitpick comments (3)
packages/system/forklift/Chart.yaml (1)
1-3: LGTM!Standard Chart.yaml structure following the
cozy-prefix convention for system packages.Consider adding a
descriptionfield for documentation clarity:apiVersion: v2 name: cozy-forklift +description: Forklift controller for VM migration version: 0.0.0 # Placeholder, the actual version will be automatically set during the build processpackages/system/forklift-operator/Makefile (1)
6-14: Consider extracting the inline Python filter into a small script for readability. It will make the update target easier to maintain and debug.packages/apps/vm-import/values.schema.json (1)
16-27: Tighten schema: enumdestinationTypeand requiredestinationNamefor multus.This will prevent invalid values from reaching rendering/runtime.
Suggested schema update
"properties": { "destinationName": { "description": "Name of the destination network (required if type is `multus`).", "type": "string" }, "destinationNamespace": { "description": "Namespace of the destination network.", "type": "string" }, "destinationType": { "description": "Destination type: `pod` for pod network, or `multus` for a Multus network.", - "type": "string" + "type": "string", + "enum": ["pod", "multus"] }, "sourceId": { "description": "The managed object reference ID of the source network in vSphere.", "type": "string" } - } + }, + "allOf": [ + { + "if": { + "properties": { "destinationType": { "const": "multus" } } + }, + "then": { "required": ["destinationName"] } + } + ] }
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@packages/apps/vm-import/templates/plan.yaml`:
- Line 9: The template sets "warm: {{ .Values.warm }}" which yields an
empty/invalid YAML when .Values.warm is undefined; change it to use Helm's
default function so the key always renders a valid boolean, e.g. replace the
expression with {{ default false .Values.warm }} (keep it unquoted) so the warm
field will be a proper boolean false when no value is provided.
|
Hey Matthieu ROBIN (@matthieu-robin), With the vm-import application, it’s possible to specify which VMs to import via: vms: [] What is the expected behavior here? My understanding is that Forklift will import the VMs as lower-level KubeVirt objects. However, this wouldn’t create the higher-level virtualmachine objects in Cozystack, so they wouldn’t be visible in the dashboard or to the end user. Am I mistaken? Also, what is expected to happen when the vm-import application is removed? Should the imported VMs be deleted as well? I think we need to clarify this workflow. |
dosubot
Bot
added
size/XXL
There was a problem hiding this comment.
Actionable comments posted: 13
🤖 Fix all issues with AI agents
In `@packages/apps/vm-import/ADOPTION_DESIGN.md`:
- Around line 54-84: Update the design doc to match the implemented behavior of
the vm-import-adoption-controller: replace the ConfigMap-based tracking section
with a clear description of the VMInstance CRD creation (including the CRD name
VMInstance and the fields populated) and add a short note explaining the
historical reason for the change from ConfigMap to VMInstance (or,
alternatively, modify the controller to create ConfigMaps instead if you prefer
the original design); ensure the doc references the controller name
vm-import-adoption-controller and the concrete resource (VMInstance) so readers
can find main.go and the CRD implementation.
- Around line 114-139: Update the HelmRelease manifest to use the stable API
version by replacing the apiVersion value "helm.toolkit.fluxcd.io/v2beta1" with
"helm.toolkit.fluxcd.io/v2" in the HelmRelease example; locate the manifest
block that defines kind: HelmRelease and change the apiVersion accordingly so
the example uses the GA Flux Helm toolkit API.
In `@packages/apps/vm-import/docs/examples/advanced-import.yaml`:
- Line 18: The thumbprint value under the thumbprint key is one octet short (19
colon-separated hex pairs) and must be a full 20-byte SHA-1 fingerprint; update
the thumbprint string in advanced-import.yaml (the thumbprint key) to contain 20
colon-separated two-digit hex pairs (matching the format used in the
simple-import example) so the example shows a valid SHA-1 fingerprint.
In `@packages/apps/vm-import/docs/scripts/adopt-vm.sh`:
- Around line 63-76: The post-increment expression ((DISK_COUNT++)) inside the
while loop causes a non-zero exit status when DISK_COUNT is 0 under set -e;
change the increment to a safe form that always returns 0 such as using
((DISK_COUNT+=1)) or DISK_COUNT=$((DISK_COUNT+1)) in the loop that reads
DISK_NAMES so the script doesn't exit early; update references to DISK_COUNT in
the same block (the while IFS= read -r disk; do ... done <<< "$DISK_NAMES"
section) and ensure DISKS_YAML building and the final echo remain unchanged.
- Around line 90-92: The step label for the network check is missing the
progress prefix; update the echo that prints "Checking network configuration..."
to follow the same step counter format (e.g., change it to "echo '[5/5] Checking
network configuration...'" or renumber all other step labels to use a 6-step
sequence) so it matches the existing pattern; locate the echo that precedes the
NETWORKS variable assignment (the block referencing VM_NAME and NAMESPACE) and
apply the consistent `[N/5]` or updated `[N/6]` prefix.
In `@packages/apps/vm-import/TEST_REPORT.md`:
- Line 106: The fenced code blocks in TEST_REPORT.md are missing language
specifiers (violates MD040); update each backtick-fenced block that currently
starts with ``` to include an explicit language hint (for example change ``` to
```text or ```bash as appropriate) so both occurrences are annotated and
markdownlint MD040 is satisfied.
- Around line 1-6: Remove the AI-authored test report file TEST_REPORT.md
(authored by "Claude Code") from the commit and do not commit AI-generated test
evidence as source-of-truth; either delete the file or move its contents to a
CI-generated artifact (e.g., capture helm template output as pipeline artifacts)
and replace the repository file with a human-verified test plan; also ensure any
references like "Ready to merge", "Kubernetes v1.34.3", "kubectl v1.35.0", or
mixed French/English notes are removed or rewritten by a human reviewer before
committing.
In `@packages/core/platform/sources/vm-adoption-controller.yaml`:
- Around line 1-14: The PackageSource uses the wrong API and flat structure;
change apiVersion to cozystack.io/v1alpha1 and restructure to the canonical
pattern: add spec.sourceRef referencing the cozystack-packages OCIRepository,
move the existing component under spec.variants[] -> components with the
vm-adoption-controller entry, and move dependsOn into the same spec.variants[]
as a list of package names (strings) instead of objects; ensure the kind stays
PackageSource and preserve namespace and path values when migrating them into
the spec.variants component definition.
In `@packages/system/vm-adoption-controller/images/controller/go.mod`:
- Around line 3-10: Update the module Go version and Kubernetes client
libraries: change the go directive from "go 1.23" to "go 1.25" and bump the k8s
libs in the require block—specifically update "k8s.io/client-go", "k8s.io/api",
and "k8s.io/apimachinery" to v0.35.0 (keep or reconcile k8s.io/klog/v2 and
kubevirt.io/api as needed for compatibility). After editing the go.mod entries,
run "go get k8s.io/client-go@v0.35.0" (and corresponding get commands for
api/apimachinery if needed) and "go mod tidy" to resolve transitive versions,
then rebuild/tests to ensure there are no breaking API changes.
In `@packages/system/vm-adoption-controller/images/controller/main.go`:
- Around line 226-251: The code currently defaults to adoption=true when the
Plan lookup via c.dynamicClient.Resource(gvr).Namespace(namespace).Get(...)
fails; change this to default to false: when Get returns an error, set enabled =
false, store that value in c.planCache[cacheKey]
(PlanCacheEntry.AdoptionEnabled) under c.cacheMutex, and log the failure at
warning level indicating adoption is being defaulted to disabled; ensure the
function returns false instead of true so adoption only proceeds when a Plan is
explicitly confirmed.
- Around line 268-272: The current call to unstructured.NestedMap (used with
vmUnstructured and assigned to spec) can return err==nil and found==false which
results in fmt.Errorf wrapping a nil error; change the check so you handle the
two cases separately: if err != nil return fmt.Errorf("failed to get VM spec:
%w", err) and if !found return a clear non-wrapped error (e.g., errors.New("VM
spec not found") or fmt.Errorf("VM spec not found")) so you never wrap a nil
error when calling unstructured.NestedMap.
In `@packages/system/vm-adoption-controller/README.md`:
- Around line 37-49: The YAML sample has duplicate top-level controller: blocks
so only the last one applies; merge the three blocks into a single controller
mapping containing watchInterval, watchNamespace, and namePrefix together
(preserve comments) so all keys take effect; update the README.md example to
show a single controller: block with watchInterval, watchNamespace, and
namePrefix fields.
🧹 Nitpick comments (12)
packages/system/vm-adoption-controller/templates/serviceaccount.yaml (1)
1-5: Hardcoded resource name prevents multiple releases and missing standard Helm labels.The name
vm-adoption-controlleris hardcoded rather than derived from the release (e.g.,{{ .Release.Name }}-vm-adoption-controlleror afullnamehelper). Since the correspondingClusterRoleBinding(cluster-scoped) also uses this same hardcoded name, deploying two releases will cause a conflict.Additionally, standard
app.kubernetes.iolabels are missing on all resources in this chart, which reduces observability and makes it harder to identify resources belonging to a specific release.Suggested improvement
apiVersion: v1 kind: ServiceAccount metadata: - name: vm-adoption-controller + name: {{ include "vm-adoption-controller.fullname" . }} namespace: {{ .Release.Namespace }} + labels: + app.kubernetes.io/name: vm-adoption-controller + app.kubernetes.io/instance: {{ .Release.Name }} + app.kubernetes.io/managed-by: {{ .Release.Service }}This applies equally to the ClusterRole, ClusterRoleBinding, Deployment, and ServiceAccount templates in this chart.
packages/system/vm-adoption-controller/images/controller/Dockerfile (3)
9-10: Onlymain.gois copied — consider copying all source files.Currently only
main.gois copied. If internal packages or additional.gofiles are added in the future, the build will silently exclude them. A more resilient pattern:Suggested fix
# Copy source -COPY main.go . +COPY . .This is safe because
go.modandgo.sumare already copied in the previous step (layer caching for dependency download is preserved).
1-1: Upgrade Go version to match repo standards. The Dockerfile correctly uses Go 1.23 fromgo.mod, but Go 1.23 is outdated—most other modules in the system use Go 1.25. Align this package with the latest version used across the codebase.
13-13: HardcodedGOARCH=amd64deviates from project multi-architecture build pattern.The codebase consistently uses
ARG TARGETOSandARG TARGETARCHacross other controllers (backup-controller, cozystack-controller, flux-plunger, kamaji, and others). Align this Dockerfile with the established pattern to enable multi-architecture builds.Suggested fix
+ARG TARGETOS=linux +ARG TARGETARCH=amd64 -RUN CGO_ENABLED=0 GOOS=linux GOARCH=amd64 go build -a -o controller main.go +RUN CGO_ENABLED=0 GOOS=${TARGETOS} GOARCH=${TARGETARCH} go build -a -o controller main.gopackages/system/vm-adoption-controller/values.yaml (1)
3-5: Emptyimage.repositoryandimage.tagdefaults — will the deployment work out of the box?Both
repositoryandtagdefault to empty strings. If a user installs this chart without overriding these values, the deployment will reference an invalid image ("":""or similar). Typically, a default image reference is provided so the chart is functional without mandatory overrides. If this is intentionally left for the packaging system to fill in, consider adding a comment noting that, or add a validation check in the deployment template.packages/system/vm-adoption-controller/templates/deployment.yaml (1)
19-43: Consider adding a liveness probe.The controller runs a polling loop with no health endpoint. If the reconciliation loop hangs (e.g., blocked on an API call without timeout), Kubernetes won't detect the failure and won't restart the pod. A minimal
/healthzendpoint in the Go binary or a file-touch–based probe would improve resilience.hack/e2e-apps/vm-import.bats (1)
86-92: Test cleanup only runs on success; resource leak on mid-test failure.If an assertion fails before the cleanup block (lines 86-91), the VMImport, Secret, and HelmRelease will remain in the cluster, potentially causing subsequent test runs to fail or collide. Consider using a
teardownfunction (Batsteardown) or atrapto ensure cleanup runs regardless of test outcome.♻️ Example using Bats teardown or trap
You could add a file-level
teardownor usesetup/teardownper test:# At the top of each `@test`, or use a file-level teardown teardown() { kubectl -n tenant-test delete vmimport.apps.cozystack.io test --ignore-not-found 2>/dev/null || true kubectl -n tenant-test delete secret vmware-credentials-test --ignore-not-found 2>/dev/null || true kubectl -n tenant-test delete vmimport.apps.cozystack.io test-minimal --ignore-not-found 2>/dev/null || true kubectl -n tenant-test delete secret vmware-credentials-test-minimal --ignore-not-found 2>/dev/null || true kubectl -n tenant-test delete vmimport.apps.cozystack.io test-no-adoption --ignore-not-found 2>/dev/null || true kubectl -n tenant-test delete secret vmware-credentials-test-no-adoption --ignore-not-found 2>/dev/null || true }packages/apps/vm-import/ADOPTION_DESIGN.md (1)
1-10: Design document is written in French while the rest of the codebase is in English.This limits accessibility for non-French-speaking contributors and reviewers. Consider translating to English for consistency, or at minimum adding an English summary at the top.
packages/system/vm-adoption-controller/images/controller/main.go (4)
43-46: Dead code:clientsetfield andvmDisk*constants are never used.
vmDiskGroup,vmDiskVersion,vmDiskKind(lines 43-46) are declared but never referenced.clientset(line 97) is created inmain()and stored in the struct but never accessed by any method — onlydynamicClientis used.This adds unnecessary dependencies and confusion.
♻️ Proposed cleanup
- // VMDisk GVR - vmDiskGroup = "apps.cozystack.io" - vmDiskVersion = "v1alpha1" - vmDiskKind = "VMDisk" -type AdoptionController struct { - clientset *kubernetes.Clientset dynamicClient dynamic.Interface planCache map[string]*PlanCacheEntry cacheMutex sync.RWMutex }And remove the
kubernetes.NewForConfig(config)call and the"k8s.io/client-go/kubernetes"import inmain().Also applies to: 74-78, 96-101
52-94: No signal handling — graceful shutdown is unreachable.
context.Background()is never cancelled, soctx.Done()inRun()(line 119) will never fire. When Kubernetes sends SIGTERM, the controller ignores it and is eventually SIGKILL'd.For a stateless polling controller this is functionally safe, but it prevents clean log messages and could leave mid-flight API calls interrupted.
♻️ Proposed signal handling
+ "os" + "os/signal" + "syscall" ... // Run controller - ctx := context.Background() + ctx, cancel := signal.NotifyContext(context.Background(), syscall.SIGTERM, syscall.SIGINT) + defer cancel() controller.Run(ctx)Also applies to: 108-126
328-331: Usestrings.TrimPrefixfor idiomatic prefix stripping.Manual length check + slice is fragile and harder to read.
♻️ Proposed fix
Add
"strings"to imports, then:- // Extract disk name (remove vm-disk- prefix if present) - if len(diskName) > 8 && diskName[:8] == "vm-disk-" { - diskName = diskName[8:] - } + // Extract disk name (remove vm-disk- prefix if present) + diskName = strings.TrimPrefix(diskName, "vm-disk-")
147-204: Each VM is fetched twice — once ingetForkliftVMs(unstructured list) and again inadoptVM(get by name).The full unstructured data is already available in
item(line 172) but is discarded. Passing theunstructured.Unstructureddirectly toadoptVMwould eliminate one API call per VM.
| # Crée la HelmRelease avec adoption | ||
| cat <<EOF | kubectl apply -f - | ||
| apiVersion: helm.toolkit.fluxcd.io/v2beta1 | ||
| kind: HelmRelease | ||
| metadata: | ||
| name: $VM_NAME | ||
| namespace: $NAMESPACE | ||
| spec: | ||
| chart: | ||
| spec: | ||
| chart: vm-instance | ||
| sourceRef: | ||
| kind: HelmRepository | ||
| name: cozystack | ||
| install: | ||
| createNamespace: false | ||
| # N'essaie pas de créer la VM (elle existe déjà) | ||
| disableWait: true | ||
| upgrade: | ||
| # Ne modifie pas la VM existante | ||
| force: false | ||
| values: | ||
| # Valeurs extraites de la VM existante | ||
| ... | ||
| EOF | ||
| ``` |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Flux HelmRelease v2beta1 deprecated v2 stable version
💡 Result:
HelmRelease v2beta1 (helm.toolkit.fluxcd.io/v2beta1) is deprecated and (in Flux) effectively removed starting with Flux v2.2.0—clusters upgraded to Flux v2.2+ must stop using v2beta1 manifests. [4], [5]
What to use instead (stable)
Use the stable (GA) API:
apiVersion: helm.toolkit.fluxcd.io/v2(kind:HelmRelease) [2], [3]
(v2beta2 existed as an intermediate beta, but it’s also deprecated and slated for removal in a future release.) [1]
Minimal migration
- Update your manifests:
helm.toolkit.fluxcd.io/v2beta1→helm.toolkit.fluxcd.io/v2[2], [3]
- Make sure your Flux/helm-controller installation is upgraded and CRDs are updated (so the cluster has the
v2HelmRelease CRD installed). [1], [3]
Notable schema removals in the v2 API (watch for these if you used them)
Some deprecated fields were removed from the HelmRelease v2 API (e.g. valuesFile → valuesFiles, and older kustomize post-render patch fields → patches). [1]
Sources: Flux v2.3 announcement (deprecations/removals) [1], Helm API reference v2 [2], ControlPlane/Flux API version matrices showing HelmRelease GA as v2 [3], Flux v2.2.0 notes about v2beta1 deprecation/removal [4], Flux controller API versioning/deprecation policy [5].
Replace v2beta1 with the stable v2 API version in the HelmRelease manifest.
Line 116 uses the deprecated helm.toolkit.fluxcd.io/v2beta1 API version. Update it to helm.toolkit.fluxcd.io/v2 (stable GA since Flux 2.1.0, with v2beta1 removal enforced in Flux v2.2.0+). Anyone following this documentation example should use the current stable API.
🤖 Prompt for AI Agents
In `@packages/apps/vm-import/ADOPTION_DESIGN.md` around lines 114 - 139, Update
the HelmRelease manifest to use the stable API version by replacing the
apiVersion value "helm.toolkit.fluxcd.io/v2beta1" with
"helm.toolkit.fluxcd.io/v2" in the HelmRelease example; locate the manifest
block that defines kind: HelmRelease and change the apiVersion accordingly so
the example uses the GA Flux Helm toolkit API.
| # Check for networks | ||
| echo "Checking network configuration..." | ||
| NETWORKS=$(kubectl get vm "$VM_NAME" -n "$NAMESPACE" -o json 2>/dev/null | \ |
There was a problem hiding this comment.
Inconsistent step label — missing [X/5] prefix.
Line 91 uses a bare "Checking network configuration..." while all other steps use the [N/5] counter format. This is the implicit step between 4 and 5.
Proposed fix
-echo "Checking network configuration..."
+echo -e "${YELLOW}[4.5/5]${NC} Checking network configuration..."Alternatively, renumber all steps to account for 6 total steps.
🤖 Prompt for AI Agents
In `@packages/apps/vm-import/docs/scripts/adopt-vm.sh` around lines 90 - 92, The
step label for the network check is missing the progress prefix; update the echo
that prints "Checking network configuration..." to follow the same step counter
format (e.g., change it to "echo '[5/5] Checking network configuration...'" or
renumber all other step labels to use a 6-step sequence) so it matches the
existing pattern; locate the echo that precedes the NETWORKS variable assignment
(the block referencing VM_NAME and NAMESPACE) and apply the consistent `[N/5]`
or updated `[N/6]` prefix.
| go 1.23 | ||
|
|
||
| require ( | ||
| k8s.io/api v0.31.0 | ||
| k8s.io/apimachinery v0.31.0 | ||
| k8s.io/client-go v0.31.0 | ||
| k8s.io/klog/v2 v2.130.1 | ||
| kubevirt.io/api v1.3.1 |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
latest Go version February 2026
💡 Result:
As of February 6, 2026, the latest stable Go release is Go 1.25.7 (released February 4, 2026). [1]
Latest major series: Go 1.25 (Go 1.25.0 released August 12, 2025). [1]
Source(s)
[1] Go Release History (go.dev) (go.dev)
Citations:
🌐 Web query:
Kubernetes client-go latest release version 2026
💡 Result:
As of Feb 6, 2026, the latest stable k8s.io/client-go release tag is v0.35.0 (published Dec 17, 2025, aligned with Kubernetes v1.35.0). [1][2]
🌐 Web query:
Go 1.23 end of life support timeline
💡 Result:
Go 1.23 support follows Go’s release policy: a major release is supported until there are two newer major releases (i.e., only the latest two major versions are supported) [1]. The security policy likewise prepares fixes for the two most recent major releases [2].
Go 1.23 end-of-life (end of upstream support) timeline
- Go 1.23.0 released: August 13, 2024 [3]
- Last 1.23 patch release: Go 1.23.12 (August 6, 2025) [4]
- End of upstream support (EOL): August 12, 2025 — when Go 1.25.0 shipped as the second newer major release after 1.23 (with 1.24 in between) [1][5]. After this date, upstream no longer backports critical/security fixes to 1.23 [1][2].
(As of February 6, 2026, the supported major releases are Go 1.24 and 1.25 [1].)
Sources
- Go Release Policy (go.dev release history page) [1]
- Go Security Policy (two most recent major releases) [2]
- Go 1.23 release announcement [3]
- Go 1.23.12 release entry [4]
- Go 1.25.0 release entry/date [5]
Upgrade Go and Kubernetes client libraries — current versions are unsupported.
Go 1.23 reached end-of-life on August 12, 2025, and no longer receives security patches or critical fixes. The Kubernetes client libraries at v0.31.0 (August 2024) are four minor versions behind the current stable v0.35.0 and similarly lack security updates.
Bump to Go 1.25.x (latest stable: 1.25.7 as of February 2026) and Kubernetes client-go v0.35.0 to restore security support and access current bug fixes.
🤖 Prompt for AI Agents
In `@packages/system/vm-adoption-controller/images/controller/go.mod` around lines
3 - 10, Update the module Go version and Kubernetes client libraries: change the
go directive from "go 1.23" to "go 1.25" and bump the k8s libs in the require
block—specifically update "k8s.io/client-go", "k8s.io/api", and
"k8s.io/apimachinery" to v0.35.0 (keep or reconcile k8s.io/klog/v2 and
kubevirt.io/api as needed for compatibility). After editing the go.mod entries,
run "go get k8s.io/client-go@v0.35.0" (and corresponding get commands for
api/apimachinery if needed) and "go mod tidy" to resolve transitive versions,
then rebuild/tests to ensure there are no breaking API changes.
ff737ca to
ac75ab9
Compare
ac75ab9 to
8f3f95b
Compare
|
You're right — the original implementation had several gaps preventing imported VMs from being visible in the dashboard. Here's a clarification of the intended workflow, along with the fixes I've pushed. Workflow: how imported VMs become visible in the dashboard
This happens automatically within ~30 seconds of import completion. What happens when vm-import is deletedDeleted (temporary migration objects):
Preserved:
Fixes in this updateArchitectural (the adoption flow was fundamentally broken)
Infrastructure
Functional
Cleanup
Files changed
|
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
packages/apps/vm-import/docs/examples/simple-import.yaml (1)
13-17: Use explicit placeholder tokens for secret values in the example.Line 14 and Line 17 are valid samples, but
<REPLACE_...>style placeholders are safer for copy/paste usage in docs.Small docs hardening tweak
- user: administrator@vsphere.local - password: MySecurePassword123! + user: "<VCENTER_USERNAME>" + password: "<VCENTER_PASSWORD>" @@ - thumbprint: "AA:BB:CC:DD:EE:FF:00:11:22:33:44:55:66:77:88:99:AA:BB:CC:DD" + thumbprint: "<VCENTER_TLS_SHA1_FINGERPRINT>"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/apps/vm-import/docs/examples/simple-import.yaml` around lines 13 - 17, Replace the hard-coded example secrets in the YAML (the values for keys user, password, and thumbprint) with explicit placeholder tokens so readers don't accidentally copy real credentials; update the "user" value to something like <REPLACE_USERNAME>, "password" to <REPLACE_PASSWORD>, and "thumbprint" to <REPLACE_THUMBPRINT> (keep the comment showing how to obtain a real thumbprint intact), ensuring examples in the file reference these placeholder tokens instead of real-looking secrets.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/apps/vm-import/ADOPTION_DESIGN.md`:
- Around line 93-105: The fenced code block that begins with "Forklift imports
VM" is untyped; update that block in ADOPTION_DESIGN.md by adding a language tag
(e.g., change ``` to ```text) so the code fence is typed (```text ... ```),
ensuring markdownlint MD040 is satisfied for the architecture diagram block.
In `@packages/apps/vm-import/docs/adoption.md`:
- Around line 17-40: The docs contradict each other about enableAdoption
behavior: update the "Method 1: Automatic Labeling (Default)" section to match
the controller implementation by explicitly stating whether enableAdoption:true
only applies labels or also creates VMInstance resources for full management;
reference the actual controller flow symbols (enableAdoption, VMInstance,
ADOPTION_DESIGN and README adoption descriptions) and either (a) change the text
to say enableAdoption:true performs only labeling with the listed read-only
limitations, or (b) document the distinct modes/flags (label-only vs full
adoption) and the exact flag/field that triggers VMInstance creation so
operators know when Cozystack will take full lifecycle control.
In `@packages/apps/vm-import/README.md`:
- Around line 186-189: Add a brief note to README.md stating that setting vms:
[] (the default) produces no migration plan and is a no-op for execution; update
the table description for the `vms` entry and add a short sentence in the “How
to use” section clarifying that the Plan template only renders when `vms` is
non-empty so an empty `vms` list results in no migration actions.
In `@packages/apps/vm-instance/templates/vm.yaml`:
- Around line 67-68: The template uses the computed variable $dvName and the
lookup call (lookup "cdi.kubevirt.io/v1beta1" "DataVolume" $.Release.Namespace
$dvName) which depends on .Values.disks[].dvName being present in the generated
schema; add a dvName field to the disks entries in values.yaml (e.g., include
dvName: "" or null for each disk item) so the chart schema generator will pick
up and expose .Values.disks[].dvName in values.schema.json, ensuring the lookup
override works in the UI/validation.
In `@packages/apps/vm-instance/values.schema.json`:
- Around line 34-37: The dvName property was manually added to
values.schema.json (which is auto-generated) and will be overwritten; instead
add a dvName entry under the disks items in values.yaml with the same
description/comment so cozyvalues-gen will include it during generation, then
run make generate to regenerate values.schema.json; reference the dvName
property, the disks items structure in values.yaml, the cozyvalues-gen
generator, and the make generate step when making the change.
---
Nitpick comments:
In `@packages/apps/vm-import/docs/examples/simple-import.yaml`:
- Around line 13-17: Replace the hard-coded example secrets in the YAML (the
values for keys user, password, and thumbprint) with explicit placeholder tokens
so readers don't accidentally copy real credentials; update the "user" value to
something like <REPLACE_USERNAME>, "password" to <REPLACE_PASSWORD>, and
"thumbprint" to <REPLACE_THUMBPRINT> (keep the comment showing how to obtain a
real thumbprint intact), ensuring examples in the file reference these
placeholder tokens instead of real-looking secrets.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4104caed-4116-4600-aabf-3f7cd86ba5a9
⛔ Files ignored due to path filters (1)
packages/apps/vm-import/logos/vm-import.svgis excluded by!**/*.svg
📒 Files selected for processing (23)
hack/e2e-apps/vm-import.batspackages/apps/vm-import/.helmignorepackages/apps/vm-import/ADOPTION_DESIGN.mdpackages/apps/vm-import/Chart.yamlpackages/apps/vm-import/Makefilepackages/apps/vm-import/README.mdpackages/apps/vm-import/charts/cozy-libpackages/apps/vm-import/docs/adoption.mdpackages/apps/vm-import/docs/examples/advanced-import.yamlpackages/apps/vm-import/docs/examples/monitoring-import.mdpackages/apps/vm-import/docs/examples/simple-import.yamlpackages/apps/vm-import/docs/scripts/adopt-vm.shpackages/apps/vm-import/templates/dashboard-resourcemap.yamlpackages/apps/vm-import/templates/migration.yamlpackages/apps/vm-import/templates/networkmap.yamlpackages/apps/vm-import/templates/plan.yamlpackages/apps/vm-import/templates/provider.yamlpackages/apps/vm-import/templates/storagemap.yamlpackages/apps/vm-import/templates/workloadmonitor.yamlpackages/apps/vm-import/values.schema.jsonpackages/apps/vm-import/values.yamlpackages/apps/vm-instance/templates/vm.yamlpackages/apps/vm-instance/values.schema.json
✅ Files skipped from review due to trivial changes (2)
- packages/apps/vm-import/docs/examples/advanced-import.yaml
- packages/apps/vm-import/docs/examples/monitoring-import.md
🚧 Files skipped from review as they are similar to previous changes (6)
- packages/apps/vm-import/Chart.yaml
- packages/apps/vm-import/docs/scripts/adopt-vm.sh
- packages/apps/vm-import/templates/workloadmonitor.yaml
- hack/e2e-apps/vm-import.bats
- packages/apps/vm-import/.helmignore
- packages/apps/vm-import/values.yaml
| {{- $dvName := default (printf "vm-disk-%s" $disk.name) $disk.dvName }} | ||
| {{- $dv := lookup "cdi.kubevirt.io/v1beta1" "DataVolume" $.Release.Namespace $dvName }} |
There was a problem hiding this comment.
Template logic is sound, but depends on schema change that won't persist.
The dvName variable and lookup logic are correctly implemented—defaulting to vm-disk-<name> while allowing explicit overrides for imported VMs. The error message correctly references the computed name.
However, this template relies on .Values.disks[].dvName being available, which depends on the schema change in values.schema.json. Since that file is auto-generated and the manual edit gets removed (per pipeline warning), .dvName may not be exposed to users through the UI/validation.
Ensure the corresponding dvName field is added to values.yaml so the schema regeneration includes it.
Also applies to: 80-80
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/apps/vm-instance/templates/vm.yaml` around lines 67 - 68, The
template uses the computed variable $dvName and the lookup call (lookup
"cdi.kubevirt.io/v1beta1" "DataVolume" $.Release.Namespace $dvName) which
depends on .Values.disks[].dvName being present in the generated schema; add a
dvName field to the disks entries in values.yaml (e.g., include dvName: "" or
null for each disk item) so the chart schema generator will pick up and expose
.Values.disks[].dvName in values.schema.json, ensuring the lookup override works
in the UI/validation.
| "dvName": { | ||
| "description": "Explicit DataVolume name to reference. If set, used instead of the default vm-disk-<name> convention.", | ||
| "type": "string" | ||
| }, |
There was a problem hiding this comment.
Manual edit to auto-generated file will be lost.
The pipeline warning confirms that dvName was removed during automated generation. This file is produced by cozyvalues-gen from values.yaml and regenerated on make generate.
To persist this field, add the dvName property to values.yaml (under the disks items structure) with appropriate description comments. The generator will then include it in the schema automatically.
Based on learnings: "In reviews for cozystack, treat any values.schema.json files as auto-generated: they are produced from values.yaml by the cozyvalues-gen tool and regenerated on make generate. Do not suggest manual edits to these files."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/apps/vm-instance/values.schema.json` around lines 34 - 37, The
dvName property was manually added to values.schema.json (which is
auto-generated) and will be overwritten; instead add a dvName entry under the
disks items in values.yaml with the same description/comment so cozyvalues-gen
will include it during generation, then run make generate to regenerate
values.schema.json; reference the dvName property, the disks items structure in
values.yaml, the cozyvalues-gen generator, and the make generate step when
making the change.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/apps/vm-import/README.md (1)
1-213: Do not manually edit this README—it is auto-generated bycozyvalues-genfromvalues.yamland will be overwritten onmake generate.This README is produced by the
cozyvalues-gentool (seepackages/apps/vm-import/Makefile). Any manual edits to documentation, parameter descriptions, or the parameters table will be lost during regeneration. To update documentation, modify the parameter descriptions invalues.yamlinstead, and they will be included in the regenerated README on the next build.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/apps/vm-import/README.md` around lines 1 - 213, The README is missing the auto-generated notice; add a clear top-of-file warning that this file is auto-generated by cozyvalues-gen from values.yaml and will be overwritten by `make generate` (matching the wording in the review), so any edits should be made in values.yaml or the generator; reference the generator and the Makefile target (cozyvalues-gen, values.yaml, Makefile) in the notice and place it immediately under the main title so it appears first in packages/apps/vm-import/README.md.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/apps/vm-import/README.md`:
- Around line 147-148: The README examples use the deprecated spec.running
field; update the kubectl patch vm examples that reference spec.running to use
spec.runStrategy instead (set runStrategy:"Always" to start continuously and
runStrategy:"Halted" to stop), i.e., locate the kubectl patch vm lines that
mention spec.running and replace them to patch spec.runStrategy with the
appropriate string values so examples align with KubeVirt v1.3+ and cozystack
runStrategy migration.
---
Nitpick comments:
In `@packages/apps/vm-import/README.md`:
- Around line 1-213: The README is missing the auto-generated notice; add a
clear top-of-file warning that this file is auto-generated by cozyvalues-gen
from values.yaml and will be overwritten by `make generate` (matching the
wording in the review), so any edits should be made in values.yaml or the
generator; reference the generator and the Makefile target (cozyvalues-gen,
values.yaml, Makefile) in the notice and place it immediately under the main
title so it appears first in packages/apps/vm-import/README.md.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2cdb45cf-be46-4021-be9e-3985cc4b5b28
📒 Files selected for processing (4)
packages/apps/vm-import/ADOPTION_DESIGN.mdpackages/apps/vm-import/README.mdpackages/apps/vm-import/docs/adoption.mdpackages/apps/vm-instance/values.yaml
✅ Files skipped from review due to trivial changes (1)
- packages/apps/vm-import/ADOPTION_DESIGN.md
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/apps/vm-import/docs/adoption.md
ef6a0de to
af8586d
Compare
github-actions
Bot
added
the
area/virtualization
77ece55 to
677cf8b
Compare
|
Andrei Kvapil (@kvaps) Hello, I did some modifications, could you please tell me what do you think about? Thanks a lot |
ac1e442 to
c075a6f
Compare
The Secret example named administrator@vsphere.local and nothing said what privileges a migration actually requires, so the path of least resistance was to hand Forklift a vCenter Administrator. That credential is read by the cluster-privileged Forklift controller, so the account's reach is the credential's blast radius. Name a dedicated account in the examples and add a prerequisites section explaining why a read-only account is not sufficient either: reading disks through the VDDK, powering the source VM off at cutover, and the snapshots a warm migration takes are all writes, while only inventory collection is genuinely read-only. A read-only account therefore lists the VMs and then fails at transfer time. The section points at the MTV Prerequisites chapter for the authoritative privilege list rather than restating it here, since that list tracks the Forklift release and would rot in this repository. It also covers the second credential set migrationHosts introduces against the ESXi hosts. Added as 3.7 rather than inserted earlier so the existing cross-references into section 3 keep pointing at the right subsections. Signed-off-by: Matthieu <matthieu@hidora.com>
fullnameOverride reached the tenant-facing schema so the adoption controller can name the rendered VirtualMachine after the VM it replaces. It feeds virtual-machine.fullname, which is metadata.name on the VirtualMachine, and nothing stopped a tenant from editing it afterwards: Helm would then prune the VirtualMachine and recreate it under the new name, re-rolling a running VM. Disks survive, since they are DataVolumes owned by their own vm-disk releases, so this is disruption rather than data loss. Keeping the field out of the schema is not an option — the API rejects what the schema does not declare, and the controller has to set it — so guard it with a CEL transition rule instead, the same mechanism vm-disk uses for storageClass. A transition rule is not evaluated when the old object lacks the field, so the controller can still stamp it at creation and only later edits are refused. Signed-off-by: Matthieu <matthieu@hidora.com>
…ates The dashboard Role covered providers, plans, migrations and the two maps, but not hosts. A tenant using migrationHosts to route disk transfer over a per-ESXi-host address therefore created Host objects its own dashboard could not display. Adding the resource alone would not have been enough: the rule is scoped by resourceNames, which takes no patterns, and host.yaml names each Host <release>-<id> rather than one of the three names already listed. Range over the declared migrationHosts to enumerate them. Signed-off-by: Matthieu <matthieu@hidora.com>
Plans are searched cluster-wide because a Plan may live in a different namespace than the VM it migrated, so resolvePlan listed every Plan in the cluster and scanned it linearly. It runs once per candidate VM in getForkliftVMs and again in adoptVM, which made a reconcile pass cost O(pending VMs x all Plans) — repeated every 15 seconds. Build the UID to Plan index from a single List the first time a pass needs it, and drop it at the top of reconcile so a Plan edited between ticks is still picked up. Lookups after the first are then free, adoptVM included. The lookup stays lazy rather than eager so resolvePlan keeps working when called outside a reconcile pass, which is how the tests exercise it. Signed-off-by: Matthieu <matthieu@hidora.com>
PROJECT_SUMMARY.md was 245 lines of development narrative — what was built, what was fixed along the way, what is left to productize — shipped inside an application package that should carry operator documentation. Its "fixes made" section already duplicated section 6 of the migration guide, and its follow-up list duplicated section 7. One part was not narrative and not duplicated: the measured comparison of the two transfer paths, which is what tells an operator whether skipGuestConversion is worth arranging. Move that table into the guide, next to the architecture it compares, and delete the rest. Signed-off-by: Matthieu <matthieu@hidora.com>
The package still called $(call settag,$(TAG)), a macro removed from main in ed0297e; it expands to nothing, so buildx was handed a bare "repo:" and failed with "invalid reference format". Every Build job on the PR has been red on this since, and the recent rebase did not clear it because the stale call lives in this Makefile rather than in the shared includes. Switch to image-tags/cache-args like the sibling controllers. Assisted-By: Claude Signed-off-by: Andrei Kvapil <andrei.kvapil@aenix.io>
Installing Forklift unconditionally means every existing iaas cluster picks up a large operator manifest, its own CRDs and broad RBAC on upgrade, plus an adoption controller holding cluster-wide create on VMInstances -- for a feature only a cluster migrating off vSphere needs. Gate the three packages behind bundles.enabledPackages instead, alongside gpu-operator and vm-default-images. The accompanying test pins the rendered package count: absence of a single document cannot be asserted directly here, because a documentSelector matching nothing fails rather than counting zero, and containsDocument with not passes whenever any one document differs. Assisted-By: Claude Signed-off-by: Andrei Kvapil <andrei.kvapil@aenix.io>
…place Forklift owns every migrated PVC by the VirtualMachine it creates -- EnsureVM patches each one with a blockOwnerDeletion ownerReference, upstream comment "so that they'll be cleaned up when the VirtualMachine is removed". Adoption then deleted that VM to free the name the VMInstance takes, and the garbage collector took the disk with it: the clone lost its source mid-flight, the DataVolume sat in CloneWithoutSource and the VM in ErrorPvcNotFound, with a completed transfer destroyed by the adoption meant to consume it. The code assumed the opposite -- "KubeVirt does not delete PVCs with a VM" -- which holds for an ordinary VM but not for a Forklift import. Two changes, both scoped to same-namespace adoption: Detach before deleting. Only the volumes this adoption consumes, and only ownerReferences pointing at this VM, so an import that never reaches adoption keeps Forklift's cleanup of abandoned migrations. Stop cloning. On the raw-copy path Forklift already wrote the disk into the target namespace with the right size, StorageClass and content, so the clone copied a whole disk to stand still and left two full copies behind for good. The disks keep referencing the imported PVC; vm-instance already falls back to a bare PersistentVolumeClaim when no DataVolume of that name exists. The cross-namespace virt-v2v path is unchanged -- there the copy is the point. Assisted-By: Claude Signed-off-by: Andrei Kvapil <andrei.kvapil@aenix.io>
df8e10e to
984936f
Compare
IvanHunters
left a comment
IvanHunters
left a comment
There was a problem hiding this comment.
Verdict
NOT LGTM
The cross-tenant isolation design is sound and well tested, but the adoption controller's Go logic has several confirmed data-destruction paths (a transient API error and a name collision both silently delete the imported VM), the disk clone chain leaks two permanent copies per disk, warm migrations can never complete, the shipped e2e suite runs nothing, and the manual adopt-vm.sh fallback produces a values file that cannot adopt. These are blocking.
Findings
[MAJOR] packages/system/vm-adoption-controller/images/controller/main.go:520, getTargetNamespace defaults to the Plan namespace on any Get error, not just NotFound
The Get error branch is unconditional (if err != nil { return namespace }), it is not apierrors.IsNotFound-gated. The Plan was already resolved in hand at the call site (resolvePlan), so this second namespaced Get is redundant and only adds a failure mode. Scenario: an admin Plan in cozy-forklift carries vm-import.cozystack.io/target-namespace: tenant-a. On a transient Get error getTargetNamespace returns cozy-forklift. On the virt-v2v path vm.Namespace == planNamespace == cozy-forklift, so targetNamespace == vm.Namespace, wrapDisksAsVMDisks early-returns (src==dst, no clone), the VMInstance is created in the privileged namespace, and releaseSourceVM takes the same-namespace branch (main.go:1146-1160) that detaches the PVC ownerRefs and deletes the Forklift VM. It is non-retryable because the source VM the controller keys on is gone. getPlanPreset (main.go:553-563) has the same swallow-and-default-on-error shape and silently drops the operator's chosen instanceType/instanceProfile on a transient error. Fix: propagate the error and requeue on anything that is not a real NotFound, or drop the redundant Get and reuse the already-resolved Plan.
[MAJOR] packages/system/vm-adoption-controller/images/controller/main.go:996, idempotency check matches a VMInstance by name only, with no identity verification
The "already created" short-circuit does a Get(vmInstanceName) in the target namespace and, on success, jumps straight to releaseSourceVM. It never reads back the vm-import.cozystack.io/original-vm-name / -namespace annotations it writes on create (main.go:1032-1033), and vmInstanceName == vm.Name. Scenario: a tenant already runs an unrelated VMInstance web, and a VMware VM named web is imported into the same namespace. The Get finds the pre-existing web, the controller treats it as its own earlier success, and releaseSourceVM detaches the import's PVC ownerRefs and deletes the imported Forklift VM. The import is silently destroyed and its disks orphaned, while the unrelated VMInstance is untouched. Fix: verify the found VMInstance carries this import's identity annotations before releasing the source VM.
[MAJOR] packages/system/vm-adoption-controller/images/controller/main.go:651, cross-namespace adoption leaks two permanent full-disk copies per disk
The virt-v2v cross-namespace path clones the imported PVC into <vmDiskName>-src (main.go:651-655), then ensureVMDisk clones that into vm-disk-<name> (main.go:627, 682). Nothing ever deletes the -src DataVolume/PVC, and the original PVC in cozy-forklift also stays (the source VM is only labeled). The only Delete in the controller is the kubevirt VM at main.go:1155, and RBAC could not delete volumes anyway: clusterrole.yaml:22-24 grants datavolumes no delete and clusterrole.yaml:35-37 grants PVCs no delete. Net result on the cross-namespace path is three full copies of every migrated disk on replicated storage, one referenced and two permanent garbage with no ownerRef for GC. The same-namespace/raw-copy path correctly avoids this via the early return. Fix: delete the intermediate -src volume after the VMDisk clone completes (and grant the verb), or clone directly into the final target.
[MAJOR] packages/system/vm-adoption-controller/images/controller/main.go:1140, cross-namespace adoption leaves the source VM running and stamps the wrong release namespace on it
On the cross-namespace branch labelVMAsAdopted only labels the Forklift VM, it never halts or deletes it (main.go:1140-1145). The source VM keeps its inherited runStrategy (extracted at main.go:767-781, default Always) and keeps running off the original disk next to the tenant clone, so both boot with the same guest identity and MAC. Separately, labelVMAsAdopted writes meta.helm.sh/release-namespace: <source VM namespace> (main.go:1191), the privileged namespace, for a release that actually lives in the tenant namespace. That is false ownership metadata: a future vm-instance-<name> release in cozy-forklift could adopt and rewrite this VM. Fix: stop/delete the source VM on the cross-namespace path once the clone is confirmed, and either omit the Helm ownership annotations on the source VM or set them to the real release namespace.
[MAJOR] packages/apps/vm-import/templates/migration.yaml:1, warm: true can never complete and a helm-upgrade that adds a VM never migrates it
The Migration is rendered once with only spec.plan.{name,namespace}, there is no spec.cutover anywhere (grep for cutover in packages/apps/vm-import/ hits docs only). warm is a real exposed value (plan.yaml:29, values.yaml:35, values.schema.json:56, advanced-import.yaml:44). A Forklift warm migration precopies indefinitely until a cutover timestamp is set, so the Succeeded condition the adoption gate waits on (main.go:448) never turns true and the VM is never adopted. Separately, the Migration name is fixed ({{ .Release.Name }}) and its spec does not change when vms grows, so a completed Forklift Migration is not re-run: a VM added via helm upgrade is silently never migrated, with no error surfaced. Fix: either do not expose warm until cutover is modeled, or provide a cutover mechanism; and give added VMs a path to migrate (per-VM Migration, or detect the diff).
[MAJOR] hack/e2e-apps/vm-import.bats:1, the shipped e2e suite is never executed and would not have coverage if it were
CI runs chainsaw suites: hack/e2e-chainsaw/README.md:5 states this directory replaces the per-app bats suite that used to live in hack/e2e-apps/*.bats, and packages/core/testing/Makefile:31 runs test-chainsaw plus only the e2e-prepare-cluster / e2e-install-cozystack / e2e-test-openapi bats. Nothing globs hack/e2e-apps/. There is no vm-import chainsaw suite. Even if the bats ran, hack/e2e-install-cozystack.bats:242 enables only cozystack.external-dns-application in bundles.enabledPackages, so the opt-in forklift/vm-import CRDs are absent and applying a VMImport would fail with "no matches for kind". The e2e coverage this PR appears to add provides none. Fix: add a chainsaw suite under hack/e2e-chainsaw/ and enable the packages in the e2e install, or drop the dead bats file and state the tested surface honestly.
[MAJOR] packages/apps/vm-import/docs/scripts/adopt-vm.sh:76, the manual-adoption fallback script produces a values file that cannot adopt
Three independent defects. (a) Lines 76-79 strip a vm-disk- prefix that a Forklift disk never has and emit only name: with no dvName, so vm-instance resolves the non-existent DataVolume vm-disk-<fullname> and renders fail "Specified disk not exists in cluster" (vm.yaml:68, 77). The dvName field this same PR added for exactly this case is never emitted. (b) The generated values file (lines 120-178) has no fullnameOverride, and the script never runs the Helm ownership steps adoption.md:62-75 requires, so the release renders a second VM instead of adopting the existing one (adoption.md:80). (c) Line 167 sets cloudInitSeed: "imported-vm" while adoption.md:82, the controller (main.go:1015), and _helpers.tpl:57,60-68 all require cloudInitSeed: "" to preserve the firmware UUID, so following the script gives the adopted VM a fresh SMBIOS UUID and churns guest identity. Fix: emit dvName and fullnameOverride, apply the ownership labels/annotations, and set cloudInitSeed: "".
[MAJOR] packages/apps/vm-instance/values.schema.json:213, the fullnameOverride immutability rule does not block unsetting the field
The only guard is "rule": "self == oldSelf" (mirrored in cozyrds/vm-instance.yaml:11), with no default and not required. A CEL transition rule is evaluated only when the field is present in both old and new objects, so deleting fullnameOverride from an adopted VMInstance passes validation. The VirtualMachine name then reverts to the chart default (vm.yaml:15), Helm renames the object, prunes the running imported VM and creates a fresh one: exactly the disruptive re-roll the rule was added to prevent. Fix: make the field required on this app, or move the immutability guard to the parent object so the unset transition is covered.
[MAJOR] packages/apps/vm-instance/templates/vm.yaml:68, the new dvName field and DataVolume-vs-PVC fallback branch ship with no automated test
This rewrites the disk-rendering block of vm-instance, a core already-deployed chart backing every VM, not only imported ones. It adds the disk.dvName override and a new branch that references a bare persistentVolumeClaim when no DataVolume of the resolved name exists (vm.yaml:120-126). The only file under packages/apps/vm-instance/tests/ is hook_securitycontext_test.yaml, nothing exercises the dvName path or the PVC-fallback path, and the vm-import bats (already dead, see above) never migrates a VM so the branch is never reached in CI. The DataVolume path is preserved for existing VMs (with dvName unset $dvName resolves to vm-disk-<name>, unchanged), so this is not a confirmed regression, but a new render branch on a chart this load-bearing needs a regression test. helm-unittest can seed lookup via kubernetesProvider (sibling charts do this): add a case seeding only a PVC (assert persistentVolumeClaim.claimName) and one setting disk.dvName (assert the override is used).
[MINOR] packages/system/vm-adoption-controller/templates/clusterrole.yaml:17, cluster-wide update/patch/delete on vminstances and vmdisks are never used
The rule grants ["get","list","watch","create","update","patch","delete"] on vminstances and vmdisks, but the controller only ever Gets and Creates them; the sole Delete targets kubevirt VirtualMachines (main.go:1155) and the volume Updates target datavolumes/PVCs. Least privilege: trim vminstances/vmdisks to get, list, watch, create. A compromised controller pod otherwise has cluster-wide delete on every tenant's VMInstances and VMDisks.
[MINOR] packages/apps/vm-import/docs/examples/simple-import.yaml:22, example manifests use a non-served apiVersion
simple-import.yaml:22 and advanced-import.yaml:21 use apiVersion: apps.cozystack.io/v1, but the served version is v1alpha1 (controller at main.go:55/60, MIGRATION_GUIDE.md:262, and 34 other references in-repo). Applying either example fails with "no matches for kind VMImport in version apps.cozystack.io/v1". Fix: change both to v1alpha1.
[MINOR] packages/system/vm-adoption-controller/images/controller/main.go:905, a Multus namespace/name network reference is silently rebound to the tenant namespace
namespace/name is stripped to name (main.go:906-910), emitted as {"name": netRef}, and vm-instance re-prefixes the target namespace (vm.yaml:141). A source VM attached to a NAD in another namespace silently rebinds to a same-named NAD in the tenant, or fails to start if none exists, with no warning. Low impact in the namespace-local NAD model, but the cross-namespace intent is lost silently: at least log a warning when the stripped namespace differs from the target.
[MINOR] packages/core/platform/templates/bundles/iaas.yaml:180, enabling vm-import-application alone hangs on DependenciesNotReady
All three new packages are fully opt-in (the .optional.default helper emits the Package only when the name is in bundles.enabledPackages; .default selects the variant, not "enabled by default"). sources/vm-import-application.yaml declares dependsOn: [cozystack.forklift, cozystack.vm-adoption-controller, ...]. Because these are independent flags, an operator who enables only cozystack.vm-import-application gets a Package whose dependencies are never emitted and it stays DependenciesNotReady forever. The status is legible, so this is a usability/documentation gap: document that all three names must be enabled together, or gate the vm-import include on its deps.
[MINOR] packages/apps/vm-import/docs/MIGRATION_GUIDE.md:113, the guide says the components ship with the iaas bundle, but they are opt-in
The prerequisite is listed as "Forklift operator and the vm-import app deployed (the iaas bundle)". After this PR the iaas bundle only wires them as opt-in enabledPackages entries, so an iaas-bundle cluster does not get them by default. Update the prose to the actual enablement step.
[MINOR] packages/system/forklift-operator/templates/forklift-operator.yaml:7484, Forklift images are pinned to a rolling tag and hardcoded to quay.io
The component images are digest-pinned (good) but the tag is the rolling release-2.11. Upstream may garbage-collect old digests of a rolling tag, so the pin can die and block pulls; an immutable/dated tag or a cozystack mirror is more durable. The refs are hardcoded to quay.io rather than routed through the images-registry mirror, so air-gapped installs must mirror them explicitly. .github/renovate.json adds a manager that refreshes the digests, so this is a durability/air-gap caveat, not a broken pin.
[NIT] packages/system/vm-adoption-controller/values.yaml:23, controller.adoptionEnabled: true is consumed by nothing
Neither deployment.yaml (args: only --namespace, --watch-interval, --name-prefix, --default-instance-type, --default-preference) nor main.go reads it; the only adoption toggle the controller honors is the per-Plan annotation vm-import.cozystack.io/adoption-enabled (main.go:488). An operator setting adoptionEnabled: false reasonably believes adoption is globally off while it keeps running. Either wire the flag through or remove it.
Operational risks
packages/apps/vm-import/templates/plan.yaml:61, tenant self-service virt-v2v has no render-time guard. For the default (non-raw-copy) pathtargetNamespaceis always.Release.Namespace, anddashboard-resourcemap.yaml:6shows tenant self-service deployment is a supported shape. A tenant that follows the README deploys the virt-v2v conversion pod into its own namespace. Whether a baseline-PSA tenant namespace rejects that privileged pod depends on the pod's securityContext and the tenant PSA level, which cannot be exercised in a hermetic review, so the terminal symptom is unproven, but there is no render-time validation of the tenant+virt-v2v combination and no controller-side signal if the Migration wedges in ImageConversion. A render-time warning orfailfor that combination is warranted.
Caveats
- The
charts-direct-editcozy-invariant onpackages/apps/vm-import/charts/cozy-libis a false positive: that path is the standard symlink topackages/library/cozy-lib, not a vendored edit. No action needed. - Tenant isolation is sound (not a finding, recorded so it is not re-litigated): a tenant-namespace import confines the Plan
targetNamespaceand the target-namespace annotation to the release namespace, an admin-namespace import correctly honors a cross-namespace target, and the guards (resolveTargetNamespace+validateVMBelongsToPlannamespace check) are backed byTestResolveTargetNamespace/TestValidateVMBelongsToPlan/TestGetForkliftVMsRejectsForeignPlanClaim. A mutation that neuters the tenant-confine branch reddens the exact test, so the coverage is non-vacuous. The forged-plan-label attack (a tenant super-admin stamping aplanUID label on a raw kubevirt VM) is closed by the namespace check. dashboard-resourcemap.yaml:21combineslist/watchwithresourceNames, which RBAC ignores for collection verbs, so onlygetis effectively constrained. This matches the pre-existing pattern in other charts and is harmless if the dashboard reads named resources viaget; flag only if the dashboard actually listsplans/migrationsto show progress, which this Role would deny.- "Validated end-to-end on a live cluster" is the author's manual result and cannot be reproduced here (hermetic review, no cluster). The CI e2e does not exercise migration or adoption (see the e2e finding).
- Render-blind surfaces not verifiable statically: the adopted VMInstance HR applying via helm-controller SSA, the CDI cross-namespace clone converging (and whether it stalls on the attached RWO source PVC, since nothing waits for clone completion before the VMInstance Create), and the same-name Forklift-VM / VMInstance handoff window. These need a live exercise.
Recommended follow-ups
- Run
cozystack-pr-testagainst a disposable dev cluster to exercise the real adoption path (both virt-v2v cross-namespace and raw-copy same-namespace): confirm the disk-copy lifecycle, that the source VM state is handled, the CDI clone converges, thevm-instancePVC-fallback branch renders, and that the defaultu1.medium/ubuntupresets resolve (an absent instance type would leave the adopted VMInstance HR Failed while the controller reports Adopted).
| // confined to its own namespace: otherwise a tenant could name another tenant's | ||
| // namespace and have this cluster-privileged controller create a VMInstance and | ||
| // clone DataVolumes there (cross-tenant escalation). | ||
| func (c *AdoptionController) getTargetNamespace(ctx context.Context, namespace, planName string) string { |
There was a problem hiding this comment.
[MAJOR] getTargetNamespace defaults to the Plan namespace on any Get error, not just NotFound
The Get error branch is unconditional (if err != nil { return namespace }), not apierrors.IsNotFound-gated, and the Plan was already resolved in hand at the call site so this second Get is redundant. Scenario: an admin Plan in cozy-forklift carrying vm-import.cozystack.io/target-namespace: tenant-a. On a transient Get error this returns cozy-forklift; on the virt-v2v path vm.Namespace == planNamespace, so wrapDisksAsVMDisks early-returns (no clone), the VMInstance is created in the privileged namespace, and releaseSourceVM (same-namespace branch, main.go:1146-1160) detaches the PVC ownerRefs and deletes the Forklift VM. Non-retryable, the source VM is gone. getPlanPreset (main.go:553-563) has the same swallow-and-default shape and drops the operator's chosen instanceType/profile on a transient error. Fix: propagate the error and requeue on anything that is not a real NotFound, or drop the redundant Get.
|
|
||
| // Idempotency: if the VMInstance already exists, the create succeeded on an | ||
| // earlier pass and only the source VM is left to release. | ||
| if _, err := c.dynamicClient.Resource(vmInstanceGVR).Namespace(targetNamespace).Get(ctx, vmInstanceName, metav1.GetOptions{}); err == nil { |
There was a problem hiding this comment.
[MAJOR] Idempotency check matches a VMInstance by name only, with no identity verification
The "already created" short-circuit does a Get(vmInstanceName) in the target namespace and, on success, jumps straight to releaseSourceVM, never reading back the vm-import.cozystack.io/original-vm-name/-namespace annotations written on create (main.go:1032-1033); vmInstanceName == vm.Name. Scenario: a tenant already runs an unrelated VMInstance web, and a VMware VM named web is imported into the same namespace. The Get finds the pre-existing web, the controller treats it as its own earlier success, and releaseSourceVM detaches the import's PVC ownerRefs and deletes the imported Forklift VM. The import is silently destroyed and its disks orphaned; the unrelated VMInstance is untouched. Fix: verify the found VMInstance carries this import's identity annotations before releasing the source VM.
| // controller performs the cross-namespace clone itself first, so the VMDisk | ||
| // only ever references a PVC in its own namespace. | ||
| localSrc := srcPVCName | ||
| if srcPVCNs != vmDiskNs { |
There was a problem hiding this comment.
[MAJOR] Cross-namespace adoption leaks two permanent full-disk copies per disk
The virt-v2v cross-namespace path clones the imported PVC into <vmDiskName>-src (main.go:651-655), then ensureVMDisk clones that into vm-disk-<name> (main.go:627, 682). Nothing ever deletes the -src DataVolume/PVC, and the original PVC in cozy-forklift also stays. The only Delete in the controller is the kubevirt VM at main.go:1155, and RBAC could not delete volumes anyway: clusterrole.yaml:22-24 grants datavolumes no delete, clusterrole.yaml:35-37 grants PVCs no delete. Net result on the cross-namespace path is three full copies of every migrated disk, one referenced and two permanent garbage with no ownerRef for GC. The same-namespace/raw-copy path avoids this via the early return. Fix: delete the intermediate -src volume after the VMDisk clone completes (and grant the verb), or clone directly into the final target.
| targetNamespace, vmInstanceName string, disks []interface{}) error { | ||
|
|
||
| helmReleaseName := "vm-instance-" + vmInstanceName | ||
| if targetNamespace != vm.Namespace { |
There was a problem hiding this comment.
[MAJOR] Cross-namespace adoption leaves the source VM running and stamps the wrong release namespace on it
On the cross-namespace branch labelVMAsAdopted only labels the Forklift VM, never halts or deletes it (main.go:1140-1145). The source VM keeps its inherited runStrategy (main.go:767-781, default Always) and runs off the original disk next to the tenant clone, so both boot with the same guest identity and MAC. Separately, labelVMAsAdopted writes meta.helm.sh/release-namespace: <source VM namespace> (main.go:1191), the privileged namespace, for a release that actually lives in the tenant namespace: false ownership metadata that would let a future vm-instance-<name> release in cozy-forklift adopt and rewrite this VM. Fix: stop/delete the source VM on the cross-namespace path once the clone is confirmed, and omit or correct the Helm ownership annotations on the source VM.
| @@ -0,0 +1,12 @@ | |||
| {{- if .Values.vms }} | |||
There was a problem hiding this comment.
[MAJOR] warm: true can never complete and a helm-upgrade that adds a VM never migrates it
The Migration is rendered once with only spec.plan.{name,namespace}; there is no spec.cutover anywhere (grep for cutover hits docs only), while warm is a real exposed value (plan.yaml:29, values.yaml:35, values.schema.json:56, advanced-import.yaml:44). A Forklift warm migration precopies indefinitely until a cutover timestamp is set, so the Succeeded condition the adoption gate waits on (main.go:448) never turns true and the VM is never adopted. Separately, the Migration name is fixed ({{ .Release.Name }}) and its spec does not change when vms grows, so a completed Forklift Migration is not re-run: a VM added via helm upgrade is silently never migrated. Fix: do not expose warm until cutover is modeled (or provide a cutover mechanism), and give added VMs a migration path.
| verbs: ["get", "list", "watch"] | ||
|
|
||
| # Create/manage/delete VMInstances and VMDisks (imported disks become managed VMDisks) | ||
| - apiGroups: ["apps.cozystack.io"] |
There was a problem hiding this comment.
[MINOR] Cluster-wide update/patch/delete on vminstances and vmdisks are never used
The rule grants [get,list,watch,create,update,patch,delete] on vminstances and vmdisks, but the controller only Gets and Creates them; the sole Delete targets kubevirt VirtualMachines (main.go:1155) and the volume Updates target datavolumes/PVCs. Least privilege: trim vminstances/vmdisks to get, list, watch, create. Otherwise a compromised controller pod has cluster-wide delete on every tenant's VMInstances and VMDisks.
| thumbprint: "AA:BB:CC:DD:EE:FF:00:11:22:33:44:55:66:77:88:99:AA:BB:CC:DD" | ||
|
|
||
| --- | ||
| apiVersion: apps.cozystack.io/v1 |
There was a problem hiding this comment.
[MINOR] Example manifests use a non-served apiVersion
simple-import.yaml:22 and advanced-import.yaml:21 use apiVersion: apps.cozystack.io/v1, but the served version is v1alpha1 (controller main.go:55/60, MIGRATION_GUIDE.md:262, and 34 other in-repo references). Applying either example fails with "no matches for kind VMImport in version apps.cozystack.io/v1". Fix: change both to v1alpha1.
| continue | ||
| } | ||
|
|
||
| // networkName format is "namespace/name" — extract just the name part |
There was a problem hiding this comment.
[MINOR] A Multus namespace/name network reference is silently rebound to the tenant namespace
namespace/name is stripped to name (main.go:906-910), emitted as {"name": netRef}, and vm-instance re-prefixes the target namespace (vm.yaml:141). A source VM attached to a NAD in another namespace silently rebinds to a same-named NAD in the tenant, or fails to start if none exists, with no warning. Low impact in the namespace-local NAD model, but the cross-namespace intent is lost silently: at least log a warning when the stripped namespace differs from the target.
| ## 3. Prerequisites | ||
|
|
||
| ### 3.1 Cluster / platform | ||
| - Forklift operator and the `vm-import` app deployed (the `iaas` bundle). |
There was a problem hiding this comment.
[MINOR] The guide says the components ship with the iaas bundle, but they are opt-in
The prerequisite is listed as "Forklift operator and the vm-import app deployed (the iaas bundle)". After this PR the iaas bundle only wires them as opt-in enabledPackages entries, so an iaas-bundle cluster does not get them by default. Update the prose to the actual enablement step (add the three package names to bundles.enabledPackages).
| watchNamespace: "" | ||
|
|
||
| # Enable adoption only for VMs with specific labels | ||
| adoptionEnabled: true |
There was a problem hiding this comment.
[MINOR] controller.adoptionEnabled is consumed by nothing
Neither deployment.yaml (args: only --namespace, --watch-interval, --name-prefix, --default-instance-type, --default-preference) nor main.go reads adoptionEnabled; the only adoption toggle the controller honors is the per-Plan annotation vm-import.cozystack.io/adoption-enabled (main.go:488). An operator setting adoptionEnabled: false reasonably believes adoption is globally off while it keeps running. Either wire the flag through or remove it. (Severity NIT.)
IvanHunters
left a comment
IvanHunters
left a comment
There was a problem hiding this comment.
Verdict
NOT LGTM
The cross-tenant isolation design is sound and well tested, but the adoption controller's Go logic has several confirmed data-destruction paths (a transient API error and a name collision both silently delete the imported VM), the disk clone chain leaks two permanent copies per disk, warm migrations can never complete, the shipped e2e suite runs nothing, and the manual adopt-vm.sh fallback produces a values file that cannot adopt. These are blocking.
Findings
[MAJOR] packages/system/vm-adoption-controller/images/controller/main.go:520, getTargetNamespace defaults to the Plan namespace on any Get error, not just NotFound
The Get error branch is unconditional (if err != nil { return namespace }), it is not apierrors.IsNotFound-gated. The Plan was already resolved in hand at the call site (resolvePlan), so this second namespaced Get is redundant and only adds a failure mode. Scenario: an admin Plan in cozy-forklift carries vm-import.cozystack.io/target-namespace: tenant-a. On a transient Get error getTargetNamespace returns cozy-forklift. On the virt-v2v path vm.Namespace == planNamespace == cozy-forklift, so targetNamespace == vm.Namespace, wrapDisksAsVMDisks early-returns (src==dst, no clone), the VMInstance is created in the privileged namespace, and releaseSourceVM takes the same-namespace branch (main.go:1146-1160) that detaches the PVC ownerRefs and deletes the Forklift VM. It is non-retryable because the source VM the controller keys on is gone. getPlanPreset (main.go:553-563) has the same swallow-and-default-on-error shape and silently drops the operator's chosen instanceType/instanceProfile on a transient error. Fix: propagate the error and requeue on anything that is not a real NotFound, or drop the redundant Get and reuse the already-resolved Plan.
[MAJOR] packages/system/vm-adoption-controller/images/controller/main.go:996, idempotency check matches a VMInstance by name only, with no identity verification
The "already created" short-circuit does a Get(vmInstanceName) in the target namespace and, on success, jumps straight to releaseSourceVM. It never reads back the vm-import.cozystack.io/original-vm-name / -namespace annotations it writes on create (main.go:1032-1033), and vmInstanceName == vm.Name. Scenario: a tenant already runs an unrelated VMInstance web, and a VMware VM named web is imported into the same namespace. The Get finds the pre-existing web, the controller treats it as its own earlier success, and releaseSourceVM detaches the import's PVC ownerRefs and deletes the imported Forklift VM. The import is silently destroyed and its disks orphaned, while the unrelated VMInstance is untouched. Fix: verify the found VMInstance carries this import's identity annotations before releasing the source VM.
[MAJOR] packages/system/vm-adoption-controller/images/controller/main.go:651, cross-namespace adoption leaks two permanent full-disk copies per disk
The virt-v2v cross-namespace path clones the imported PVC into <vmDiskName>-src (main.go:651-655), then ensureVMDisk clones that into vm-disk-<name> (main.go:627, 682). Nothing ever deletes the -src DataVolume/PVC, and the original PVC in cozy-forklift also stays (the source VM is only labeled). The only Delete in the controller is the kubevirt VM at main.go:1155, and RBAC could not delete volumes anyway: clusterrole.yaml:22-24 grants datavolumes no delete and clusterrole.yaml:35-37 grants PVCs no delete. Net result on the cross-namespace path is three full copies of every migrated disk on replicated storage, one referenced and two permanent garbage with no ownerRef for GC. The same-namespace/raw-copy path correctly avoids this via the early return. Fix: delete the intermediate -src volume after the VMDisk clone completes (and grant the verb), or clone directly into the final target.
[MAJOR] packages/system/vm-adoption-controller/images/controller/main.go:1140, cross-namespace adoption leaves the source VM running and stamps the wrong release namespace on it
On the cross-namespace branch labelVMAsAdopted only labels the Forklift VM, it never halts or deletes it (main.go:1140-1145). The source VM keeps its inherited runStrategy (extracted at main.go:767-781, default Always) and keeps running off the original disk next to the tenant clone, so both boot with the same guest identity and MAC. Separately, labelVMAsAdopted writes meta.helm.sh/release-namespace: <source VM namespace> (main.go:1191), the privileged namespace, for a release that actually lives in the tenant namespace. That is false ownership metadata: a future vm-instance-<name> release in cozy-forklift could adopt and rewrite this VM. Fix: stop/delete the source VM on the cross-namespace path once the clone is confirmed, and either omit the Helm ownership annotations on the source VM or set them to the real release namespace.
[MAJOR] packages/apps/vm-import/templates/migration.yaml:1, warm: true can never complete and a helm-upgrade that adds a VM never migrates it
The Migration is rendered once with only spec.plan.{name,namespace}, there is no spec.cutover anywhere (grep for cutover in packages/apps/vm-import/ hits docs only). warm is a real exposed value (plan.yaml:29, values.yaml:35, values.schema.json:56, advanced-import.yaml:44). A Forklift warm migration precopies indefinitely until a cutover timestamp is set, so the Succeeded condition the adoption gate waits on (main.go:448) never turns true and the VM is never adopted. Separately, the Migration name is fixed ({{ .Release.Name }}) and its spec does not change when vms grows, so a completed Forklift Migration is not re-run: a VM added via helm upgrade is silently never migrated, with no error surfaced. Fix: either do not expose warm until cutover is modeled, or provide a cutover mechanism; and give added VMs a path to migrate (per-VM Migration, or detect the diff).
[MAJOR] hack/e2e-apps/vm-import.bats:1, the shipped e2e suite is never executed and would not have coverage if it were
CI runs chainsaw suites: hack/e2e-chainsaw/README.md:5 states this directory replaces the per-app bats suite that used to live in hack/e2e-apps/*.bats, and packages/core/testing/Makefile:31 runs test-chainsaw plus only the e2e-prepare-cluster / e2e-install-cozystack / e2e-test-openapi bats. Nothing globs hack/e2e-apps/. There is no vm-import chainsaw suite. Even if the bats ran, hack/e2e-install-cozystack.bats:242 enables only cozystack.external-dns-application in bundles.enabledPackages, so the opt-in forklift/vm-import CRDs are absent and applying a VMImport would fail with "no matches for kind". The e2e coverage this PR appears to add provides none. Fix: add a chainsaw suite under hack/e2e-chainsaw/ and enable the packages in the e2e install, or drop the dead bats file and state the tested surface honestly.
[MAJOR] packages/apps/vm-import/docs/scripts/adopt-vm.sh:76, the manual-adoption fallback script produces a values file that cannot adopt
Three independent defects. (a) Lines 76-79 strip a vm-disk- prefix that a Forklift disk never has and emit only name: with no dvName, so vm-instance resolves the non-existent DataVolume vm-disk-<fullname> and renders fail "Specified disk not exists in cluster" (vm.yaml:68, 77). The dvName field this same PR added for exactly this case is never emitted. (b) The generated values file (lines 120-178) has no fullnameOverride, and the script never runs the Helm ownership steps adoption.md:62-75 requires, so the release renders a second VM instead of adopting the existing one (adoption.md:80). (c) Line 167 sets cloudInitSeed: "imported-vm" while adoption.md:82, the controller (main.go:1015), and _helpers.tpl:57,60-68 all require cloudInitSeed: "" to preserve the firmware UUID, so following the script gives the adopted VM a fresh SMBIOS UUID and churns guest identity. Fix: emit dvName and fullnameOverride, apply the ownership labels/annotations, and set cloudInitSeed: "".
[MAJOR] packages/apps/vm-instance/values.schema.json:213, the fullnameOverride immutability rule does not block unsetting the field
The only guard is "rule": "self == oldSelf" (mirrored in cozyrds/vm-instance.yaml:11), with no default and not required. A CEL transition rule is evaluated only when the field is present in both old and new objects, so deleting fullnameOverride from an adopted VMInstance passes validation. The VirtualMachine name then reverts to the chart default (vm.yaml:15), Helm renames the object, prunes the running imported VM and creates a fresh one: exactly the disruptive re-roll the rule was added to prevent. Fix: make the field required on this app, or move the immutability guard to the parent object so the unset transition is covered.
[MAJOR] packages/apps/vm-instance/templates/vm.yaml:68, the new dvName field and DataVolume-vs-PVC fallback branch ship with no automated test
This rewrites the disk-rendering block of vm-instance, a core already-deployed chart backing every VM, not only imported ones. It adds the disk.dvName override and a new branch that references a bare persistentVolumeClaim when no DataVolume of the resolved name exists (vm.yaml:120-126). The only file under packages/apps/vm-instance/tests/ is hook_securitycontext_test.yaml, nothing exercises the dvName path or the PVC-fallback path, and the vm-import bats (already dead, see above) never migrates a VM so the branch is never reached in CI. The DataVolume path is preserved for existing VMs (with dvName unset $dvName resolves to vm-disk-<name>, unchanged), so this is not a confirmed regression, but a new render branch on a chart this load-bearing needs a regression test. helm-unittest can seed lookup via kubernetesProvider (sibling charts do this): add a case seeding only a PVC (assert persistentVolumeClaim.claimName) and one setting disk.dvName (assert the override is used).
[MINOR] packages/system/vm-adoption-controller/templates/clusterrole.yaml:17, cluster-wide update/patch/delete on vminstances and vmdisks are never used
The rule grants ["get","list","watch","create","update","patch","delete"] on vminstances and vmdisks, but the controller only ever Gets and Creates them; the sole Delete targets kubevirt VirtualMachines (main.go:1155) and the volume Updates target datavolumes/PVCs. Least privilege: trim vminstances/vmdisks to get, list, watch, create. A compromised controller pod otherwise has cluster-wide delete on every tenant's VMInstances and VMDisks.
[MINOR] packages/apps/vm-import/docs/examples/simple-import.yaml:22, example manifests use a non-served apiVersion
simple-import.yaml:22 and advanced-import.yaml:21 use apiVersion: apps.cozystack.io/v1, but the served version is v1alpha1 (controller at main.go:55/60, MIGRATION_GUIDE.md:262, and 34 other references in-repo). Applying either example fails with "no matches for kind VMImport in version apps.cozystack.io/v1". Fix: change both to v1alpha1.
[MINOR] packages/system/vm-adoption-controller/images/controller/main.go:905, a Multus namespace/name network reference is silently rebound to the tenant namespace
namespace/name is stripped to name (main.go:906-910), emitted as {"name": netRef}, and vm-instance re-prefixes the target namespace (vm.yaml:141). A source VM attached to a NAD in another namespace silently rebinds to a same-named NAD in the tenant, or fails to start if none exists, with no warning. Low impact in the namespace-local NAD model, but the cross-namespace intent is lost silently: at least log a warning when the stripped namespace differs from the target.
[MINOR] packages/core/platform/templates/bundles/iaas.yaml:180, enabling vm-import-application alone hangs on DependenciesNotReady
All three new packages are fully opt-in (the .optional.default helper emits the Package only when the name is in bundles.enabledPackages; .default selects the variant, not "enabled by default"). sources/vm-import-application.yaml declares dependsOn: [cozystack.forklift, cozystack.vm-adoption-controller, ...]. Because these are independent flags, an operator who enables only cozystack.vm-import-application gets a Package whose dependencies are never emitted and it stays DependenciesNotReady forever. The status is legible, so this is a usability/documentation gap: document that all three names must be enabled together, or gate the vm-import include on its deps.
[MINOR] packages/apps/vm-import/docs/MIGRATION_GUIDE.md:113, the guide says the components ship with the iaas bundle, but they are opt-in
The prerequisite is listed as "Forklift operator and the vm-import app deployed (the iaas bundle)". After this PR the iaas bundle only wires them as opt-in enabledPackages entries, so an iaas-bundle cluster does not get them by default. Update the prose to the actual enablement step.
[MINOR] packages/system/forklift-operator/templates/forklift-operator.yaml:7484, Forklift images are pinned to a rolling tag and hardcoded to quay.io
The component images are digest-pinned (good) but the tag is the rolling release-2.11. Upstream may garbage-collect old digests of a rolling tag, so the pin can die and block pulls; an immutable/dated tag or a cozystack mirror is more durable. The refs are hardcoded to quay.io rather than routed through the images-registry mirror, so air-gapped installs must mirror them explicitly. .github/renovate.json adds a manager that refreshes the digests, so this is a durability/air-gap caveat, not a broken pin.
[NIT] packages/system/vm-adoption-controller/values.yaml:23, controller.adoptionEnabled: true is consumed by nothing
Neither deployment.yaml (args: only --namespace, --watch-interval, --name-prefix, --default-instance-type, --default-preference) nor main.go reads it; the only adoption toggle the controller honors is the per-Plan annotation vm-import.cozystack.io/adoption-enabled (main.go:488). An operator setting adoptionEnabled: false reasonably believes adoption is globally off while it keeps running. Either wire the flag through or remove it.
Operational risks
packages/apps/vm-import/templates/plan.yaml:61, tenant self-service virt-v2v has no render-time guard. For the default (non-raw-copy) pathtargetNamespaceis always.Release.Namespace, anddashboard-resourcemap.yaml:6shows tenant self-service deployment is a supported shape. A tenant that follows the README deploys the virt-v2v conversion pod into its own namespace. Whether a baseline-PSA tenant namespace rejects that privileged pod depends on the pod's securityContext and the tenant PSA level, which cannot be exercised in a hermetic review, so the terminal symptom is unproven, but there is no render-time validation of the tenant+virt-v2v combination and no controller-side signal if the Migration wedges in ImageConversion. A render-time warning orfailfor that combination is warranted.
Caveats
- The
charts-direct-editcozy-invariant onpackages/apps/vm-import/charts/cozy-libis a false positive: that path is the standard symlink topackages/library/cozy-lib, not a vendored edit. No action needed. - Tenant isolation is sound (not a finding, recorded so it is not re-litigated): a tenant-namespace import confines the Plan
targetNamespaceand the target-namespace annotation to the release namespace, an admin-namespace import correctly honors a cross-namespace target, and the guards (resolveTargetNamespace+validateVMBelongsToPlannamespace check) are backed byTestResolveTargetNamespace/TestValidateVMBelongsToPlan/TestGetForkliftVMsRejectsForeignPlanClaim. A mutation that neuters the tenant-confine branch reddens the exact test, so the coverage is non-vacuous. The forged-plan-label attack (a tenant super-admin stamping aplanUID label on a raw kubevirt VM) is closed by the namespace check. dashboard-resourcemap.yaml:21combineslist/watchwithresourceNames, which RBAC ignores for collection verbs, so onlygetis effectively constrained. This matches the pre-existing pattern in other charts and is harmless if the dashboard reads named resources viaget; flag only if the dashboard actually listsplans/migrationsto show progress, which this Role would deny.- "Validated end-to-end on a live cluster" is the author's manual result and cannot be reproduced here (hermetic review, no cluster). The CI e2e does not exercise migration or adoption (see the e2e finding).
- Render-blind surfaces not verifiable statically: the adopted VMInstance HR applying via helm-controller SSA, the CDI cross-namespace clone converging (and whether it stalls on the attached RWO source PVC, since nothing waits for clone completion before the VMInstance Create), and the same-name Forklift-VM / VMInstance handoff window. These need a live exercise.
Recommended follow-ups
- Run
cozystack-pr-testagainst a disposable dev cluster to exercise the real adoption path (both virt-v2v cross-namespace and raw-copy same-namespace): confirm the disk-copy lifecycle, that the source VM state is handled, the CDI clone converges, thevm-instancePVC-fallback branch renders, and that the defaultu1.medium/ubuntupresets resolve (an absent instance type would leave the adopted VMInstance HR Failed while the controller reports Adopted).
| // confined to its own namespace: otherwise a tenant could name another tenant's | ||
| // namespace and have this cluster-privileged controller create a VMInstance and | ||
| // clone DataVolumes there (cross-tenant escalation). | ||
| func (c *AdoptionController) getTargetNamespace(ctx context.Context, namespace, planName string) string { |
There was a problem hiding this comment.
[MAJOR] getTargetNamespace defaults to the Plan namespace on any Get error, not just NotFound
The Get error branch is unconditional (if err != nil { return namespace }), not apierrors.IsNotFound-gated, and the Plan was already resolved in hand at the call site so this second Get is redundant. Scenario: an admin Plan in cozy-forklift carrying vm-import.cozystack.io/target-namespace: tenant-a. On a transient Get error this returns cozy-forklift; on the virt-v2v path vm.Namespace == planNamespace, so wrapDisksAsVMDisks early-returns (no clone), the VMInstance is created in the privileged namespace, and releaseSourceVM (same-namespace branch, main.go:1146-1160) detaches the PVC ownerRefs and deletes the Forklift VM. Non-retryable, the source VM is gone. getPlanPreset (main.go:553-563) has the same swallow-and-default shape and drops the operator's chosen instanceType/profile on a transient error. Fix: propagate the error and requeue on anything that is not a real NotFound, or drop the redundant Get.
|
|
||
| // Idempotency: if the VMInstance already exists, the create succeeded on an | ||
| // earlier pass and only the source VM is left to release. | ||
| if _, err := c.dynamicClient.Resource(vmInstanceGVR).Namespace(targetNamespace).Get(ctx, vmInstanceName, metav1.GetOptions{}); err == nil { |
There was a problem hiding this comment.
[MAJOR] Idempotency check matches a VMInstance by name only, with no identity verification
The "already created" short-circuit does a Get(vmInstanceName) in the target namespace and, on success, jumps straight to releaseSourceVM, never reading back the vm-import.cozystack.io/original-vm-name/-namespace annotations written on create (main.go:1032-1033); vmInstanceName == vm.Name. Scenario: a tenant already runs an unrelated VMInstance web, and a VMware VM named web is imported into the same namespace. The Get finds the pre-existing web, the controller treats it as its own earlier success, and releaseSourceVM detaches the import's PVC ownerRefs and deletes the imported Forklift VM. The import is silently destroyed and its disks orphaned; the unrelated VMInstance is untouched. Fix: verify the found VMInstance carries this import's identity annotations before releasing the source VM.
| // controller performs the cross-namespace clone itself first, so the VMDisk | ||
| // only ever references a PVC in its own namespace. | ||
| localSrc := srcPVCName | ||
| if srcPVCNs != vmDiskNs { |
There was a problem hiding this comment.
[MAJOR] Cross-namespace adoption leaks two permanent full-disk copies per disk
The virt-v2v cross-namespace path clones the imported PVC into <vmDiskName>-src (main.go:651-655), then ensureVMDisk clones that into vm-disk-<name> (main.go:627, 682). Nothing ever deletes the -src DataVolume/PVC, and the original PVC in cozy-forklift also stays. The only Delete in the controller is the kubevirt VM at main.go:1155, and RBAC could not delete volumes anyway: clusterrole.yaml:22-24 grants datavolumes no delete, clusterrole.yaml:35-37 grants PVCs no delete. Net result on the cross-namespace path is three full copies of every migrated disk, one referenced and two permanent garbage with no ownerRef for GC. The same-namespace/raw-copy path avoids this via the early return. Fix: delete the intermediate -src volume after the VMDisk clone completes (and grant the verb), or clone directly into the final target.
| targetNamespace, vmInstanceName string, disks []interface{}) error { | ||
|
|
||
| helmReleaseName := "vm-instance-" + vmInstanceName | ||
| if targetNamespace != vm.Namespace { |
There was a problem hiding this comment.
[MAJOR] Cross-namespace adoption leaves the source VM running and stamps the wrong release namespace on it
On the cross-namespace branch labelVMAsAdopted only labels the Forklift VM, never halts or deletes it (main.go:1140-1145). The source VM keeps its inherited runStrategy (main.go:767-781, default Always) and runs off the original disk next to the tenant clone, so both boot with the same guest identity and MAC. Separately, labelVMAsAdopted writes meta.helm.sh/release-namespace: <source VM namespace> (main.go:1191), the privileged namespace, for a release that actually lives in the tenant namespace: false ownership metadata that would let a future vm-instance-<name> release in cozy-forklift adopt and rewrite this VM. Fix: stop/delete the source VM on the cross-namespace path once the clone is confirmed, and omit or correct the Helm ownership annotations on the source VM.
| @@ -0,0 +1,12 @@ | |||
| {{- if .Values.vms }} | |||
There was a problem hiding this comment.
[MAJOR] warm: true can never complete and a helm-upgrade that adds a VM never migrates it
The Migration is rendered once with only spec.plan.{name,namespace}; there is no spec.cutover anywhere (grep for cutover hits docs only), while warm is a real exposed value (plan.yaml:29, values.yaml:35, values.schema.json:56, advanced-import.yaml:44). A Forklift warm migration precopies indefinitely until a cutover timestamp is set, so the Succeeded condition the adoption gate waits on (main.go:448) never turns true and the VM is never adopted. Separately, the Migration name is fixed ({{ .Release.Name }}) and its spec does not change when vms grows, so a completed Forklift Migration is not re-run: a VM added via helm upgrade is silently never migrated. Fix: do not expose warm until cutover is modeled (or provide a cutover mechanism), and give added VMs a migration path.
| verbs: ["get", "list", "watch"] | ||
|
|
||
| # Create/manage/delete VMInstances and VMDisks (imported disks become managed VMDisks) | ||
| - apiGroups: ["apps.cozystack.io"] |
There was a problem hiding this comment.
[MINOR] Cluster-wide update/patch/delete on vminstances and vmdisks are never used
The rule grants [get,list,watch,create,update,patch,delete] on vminstances and vmdisks, but the controller only Gets and Creates them; the sole Delete targets kubevirt VirtualMachines (main.go:1155) and the volume Updates target datavolumes/PVCs. Least privilege: trim vminstances/vmdisks to get, list, watch, create. Otherwise a compromised controller pod has cluster-wide delete on every tenant's VMInstances and VMDisks.
| thumbprint: "AA:BB:CC:DD:EE:FF:00:11:22:33:44:55:66:77:88:99:AA:BB:CC:DD" | ||
|
|
||
| --- | ||
| apiVersion: apps.cozystack.io/v1 |
There was a problem hiding this comment.
[MINOR] Example manifests use a non-served apiVersion
simple-import.yaml:22 and advanced-import.yaml:21 use apiVersion: apps.cozystack.io/v1, but the served version is v1alpha1 (controller main.go:55/60, MIGRATION_GUIDE.md:262, and 34 other in-repo references). Applying either example fails with "no matches for kind VMImport in version apps.cozystack.io/v1". Fix: change both to v1alpha1.
| continue | ||
| } | ||
|
|
||
| // networkName format is "namespace/name" — extract just the name part |
There was a problem hiding this comment.
[MINOR] A Multus namespace/name network reference is silently rebound to the tenant namespace
namespace/name is stripped to name (main.go:906-910), emitted as {"name": netRef}, and vm-instance re-prefixes the target namespace (vm.yaml:141). A source VM attached to a NAD in another namespace silently rebinds to a same-named NAD in the tenant, or fails to start if none exists, with no warning. Low impact in the namespace-local NAD model, but the cross-namespace intent is lost silently: at least log a warning when the stripped namespace differs from the target.
| ## 3. Prerequisites | ||
|
|
||
| ### 3.1 Cluster / platform | ||
| - Forklift operator and the `vm-import` app deployed (the `iaas` bundle). |
There was a problem hiding this comment.
[MINOR] The guide says the components ship with the iaas bundle, but they are opt-in
The prerequisite is listed as "Forklift operator and the vm-import app deployed (the iaas bundle)". After this PR the iaas bundle only wires them as opt-in enabledPackages entries, so an iaas-bundle cluster does not get them by default. Update the prose to the actual enablement step (add the three package names to bundles.enabledPackages).
| watchNamespace: "" | ||
|
|
||
| # Enable adoption only for VMs with specific labels | ||
| adoptionEnabled: true |
There was a problem hiding this comment.
[MINOR] controller.adoptionEnabled is consumed by nothing
Neither deployment.yaml (args: only --namespace, --watch-interval, --name-prefix, --default-instance-type, --default-preference) nor main.go reads adoptionEnabled; the only adoption toggle the controller honors is the per-Plan annotation vm-import.cozystack.io/adoption-enabled (main.go:488). An operator setting adoptionEnabled: false reasonably believes adoption is globally off while it keeps running. Either wire the flag through or remove it. (Severity NIT.)
…imported VM getTargetNamespace and getPlanPreset re-fetched the Plan the caller had already resolved; a transient (non-NotFound) Get error defaulted the target to the Plan's own namespace, collapsing a cross-namespace adoption into an in-place one and deleting the source VM non-retryably. Read the already- resolved Plan instead, which removes the failure mode entirely. The idempotency short-circuit matched an existing VMInstance by name only (vmInstanceName == vm.Name), so a tenant's unrelated VMInstance that merely shared an imported VM's name made the controller release -- and delete -- the imported VM while leaving the stranger's VMInstance untouched. Verify the found VMInstance carries this import's identity annotations before releasing the source VM. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Andrei Kvapil <andrei.kvapil@aenix.io>
…ted VMs, trim RBAC On the cross-namespace path labelVMAsAdopted wrote the source VM's own (privileged) namespace as meta.helm.sh/release-namespace, so a future vm-instance release created in that privileged namespace could adopt and rewrite the VM. Record the actual adoption-target namespace instead. The controller only gets and creates VMInstances and VMDisks, so drop the unused update/patch/delete verbs from its ClusterRole (least privilege). Remove the dead controller.adoptionEnabled value: nothing reads it, the real toggle is the per-Plan vm-import.cozystack.io/adoption-enabled annotation. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Andrei Kvapil <andrei.kvapil@aenix.io>
…fallback The vm-import work rewrote the disk-rendering block of vm-instance (a core chart backing every VM) to add a per-disk dvName override and a branch that references a bare PersistentVolumeClaim when no DataVolume of the resolved name exists. Neither path had a test. Add helm-unittest cases for the dvName override, the unchanged vm-disk-<name> default, and the PVC fallback. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Andrei Kvapil <andrei.kvapil@aenix.io>
The Migration is rendered once with only spec.plan and no spec.cutover, so a Forklift warm migration precopies indefinitely and the Succeeded condition the adoption controller waits on never turns true -- the VM is never adopted. Remove the warm value, schema property, template field, examples and docs until a cutover mechanism is modeled. Also fix the example manifests' non-served apiVersion (apps.cozystack.io/v1 -> v1alpha1) and correct the MIGRATION_GUIDE prose: the forklift, vm-adoption-controller and vm-import-application packages are opt-in bundles.enabledPackages entries, not shipped with the iaas bundle by default. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Andrei Kvapil <andrei.kvapil@aenix.io>
…e is dropped on adoption The network mapping strips a networkName's namespace/ prefix and vm-instance re-adds the target namespace, so a source VM attached to a NAD in another namespace silently rebinds to a same-named NAD in the adoption target -- or fails to start if none exists -- with no signal. Log a warning naming the dropped namespace. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Andrei Kvapil <andrei.kvapil@aenix.io>
CI runs chainsaw suites; hack/e2e-apps/*.bats is no longer globbed by any runner (packages/core/testing/Makefile), and even if it ran the e2e install enables only external-dns so the opt-in vm-import CRDs are absent. The file provided zero coverage and misrepresented the tested surface. The honest automated coverage is helm-unittest plus the controller's go test. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Andrei Kvapil <andrei.kvapil@aenix.io>
…tually adopts The fallback script emitted disks with no dvName (so vm-instance looked up a nonexistent vm-disk-<name> and failed), no fullnameOverride and no Helm ownership step (so the release rendered a second VM instead of adopting), and cloudInitSeed: imported-vm (churning the firmware UUID). Emit dvName per disk, add fullnameOverride plus the Helm ownership label/annotations, and set cloudInitSeed to empty so the existing-VM lookup preserves the UUID. Also drop a fail-open || true in the summary. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Andrei Kvapil <andrei.kvapil@aenix.io>
|
Thanks — worked through the review. Addressed on the branch: Data-destruction paths:
Charts / e2e / script:
Minors: RBAC trimmed to get/list/watch/create on vminstances/vmdisks; example Follow-up (not in this push): the two cross-namespace clone-lifecycle findings — the leaked
These fixes were prepared with AI assistance and reviewed before pushing. |
| ## @field {*SourceUpload} [upload] - Upload local image. | ||
| ## @field {*SourceHTTP} [http] - Download image from an HTTP source. | ||
| ## @field {*SourceDisk} [disk] - Clone an existing vm-disk. | ||
| ## @field {*SourcePVC} [pvc] - Clone an existing PersistentVolumeClaim. |
There was a problem hiding this comment.
Should a user be exposed to the underlying core k8s object? I think that's breaking the isolation supposed to exist between a tenant of Cozystack and the somewhat lower level API of k8s.
Timofei Larkin (lllamnyp)
left a comment
Timofei Larkin (lllamnyp)
left a comment
There was a problem hiding this comment.
Thanks for this — the Forklift integration itself is solid work, the templates are careful, and the guard comments explaining why each namespace check exists made this much easier to review. My comments are scoped to the tenant-facing API surface rather than the implementation, which others have covered thoroughly.
Blocking
vddkInitImage and virtV2vImage let a tenant name a container image the cluster will run. (packages/apps/vm-import/values.yaml:12,22 → templates/provider.yaml:15, templates/plan.yaml:32) This is a hard no in Cozystack: no arbitrary code runs in the cluster without a virtualization boundary. virtV2vImage is worse than a generic image field because it names the conversion pod, which values.yaml:18 correctly describes as privileged. Neither field should be reachable from a tenant-supplied value. Both look like platform configuration — a cluster-level setting on the Forklift package, or a fixed reference in the chart, with the VDDK image built and pinned by the platform rather than named per-import.
Fields that only work with cluster-admin assistance should not be on the tenant API at all. sourceSecretName is required (values.schema.json:156-159), but no tenant access level can create a Secret: packages/system/cozystack-basics/templates/clusterroles.yaml grants the core group only pods, services, persistentvolumes, endpoints, events, resourcequotas at view/use (:99-110) and adds delete on those same five at admin (:216-224); secrets appears in no tenant role, and core.cozystack.io/tenantsecrets is get,list,watch only (:47-50, :188-192). So a tenant cannot fill in a valid VMImport without an admin creating the Secret out of band. migrationHosts[].secretName (values.yaml:77) has the same problem. Unlike postgres, which lets credentials come in through values and materializes the Secret itself, vm-import offers no in-values path — the field a tenant can't satisfy is the only one there is.
The same rule catches tenantNamespace (values.yaml:37-38) and networkMap[].destinationNamespace (values.yaml:54). Both are documented as honored only outside a tenant-* namespace, and both templates silently coerce them back (plan.yaml:7-11, networkmap.yaml:26-30). A field that every tenant can set, that is silently ignored for every tenant, is worse than an absent one.
The minimal standard for adoption is the clean automated path: the import produces managed Cozystack objects, with nothing left for a human to wire up by name. Right now that holds on the cross-namespace path — ensureVMDisk (main.go:625-685) creates a real VMDisk — but not on the same-namespace one, where wrapDisksAsVMDisks returns early (main.go:701-704) and the adopted VMInstance points at raw Forklift PVCs. Those disks have no Cozystack representation: they are not VMDisks, can't be resized, cloned, or backed up through the VMDisk API, and the imported-0 values land in a disks[].name field whose x-cozystack-options {source: vmdisk} picker will never contain them.
That gap is what put three system-internal knobs onto the tenant API, and I'd like to see all three come back off once it closes:
VMDisk.source.pvc— a tenant has no way to create or name a PVC, and cloning a Cozystack-managed disk is alreadysource.disk. In this PR the only caller is the adoption controller.VMInstance.disks[].dvName— with thepersistentVolumeClaimfallback (vm.yaml:120-127) this resolves to any PVC in the namespace by name, so a tenant can attach the volume behind a Postgres cluster or a Bucket straight into a VM. Separate from the test-coverage point already raised: even fully tested, this is more reach than the field needs. If it has to stay for now, constraining it tovm-disk-*would close the hole.VMInstance.fullnameOverride— reaches the schema through ayq -iline inpackages/apps/vm-instance/Makefile:13that runs aftercozyvalues-gen, so it exists invalues.schema.jsonand the ApplicationDefinition but not invalues.yaml,README.md, orapi/apps/v1alpha1/vminstance/types.go. The documented VMInstance API and the one the dashboard renders have quietly diverged, and the field's own description says it is there for the adoption controller.
Worth settling alongside this: whether VMImport is a tenant application at all. It is registered in the tenant catalog under IaaS (packages/system/vm-import-rd/cozyrds/vm-import.yaml:20-25), yet its default path needs the privileged conversion pod that, per plan.yaml:60-64, a baseline tenant forbids — in a tenant namespace, "this namespace" is the tenant's own. If the answer is admin-only, that should be structural rather than five paragraphs of "ignored when you set it."
Medium
Given the size of the new surface — a new app, a new cluster-scoped controller, a new ApplicationDefinition, and changes to two existing tenant APIs — this really belongs in the community repo as a design proposal first. The API questions above are design questions, and settling them there is cheaper than iterating on an 81-file PR.
Minor
The VMImport ApplicationDefinition still declares warm: packages/system/vm-import-rd/cozyrds/vm-import.yaml:11 (openAPISchema) and :30 (keysOrder). The field is gone from values.yaml and values.schema.json, and hack/update-crd.sh regenerates both from the chart — a make generate in packages/apps/vm-import should clear it. As it stands the tenant API advertises a migration mode the chart ignores.
storageMap[].storageClass (values.yaml:65) and networkMap[].destinationName (values.yaml:53) are the sanctioned kind of exposure but lack @x-cozystack-options {source: storageclass} / {source: network}, so the tenant types cluster object names blind where every comparable field offers a picker.
main.go:930-941 sets spec.firmware on the VMInstance, but firmware is in neither values.yaml nor values.schema.json for vm-instance, so structural pruning drops it and UEFI guests silently adopt as BIOS. The code comment points at #3002 — worth making that a stated merge-order dependency rather than a comment.
The fork-PR e2e lane never ran. `resolve` looked the PR up with `listPullRequestsAssociatedWithCommit` against the base repo, and that endpoint returns nothing for a fork PR head: the commit reaches cozystack/cozystack only as `refs/pull/<n>/head`, and the association index does not walk `refs/pull/*`. So every fork PR fail-closed on "expected exactly one open PR for <sha>, found 0", stamped a red required "E2E Tests" its author could not clear by pushing, and skipped publish/e2e/report. It resolves same-repo heads correctly, which is why the hole stayed invisible — same-repo PRs get "E2E Tests" from pull-requests.yaml and never enter this workflow. Of 935 runs, 930 skipped as designed and all 5 that reached `resolve` stopped there; three on `found 0`, and one of those reds was later overwritten by a mirrored same-repo run, which hid it again. Enumerate the base repo's open pull requests instead and match on the head SHA. Same trust properties as the call it replaces — plain base-repo API data the fork cannot forge — and `state: 'open'` at the source keeps the rule that a closed or merged PR never counts. The exactly-one guard and the head_sha match are unchanged, so a stacked lower PR still passes and two PRs genuinely at the same head still fail closed. Pagination stops being a corner case and becomes load-bearing: the repo has 180 open PRs, and the fork PR that surfaced this sits on page two. Verified against the live API for the failing case: PR cozystack#1982 at 984936f now resolves to exactly one match, with labels present so the full-e2e override still reads. actionlint, zizmor and `node --check` on the extracted script all pass. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com>
… behind Upstream publishes these images only under rolling channel tags, so every rebuild of release-2.11 garbage-collects the digest we pinned. Fourteen of the sixteen pins were already dead: the operator image failed with 'not found' and the whole package could not install. The two that survived are exactly the two pinned to dated tags, forklift-console-plugin:release-v2.6.7 and forklift-must-gather:v2.7.1. Re-pin the fourteen to the digests their tags resolve to today, in the Makefile and the rendered manifest together. Verified by installing the package on a live cluster: the operator reaches Running and reconciles its operand. This buys working installs, not durability. Pinning a digest against a tag that moves cannot be durable, and there are no immutable tags upstream to pin instead, so the lasting fix is mirroring these images the way the platform mirrors everything else. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Andrei Kvapil <andrei.kvapil@aenix.io>
VMware imports need the proprietary VDDK image, which Cozystack can neither ship nor mirror. Deliver it the way every other platform-wide setting travels: an operator sets the vddk-image key in the cozystack ConfigMap (or migration.vddkImage in the platform values) and it lands in the _cluster map that every app already receives, alongside root-host and the wildcard Secret name. Only a reference rides the channel, never a credential -- the same rule the wildcard-certificate block states. Empty is the normal state and means the VMware path is unavailable, not broken. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Andrei Kvapil <andrei.kvapil@aenix.io>
A tenant naming a container image the cluster will run is not something Cozystack permits, and virtV2vImage is the worse of the two because it names the privileged conversion pod. Neither is a tenant decision. vddkInitImage becomes platform configuration read from the _cluster channel. If the operator has not configured it, a vsphere VMImport now fails at render with a message naming the missing setting, so the tenant learns at order time rather than partway through a transfer. virtV2vImage is dropped outright; Forklift falls back to its own default and the operator pins it through the Forklift package. Regenerating the schema, README and ApplicationDefinition also clears the stale warm advertisement the generated ApplicationDefinition still carried after the option was removed from the chart. Adds a provider test suite covering both refusal paths and the configured one. BREAKING CHANGE: VMImport no longer accepts vddkInitImage or virtV2vImage. The VDDK image is now set once per cluster via the vddk-image key in the cozystack ConfigMap. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Andrei Kvapil <andrei.kvapil@aenix.io>
|
Closing in favour of #3931, which implements the redesigned tenant API described in cozystack/community#62. The mechanism this PR established carries over; what changed is the tenant-facing surface. Review here established that several fields could not stay on a tenant API — container images the cluster would run, and Secret references no tenant access level can satisfy — and reworking that in place would have meant iterating on an 80-file PR. The new API is a pair of CRDs in Your work is carried into #3931 rather than restarted. The Forklift packaging — operator, ForkliftController CR, digest pinning — moves over, and you are credited as co-author there ( The lab findings from this PR are what shaped the new design and could not have been found from code review: the Thank you for carrying this since February, and for the lab work behind it. |
6 participants
What this PR does
Integrate Forklift v2.11.5 as the KubeVirt-native migration toolkit and add a tenant-facing
vm-importapp to migrate virtual machines from VMware vSphere into Cozystack. VMs are imported through Forklift (virt-v2v guest conversion, or VDDK raw-copy) and then adopted as managedVMInstance/VMDiskresources by a dedicatedvm-adoption-controller, so a migrated VM becomes a dashboard-managed workload in the user's own tenant. Cold and warm migrations and network/storage mapping are supported.New packages
packages/system/forklift-operator/— Forklift operator (CRDs + deployment); component images are pinned to immutable digestspackages/system/forklift/— ForkliftController CRpackages/system/vm-adoption-controller/— adopts Forklift-imported VMs as Cozystack VMInstancespackages/apps/vm-import/— tenant-facing VM import applicationpackages/system/vm-import-rd/— ApplicationDefinition for the dashboard (IaaS category)bundles/iaas.yamlwiringIsolation
vm-importis tenant-facing, so cross-namespace targets are guarded on both the raw-copy chart render (Plan.spec.targetNamespace) and the adoption controller: a release inside atenant-*namespace is confined to its own namespace and cannot direct an import at another tenant.Test plan
helm unittestforvm-import,vm-disk,forklift-operator,vm-adoption-controllergo testfor the adoption controller (the cross-tenant guard, plan/migration resolution, adoption gating)Summary by CodeRabbit
New Features
Documentation
Tests
Packaging