Correct the Forklift install pages after the RKE2 test run - #2995
Open
ctauchen wants to merge 1 commit into
Open
Correct the Forklift install pages after the RKE2 test run#2995ctauchen wants to merge 1 commit into
ctauchen wants to merge 1 commit into
Conversation
A test run of the v2.12.5-v3.24.0-2.0 release build on a five-node RKE2 cluster found five problems in the install pages. Each one was true on a uniform, well-provisioned cluster and wrong or unusable on a real one. The seccomp and AppArmor profile fields belong to the one ForkliftController, so they apply to every conversion pod in the cluster. The page said to set only the seccomp pair "on nodes that do not enforce AppArmor", which assumes every node is the same. A cluster whose conversion-capable nodes are mixed has no valid setting: naming the AppArmor profile makes the kubelet refuse the pod where AppArmor is absent, and omitting it lets the pod start where AppArmor is enforced, and the remount then fails. The page now gives the two valid configurations, and points to convertorNodeSelector on the Plan for mixed clusters. The storage prerequisite asked only for CDI and a default StorageClass. Migrations also fail before conversion when the volume cannot honour the file locking qemu-img takes, which NFS-backed storage needs mount options for. The address-preservation check named only the interface-scoped annotations. A primary NIC mapped by a type: pod item carries the unscoped keys instead, so a reader following the page concluded that preservation had failed when it had worked. Both forms are now shown, one per case. The command for reading the profile installer's log aggregated every pod into unlabelled lines, losing the node attribution the following step depends on. It now reads one pod at a time. The calico struct was shown as a fragment, which left its place in the YAML ambiguous. One complete map, with a type: pod item and a type: multus item, now shows it as a property of destination. Both the 3.24-2 and the unversioned copies are updated, since nothing compares them.
✅ Deploy Preview succeeded!Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify project configuration. |
✅ Deploy Preview for calico-docs-preview-next ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The new NFS example recommends nolock, which disables file locking and contradicts the stated qemu-img locking requirement, risking incorrect guidance in four updated docs.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Updates the Calico Enterprise Forklift (non-OLM) installation docs for both Kubernetes and OpenShift to reflect real-world behavior observed on an RKE2 test cluster, improving correctness around storage prerequisites, seccomp/AppArmor setup, network mapping, and verification steps.
Changes:
- Add a storage prerequisite clarifying that migrated-disk StorageClasses must support
qemu-imgfile locking (including guidance for NFS-backed storage). - Clarify cluster-wide implications of
ForkliftControllervirt-v2v profile settings, improve log collection to preserve per-node attribution, and document mixed-cluster handling viaspec.convertorNodeSelector. - Provide a complete
NetworkMapexample showing wheredestination.calicobelongs, and clarify how to verify address-preservation annotations for primary vs secondary NICs.
File summaries
| File | Description |
|---|---|
| calico-enterprise/networking/kubevirt/install-forklift-openshift.mdx | Adds storage locking prerequisite; clarifies NetworkMap.destination.calico placement and address-preservation verification. |
| calico-enterprise/networking/kubevirt/install-forklift-kubernetes.mdx | Adds storage locking prerequisite; improves virt-v2v profile guidance/log reading; clarifies mixed AppArmor clusters; updates network/annotation examples. |
| calico-enterprise_versioned_docs/version-3.24-2/networking/kubevirt/install-forklift-openshift.mdx | Mirrors OpenShift doc fixes into the 3.24-2 versioned copy. |
| calico-enterprise_versioned_docs/version-3.24-2/networking/kubevirt/install-forklift-kubernetes.mdx | Mirrors Kubernetes doc fixes into the 3.24-2 versioned copy. |
Review details
- Files reviewed: 4/4 changed files
- Comments generated: 4
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+46
to
+48
| The StorageClass backing the migrated disks must support the file locking `qemu-img` takes on an image it opens. | ||
| A volume that cannot honor those locks fails the migration before conversion starts. | ||
| NFS-backed storage needs mount options that permit locking, such as `nfsvers=3,nolock` or NFSv4, set on the StorageClass rather than only on the export. |
Comment on lines
29
to
+31
| - A StorageClass for the migrated VM disks. | ||
| It must support the file locking `qemu-img` takes on an image it opens: a volume that cannot honor those locks fails the migration before conversion starts. | ||
| NFS-backed storage needs mount options that permit locking, such as `nfsvers=3,nolock` or NFSv4, set on the StorageClass rather than only on the export. |
Comment on lines
+46
to
+48
| The StorageClass backing the migrated disks must support the file locking `qemu-img` takes on an image it opens. | ||
| A volume that cannot honor those locks fails the migration before conversion starts. | ||
| NFS-backed storage needs mount options that permit locking, such as `nfsvers=3,nolock` or NFSv4, set on the StorageClass rather than only on the export. |
Comment on lines
29
to
+31
| - A StorageClass for the migrated VM disks. | ||
| It must support the file locking `qemu-img` takes on an image it opens: a volume that cannot honor those locks fails the migration before conversion starts. | ||
| NFS-backed storage needs mount options that permit locking, such as `nfsvers=3,nolock` or NFSv4, set on the StorageClass rather than only on the export. |
Collaborator
Author
|
PTAL @lwr20 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
A test run of the Forklift v2.12.5-v3.24.0-2.0 release build on a five-node RKE2 cluster (CE 3.24-2, BPF, KubeVirt) found five problems in the install pages. Migration itself worked end to end. Each problem was true on a uniform, well-provisioned cluster and wrong or unusable on a real one, which matters because this non-OLM route exists for RKE2 customers.
Mixed-cluster limitation, step 4 of the Kubernetes page. The four virt_v2v profile fields belong to the one ForkliftController, so they apply to every conversion pod in the cluster, and there is no per-node or per-Plan override. The page said to set only the seccomp pair "on nodes that do not enforce AppArmor", which quietly assumes every node is the same. A cluster whose conversion-capable nodes are mixed has no valid setting: naming the AppArmor profile makes the kubelet refuse the conversion pod where AppArmor is absent, and omitting it lets the pod start where AppArmor is enforced, and the remount then fails. The page now gives the two valid configurations and points to convertorNodeSelector on the Plan for mixed clusters. Both configurations were verified separately on the test cluster.
Storage prerequisite, Before you begin. The pages asked only for CDI and a StorageClass. Migrations also fail before conversion starts when the volume cannot honor the file locking qemu-img takes on an image it opens, which NFS-backed storage needs mount options for. This cost a day on the release candidate.
Address-preservation annotations. The verification step named only the interface-scoped keys. A primary NIC, mapped by a type: pod item with a calico block, carries the keys with no interface name instead, so a reader following the page concluded that preservation had failed when it had worked. Both forms are real, and the page now shows one per case, with a pointer to the existing guidance on choosing the interface name.
Profile installer log, step 4 of the Kubernetes page. The page promised a log that "says which it did on each node", then gave a command that aggregates every pod into unlabelled lines. On a mixed cluster that is exactly the information needed for the step that follows. It now reads one pod at a time, under each node's name.
The calico struct, step 5. Its fields and a complete type: multus item were shown, but never a complete item containing calico, which left its place in the YAML ambiguous. One complete map, with a type: pod item and a type: multus item, now shows calico as a property of destination, matching the CRD.
Both the 3.24-2 and the unversioned copies of both pages are updated, since nothing in CI compares them.
Not in this PR, and raised separately: a sixth issue in the report, the ForkliftController never reaching Successful on Kubernetes, needs no doc change. It was fixed in tigera/forklift #2 and #3, and the published images were re-cut from that fix, so there is nothing to document around.
Changed pages on the deploy preview, 3.24-2:
https://deploy-preview-2995--tigera.netlify.app/calico-enterprise/3.24/networking/kubevirt/install-forklift-kubernetes
https://deploy-preview-2995--tigera.netlify.app/calico-enterprise/3.24/networking/kubevirt/install-forklift-openshift
The unversioned copies hold the same content and build on the next preview:
https://deploy-preview-2995--calico-docs-preview-next.netlify.app/calico-enterprise/next/networking/kubevirt/install-forklift-kubernetes