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 disk size #14595

Draft
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

hamistao
Copy link
Contributor

@hamistao hamistao commented Dec 6, 2024

This is a follow up PR for #14511 and this aims to address the problems outlined in #14511 (comment).

To do this, we need a way to determine wether the instance volume had its quota set based on volume.size on the pool level. For this, we are introducing the volatile.rootfs.size config key for instance volumes, previously it was only recognized in image volumes. This config key is used to represent the current quota of an instance volume at any point in time. This way we can determine whether a volume had its quota set based on volume.size or not.

Also, any changes to the disk only cascade onto the volume's volatile.rootfs.size when the quota is applied on the volume. Thus, we are able to handle instances that don't support online resizing as the field we are looking for will only change when a quota is applied on the volume, i.e. when the instance restarts.

This key was chosen instead of the size key because the intended behavior of this config key fits better the pattern we see with volatile.* keys, that can be changed by LXD itself, without the user specifying them. Plus this way we don't need complicated volume validation, where size would be unsettable only if a user is setting it.

@github-actions github-actions bot added Documentation Documentation needs updating API Changes to the REST API labels Dec 6, 2024

## `storage_volume_rootfs_size`

Introduces a `volatile.rootfs.size` key, previously only supported for images, for container and virtual machine volumes that is used to store that volume's current size at any point.
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be showing up in one of the reference documents that are

Suggested change
Introduces a `volatile.rootfs.size` key, previously only supported for images, for container and virtual machine volumes that is used to store that volume's current size at any point.
Introduces a `volatile.rootfs.size` key, previously only supported for images, for container and virtual machine volumes. This key is used to store a volume's current size at any point.

Copy link
Contributor

Choose a reason for hiding this comment

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

Clarifying that the clause following "virtual machine volumes that ..." is meant to refer to key and not volumes.

Question: Should the volatile.rootfs.size key show up in a reference page somewhere? I could not find it in the PR's RTD build. For example, should it show up here? Or in the storage volume configuration options like here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Clarifying that the clause following "virtual machine volumes that ..." is meant to refer to key and not volumes.

Yes, it is. Now I see how that is confusing,

Question: Should the volatile.rootfs.size key show up in a reference page somewhere?

I always assumed it was a pattern to not include volatile.* keys in the docs as they are not intended (I think) to be set by users. Although for some reason it is apparentely allowed.
There seems to be a lack of pattern, the only volatile.* keys that are included are defined here and here(https://github.com/hamistao/lxd/blob/disk_size_fixes/lxd/instance/instancetype/instance.go#L111). Everywhere else they are do not follow a metadata doc comment and I do not see a pattern for why some of them are included and some aren't.

example: 502239232
format: int64
type: integer
x-go-name: Total
usage:
description: Disk usage in bytes
description: Disk usage in bytes. We use -1 to indicate the storage driver for the instance's pool does not support getting the disk usage.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
description: Disk usage in bytes. We use -1 to indicate the storage driver for the instance's pool does not support getting the disk usage.
description: Disk usage in bytes. We use -1 to indicate that the storage driver for the instance's pool does not support retrieving the disk usage.

@@ -3287,12 +3302,20 @@ func (b *lxdBackend) GetInstanceUsage(inst instance.Instance) (*VolumeUsage, err
}
}

// If the instance volume is not block based/typed and neither bound by the device's size config key,
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// If the instance volume is not block based/typed and neither bound by the device's size config key,
// If the instance volume is neither block based/typed nor bound by the device's size config key,

All the handling of `volatile.rootfs.size` for images considers that it can be a '123 GiB' type string as well, so this also considers these strings when validating the key

Signed-off-by: hamistao <[email protected]>
This makes it so that the an instance volume's `size` config is aligned with the volume's actual size in any point in time.
i.e. some configs like disk device `size` and pool `volume.config`  propagate into `volume.size`, similarly to what happens with custom volumes.

Signed-off-by: hamistao <[email protected]>
For running instances, the instance volume 'size' config key is the most accurate representation of the instance root disk size. The device's own 'size' may be inaccurate when the instance does not support online resizing.
When getting instance disk total size of a stopped instance, you want to see what is the size of the root disk you will be dealing with when you start the instance, even if the volume quota is only applied on instance start.

Signed-off-by: hamistao <[email protected]>
…nces

When getting instance disk total size of a stopped instance, you want to see what size is the root disk you will be dealing with when you start it. But it could be the case that the current volume size is different from the instance configuration as the volume when gets resizes (if needed) when the instance starts. So for the volume state, we always get the actual current size of the volume.

Signed-off-by: hamistao <[email protected]>
@hamistao hamistao marked this pull request as ready for review December 12, 2024 04:47
@hamistao hamistao requested a review from tomponline December 12, 2024 05:20
@tomponline tomponline changed the title Follow up on showing correct disk size Storage: Show correct disk size Dec 12, 2024
@tomponline
Copy link
Member

For this, we are introducing the volatile.rootfs.size config key for instance volumes

Is this only for container filesystem based volumes or for VM block volumes too?

Is this key used for VM image volumes too?

@hamistao
Copy link
Contributor Author

Is this only for container filesystem based volumes or for VM block volumes too?

This is for all instance types, since all can be restrained by setting the device's size key.

Is this key used for VM image volumes too?

Yes, it is currently only used for image volumes.

@tomponline
Copy link
Member

Yes, it is currently only used for image volumes.

thats not what i asked, is it currently populated for VM image volumes as well as container image volumes?

@hamistao
Copy link
Contributor Author

thats not what i asked, is it currently populated for VM image volumes as well as container image volumes?

Oh sorry, I misread your question. Yes, it is used for both container and VM images.

@hamistao hamistao marked this pull request as draft December 12, 2024 17:18
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