MTV-2996: AWS EC2#928
Draft
Jenny-Anne wants to merge 5 commits into
Draft
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughThis pull request adds comprehensive AWS EC2 virtual machine migration documentation to the Forklift documentation repository. It introduces prerequisites, provider setup workflows, network/storage mapping procedures, plan creation guidance, and CLI-based migration steps, organized into planning and migration guide assemblies. ChangesAWS EC2 Migration Documentation
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@documentation/modules/common-attributes.adoc`:
- Line 20: The global attribute `:aws:` is currently defined as `:aws: AWS`
which breaks conditional gating; change the text-substitution attribute to a new
name (e.g. `:aws-name:`) and remove or leave `:aws:` undefined so
`ifdef::aws[]`/`ifndef::aws[]` conditionals work correctly—update
common-attributes.adoc to define `:aws-name: AWS` instead of `:aws:` and then
update any docs that reference the textual AWS name to use `:aws-name:` while
leaving `:aws:` reserved for conditional toggling used by
`ifdef::aws[]`/`:!aws:` blocks (also verify assembly_planning-migration-aws.adoc
and modules that rely on `ifdef::aws[]` now gate correctly).
In `@documentation/modules/proc_migrating-vms-cli-aws.adoc`:
- Line 78: Remove the unresolved SME review note and either add concrete
examples for creating network and storage maps using the oc mtv CLI or delete
the comment; specifically include representative oc mtv create-network-map and
oc mtv create-storage-map command examples (showing required flags such as
--source-network/--target-network and --source-storage/--target-storage, any
mapping file or inline mappings, and a brief expected-success output) so readers
can follow exact commands, or remove the SME marker entirely if you do not want
to include examples now.
- Around line 25-33: The example for the provider creation command currently
includes the insecure flag --provider-insecure-skip-tls by default; remove that
flag from the primary example (the `mtv create provider <provider_name> --type
ec2 --ec2-region <region> --username $EC2_KEY --password $EC2_SECRET` command)
and instead add a short separate note or alternate example showing the optional
use of --provider-insecure-skip-tls for cases where TLS validation must be
bypassed.
In `@documentation/modules/ref_aws-ec2-limitations.adoc`:
- Line 16: The doc statement "Cross-account migration is not supported"
conflicts with the provider procedure that documents a "Use cross-account
credentials" flow; decide which behavior is correct and make both modules
consistent. Either update this sentence in ref_aws-ec2-limitations.adoc to state
that cross-account migration is supported and reference the provider procedure's
"Use cross-account credentials" flow, or remove/adjust the cross-account
credentials flow in the provider procedure to match the limitation; ensure both
places use identical wording about cross-account support and, if supported, add
any prerequisites or limitations mentioned in the provider procedure.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ab669ac6-aa00-443a-b8d0-c0d0c82a41ff
📒 Files selected for processing (12)
documentation/doc-Migrating_your_virtual_machines/assemblies/assembly_migrating-from-aws.adocdocumentation/doc-Migrating_your_virtual_machines/master.adocdocumentation/doc-Planning_your_migration/assemblies/assembly_planning-migration-aws.adocdocumentation/doc-Planning_your_migration/master.adocdocumentation/modules/common-attributes.adocdocumentation/modules/proc_adding-source-provider.adocdocumentation/modules/proc_creating-form-based-network-maps-ui-aws.adocdocumentation/modules/proc_creating-form-based-storage-maps-ui-aws.adocdocumentation/modules/proc_creating-plan-wizard-aws.adocdocumentation/modules/proc_migrating-vms-cli-aws.adocdocumentation/modules/ref_aws-ec2-limitations.adocdocumentation/modules/ref_aws-ec2-prerequisites.adoc
Jenny-Anne
force-pushed
the
MTV-2996-aws-ec2
branch
from
6aa102a to
f44b95c
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (1)
documentation/modules/proc_migrating-vms-cli-aws.adoc (1)
18-22:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRemove unresolved internal SME questions before publishing.
These inline authoring questions are still present in the user-facing module and should be resolved or removed before merge. See Line 18-Line 22.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@documentation/modules/proc_migrating-vms-cli-aws.adoc` around lines 18 - 22, Remove the inline authoring questions (the four comment lines asking about Secret/Provider, NetworkMap, StorageMap, and Plan manifests for AWS EC2) from the module and either replace them with finalized guidance or delete them entirely so no unresolved SME questions remain in the published content; locate the four lines shown in the diff and update the text to a final user-facing sentence or remove the placeholders.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@documentation/modules/proc_creating-plan-wizard-aws.adoc`:
- Around line 5-7: Remove the unresolved author-note questions "// Is
cross-account migration supported?" and "// Is this correct (EBS volumes map to
EBS-backed storage classes only)?" from the document and either resolve them
with final wording or replace them with definitive statements; update the
section that referenced cross-account migration and the EBS storage-class
mapping to state the supported behavior and mapping explicitly (e.g.,
"Cross-account migration is/ is not supported: <final behavior>" and "EBS
volumes map to <final storage-class mapping>"), and if you need copy help use
the reviewer’s offer to draft the final sentences instead of leaving inline
questions.
- Around line 22-23: The two docs conflict: the plan-wizard AWS module currently
states "Cross-account migration is not supported" while the provider docs
describe a "Use cross-account credentials" path; decide which behavior is
correct, verify actual implementation, and make the texts consistent: either
update the bullets in documentation/modules/proc_creating-plan-wizard-aws.adoc
to state that cross-account migration is supported and describe the required
cross-account credential steps (replace the line "* {project-short} supports
migration within the same {aws} account only. Cross-account migration is not
supported."), or update the provider procedure to remove or disable the "Use
cross-account credentials" path if cross-account is truly unsupported—ensure
both places reference the same verified behavior and include a short note naming
the required credentials/constraints (e.g., cross-account role, region
restriction) so readers aren’t confused.
In `@documentation/modules/proc_creating-yaml-based-network-maps-ui-aws.adoc`:
- Around line 29-56: Remove the shell/heredoc wrapper so the snippet is pure
YAML for the UI editor: delete the leading "$ cat << EOF | {oc} apply -f -"
line and the trailing "EOF" and "----" so the snippet begins with "apiVersion:
forklift.konveyor.io/v1beta1" and contains the "kind: NetworkMap" and the
spec/map/provider blocks as plain YAML acceptable to the UI (preserve the
existing "apiVersion", "kind: NetworkMap", "metadata", "spec.map" entries and
provider/source/destination fields).
In `@documentation/modules/proc_creating-yaml-based-storage-maps-ui-aws.adoc`:
- Around line 38-58: The UI YAML/JSON example currently includes a shell heredoc
wrapper ("$ cat << EOF | {oc} apply -f - ... EOF") which will break if pasted
into the editor; replace the entire heredoc block with only the raw YAML
manifest for the StorageMap (beginning with "apiVersion:
forklift.konveyor.io/v1beta1" and "kind: StorageMap") so users can paste the
manifest directly into the UI editor and edit fields like metadata.name,
metadata.namespace, spec.map, spec.provider.source.name, and
spec.provider.destination.name without shell syntax.
In `@documentation/modules/proc_migrating-vms-cli-aws.adoc`:
- Around line 35-37: The example ownerReferences block is incomplete and will
produce an invalid manifest; update the sample ownerReferences entries so they
include the required fields by either removing the ownerReferences block from
the base example or providing full placeholder values (add name: <PROVIDER_NAME>
and uid: <PROVIDER_UID> alongside the existing apiVersion and kind) for both
occurrences shown; ensure the placeholders are clearly labeled so users replace
them when copying the manifest.
---
Duplicate comments:
In `@documentation/modules/proc_migrating-vms-cli-aws.adoc`:
- Around line 18-22: Remove the inline authoring questions (the four comment
lines asking about Secret/Provider, NetworkMap, StorageMap, and Plan manifests
for AWS EC2) from the module and either replace them with finalized guidance or
delete them entirely so no unresolved SME questions remain in the published
content; locate the four lines shown in the diff and update the text to a final
user-facing sentence or remove the placeholders.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8aae723f-0bec-4411-9512-ad0c95f9ed8a
📒 Files selected for processing (15)
documentation/doc-Migrating_your_virtual_machines/assemblies/assembly_migrating-from-aws.adocdocumentation/doc-Migrating_your_virtual_machines/master.adocdocumentation/doc-Planning_your_migration/assemblies/assembly_planning-migration-aws.adocdocumentation/doc-Planning_your_migration/assemblies/assembly_provider-specific-requirements-for-migration.adocdocumentation/doc-Planning_your_migration/master.adocdocumentation/modules/common-attributes.adocdocumentation/modules/proc_adding-source-provider.adocdocumentation/modules/proc_canceling-migration-cli-entire.adocdocumentation/modules/proc_creating-form-based-network-maps-ui-aws.adocdocumentation/modules/proc_creating-form-based-storage-maps-ui-aws.adocdocumentation/modules/proc_creating-plan-wizard-aws.adocdocumentation/modules/proc_creating-yaml-based-network-maps-ui-aws.adocdocumentation/modules/proc_creating-yaml-based-storage-maps-ui-aws.adocdocumentation/modules/proc_migrating-vms-cli-aws.adocdocumentation/modules/ref_aws-ec2-prerequisites.adoc
✅ Files skipped from review due to trivial changes (10)
- documentation/modules/proc_canceling-migration-cli-entire.adoc
- documentation/modules/common-attributes.adoc
- documentation/doc-Planning_your_migration/assemblies/assembly_provider-specific-requirements-for-migration.adoc
- documentation/doc-Planning_your_migration/master.adoc
- documentation/doc-Migrating_your_virtual_machines/master.adoc
- documentation/doc-Planning_your_migration/assemblies/assembly_planning-migration-aws.adoc
- documentation/modules/proc_creating-form-based-storage-maps-ui-aws.adoc
- documentation/doc-Migrating_your_virtual_machines/assemblies/assembly_migrating-from-aws.adoc
- documentation/modules/ref_aws-ec2-prerequisites.adoc
- documentation/modules/proc_creating-form-based-network-maps-ui-aws.adoc
Add missing name and uid fields to ownerReferences block in Secret manifest to match VMware, OpenStack, and RHV patterns. Also add createdForProviderType label. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Jenny-Anne
commented
May 25, 2026
| // | ||
| // * documentation/doc-Planning_your_migration/assemblies/assembly_planning-migration-aws.adoc | ||
|
|
||
| // Is cross-account migration supported? |
Collaborator
Author
There was a problem hiding this comment.
Please confirm:
- Is cross-account migration supported or not?
- Can EBS volumes map to EBS-backed storage classes only?
Jenny-Anne
commented
May 25, 2026
| ** `ec2:DescribeVolumes` | ||
| ** `ec2:CreateVolume` | ||
| ** `ec2:CreateTags` | ||
|
|
Collaborator
Author
There was a problem hiding this comment.
^^ Does this list cover the minimum required IAM permissions?
Member
There was a problem hiding this comment.
yaacov
reviewed
Jun 12, 2026
|
|
||
| The wizard is designed to lead you step-by-step in creating a migration plan. | ||
|
|
||
| Limitations:: |
Member
There was a problem hiding this comment.
an important limitataion missing is that we only support VMs using RHEL OSs , e.g. we don't support migration of Windows or AWS OS ( AWS OS is an OS commonly used on EC2 and it is important to note we don't support them, on any non RHEL OS at this release )
yaacov
reviewed
Jun 12, 2026
| metadata: | ||
| name: <secret> | ||
| namespace: <namespace> | ||
| ownerReferences: |
Member
There was a problem hiding this comment.
when using the API directly adding an owner is not part of the regular flow, see the upstream docs:
Ref:
https://github.com/kubev2v/forklift/tree/main/pkg/provider/ec2#manual-setup-yaml
yaacov
reviewed
Jun 12, 2026
| type: Opaque | ||
| stringData: | ||
| accessKeyId: <aws_access_key_id> | ||
| secretAccessKey: <aws_secret_access_key> |
Member
There was a problem hiding this comment.
you are missing the target secret fields, see ref docs:
https://github.com/kubev2v/forklift/tree/main/pkg/provider/ec2#secret-fields
yaacov
reviewed
Jun 12, 2026
| secret: | ||
| name: <secret> | ||
| namespace: <namespace> | ||
| EOF |
Member
There was a problem hiding this comment.
you are missing target az, see ref:
https://github.com/kubev2v/forklift/tree/main/pkg/provider/ec2#provider-resource
yaacov
reviewed
Jun 12, 2026
| ---- | ||
| $ cat << EOF | {oc} apply -f - | ||
| apiVersion: forklift.konveyor.io/v1beta1 | ||
| kind: NetworkMap |
Member
There was a problem hiding this comment.
that is just a regular network map, not different then any other
yaacov
reviewed
Jun 12, 2026
| spec: | ||
| map: | ||
| - destination: | ||
| storageClass: <storage_class> |
Member
There was a problem hiding this comment.
storage class must be ec2 backed storage class pointing to the same az we point the provider az when we created the provider
yaacov
reviewed
Jun 12, 2026
| [source,yaml,subs="attributes+"] | ||
| ---- | ||
| $ cat << EOF | {oc} apply -f - | ||
| apiVersion: forklift.konveyor.io/v1beta1 |
Member
There was a problem hiding this comment.
this is a regular plan + migration like we have for other proivders, nothing special for ec2
yaacov
reviewed
Jun 12, 2026
| The following prerequisites apply to {aws} {ec2} migrations: | ||
|
|
||
| * Red Hat OpenShift Service on {aws} ({rosa}) or OpenShift Dedicated (OSD) on {aws} clusters | ||
| * EBS CSI driver installed and configured on the target cluster |
Member
There was a problem hiding this comment.
when creating the storage map, we will map to the storage class using this csi driver
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
2 participants
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.
Jira: https://redhat.atlassian.net/browse/MTV-2996
Previews:
Planning guide:
Planning a migration of virtual machines from AWS EC2: https://forklift-documentation-git-fork-je-a0bdd0-yaacov-8047s-projects.vercel.app/downstream/documentation/doc-Planning_your_migration/master.html#assembly_planning-migration-aws_mtv
Migrating guide:
Migrating from AWS EC2: https://forklift-documentation-git-fork-je-a0bdd0-yaacov-8047s-projects.vercel.app/downstream/documentation/doc-Migrating_your_virtual_machines/master.html#assembly_migrating-from-aws_mtv
Summary by CodeRabbit