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: add support for instance/profile host path devices [WD-17682] #1019

Merged
merged 3 commits into from
Dec 13, 2024

Conversation

mas-who
Copy link
Collaborator

@mas-who mas-who commented Dec 2, 2024

Done

  • Adjusted devices table in instance/profile overview page to show host path disk devices
  • Added support for create/edit host path disk devices in instance/profile configuration pages
  • Adjusted the workflow for adding disk devices, now it is a multi-step flow, starting with selecting the type of disk device (volume or host path).
  • Adjusted/added e2e tests to reflect the above changes.

QA

  1. Run the LXD-UI:
  2. Perform the following QA steps:
    • Create a new profile, add both custom volume and host path disk devices to it.
    • Create an instance and apply the profile to that instance, make sure all disk devices are inherited.
    • Create custom volume and host path disk devices for the above instance, using different volume and mount point.
    • Start the instance, go to its terminal and make sure that path corresponding to every disk device exists. Make sure that the mounted host path has the correct content (should be the same to the path on your machine).
    • Go to the instance/profile overview page, check the device list table and make sure disk devices are displayed correctly.
    • Try modifying the volume and host path disk devices on the profile and instance.

@webteam-app
Copy link

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 good on the overview page, but on the configuration page it is still a bit broken:

image

I think the easiest solution is to filter the host path devices from displaying in the configuration form.

@edlerd
Copy link
Collaborator

edlerd commented Dec 9, 2024

Edit: the resource link for the host path on the overview page, should then direct to the yaml editor.

@mas-who
Copy link
Collaborator Author

mas-who commented Dec 9, 2024

This looks good on the overview page, but on the configuration page it is still a bit broken:

image

I think the easiest solution is to filter the host path devices from displaying in the configuration form.

Aaah yes didn't catch this one. I don't think filtering them is correct though, since these are still legit disk devices. Perhaps instead of "Pool/Volume", we can have "Host path" and remove Mount point. Also we can disable the edit functionality for a host path disk device for now. Wdyt?

@edlerd
Copy link
Collaborator

edlerd commented Dec 9, 2024

Aaah yes didn't catch this one. I don't think filtering them is correct though, since these are still legit disk devices. Perhaps instead of "Pool/Volume", we can have "Host path" and remove Mount point. Also we can disable the edit functionality for a host path disk device for now. Wdyt?

We could, then we'd half support them :) Ideally we allow adding them and editing as well, I wanted to keep the scope small, so hiding seemed like the easiest fix. Up to you how to solve it. Just having a read only would work as well, but then should add a way that guides to the YAML editor for editing it.

@mas-who
Copy link
Collaborator Author

mas-who commented Dec 10, 2024

Aaah yes didn't catch this one. I don't think filtering them is correct though, since these are still legit disk devices. Perhaps instead of "Pool/Volume", we can have "Host path" and remove Mount point. Also we can disable the edit functionality for a host path disk device for now. Wdyt?

We could, then we'd half support them :) Ideally we allow adding them and editing as well, I wanted to keep the scope small, so hiding seemed like the easiest fix. Up to you how to solve it. Just having a read only would work as well, but then should add a way that guides to the YAML editor for editing it.

I think adding/editing host path disk devices may require some design input. For example, maybe we follow a similar approach as migrate instance modal where we have multiple steps, and the first step would be to choose the type of disk device to add. Furthermore, to add/edit a host path disk device, we can let the user provide the input by using a directory picker.

In any case, for now limiting the cope to read only seems appropriate with the redirection to the yaml editor. I can create an issue to do a follow up task for the new disk device type?

@mas-who mas-who force-pushed the fix-host-path-device branch from f13a0eb to 626aa70 Compare December 11, 2024 09:54
@mas-who mas-who marked this pull request as draft December 11, 2024 13:42
@mas-who
Copy link
Collaborator Author

mas-who commented Dec 11, 2024

@edlerd please do wait a bit to review this PR, I'm not entirely happy with the implementation yet, will remove draft status once it's ready

@mas-who mas-who force-pushed the fix-host-path-device branch 3 times, most recently from d76c478 to 0846d1f Compare December 12, 2024 11:21
@mas-who mas-who changed the title fix: fix host path device display feat: add support for instance/profile host path devices [WD-17682] Dec 12, 2024
@mas-who mas-who marked this pull request as ready for review December 12, 2024 11:38
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 a really nice interaction now. Some ideas to refine the UX a bit and some other ideas about code. Most of them are very small suggestions

src/pages/storage/HostPathDeviceModal.tsx Outdated Show resolved Hide resolved
src/pages/storage/CustomVolumeModal.tsx Outdated Show resolved Hide resolved
src/pages/storage/HostPathDeviceModal.tsx Outdated Show resolved Hide resolved
src/pages/storage/AttachDiskDeviceModal.tsx Outdated Show resolved Hide resolved
src/components/DeviceListTable.tsx Outdated Show resolved Hide resolved
src/pages/storage/AttachDiskDeviceModal.tsx Outdated Show resolved Hide resolved
src/pages/storage/HostPathDeviceModal.tsx Outdated Show resolved Hide resolved
src/pages/storage/HostPathDeviceModal.tsx Show resolved Hide resolved
src/pages/storage/HostPathDeviceModal.tsx Outdated Show resolved Hide resolved
src/util/helpers.tsx Outdated Show resolved Hide resolved
@mas-who mas-who force-pushed the fix-host-path-device branch 2 times, most recently from f0e07a8 to 195bc45 Compare December 12, 2024 15:17
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.

Thanks for the quick iteration, just a handful of ideas then this should be good to merge.

src/pages/storage/AttachDiskDeviceModal.tsx Outdated Show resolved Hide resolved
src/pages/storage/AttachDiskDeviceModal.tsx Outdated Show resolved Hide resolved
src/util/devices.tsx Outdated Show resolved Hide resolved
src/util/instanceValidation.tsx Show resolved Hide resolved
src/pages/storage/CustomVolumeModal.tsx Show resolved Hide resolved
@mas-who mas-who force-pushed the fix-host-path-device branch from 195bc45 to 5260624 Compare December 12, 2024 20:30
@mas-who mas-who force-pushed the fix-host-path-device branch from 5260624 to 3ce9a14 Compare December 12, 2024 20:37
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.

Thanks for the change, I find the code still got a bit too complicated around change device. Maybe it can be improved?

src/components/forms/DiskDeviceFormCustom.tsx Outdated Show resolved Hide resolved
src/util/devices.tsx Outdated Show resolved Hide resolved
@mas-who mas-who force-pushed the fix-host-path-device branch from 3ce9a14 to 662280b Compare December 13, 2024 11:44
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.

Code lgtm! Thanks for the additional features and making this nice and round :)

Didn't QA yet, if you are confident on the QA feel free to merge right away.

@mas-who
Copy link
Collaborator Author

mas-who commented Dec 13, 2024

Code lgtm! Thanks for the additional features and making this nice and round :)

Didn't QA yet, if you are confident on the QA feel free to merge right away.

I'm confident about the functionality as I've tested it extensively. However, I think it's still a good idea to have a QA pass, perhaps just for things that are not covered in the PR description?

@edlerd
Copy link
Collaborator

edlerd commented Dec 13, 2024

QA looks also very good 👍

@edlerd edlerd merged commit 6e88058 into canonical:main Dec 13, 2024
11 checks passed
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.

3 participants