Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Introduce enhancement for mapping OVA disks #1213

Merged
merged 3 commits into from
Dec 19, 2024

Conversation

mansam
Copy link
Contributor

@mansam mansam commented Nov 26, 2024

Storage Mapping for multi-disk OVA Imports

Summary

In the current release of Forklift it is possible to import OVA appliances
that have been exported from a vSphere environment. However, all of
an appliance's disks are mapped to the same storage class, and it is
not possible for the end user to indicate that certain disks should be
mapped differently from others. In order to support more use cases, it
should be possible to select map a destination storage class for each
disk of an OVA appliance individually. Forklift does not normally allow
such fine-grained storage mapping because it becomes untenable with a
large number of VMs, but because an OVA plan is expected to involve only
a small number of VMs it should be acceptable.

Example StorageMap

spec:
  map:
    - destination:
        storageClass: fast
      source:
        id: vm1-disk1
    - destination:
        storageClass: slow
      source:
        id: vm1-disk2
    - destination:
        storageClass: fast
      source:
        id: vm2-disk1
    - destination:
        storageClass: default
      source:
        id: default
  provider:
    destination:
      name: host
      namespace: default
    source:
      name: ova
      namespace: default

Goals

  • Forklift will enable end users to individually map each disk of a
    vSphere OVA appliance to a destination storage class of their choice.

Non-Goals

  • Forklift will not allow fine-grained storage mapping for any VM
    providers other than OVA.

Proposal

The OVA inventory adapter should be modified to surface each disk from
each OVA appliance as its own source storage class. The OVA VM builder
would then be modified to respect these mappings when building the
DataVolumes rather than assuming only a single mapping.

Security, Risks, and Mitigations

No new security risks are introduced by permitting disks to be mapped
individually.

Design Details

Test Plan

Existing tests for the OVA provider should be updated to include
individual disk mappings. No additional tests should be necessary.

Upgrade / Downgrade Strategy

Permitting OVA disks to be mapped individually does not require any
changes to the update or downgrade path of Forklift itself, although
Plans created with this feature would not be compatible with a downgraded
version of Forklift as the previous version would not recognize the individual
disk mappings.

Open Questions

Should we continue, to support a default mapping or require each disk to
be specifically mapped? Allowing a default would preserve the current
ease of use for simple cases, but may make it easier to make mistakes
since plan validation would not be able to ensure that each of the disks
had been mapped as desired.

Signed-off-by: Sam Lucidi <slucidi@redhat.com>
@mansam mansam marked this pull request as ready for review November 26, 2024 19:11
@mansam mansam requested review from mnecas and yaacov as code owners November 26, 2024 19:11
docs/enhancements/multidisk-ova.md Outdated Show resolved Hide resolved
docs/enhancements/multidisk-ova.md Outdated Show resolved Hide resolved
docs/enhancements/multidisk-ova.md Outdated Show resolved Hide resolved
Signed-off-by: Sam Lucidi <slucidi@redhat.com>
Copy link

sonarqubecloud bot commented Dec 3, 2024

Copy link
Member

@mnecas mnecas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Comment on lines +105 to +109
Should we continue, to support a default mapping or require each disk to
be specifically mapped? Allowing a default would preserve the current
ease of use for simple cases, but may make it easier to make mistakes
since plan validation would not be able to ensure that each of the disks
had been mapped as desired.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After further discussion, we have decided to explicitly specify the mappings and leave the "default" option up to the forklift-console-plugin.

@mnecas mnecas merged commit 3407c1c into kubev2v:main Dec 19, 2024
13 of 15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants