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 (backport #2680) #2718

Closed
wants to merge 4 commits into from

Conversation

mergify[bot]
Copy link

@mergify mergify bot commented Apr 4, 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


This is an automatic backport of pull request #2680 done by Mergify.

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>
(cherry picked from commit 9707ff0)

# Conflicts:
#	scripts/validate
Signed-off-by: Tim Serong <tserong@suse.com>
(cherry picked from commit ffe9a83)

# Conflicts:
#	go.mod
#	go.sum
#	vendor/github.com/longhorn/backing-image-manager/pkg/rpc/rpc.pb.go
#	vendor/github.com/longhorn/backing-image-manager/pkg/util/util.go
#	vendor/modules.txt
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>
(cherry picked from commit d438e9b)
…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>
(cherry picked from commit 6df0510)
@mergify mergify bot requested a review from a team as a code owner April 4, 2024 00:25
@mergify mergify bot added the conflicts label Apr 4, 2024
Copy link
Author

mergify bot commented Apr 4, 2024

Cherry-pick of 9707ff0 has failed:

On branch mergify/bp/v1.5.x/pr-2680
Your branch is up to date with 'origin/v1.5.x'.

You are currently cherry-picking commit 9707ff03.
  (fix conflicts and run "git cherry-pick --continue")
  (use "git cherry-pick --skip" to skip this patch)
  (use "git cherry-pick --abort" to cancel the cherry-pick operation)

Unmerged paths:
  (use "git add <file>..." to mark resolution)
	both modified:   scripts/validate

no changes added to commit (use "git add" and/or "git commit -a")

Cherry-pick of ffe9a83 has failed:

On branch mergify/bp/v1.5.x/pr-2680
Your branch is ahead of 'origin/v1.5.x' by 1 commit.
  (use "git push" to publish your local commits)

You are currently cherry-picking commit ffe9a837.
  (fix conflicts and run "git cherry-pick --continue")
  (use "git cherry-pick --skip" to skip this patch)
  (use "git cherry-pick --abort" to cancel the cherry-pick operation)

Changes to be committed:
	modified:   vendor/github.com/RoaringBitmap/roaring/README.md
	modified:   vendor/github.com/RoaringBitmap/roaring/arraycontainer.go
	modified:   vendor/github.com/RoaringBitmap/roaring/runcontainer.go
	modified:   vendor/github.com/longhorn/backing-image-manager/api/types.go
	modified:   vendor/github.com/longhorn/backing-image-manager/pkg/rpc/rpc.proto

Unmerged paths:
  (use "git add <file>..." to mark resolution)
	both modified:   go.mod
	both modified:   go.sum
	both modified:   vendor/github.com/longhorn/backing-image-manager/pkg/rpc/rpc.pb.go
	both modified:   vendor/github.com/longhorn/backing-image-manager/pkg/util/util.go
	both modified:   vendor/modules.txt

To fix up this pull request, you can check it out locally. See documentation: https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/reviewing-changes-in-pull-requests/checking-out-pull-requests-locally

@shuo-wu
Copy link
Contributor

shuo-wu commented Apr 6, 2024

Close this and the conflict is resolved in #2722

@shuo-wu shuo-wu closed this Apr 6, 2024
@mergify mergify bot deleted the mergify/bp/v1.5.x/pr-2680 branch April 6, 2024 06:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants