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) #2723

Merged
merged 4 commits into from
Apr 9, 2024

Conversation

shuo-wu
Copy link
Contributor

@shuo-wu shuo-wu commented Apr 6, 2024

Which issue(s) this PR fixes:

longhorn/longhorn#7923

What this PR does / why we need it:

This resolves the conflict for PR #2719

Special notes for your reviewer:

Additional documentation or context

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)
Signed-off-by: Tim Serong <tserong@suse.com>
(cherry picked from commit ffe9a83)
Signed-off-by: Shuo Wu <shuo.wu@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>
(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)
@innobead innobead merged commit e6d5aa7 into longhorn:v1.6.x Apr 9, 2024
4 checks passed
@shuo-wu shuo-wu deleted the mergify/bp/v1.6.x/pr-2680 branch July 19, 2024 19:48
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.

3 participants