feat(vm-import): add VMware VM import via Forklift#1982
feat(vm-import): add VMware VM import via Forklift#1982matthieu-robin wants to merge 8 commits intocozystack:maincozystack/cozystack:mainfrom matthieu-robin:vmware-importmatthieu-robin/cozystack:vmware-importCopy head branch name to clipboard
Conversation
|
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)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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, 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-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
|
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.
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, 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. |
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.
| stringData: | ||
| user: migration-svc@vsphere.local | ||
| password: "ComplexP@ssw0rd!" | ||
| thumbprint: "11:22:33:44:55:66:77:88:99:AA:BB:CC:DD:EE:FF:00:11:22:33" |
There was a problem hiding this comment.
Placeholder thumbprint is one octet short of a valid SHA-1 fingerprint.
SHA-1 fingerprints are 20 bytes (20 colon-separated hex pairs). This example has only 19 pairs. Users referencing this format may produce invalid thumbprints. The simple-import example correctly uses 20 pairs.
Proposed fix
- thumbprint: "11:22:33:44:55:66:77:88:99:AA:BB:CC:DD:EE:FF:00:11:22:33"
+ thumbprint: "11:22:33:44:55:66:77:88:99:AA:BB:CC:DD:EE:FF:00:11:22:33:44"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| thumbprint: "11:22:33:44:55:66:77:88:99:AA:BB:CC:DD:EE:FF:00:11:22:33" | |
| thumbprint: "11:22:33:44:55:66:77:88:99:AA:BB:CC:DD:EE:FF:00:11:22:33:44" |
🤖 Prompt for AI Agents
In `@packages/apps/vm-import/docs/examples/advanced-import.yaml` at 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.
| # 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.
| // Extract running state | ||
| running, _, _ := unstructured.NestedBool(spec, "running") | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, understand the context of the code in question
cd packages/system/vm-adoption-controller/images/controller
head -300 main.go | tail -40Repository: cozystack/cozystack
Length of output: 1505
🏁 Script executed:
# Check the vm-instance chart structure and look for how running state is handled
find . -path "*/vm-instance/*" -type f -name "*.yaml" -o -name "*.yml" | head -20Repository: cozystack/cozystack
Length of output: 1653
🏁 Script executed:
# Search for runStrategy usage in the codebase
rg 'runStrategy' --type yaml -A 3 -B 3Repository: cozystack/cozystack
Length of output: 2662
🏁 Script executed:
# Check the vm-instance chart values for running/runStrategy handling
find . -path "*/charts/vm-instance/*" -type f \( -name "values.yaml" -o -name "*.yaml" \) | head -10Repository: cozystack/cozystack
Length of output: 45
🏁 Script executed:
# Read the vm.yaml template to see how the VM is created
cat packages/apps/vm-instance/templates/vm.yamlRepository: cozystack/cozystack
Length of output: 4306
🏁 Script executed:
# Read the values.yaml for vm-instance
cat packages/apps/vm-instance/values.yamlRepository: cozystack/cozystack
Length of output: 2544
🏁 Script executed:
# Check how the 'running' variable is used after line 276
sed -n '276,350p' packages/system/vm-adoption-controller/images/controller/main.goRepository: cozystack/cozystack
Length of output: 2460
🏁 Script executed:
# Search for Forklift references and integration
rg -i 'forklift|import' --type yaml --type go | grep -i forklift | head -20Repository: cozystack/cozystack
Length of output: 2287
🏁 Script executed:
# Find where VMInstance is created and what values are passed
sed -n '325,450p' packages/system/vm-adoption-controller/images/controller/main.goRepository: cozystack/cozystack
Length of output: 4316
🏁 Script executed:
# Search for how the running value is used when creating VMInstance
rg 'running' packages/system/vm-adoption-controller/images/controller/main.go -A 2 -B 2Repository: cozystack/cozystack
Length of output: 523
🏁 Script executed:
# Look for Helm/Values operations that might pass the running parameter
rg 'Values|values|\.running' packages/system/vm-adoption-controller/images/controller/main.goRepository: cozystack/cozystack
Length of output: 45
🏁 Script executed:
# Search for runStrategy handling in the adoption controller or related code
rg 'runStrategy' packages/system/vm-adoption-controller/ -A 3 -B 3Repository: cozystack/cozystack
Length of output: 45
🏁 Script executed:
# Check the Forklift integration and plan structure
cat packages/system/forklift/templates/forklift-cr.yamlRepository: cozystack/cozystack
Length of output: 356
🏁 Script executed:
# Search for any KubeVirt CRD schema that shows spec.running and runStrategy interaction
rg 'spec\.running|runStrategy' packages/system/capi-providers-infraprovider/files/infrastructure-components.yaml | head -50Repository: cozystack/cozystack
Length of output: 140
🏁 Script executed:
# Check if there's any test or example showing Forklift VM import behavior
find . -name "*test*" -o -name "*example*" -type f | xargs grep -l "forklift\|runStrategy" 2>/dev/null | head -10Repository: cozystack/cozystack
Length of output: 45
🏁 Script executed:
# Look more carefully at the infrastructure-components to understand the KubeVirt CRD
sed -n '1,100p' packages/system/capi-providers-infraprovider/files/infrastructure-components.yaml | grep -A 20 "spec:"Repository: cozystack/cozystack
Length of output: 2226
🏁 Script executed:
# Search the entire codebase for any handling of runStrategy in adoption or vm-instance context
rg 'runStrategy' --type yaml --type go -B 5 -A 5Repository: cozystack/cozystack
Length of output: 4262
🏁 Script executed:
# Check if there's any KubeVirt version specification or documentation about running vs runStrategy
find . -name "*.md" -o -name "*.txt" -o -name "*readme*" -o -name "*kube*" | xargs grep -i "runStrategy\|spec.running" 2>/dev/null | head -20Repository: cozystack/cozystack
Length of output: 342
🏁 Script executed:
# Look at the vm-update-hook template to see if it handles running state changes
cat packages/apps/vm-instance/templates/vm-update-hook.yamlRepository: cozystack/cozystack
Length of output: 3439
🏁 Script executed:
# Check the complete vm.yaml to see full context of how running is used
wc -l packages/apps/vm-instance/templates/vm.yaml && grep -n "running\|runStrategy" packages/apps/vm-instance/templates/vm.yamlRepository: cozystack/cozystack
Length of output: 146
🏁 Script executed:
# Check adoption documentation to see what it says about running/runStrategy
cat packages/apps/vm-import/docs/adoption.mdRepository: cozystack/cozystack
Length of output: 8896
🏁 Script executed:
# Search for Forklift documentation or examples that show how it creates VMs
find . -path "*forklift*" -type f \( -name "*.md" -o -name "*.yaml" \) | head -10 | xargs cat 2>/dev/null | grep -A 20 -B 5 "runStrategy\|spec.running" | head -50Repository: cozystack/cozystack
Length of output: 45
Extract and preserve both spec.running and spec.runStrategy when adopting VMs.
The controller currently extracts only spec.running using NestedBool, which returns false as the zero value if the field is absent. If a Forklift-imported VM uses spec.runStrategy (e.g., Always) instead of or in addition to spec.running, the adoption will create a VMInstance with running: false. Since the vm-instance chart template sets spec.running: {{ .Values.running }} on the VirtualMachine, this overwrites the imported VM's running state and will stop a running VM during adoption.
Implement logic to:
- Check
spec.runStrategyifspec.runningis not present and map it to the running boolean (e.g.,Always/RerunOnFailure→ true,Halted→ false,Manual→ preserve current state) - Handle the KubeVirt constraint that
runStrategyandrunningare mutually exclusive
The adoption documentation itself demonstrates this concern via fallback: RUNNING=$(kubectl get vm ... || echo "false").
| ```yaml | ||
| # Watch interval | ||
| controller: | ||
| watchInterval: 30 # Check for new VMs every 30 seconds | ||
|
|
||
| # Optional: watch specific namespace only | ||
| controller: | ||
| watchNamespace: "tenant-production" | ||
|
|
||
| # Optional: prefix for created VMInstance names | ||
| controller: | ||
| namePrefix: "imported-" | ||
| ``` |
There was a problem hiding this comment.
Duplicate controller: keys in the YAML example will mislead users.
The three separate controller: blocks at the same YAML level mean only the last one (namePrefix) takes effect. Users copying this will lose watchInterval and watchNamespace. Combine them into a single block:
📝 Suggested fix
-```yaml
-# Watch interval
-controller:
- watchInterval: 30 # Check for new VMs every 30 seconds
-
-# Optional: watch specific namespace only
-controller:
- watchNamespace: "tenant-production"
-
-# Optional: prefix for created VMInstance names
-controller:
- namePrefix: "imported-"
-```
+```yaml
+controller:
+ watchInterval: 30 # Check for new VMs every 30 seconds
+ watchNamespace: "tenant-production" # Optional: watch specific namespace only
+ namePrefix: "imported-" # Optional: prefix for created VMInstance names
+```🤖 Prompt for AI Agents
In `@packages/system/vm-adoption-controller/README.md` around lines 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.
ac75ab9 to
8f3f95b
Compare
|
Hey @kvaps, 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
Integrate Forklift v2.10.4 (KubeVirt-native migration toolkit) to enable importing VMs from VMware vSphere into KubeVirt. Supports cold and warm migrations with automatic guest conversion via virt-v2v. New packages: - system/forklift-operator: Forklift operator CRDs + deployment - system/forklift: ForkliftController CR - apps/vm-import: user-facing app for VMware VM migration - system/vm-import-rd: ApplicationDefinition for dashboard Closes cozystack#250 Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> Signed-off-by: Matthieu <matthieu@hidora.com>
- Add .PHONY: generate to Makefile - Fix GitHub capitalization in README - Validate destinationType (pod/multus) and require destinationName for multus - Gate network and storage map references independently in Plan Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> Signed-off-by: Matthieu <matthieu@hidora.com>
Add comprehensive VMware VM import functionality with automatic adoption
into Cozystack ecosystem via new vm-adoption-controller.
## New Components
### vm-adoption-controller (packages/system/vm-adoption-controller/)
- Kubernetes controller that automatically adopts Forklift-imported VMs
- Watches VirtualMachines with forklift.konveyor.io/plan label
- Creates VMInstance CRDs for Cozystack dashboard integration
- 446 lines of production-ready Go code with robust error handling
Key features:
- Safe type assertions to prevent panics
- Automatic rollback on labeling failures
- Plan caching (5min TTL) to reduce API calls
- Configurable defaults (instanceType, preference)
- Reduced watch interval (15s instead of 30s)
- Comprehensive logging for debugging
RBAC:
- Minimal permissions (no delete, no wildcards)
- Hardened container (non-root, read-only FS, no caps)
### vm-import enhancements (packages/apps/vm-import/)
- Add enableAdoption parameter (default: true)
- Add adoption annotations on Plan resources
- Add helm.sh/resource-policy: keep on Providers
- Fix enableAdoption: false handling (use hasKey instead of default)
- Comprehensive documentation with adoption lifecycle
Documentation added:
- ADOPTION_DESIGN.md: architectural design and rationale
- TEST_REPORT.md: validation test results
- docs/adoption.md: detailed adoption guide
- docs/examples/: simple and advanced import examples
- docs/scripts/adopt-vm.sh: manual adoption helper
## Modified Components
### forklift-operator (packages/system/forklift-operator/)
- Fix empty namespace fields (add {{ .Release.Namespace }})
- Fix env vars with numeric values (add quotes)
### Integration (packages/core/platform/sources/)
- Add vm-adoption-controller.yaml PackageSource
- Add vm-adoption-controller dependency to vm-import-application
## Architecture
VMware vSphere
↓
Forklift Migration (creates VirtualMachine)
↓
vm-adoption-controller (detects & adopts)
↓
VMInstance CRD (Cozystack)
↓
Dashboard visibility ✓
## Lifecycle
- VMs are NOT deleted when vm-import is removed
- Providers are kept for reuse (helm.sh/resource-policy: keep)
- VMInstances remain manageable via Cozystack dashboard
- Full start/stop/console/resources management available
## Testing
- ✅ Go compilation successful
- ✅ Helm templates validated
- ✅ Template rendering with various configurations
- ✅ Behavior with empty vms: [] validated
- ⏳ E2E tests require Forklift + vCenter setup
## Addresses
Closes kvaps' concerns about dashboard visibility and VM lifecycle.
VMs are now automatically adopted and fully manageable in Cozystack.
Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Signed-off-by: Matthieu <matthieu@hidora.com>
Add three E2E test scenarios for vm-import package: - Full configuration with networkMap, storageMap, and adoption - Minimal configuration without optional mappings - Adoption disabled scenario Tests verify: - Kubernetes resource creation (HelmRelease, Providers, Plan, etc.) - Forklift integration with correct annotations - Conditional resource generation - Multi-tenant RBAC permissions - Cozystack adoption controller annotations Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com> Signed-off-by: Matthieu <matthieu@hidora.com>
The vm-adoption-controller had several architectural issues preventing imported VMs from appearing in the Cozystack dashboard: - Helm created a duplicate VM instead of adopting the existing one. Fixed by passing fullnameOverride and pre-annotating the Forklift VM with Helm ownership metadata (meta.helm.sh/release-name). - Disk lookup failed because vm-instance expected vm-disk-<name> but Forklift DVs have arbitrary names. Added dvName field for direct DataVolume references. - Multus networks were lost (subnets always empty). Now extracted from the existing VM spec. - Used deprecated running boolean instead of runStrategy enum. - cloudInitSeed changed firmware UUID, risking cloud-init re-execution. Also fixes: PackageSource wrong API, controller missing from IaaS bundle, default adoption=true on Plan lookup failure, DISK_COUNT crash in adopt-vm.sh, and cleans up AI-generated documentation. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Matthieu <matthieu@hidora.com>
- Move dvName field definition to values.yaml so cozyvalues-gen picks it up instead of manually editing the auto-generated schema - Add language tag to fenced code block in ADOPTION_DESIGN.md (MD040) - Document that vms: [] is a no-op in README.md - Rewrite docs/adoption.md to match actual implementation: automatic adoption creates VMInstance CRDs with full dashboard management, not read-only labeling Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Matthieu <matthieu@hidora.com>
… examples spec.running is deprecated since KubeVirt v1.3. Use runStrategy: Always/Halted in kubectl patch examples. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Matthieu <matthieu@hidora.com>
Previous run failed due to Keycloak OIDC timeout (flaky infra). Signed-off-by: Matthieu Robin <matthieu@music-all.com> Signed-off-by: Matthieu <matthieu@hidora.com>
ef6a0de to
af8586d
Compare
Summary
vm-importapp package enabling users to configure VMware source, select VMs, and map networks/storageNew packages
packages/system/forklift-operator/— Forklift operator (CRDs + deployment)packages/system/forklift/— ForkliftController CRpackages/apps/vm-import/— User-facing VM import applicationpackages/system/vm-import-rd/— ApplicationDefinition for dashboard (IaaS category)packages/core/platform/sources/forklift.yaml— PackageSource for Forkliftpackages/core/platform/sources/vm-import-application.yaml— PackageSource for vm-importModified
packages/core/platform/templates/bundles/iaas.yaml— Added Forklift + vm-import to IaaS bundleCloses #250
Test plan
helm templaterenders correctly for all 4 packages🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Documentation
Tests
Packaging