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

Ensure clustered backend pools are created and not left in pending state #513

Merged
merged 1 commit into from
Nov 13, 2023

Conversation

edlerd
Copy link
Collaborator

@edlerd edlerd commented Oct 27, 2023

Done

  • adding tests to ensure creating a storage pool on a clustered backend in created state. This would previously be a pending state for the pool, leaving it not fully usable.
  • fixing pool creation for the cluster use case

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 @lorumic or @edlerd for access.
    • With a local copy of this branch, run as described here.
  2. Perform the following QA steps:
    • create a pool in a clustered backend (for the ZFS driver to work, the clustered test backend must not use containers, but VMs)
    • edit a pool in a clustered backend

@webteam-app
Copy link

Demo starting at https://lxd-ui-513.demos.haus

Copy link
Contributor

@lorumic lorumic left a comment

Choose a reason for hiding this comment

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

I think this change needs more work.

When I try to create with a size value, I get this error:

image

The error is shown on the creation page, without any navigation, but the storage pool is actually created and has the "Pending" state. However, since no navigation happens, the user thinks that they can retry by just removing the size value, but that would fail, because a storage pool with that name already exists, resulting in only more frustration.

This needs UX improvement:

  • Disable the size field, if it's not possible to set a fixed size because it's cluster member specific.
  • Disable ZFS driver option if the cluster is not VM-based - I don't know if this is feasible, but it's better than getting a postponed feedback, so it would be good to investigate a way to do it.

@edlerd
Copy link
Collaborator Author

edlerd commented Oct 31, 2023

When I try to create with a size value, I get this error:

Did you run the cluster nodes as containers or as VMs when testing?

  • Disable the size field, if it's not possible to set a fixed size because it's cluster member specific.

It should be possible. Did you review the code yet? Under the hood we are triggering requests for each cluster member to create a local pool with all fields. Afterwards one request to create the cluster wide pool. It could be the last request must not include a size though.

  • Disable ZFS driver option if the cluster is not VM-based - I don't know if this is feasible, but it's better than getting a postponed feedback, so it would be good to investigate a way to do it.

That is a good idea. Will try to find a way to know if the cluster nodes are running as container. If not, I am not sure about the relevance, it might be very uncommon to run a cluster on containers and its just us using this as a testing backend.

@lorumic
Copy link
Contributor

lorumic commented Oct 31, 2023

Did you run the cluster nodes as containers or as VMs when testing?

VMs.

Did you review the code yet? Under the hood we are triggering requests for each cluster member to create a local pool with all fields. Afterwards one request to create the cluster wide pool. It could be the last request must not include a size though.

Yes, I had already seen and understood the 3 changes in the code. And still, I'm always getting that error for a local cluster made of 3 VMs created following the guide in the wiki.

@edlerd
Copy link
Collaborator Author

edlerd commented Oct 31, 2023

Yes, I had already seen and understood the 3 changes in the code. And still, I'm always getting that error for a local cluster made of 3 VMs created following the guide in the wiki.

I see, thanks for providing the details. I think we have to strip the size (and likely other node specific properties) from the last request triggering the cluster-wide storage pool creation.

@edlerd
Copy link
Collaborator Author

edlerd commented Nov 13, 2023

Adjusted the storage creation for clustered backends. It omits node-specific config for the cluster storage. Also updated the edit storage, which needs to also update the cluster wide pool and needs the same adjustment.

@edlerd edlerd requested a review from lorumic November 13, 2023 12:26
Copy link
Contributor

@lorumic lorumic left a comment

Choose a reason for hiding this comment

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

All good, just one tiny question about a function name.

src/api/storage-pools.tsx Outdated Show resolved Hide resolved
@edlerd edlerd merged commit e5261be into canonical:main Nov 13, 2023
5 checks passed
@edlerd edlerd deleted the create-storage-in-cluster branch November 13, 2023 14:13
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