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

feat: support vmdk import for vm instance creation [WD-14587] #863

Merged
merged 1 commit into from
Sep 20, 2024

Conversation

mas-who
Copy link
Collaborator

@mas-who mas-who commented Aug 27, 2024

Done

  • Add support for VMDK import to create VM instances.
  • Combined the old "Upload Instance" modal with the new functionality. Users may select the relevant file type to access additional form inputs.

QA

  1. Run the LXD-UI:
  2. Perform the following QA steps:
    • You can download the ubuntu .vmdk image from https://cloud-images.ubuntu.com/releases/22.04/release-20240514/
    • Go to create instance page
    • Click on "Upload instance file" button
    • Select "External format" file type
    • Select the .vmdk file and confirm upload.
    • You should see the file upload progress and following that you should be redirected to the instance list page where you can see the conversion progress.

@webteam-app
Copy link

@mas-who mas-who changed the title feat: support vmdk import for vm instance creation feat: support vmdk import for vm instance creation [WD-13429] Aug 27, 2024
@mas-who mas-who force-pushed the vmdk-import branch 4 times, most recently from 77dff5e to beecc25 Compare August 28, 2024 17:04
Copy link
Collaborator

@edlerd edlerd left a comment

Choose a reason for hiding this comment

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

QA for the happy path looks fine, some comments on the interaction and small ideas for the code.

src/pages/instances/actions/ConvertToInstanceBtn.tsx Outdated Show resolved Hide resolved
src/util/helpers.tsx Outdated Show resolved Hide resolved
src/util/helpers.tsx Outdated Show resolved Hide resolved
src/pages/instances/actions/ConvertToInstanceBtn.tsx Outdated Show resolved Hide resolved
src/pages/instances/actions/ConvertToInstanceBtn.tsx Outdated Show resolved Hide resolved
src/pages/instances/actions/ConvertToInstanceBtn.tsx Outdated Show resolved Hide resolved
src/pages/instances/actions/ConvertToInstanceBtn.tsx Outdated Show resolved Hide resolved
src/pages/instances/actions/ConvertToInstanceBtn.tsx Outdated Show resolved Hide resolved
src/pages/instances/actions/ConvertToInstanceBtn.tsx Outdated Show resolved Hide resolved
src/pages/instances/actions/ConvertToInstanceBtn.tsx Outdated Show resolved Hide resolved
@mas-who mas-who force-pushed the vmdk-import branch 3 times, most recently from 1581612 to da8c460 Compare August 29, 2024 09:34
@mas-who
Copy link
Collaborator Author

mas-who commented Aug 29, 2024

@edlerd I've also added a function isImageTypeRaw to determine if a image is raw format, and based on that we would remove the format conversion option. The approach is based on heuristics so it's not foolproof, but I think this should be fine since if we fail to catch a raw image type then worst case it will go through conversion. Let me know what you think?

Copy link
Collaborator

@edlerd edlerd left a comment

Choose a reason for hiding this comment

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

This looks pretty neat already.

Some more suggestions on copy. Seeing it in action always sparks more ideas.
Also, some notes on the file name to instance name functionality.

src/pages/instances/forms/ConvertToInstanceForm.tsx Outdated Show resolved Hide resolved
src/util/instances.tsx Outdated Show resolved Hide resolved
src/pages/instances/forms/ConvertToInstanceForm.tsx Outdated Show resolved Hide resolved
src/pages/instances/forms/ConvertToInstanceForm.tsx Outdated Show resolved Hide resolved
src/pages/instances/forms/ConvertToInstanceForm.tsx Outdated Show resolved Hide resolved
src/pages/instances/forms/ConvertToInstanceForm.tsx Outdated Show resolved Hide resolved
src/pages/instances/forms/ConvertToInstanceForm.tsx Outdated Show resolved Hide resolved
src/pages/instances/forms/ConvertToInstanceForm.tsx Outdated Show resolved Hide resolved
src/pages/instances/forms/ConvertToInstanceForm.tsx Outdated Show resolved Hide resolved
@edlerd
Copy link
Collaborator

edlerd commented Sep 9, 2024

We discussed last week with piper to

  1. Merge the "Upload instance" and "Convert from image buttons". Think of a good description, so users can find both features from that caption
  2. Use radio buttons to switch between the two form contents. Place them on the top of the modal. The two different forms are very similar, so switching would only add/remove a couple of fields. Implementation wise, we can keep the two forms in dedicated files, but include them dynamically based on the active radio.
  3. We already had good ideas for the labels of the two radio buttons. @piperdeck should have a link to the design file with sketches and the copy. Though those sketches did not have the radio buttons yet. Though, the labels should translate easily into the radio btn idea. Important is that we list the file types -- maybe in a muted font as part of the label.

@mas-who mas-who force-pushed the vmdk-import branch 3 times, most recently from 459465d to e28e7db Compare September 10, 2024 16:06
@mas-who mas-who marked this pull request as ready for review September 10, 2024 16:06
@mas-who
Copy link
Collaborator Author

mas-who commented Sep 10, 2024

@edlerd this PR is now ready for review. I've implemented the following as discussed:

  1. Combine "upload instance" button with the new vm conversion button. In the upload modal, a file type selection is done via radio inputs. The content of the modal will change depending on the file type selected.
  2. For external format file selection, if a raw image is selected, then the "format" conversion option will be removed and disabled.
  3. For older LXD versions, file type selection will be hidden so users can only upload LXD backup archive files.
  4. Added some logic to make the upload modal fixed height for larger screens (if there is enough viewport height). The aim here is to reduce the amount of layout shift in the modal when selecting different file formats. For screens with limited viewport height, the modal height will be dynamic and the form content will be scrollable. Let me know what you think.

Copy link
Collaborator

@edlerd edlerd left a comment

Choose a reason for hiding this comment

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

This is great, some comments on the layout below. Will give a more thorough review tomorrow.

src/pages/instances/forms/UploadInstanceBackupFileForm.tsx Outdated Show resolved Hide resolved
src/pages/instances/forms/UploadInstanceFileModal.tsx Outdated Show resolved Hide resolved
@mas-who mas-who changed the title feat: support vmdk import for vm instance creation [WD-13429] feat: support vmdk import for vm instance creation [WD-14587] Sep 10, 2024
@edlerd
Copy link
Collaborator

edlerd commented Sep 16, 2024

For the file type extension, we can certainly restrict it during upload. However, the files does not necessarily need an extension for it to be valid. For example, a file named lxd_vmdk_vm or lxd_vmdk_vm.vmdk can both be a VMDK image. On the extreme side, a user can literally name their file lxd_vmdk_vm.png and it could still be a valid VMDK image. In this situation, we rely on the LXD backend to determine if a file uploaded is valid since it directly checks the binary content of the file. We would then show the error returned from the server if the file is invalid. Restricting file selection by extension may result in the UI blocking selection for valid files.

I think there is a way to have the file picker set to a list of allowed file extensions, but the user can still switch the filter to "any file type" if they really want. I think having the filter on the right file type might improve the experience.

@edlerd
Copy link
Collaborator

edlerd commented Sep 16, 2024

I.e. choosing a file from a download directory with dozens or hundreds of files will be much easier with a filter on file extension applied.

@mas-who
Copy link
Collaborator Author

mas-who commented Sep 16, 2024

I.e. choosing a file from a download directory with dozens or hundreds of files will be much easier with a filter on file extension applied.

Yeah fair, I think then for instance backup files, we will filter file extensions to .tar.bz2, .tar.gz, .tar.xz, .tar.lzma, .tar, .squashfs, .qcow2 and .tar.zst as per supported compression listed in LXD source. For external format file uploads, we will restrict to .img, .qcow, .qcow2, .vdi, .vhdx and .vmdk extensions as listed in the lxd-migrate docs.

@mas-who mas-who force-pushed the vmdk-import branch 3 times, most recently from e46d50d to 6c6ce94 Compare September 16, 2024 14:26
@piperdeck
Copy link

@mas-who I think I'm liking the fully responsive behaviour of the modal. What are your thoughts on it?

@mas-who
Copy link
Collaborator Author

mas-who commented Sep 16, 2024

@mas-who I think I'm liking the fully responsive behaviour of the modal. What are your thoughts on it?

@piperdeck I'm happy with this behaviour if you are happy! :)

@mas-who mas-who force-pushed the vmdk-import branch 2 times, most recently from 1ed93a1 to 68f3f79 Compare September 18, 2024 15:25
@mas-who
Copy link
Collaborator Author

mas-who commented Sep 18, 2024

@edlerd made the following changes based on feedback from @piperdeck :

  1. adjusted the accept attribute for instance backup file picker. Turns out the support for this attribute on safari is not great, multi-segment file extensions such as tar.gz, tar.bz2 have issues getting recognised resulting in the file picker to filter out those file extensions on safari. Instead, using MIME types should resolve this issue.

  2. If an instance name is set on the create instance page, opening the file upload modal will automatically fill out the instance name input field. We do not generate an instance name based on the file name in this scenario

@piperdeck
Copy link

Looks good to go!

@mas-who mas-who force-pushed the vmdk-import branch 2 times, most recently from ccf7a37 to c0d2a5b Compare September 19, 2024 13:51
Copy link
Collaborator

@edlerd edlerd left a comment

Choose a reason for hiding this comment

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

QA LGTM
Code also looks good, added a couple of very tiny suggestions to improve on it slightly, should be good to go.

src/pages/instances/forms/UploadExternalFormatFileForm.tsx Outdated Show resolved Hide resolved
src/pages/instances/forms/UploadExternalFormatFileForm.tsx Outdated Show resolved Hide resolved
src/pages/instances/forms/UploadExternalFormatFileForm.tsx Outdated Show resolved Hide resolved
src/pages/instances/forms/UploadExternalFormatFileForm.tsx Outdated Show resolved Hide resolved
src/pages/instances/forms/UploadExternalFormatFileForm.tsx Outdated Show resolved Hide resolved
src/pages/instances/forms/UploadInstanceBackupFileForm.tsx Outdated Show resolved Hide resolved
src/pages/instances/forms/UploadInstanceBackupFileForm.tsx Outdated Show resolved Hide resolved
src/pages/instances/forms/UploadInstanceBackupFileForm.tsx Outdated Show resolved Hide resolved
src/util/uploadExternalFormatFile.tsx Outdated Show resolved Hide resolved
src/util/uploadExternalFormatFile.tsx Outdated Show resolved Hide resolved
Copy link
Collaborator

@edlerd edlerd left a comment

Choose a reason for hiding this comment

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

LGTM, good work :)

@mas-who mas-who merged commit bcb130c into canonical:main Sep 20, 2024
12 checks passed
github-actions bot pushed a commit that referenced this pull request Sep 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants