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 qcow2 backing images #208

Merged
merged 3 commits into from
Mar 26, 2024

Conversation

tserong
Copy link
Contributor

@tserong tserong commented Feb 29, 2024

Which issue(s) this PR fixes:

Issue longhorn/longhorn#7923

What this PR does / why we need it:

This commit adds VirtualSize to the BackingImage, BackingImageStatus, BackingImageSpec and FileInfo structs. This will allow longhorn-manager to in turn pick up the virtual image size and expose it in the status section of the BackingImage and BackingImageManager CRDs.

The virtual size of an image is determined by running qemu-img info. For qcow2 images, the virtual size will usually be larger than the actual physical file size. For raw images, qemu-img info still reports virtual size, but it's the same as the physical file size in this case.

It's important to note that we can only report the virtual size of an image once the syncing file is ready. If the syncing file is not yet ready (or if for some reason the call to qemu-img info fails), virtual size will be set to zero.

Special notes for your reviewer:

In order to actually be useful, this requires matching changes in longhorn-manager. I'm still working on that piece :-)

My implementation of util.GetImageVirtualSize() uses qemu-img info --output=json then unmarshalls the JSON to extract the virtual size. There's a similar existing function - util.DetectFileFormat() - which instead parses the human readable output using bufio.NewScanner, but IMO the JSON form makes for simpler code. I wasn't sure if I should mess with util.DetectFileFormat() while working on this PR, so I left it alone.

Note that there's an extra commit in here to fix make when run locally (see the commit message for details). I can split that out into a separate PR if you'd prefer.

Additional documentation or context

Here's some sample output from backing-image-manager backing-image list with this change applied:

  1. A ~955MiB (1,001,652,224 bytes) qcow2 image with a virtualSize of 20GiB (21,474,836,480 bytes):
    "default-image-gjm5l": {
    	"name": "default-image-gjm5l",
    	"uuid": "c5ba7ef9",
    	"size": 1001652224,
    	"virtualSize": 21474836480,
    	"expectedChecksum": "1bbfc258229011fc4be34d35e823dab72431f577de5027d88f9236bbb8b786f25ca976422dfb55a02194215b0b717fbba033516c2b8c32b46c6587a580aaa748",
    	"status": {
    		"state": "ready",
    		"currentChecksum": "1bbfc258229011fc4be34d35e823dab72431f577de5027d88f9236bbb8b786f25ca976422dfb55a02194215b0b717fbba033516c2b8c32b46c6587a580aaa748",
    		"sendingReference": 0,
    		"errorMsg": "",
    		"senderManagerAddress": "",
    		"progress": 100
    	}
    }
    
  2. A ~13.5GiB (14,548,992,000 bytes) ISO (raw) image, while syncing. Note that virtualSize is zero here because we don't know it yet:
    "default-image-87b7d": {
    	"name": "default-image-87b7d",
    	"uuid": "aa95cadc",
    	"size": 14548992000,
    	"virtualSize": 0,
    	"expectedChecksum": "88a21ac07b673bd60fd8733e6dd5d93444651fd4e806c9922904b431d75b90aa1bac7da4553ffff3a8badc64b0be44e41f5155961fcc565aa2b0f5045862a367",
    	"status": {
    		"state": "in-progress",
    		"currentChecksum": "",
    		"sendingReference": 0,
    		"errorMsg": "",
    		"senderManagerAddress": "",
    		"progress": 13
    	}
    },
    
  3. The same image once sync is complete. Note that size and virtualSize are identical now:
    "default-image-87b7d": {
    	"name": "default-image-87b7d",
    	"uuid": "aa95cadc",
    	"size": 14548992000,
    	"virtualSize": 14548992000,
    	"expectedChecksum": "88a21ac07b673bd60fd8733e6dd5d93444651fd4e806c9922904b431d75b90aa1bac7da4553ffff3a8badc64b0be44e41f5155961fcc565aa2b0f5045862a367",
    	"status": {
    		"state": "ready",
    		"currentChecksum": "88a21ac07b673bd60fd8733e6dd5d93444651fd4e806c9922904b431d75b90aa1bac7da4553ffff3a8badc64b0be44e41f5155961fcc565aa2b0f5045862a367",
    		"sendingReference": 0,
    		"errorMsg": "",
    		"senderManagerAddress": "",
    		"progress": 100
    	}
    },
    

@tserong tserong requested a review from a team as a code owner February 29, 2024 11:27
@tserong
Copy link
Contributor Author

tserong commented Mar 1, 2024

Force push to make the 512 byte padding in createQcow2File() work properly regardless of the size of the image file.

shuo-wu
shuo-wu previously approved these changes Mar 6, 2024
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.

LGTM

pkg/sync/sync_file.go Outdated Show resolved Hide resolved
@tserong
Copy link
Contributor Author

tserong commented Mar 7, 2024

(rebased on latest master)

pkg/util/util.go Outdated Show resolved Hide resolved
tserong added a commit to tserong/longhorn-manager that referenced this pull request Mar 8, 2024
This manually applies the relevant changes from
longhorn/backing-image-manager#208 to
vendor/github.com/longhorn/backing-image-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 <[email protected]>
@tserong
Copy link
Contributor Author

tserong commented Mar 18, 2024

(rebased on latest master)

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 <[email protected]>
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 <[email protected]>
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 <[email protected]>
shuo-wu
shuo-wu previously approved these changes Mar 19, 2024
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.

LGTM

pkg/util/util.go Show resolved Hide resolved
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 <[email protected]>
@innobead
Copy link
Member

@ChanYiLin Please review this.

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 <[email protected]>
This commit adds VirtualSize to the BackingImage, BackingImageStatus,
BackingImageSpec and FileInfo structs. This will allow longhorn-manager
to in turn pick up the virtual image size and expose it in the
status section of the BackingImage and BackingImageManager CRDs.

The virtual size of an image is determined by running `qemu-img info`.
For qcow2 images, the virtual size will usually be larger than the
actual physical file size.  For raw images, `qemu-img info` still reports
virtual size, but it's the same as the physical file size in this case.

It's important to note that we can only report the virtual size of an
image once the syncing file is ready.  If the syncing file is not yet
ready (or if for some reason the call to `qemu-img info` fails), virtual
size will be set to zero.

Related issue: longhorn/longhorn#7923

Signed-off-by: Tim Serong <[email protected]>
Copy link
Contributor

@ChanYiLin ChanYiLin 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 for the contribution

@mergify mergify bot merged commit c5288d7 into longhorn:master Mar 26, 2024
6 checks passed
@shuo-wu
Copy link
Contributor

shuo-wu commented Mar 26, 2024

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

Copy link

mergify bot commented Mar 26, 2024

backport v1.5.x v1.6.x

✅ Backports have been created

@shuo-wu
Copy link
Contributor

shuo-wu commented Mar 26, 2024

Would you help update longhorn-manager and longhorn-instance-manager to fit this change?

@tserong
Copy link
Contributor Author

tserong commented Mar 27, 2024

Sure, will do.

@tserong tserong deleted the wip-add-virtualSize branch March 27, 2024 05:11
@tserong
Copy link
Contributor Author

tserong commented Mar 27, 2024

The longhorn-manager PR is longhorn/longhorn-manager#2680.

I'm not sure yet what needs to change in longhorn-instance-manager as I can't find any references to backing-image-manager in the longhorn-instance-manager source. Can you please advise?

@shuo-wu
Copy link
Contributor

shuo-wu commented Mar 27, 2024

I can't find any references to backing-image-manager in the longhorn-instance-manager source. Can you please advise?

I just re-checked the code. You are right there is no reference in longhorn-instance-manager now. Please ignore this.

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 <[email protected]>
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 <[email protected]>
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 <[email protected]>
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 <[email protected]>
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 <[email protected]>
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 <[email protected]>
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 <[email protected]>
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 <[email protected]>
tserong added a commit to tserong/harvester that referenced this pull request Nov 29, 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 <[email protected]>
(cherry picked from commit 79f6cdc)
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.

4 participants