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

Storage: PowerFlex fixes and improvements #14865

Merged
merged 10 commits into from
Jan 31, 2025

Conversation

roosterfish
Copy link
Contributor

@roosterfish roosterfish commented Jan 27, 2025

This has to land after #14710 got merged as it was tested together.

Summary of changes

  1. Don't map the volume before actually performing the resize in SetVolumeQuota. As we can get the current volume's size directly from PowerFlex it does not need to be mapped beforehand. This also allows reducing errors in dmesg caused by the resize in case the vol is already mapped to the system.
  2. Prevent volume creation in case the PowerFlex pool isn't using zero-padding. This is to prevent leaking data from a previous volume in case the storage pool on PowerFlex wasn't setup appropriately.
  3. Lock the PowerFlex client's API token to not cause concurrent logins and to save time in case of parallel access.
  4. Always copy the PowerFlex client's request body for retries. Before, if the to be retried request contained a request body, it was already read in the first request which leads any follow up request to not anymore have a request body.
  5. Small improvement to use NewStatusError
  6. Always allow the filler to resize as PowerFlex doesn't use optimized image storage. This caused issues when trying to launch images from an image that were bigger than the volume size. There is now a test for this in lxd-ci/tests/storage-vm.
  7. Move common disk discovery functions into the block package.

@roosterfish
Copy link
Contributor Author

Linter errors should disappear with #14710.

tomponline added a commit to canonical/lxd-ci that referenced this pull request Jan 27, 2025
Observed whilst running the tests for
canonical/lxd#14865.

Also added a note to run more robust PowerFlex tests that cope with
zero-padding disabled.
@roosterfish roosterfish changed the title PowerFlex fixes and improvements Storage: PowerFlex fixes and improvements Jan 29, 2025
@roosterfish roosterfish force-pushed the powerflex_additions branch 2 times, most recently from e1941f0 to d4cce84 Compare January 29, 2025 16:54
@github-actions github-actions bot added the Documentation Documentation needs updating label Jan 29, 2025
@roosterfish roosterfish force-pushed the powerflex_additions branch 2 times, most recently from c07cfb7 to 7458ef7 Compare January 30, 2025 08:50
@roosterfish roosterfish marked this pull request as ready for review January 30, 2025 09:22
@roosterfish roosterfish marked this pull request as draft January 30, 2025 09:39
@roosterfish roosterfish force-pushed the powerflex_additions branch 2 times, most recently from c924c13 to 8b7066a Compare January 30, 2025 16:13
@github-actions github-actions bot removed the Documentation Documentation needs updating label Jan 30, 2025
@roosterfish roosterfish marked this pull request as ready for review January 30, 2025 17:08
@roosterfish roosterfish marked this pull request as draft January 30, 2025 17:11
@roosterfish
Copy link
Contributor Author

Running the test suite now for nvme and sdc mode, will mark ready for review on success.

We don't have to map the PowerFlex volume to get its size.
The value can be obtained from the API which is faster than mapping the volume locally.
Furthermore we prevent potential side effects if the mapped volume observes changes in size whilst being mapped.

Signed-off-by: Julian Pelizäus <[email protected]>
…is disabled

Block the creation of new volumes in case the storage pool isn't using
the zero-padding feature.

Signed-off-by: Julian Pelizäus <[email protected]>
This ensures a token which is valid isn't reset by another caller.
Also return the valid token so it cannot be modified anymore which effectively
detaches it from the internal struct which is protected by the lock.

It didn't cause any issues during tests but improves the token handling in general.

Signed-off-by: Julian Pelizäus <[email protected]>
In case the API's access token expires a new token is generated and the request
is retried one more time.
As the body is provided as a reader, this reader will be empty when issuing the second request
as its contents were already read in the request from before.
Prevent this by creating a copy of the readers contents and provide a fresh reader from the copy
for every new request.

To not require copying the reader, we pass the actual request body map directly to the requestAuthenticated func
and create the reader there on demand for every retry.

This was causing 400 Bad Requests in some cases when the token expired and the request which
is going to be retried had something initially set in it's request body.
For requests without anything in the body this was still working fine and therefore stayed unnoticed until now.
It first occurred in a very slow PowerFlex environment in which an instance migration took longer
than the token lifetime of around 10mins.

Signed-off-by: Julian Pelizäus <[email protected]>
@roosterfish roosterfish marked this pull request as ready for review January 31, 2025 12:30
@roosterfish
Copy link
Contributor Author

roosterfish commented Jan 31, 2025

Tests for both NVMe and SDC passed successfully. Ready for review.

Copy link
Member

@tomponline tomponline left a comment

Choose a reason for hiding this comment

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

LGTM thanks!

@tomponline tomponline merged commit c9f2283 into canonical:main Jan 31, 2025
26 checks passed
@roosterfish roosterfish deleted the powerflex_additions branch January 31, 2025 14:49
tomponline added a commit that referenced this pull request Feb 4, 2025
This PR adds `pure` storage driver for Pure Storage remote storage.
Driver supports communication over either iSCSI or NVMe/TCP.

- [x] For iSCSI, this PR is required to ensure LXD redirects `iscsiadm`
to the LXD host: canonical/lxd-pkg-snap#646
- [x] Rebase after #14710
- [x] Rebase after #14865 (for
WaitDiskSize func)
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.

3 participants