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: Show correct instance root disk size on API #14511

Merged
merged 8 commits into from
Dec 11, 2024

Conversation

hamistao
Copy link
Contributor

@hamistao hamistao commented Nov 22, 2024

This aims to get instance root disk sizes correctly, as it currently often shows as 0 or null on the API, either though /1.0/instances?recursion=2 or /1.0/instances/<instance_name>/state, as reported by #14277.

This is still not a perfect solution but is a significant improvement as it:

  • Fixes not returning the disk total size when getting the disk usage is not supported by the driver (relevant for dir and lvm)
  • Returns default block size for block typed/based instances when the disk device does not have the size key set.
  • Returns disk size -1 for instances that have unbounded storage and have access to the entire pool storage.

@github-actions github-actions bot added the Documentation Documentation needs updating label Nov 22, 2024
@hamistao hamistao force-pushed the instance_metrics_on_list branch from 752fbf9 to 0ad2e26 Compare November 22, 2024 14:04
@github-actions github-actions bot removed the Documentation Documentation needs updating label Nov 22, 2024
@hamistao hamistao force-pushed the instance_metrics_on_list branch from 0ad2e26 to 760adc8 Compare November 26, 2024 03:37
@github-actions github-actions bot added Documentation Documentation needs updating API Changes to the REST API labels Nov 26, 2024
@hamistao hamistao force-pushed the instance_metrics_on_list branch 2 times, most recently from 0a5d38d to 4ccdd4c Compare November 26, 2024 13:12
@hamistao hamistao changed the title Show instance root disk size correctly on instance list lxd: Show correct instance root disk size on API Nov 26, 2024
@hamistao hamistao force-pushed the instance_metrics_on_list branch 3 times, most recently from f3a8f96 to 47d7fe1 Compare November 27, 2024 00:39
@hamistao
Copy link
Contributor Author

There are two situations that aren't fixed by this PR:

@hamistao hamistao marked this pull request as ready for review November 27, 2024 02:20
Copy link
Contributor

@roosterfish roosterfish left a comment

Choose a reason for hiding this comment

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

There seems to be one more usage of DefaultBlockSize in the instance scriptlet https://github.com/canonical/lxd/blob/main/lxd/scriptlet/instance_placement.go#L224.

Can we do something similar to https://github.com/canonical/lxd/blob/main/lxd/project/limits/permissions.go#L1472 to load the storage driver and fetch the respective DefaultBlockSize? Not sure if it will cause an import loop though.

lxd/storage/backend_lxd.go Show resolved Hide resolved
test/suites/storage.sh Outdated Show resolved Hide resolved
shared/api/instance_state.go Outdated Show resolved Hide resolved
doc/rest-api.yaml Outdated Show resolved Hide resolved
@hamistao hamistao force-pushed the instance_metrics_on_list branch 2 times, most recently from aed4e82 to 6d75211 Compare November 28, 2024 13:19
@hamistao
Copy link
Contributor Author

@roosterfish I believe everything was addressed. I did something different from what is done on https://github.com/canonical/lxd/blob/main/lxd/project/limits/permissions.go#L1472 to get DefaultBlockSize on InstancePlacementRun, but I think you will like this approach.

Tests for VMs will follow on LXD CI.

@hamistao hamistao marked this pull request as draft November 29, 2024 18:50
@hamistao
Copy link
Contributor Author

Going back to Draft to add support for the situations described in #14511 (comment).

@hamistao hamistao force-pushed the instance_metrics_on_list branch 7 times, most recently from 61cba04 to e422622 Compare December 5, 2024 22:59
@hamistao hamistao force-pushed the instance_metrics_on_list branch 2 times, most recently from 7db2378 to b964ab2 Compare December 6, 2024 07:30
@hamistao
Copy link
Contributor Author

hamistao commented Dec 6, 2024

This includes the simpler fixes related to showing accurate disk sizes on the API. To address the use cases described in #14511 (comment), a follow up draft PR was opened to unblock these changes from getting merged: #14511 (comment).

@hamistao hamistao force-pushed the instance_metrics_on_list branch from b964ab2 to c239c75 Compare December 9, 2024 06:19
@hamistao
Copy link
Contributor Author

hamistao commented Dec 9, 2024

@tomponline This includes the simpler fixes for getting correct root disk sizes and don't address the scenarios described in #14511 (comment). These are addressed in a follow up PR: #14595.

The tests for these changes are included in the follow up PR, but we can check that they are passing on the follow up's test runs.

All that said, this is ready for a review :D

lxc/info.go Show resolved Hide resolved
@hamistao hamistao marked this pull request as ready for review December 9, 2024 13:38
tomponline added a commit that referenced this pull request Dec 9, 2024
This includes some improvements that were originally a part of the fix
implemented in #14511, but ended up
not being needed for that fix. They are still nice to have and thus were
included here.
When driver does not support getting the volume usage, we are returning prematurely and abstaining from getting the disk total size.

Signed-off-by: hamistao <pedro.ribeiro@canonical.com>
This proposes using -1 as a disk size to indicate we do not impose
limits on the volume level and thus it has access to the entire pool
storage.

Signed-off-by: hamistao <pedro.ribeiro@canonical.com>
Signed-off-by: hamistao <pedro.ribeiro@canonical.com>
Signed-off-by: hamistao <pedro.ribeiro@canonical.com>
Signed-off-by: hamistao <pedro.ribeiro@canonical.com>
Previously, when usage was not supported, we returned null root disk and thus did not show disk total size, even when it was possible. Now we represent not supporting showing disk usage with usage=-1.

Signed-off-by: hamistao <pedro.ribeiro@canonical.com>
Signed-off-by: hamistao <pedro.ribeiro@canonical.com>
Signed-off-by: hamistao <pedro.ribeiro@canonical.com>
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.

Thanks!

@tomponline
Copy link
Member

@edlerd please can you see whether this helps with storage reporting in the UI. Thanks

@tomponline tomponline merged commit d56d152 into canonical:main Dec 11, 2024
27 checks passed
@hamistao hamistao deleted the instance_metrics_on_list branch December 11, 2024 18:43
@tomponline tomponline changed the title lxd: Show correct instance root disk size on API Storage: Show correct instance root disk size on API Dec 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API Changes to the REST API Documentation Documentation needs updating
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants