canonical / lxd-ui

Easy and accessible container and virtual machine management. A browser interface for LXD
GNU General Public License v3.0
269 stars 33 forks source link

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

Closed mas-who closed 2 weeks ago

mas-who commented 1 month ago

Done

QA

  1. Run the LXD-UI:
    • On the demo server via the link posted by @webteam-app below. This is only available for PRs created by collaborators of the repo. Ask @mas-who or @edlerd for access.
    • With a local copy of this branch, build and run as described in the docs.
  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 commented 1 month ago

Demo

Jenkins

demos.haus

mas-who commented 1 month ago

@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?

edlerd commented 3 weeks ago

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 commented 3 weeks ago

@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.
piperdeck commented 2 weeks ago

This is a nitpick, but is there a reason we're using so much vertical space here? I think the modal might look a little bit better if its minimum vertical size was a bit smaller than this...

CleanShot 2024-09-15 at 21 10 21@2x

I'm noticing that we're not doing a lot to filter or validate the file extension of the file to be uploaded. Was this intentional? Seems like it might be a good idea to check and make sure the file that the user attaches at least has the correct extension, or maybe even set that as a parameter in the file upload dialog?

https://github.com/user-attachments/assets/36418b0d-18ca-4f19-aef6-2c8d719d9586

For example, when uploading pictures to Google Photos, the file picker is invoked in such a way as to disable anything that doesn't have an accepted file extension:

https://github.com/user-attachments/assets/aff5ac7a-b23b-40dc-8d75-9c861403fcfd

mas-who commented 2 weeks ago

This is a nitpick, but is there a reason we're using so much vertical space here? I think the modal might look a little bit better if its minimum vertical size was a bit smaller than this...

CleanShot 2024-09-15 at 21 10 21@2x

This is done so that the modal does not change size depending on the file type selected. For the backup archive file type, there are less options to configure for the upload, so it appears to not utilise all the available space of the modal. Do you think we should rather have the modal adjust its size to the available content?

I'm noticing that we're not doing a lot to filter or validate the file extension of the file to be uploaded. Was this intentional? Seems like it might be a good idea to check and make sure the file that the user attaches at least has the correct extension, or maybe even set that as a parameter in the file upload dialog?

CleanShot.2024-09-15.at.21.21.33.mp4 For example, when uploading pictures to Google Photos, the file picker is invoked in such a way as to disable anything that doesn't have an accepted file extension:

CleanShot.2024-09-15.at.21.25.04.mp4

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.

edlerd commented 2 weeks ago

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 commented 2 weeks ago

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 commented 2 weeks ago

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.

piperdeck commented 2 weeks ago

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

mas-who commented 2 weeks ago

@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 commented 2 weeks ago

@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 commented 2 weeks ago

Looks good to go!