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

Expose virtual size of backing images #2680

Merged
merged 4 commits into from
Apr 2, 2024

Conversation

tserong
Copy link
Contributor

@tserong tserong commented Mar 8, 2024

Which issue(s) this PR fixes:

longhorn/longhorn#7923

What this PR does / why we need it:

This commit adds virtualSize to the BackingImageStatus and BackingImageFileInfo structs, and thus to the BackingImage and BackingImageManager CRDs. We can see how this works in practice by creating a new backing image downloaded from a URL, then periodically running kubectl -n longhorn-system get lhbi. I'm using https://download.opensuse.org/distribution/leap/15.5/appliances/openSUSE-Leap-15.5-Minimal-VM.x86_64-Cloud.qcow2 in the example below.

Initially, when the download is just started, before the file size is known:

> kubectl -n longhorn-system get lhbi/default-image-7mplj
NAME                  UUID       SOURCETYPE   SIZE        VIRTUALSIZE   AGE
default-image-7mplj   6d5a98b0   download     0           0             9s

A little later, while the download is running:

> kubectl -n longhorn-system get lhbi/default-image-7mplj
NAME                  UUID       SOURCETYPE   SIZE        VIRTUALSIZE   AGE
default-image-7mplj   6d5a98b0   download     255701504   0             3m33s

Finally, once the download is complete:

> kubectl -n longhorn-system get lhbi/default-image-7mplj
NAME                  UUID       SOURCETYPE   SIZE        VIRTUALSIZE   AGE
default-image-7mplj   6d5a98b0   download     255701504   821035008     4m26s

Compare size and virtualSize above with the image downloaded manually:

> wget https://download.opensuse.org/distribution/leap/15.5/appliances/openSUSE-Leap-15.5-Minimal-VM.x86_64-Cloud.qcow2
[...]

> ls -l openSUSE-Leap-15.5-Minimal-VM.x86_64-Cloud.qcow2
-rw-r--r-- 1 tserong users 255701504 Dec 19 23:26 openSUSE-Leap-15.5-Minimal-VM.x86_64-Cloud.qcow2

> qemu-img info openSUSE-Leap-15.5-Minimal-VM.x86_64-Cloud.qcow2 | grep virtual
virtual size: 783 MiB (821035008 bytes)

Special notes for your reviewer:

I've pulled in the latest backing-image-manager by updating go.mod to directly reference the latest commit from the master branch (longhorn/backing-image-manager@c5288d7). This can be seen as github.com/longhorn/backing-image-manager v1.7.0-dev.0.20240326182459-c5288d745f4a in the require section in go.mod. I'm not sure if that's how this sort of thing is usually handled - would it be better to make a v 1.7.0-dev tag in backing-image-manager and reference that instead?

Additional documentation or context

N/A

@tserong tserong requested a review from a team as a code owner March 8, 2024 10:59
@tserong tserong marked this pull request as draft March 8, 2024 11:00
@tserong
Copy link
Contributor Author

tserong commented Mar 8, 2024

(marked as draft bearing in mind my notes above)

@tserong
Copy link
Contributor Author

tserong commented Mar 8, 2024

JFTR, some extra output with a mix of images, both raw and qcow2, from various sources:

> kubectl -n longhorn-system get lhbi
NAME                  UUID       SOURCETYPE           SIZE          VIRTUALSIZE   AGE
default-image-2r5jt   421c8c11   download             21430272      117440512     8d
default-image-7b2wq   a93b9a55   upload               261693952     845152256     14d
default-image-7mplj   6d5a98b0   download             255701504     821035008     5h1m
default-image-87b7d   aa95cadc   download             14548992000   14548992000   14d
default-image-8d7w7   53a3b08d   export-from-volume   10737418240   10737418240   7d8h
default-image-jnvqc   c41a4745   upload               20971520      20971520      8d
default-image-nvdn4   8892d329   download             750583808     3758096384    99m
default-image-t4gzh   cfb51734   upload               1001652224    21474836480   4h48m
default-image-wncvq   d98d70f9   download             85168128      85168128      5h56m
default-image-xzpdx   1e801407   upload               20971520      20971520      14d
test                  aae0b6c4   download             2457272320    25769803776   66m

tserong added a commit to tserong/harvester that referenced this pull request Mar 18, 2024
This manually applies the relevant changes from
longhorn/backing-image-manager#208 and
longhorn/longhorn-manager#2680 to
vendor/github.com/longhorn/backing-image-manager and
vendor/github.com/longhorn/longhorn-manager because using
`go get ; go mod tidy ; go mod vendor` pulls in a giant mess of
other stuff that's changed in the meantime :-/

Signed-off-by: Tim Serong <tserong@suse.com>
tserong added a commit to tserong/harvester that referenced this pull request Mar 18, 2024
This requires:

- longhorn/backing-image-manager#208
- longhorn/longhorn-manager#2680

With this change, we can do this:

```
NAME          DISPLAY-NAME                                         SIZE          VIRTUALSIZE   AGE
image-26r4s   sles15-sp5-minimal-vm.x86_64-kvm-and-xen-qu1.qcow2   267845632     25769803776   6m36s
image-bgnhb   sle-micro.x86_64-5.5.0-default-qcow-gm.qcow2         1001652224    21474836480   20m
image-nmmvj   sle-15-sp5-full-x86_64-gm-media1.iso                 14548992000   14548992000   19m
```

Related issue: harvester#4905

Signed-off-by: Tim Serong <tserong@suse.com>
tserong added a commit to tserong/harvester that referenced this pull request Mar 18, 2024
This requires:

- longhorn/backing-image-manager#208
- longhorn/longhorn-manager#2680

With this change, we can do this:

  # kubectl get vmimages
  NAME          DISPLAY-NAME                                         SIZE          VIRTUALSIZE   AGE
  image-26r4s   sles15-sp5-minimal-vm.x86_64-kvm-and-xen-qu1.qcow2   267845632     25769803776   6m36s
  image-bgnhb   sle-micro.x86_64-5.5.0-default-qcow-gm.qcow2         1001652224    21474836480   20m
  image-nmmvj   sle-15-sp5-full-x86_64-gm-media1.iso                 14548992000   14548992000   19m

Related issue: harvester#4905

Signed-off-by: Tim Serong <tserong@suse.com>
tserong added a commit to tserong/harvester that referenced this pull request Mar 19, 2024
This requires:

- longhorn/backing-image-manager#208
- longhorn/longhorn-manager#2680

With this change, we can do this:

  # kubectl get vmimages
  NAME          DISPLAY-NAME                                         SIZE          VIRTUALSIZE   AGE
  image-26r4s   sles15-sp5-minimal-vm.x86_64-kvm-and-xen-qu1.qcow2   267845632     25769803776   6m36s
  image-bgnhb   sle-micro.x86_64-5.5.0-default-qcow-gm.qcow2         1001652224    21474836480   20m
  image-nmmvj   sle-15-sp5-full-x86_64-gm-media1.iso                 14548992000   14548992000   19m

Related issue: harvester#4905

Signed-off-by: Tim Serong <tserong@suse.com>
@tserong tserong marked this pull request as ready for review March 27, 2024 07:36
Copy link
Contributor

@shuo-wu shuo-wu left a comment

Choose a reason for hiding this comment

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

Will you run longhorn-manager/k8s/generate_code.sh after adding a new field?
Besides, would you help update https://github.com/longhorn/longhorn/blob/master/deploy/longhorn.yaml and https://github.com/longhorn/longhorn/blob/master/chart/templates/crds.yaml?

controller/backing_image_controller.go Outdated Show resolved Hide resolved
@shuo-wu shuo-wu requested a review from ChanYiLin March 27, 2024 19:09
@tserong
Copy link
Contributor Author

tserong commented Mar 28, 2024

Will you run longhorn-manager/k8s/generate_code.sh after adding a new field?

I did that already and it updated k8s/crds.yaml, which is included in this PR. Is there something else I missed?

Besides, would you help update https://github.com/longhorn/longhorn/blob/master/deploy/longhorn.yaml and https://github.com/longhorn/longhorn/blob/master/chart/templates/crds.yaml?

Will do!

@shuo-wu
Copy link
Contributor

shuo-wu commented Mar 28, 2024

I did that already and it updated k8s/crds.yaml, which is included in this PR. Is there something else I missed?

I am not sure. Hence I recommend running that script. 😃

When doing local builds (i.e. without $DRONE_REPO and $DRONE_PULL_REQUEST,
or $DRONE_COMMIT_REF set), the build fails with "fatal: Needed a single
revision" in `scripts/validate`:

```
> make REPO=tserong
[...]
Running validation
Running: go vet
refs/heads/wip-add-virtualSize
fatal: Needed a single revision
FATA[0032] exit status 128
make: *** [Makefile:11: ci] Error 1
```

If I run the commands in `scripts/validate` by hand, I see this:

```
> git symbolic-ref -q HEAD && REV="origin/HEAD" || REV="HEAD^"
refs/heads/wip-add-virtualSize
> echo $REV
origin/HEAD
> headSHA=$(git rev-parse --short=12 ${REV})
fatal: Needed a single revision
```

I don't know if this is just something weird about my environment, but
if I change "origin/HEAD" to "HEAD", it works fine:

```
> headSHA=$(git rev-parse --short=12 HEAD)
> echo $headSHA
eb27fbf6e678
```

Signed-off-by: Tim Serong <tserong@suse.com>
Signed-off-by: Tim Serong <tserong@suse.com>
This commit adds virtualSize to the BackingImageStatus and
BackingImageFileInfo structs, and thus to the BackingImage and
BackingImageManager CRDs.  We can see how this works in practice by
creating a new backing image downloaded from a URL, then periodically
running `kubectl -n longhorn-system get lhbi`.  I'm using
https://download.opensuse.org/distribution/leap/15.5/appliances/openSUSE-Leap-15.5-Minimal-VM.x86_64-Cloud.qcow2
in the example below.

Initially, when the download is just started, before the file size is known:

```
> kubectl -n longhorn-system get lhbi/default-image-7mplj
NAME                  UUID       SOURCETYPE   SIZE        VIRTUALSIZE   AGE
default-image-7mplj   6d5a98b0   download     0           0             9s
```

A little later, while the download is running:

```
> kubectl -n longhorn-system get lhbi/default-image-7mplj
NAME                  UUID       SOURCETYPE   SIZE        VIRTUALSIZE   AGE
default-image-7mplj   6d5a98b0   download     255701504   0             3m33s
```

Finally, once the download is complete:

```
> kubectl -n longhorn-system get lhbi/default-image-7mplj
NAME                  UUID       SOURCETYPE   SIZE        VIRTUALSIZE   AGE
default-image-7mplj   6d5a98b0   download     255701504   821035008     4m26s
```

Compare size and virtualSize above with the image downloaded manually:

```
> wget https://download.opensuse.org/distribution/leap/15.5/appliances/openSUSE-Leap-15.5-Minimal-VM.x86_64-Cloud.qcow2
[...]

> ls -l openSUSE-Leap-15.5-Minimal-VM.x86_64-Cloud.qcow2
-rw-r--r-- 1 tserong users 255701504 Dec 19 23:26 openSUSE-Leap-15.5-Minimal-VM.x86_64-Cloud.qcow2

> qemu-img info openSUSE-Leap-15.5-Minimal-VM.x86_64-Cloud.qcow2 | grep virtual
virtual size: 783 MiB (821035008 bytes)
```

Related issue: longhorn/longhorn#7923

Signed-off-by: Tim Serong <tserong@suse.com>
…atch

Doing this means if there's somehow a mismatch between backing image
size or virtual size reported by backing image manager vs. what's currently
in the backing image resource's status, the error will be made visible in
status.diskFileStatusMap.$DISKUUID.message, i.e. the user should be able
to can see the problem by running `kubectl -n longhorn-system get lhbi -o
yaml`.  This is the same logic as is already used in
updateStatusWithFileInfo().

The one thing I'm struggling with here is how to inject such a failure
into a running system in order to prove that this change works correctly.

Signed-off-by: Tim Serong <tserong@suse.com>
@tserong
Copy link
Contributor Author

tserong commented Apr 2, 2024

(rebased on master)

@shuo-wu shuo-wu merged commit 6df0510 into longhorn:master Apr 2, 2024
5 checks passed
@tserong tserong deleted the wip-add-virtualSize branch April 2, 2024 23:14
@shuo-wu
Copy link
Contributor

shuo-wu commented Apr 4, 2024

@mergify backport v1.5.x v1.6.x

Copy link

mergify bot commented Apr 4, 2024

backport v1.5.x v1.6.x

✅ Backports have been created

tserong added a commit to tserong/harvester that referenced this pull request Apr 8, 2024
This is necessary to bring in support for backing image virtual size:

- longhorn/backing-image-manager#208
- longhorn/longhorn-manager#2680

Signed-off-by: Tim Serong <tserong@suse.com>
tserong added a commit to tserong/harvester that referenced this pull request Apr 8, 2024
This is necessary to bring in support for backing image virtual size:

- longhorn/backing-image-manager#208
- longhorn/longhorn-manager#2680

Note the the longhorn chart version in dependency_charts/longhorn is v1.6.0-dev
because that's what it's set to at the time of writing in the upstream longhorn
git repo, but the tags for the various longhorn images are all "master-head".
IMO we really shouldn't take this change until we at least get a proper v1.7.0
RC or dev tag release.

Signed-off-by: Tim Serong <tserong@suse.com>
tserong added a commit to tserong/harvester that referenced this pull request Apr 23, 2024
This is necessary to bring in support for backing image virtual size:

- longhorn/backing-image-manager#208
- longhorn/longhorn-manager#2680

Note the the longhorn chart version in dependency_charts/longhorn is v1.6.0-dev
because that's what it's set to at the time of writing in the upstream longhorn
git repo, but the tags for the various longhorn images are all "master-head".
IMO we really shouldn't take this change until we at least get a proper v1.7.0
RC or dev tag release.

Signed-off-by: Tim Serong <tserong@suse.com>
tserong added a commit to tserong/harvester that referenced this pull request Apr 23, 2024
This is necessary to bring in support for backing image virtual size:

- longhorn/backing-image-manager#208
- longhorn/longhorn-manager#2680

Note the the longhorn chart version in dependency_charts/longhorn is v1.6.0-dev
because that's what it's set to at the time of writing in the upstream longhorn
git repo, but the tags for the various longhorn images are all "master-head".
IMO we really shouldn't take this change until we at least get a proper v1.7.0
RC or dev tag release.

Signed-off-by: Tim Serong <tserong@suse.com>
tserong added a commit to tserong/harvester that referenced this pull request May 24, 2024
With this change, we can do this:

  # kubectl get vmimages
  NAME          DISPLAY-NAME                                         SIZE          VIRTUALSIZE   AGE
  image-26r4s   sles15-sp5-minimal-vm.x86_64-kvm-and-xen-qu1.qcow2   267845632     25769803776   6m36s
  image-bgnhb   sle-micro.x86_64-5.5.0-default-qcow-gm.qcow2         1001652224    21474836480   20m
  image-nmmvj   sle-15-sp5-full-x86_64-gm-media1.iso                 14548992000   14548992000   19m

This in turn means we can update the UI to take image virtual size into
account when creating VMs.

Note that this requires the following Longhorn PRs, which have been
backported to longhorn 1.5.5 and 1.6.2:

- longhorn/backing-image-manager#208
- longhorn/longhorn-manager#2680
- longhorn/longhorn#8267

Related issue: harvester#4905

Signed-off-by: Tim Serong <tserong@suse.com>
tserong added a commit to tserong/harvester that referenced this pull request Jun 20, 2024
With this change, we can do this:

  # kubectl get vmimages
  NAME          DISPLAY-NAME                                         SIZE          VIRTUALSIZE   AGE
  image-26r4s   sles15-sp5-minimal-vm.x86_64-kvm-and-xen-qu1.qcow2   267845632     25769803776   6m36s
  image-bgnhb   sle-micro.x86_64-5.5.0-default-qcow-gm.qcow2         1001652224    21474836480   20m
  image-nmmvj   sle-15-sp5-full-x86_64-gm-media1.iso                 14548992000   14548992000   19m

This in turn means we can update the UI to take image virtual size into
account when creating VMs.

Note that this requires the following Longhorn PRs, which have been
backported to longhorn 1.5.5 and 1.6.2:

- longhorn/backing-image-manager#208
- longhorn/longhorn-manager#2680
- longhorn/longhorn#8267

Related issue: harvester#4905

Signed-off-by: Tim Serong <tserong@suse.com>
tserong added a commit to tserong/harvester that referenced this pull request Jul 3, 2024
With this change, we can do this:

  # kubectl get vmimages
  NAME          DISPLAY-NAME                                         SIZE          VIRTUALSIZE   AGE
  image-26r4s   sles15-sp5-minimal-vm.x86_64-kvm-and-xen-qu1.qcow2   267845632     25769803776   6m36s
  image-bgnhb   sle-micro.x86_64-5.5.0-default-qcow-gm.qcow2         1001652224    21474836480   20m
  image-nmmvj   sle-15-sp5-full-x86_64-gm-media1.iso                 14548992000   14548992000   19m

This in turn means we can update the UI to take image virtual size into
account when creating VMs.

Note that this requires the following Longhorn PRs, which have been
backported to longhorn 1.5.5 and 1.6.2:

- longhorn/backing-image-manager#208
- longhorn/longhorn-manager#2680
- longhorn/longhorn#8267

Related issue: harvester#4905

Signed-off-by: Tim Serong <tserong@suse.com>
tserong added a commit to harvester/harvester that referenced this pull request Jul 4, 2024
With this change, we can do this:

  # kubectl get vmimages
  NAME          DISPLAY-NAME                                         SIZE          VIRTUALSIZE   AGE
  image-26r4s   sles15-sp5-minimal-vm.x86_64-kvm-and-xen-qu1.qcow2   267845632     25769803776   6m36s
  image-bgnhb   sle-micro.x86_64-5.5.0-default-qcow-gm.qcow2         1001652224    21474836480   20m
  image-nmmvj   sle-15-sp5-full-x86_64-gm-media1.iso                 14548992000   14548992000   19m

This in turn means we can update the UI to take image virtual size into
account when creating VMs.

Note that this requires the following Longhorn PRs, which have been
backported to longhorn 1.5.5 and 1.6.2:

- longhorn/backing-image-manager#208
- longhorn/longhorn-manager#2680
- longhorn/longhorn#8267

Related issue: #4905

Signed-off-by: Tim Serong <tserong@suse.com>
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.

2 participants