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

Allow image size specification using any SI or IEC unit #310

Merged
merged 1 commit into from
Aug 21, 2024

Conversation

AdamWill
Copy link
Contributor

Currently, oz only allows image sizes to be specified as integer amounts of gibibytes or tebibytes (that's IEC power-of-two units). This is unfortunately inflexible. Consider that storage media are typically specified in SI power-of-ten sizes, so a USB stick may be 16 gigabytes (SI power-of-ten GB) in size - that's 16,000,000,000 bytes. Let's say we want to build an image that will fit on such a USB stick. oz will only allow us to build an image of 15,032,385,536 bytes (14 gibibytes) or 16,106,127,360 bytes (15 gibibytes). So we're either slightly too big, or leaving nearly a gigabyte on the table.

This allows the image size to be specified in the TDL with most any IEC or SI unit suffix, from B (bytes) all the way up to YiB (yobibytes). A size with no suffix or the suffix "G" is taken as gibibytes and a size with the suffix "T" is taken as tebibytes, as before, but other ambiguous suffixes are not accepted. All casing is accepted. Behind the scenes, we convert the size to bytes and specify it that way in the libvirt XML when creating the image in _internal_generate_diskimage.

This does change the interface of generate_diskimage(), by making the unit for the size argument bytes instead of gibibytes. I can't see a clean way to avoid this while allowing flexibility. I have checked, and AFAICT, among active projects, only oz itself and the ImageFactory TinMan plugin call this function. The TinMan plugin will need a trivial change to its fallback default value.

This is currently tagged WIP because I am not sure it's safe to include the sizeof_fmt snippet; its licensing is complicated. I'm asking Richard Fontana for his wisdom on this ATM. If it's not safe to include we'll have to rewrite it or just print the size in bytes or something.

@AdamWill
Copy link
Contributor Author

Couple of notes:

  1. the typing here is weird, but I've left it as-is. _parse_disksize returns a string, before and after this patch. But the generate_diskimage methods and _internal_generate_diskimage implicitly expect ints, since the default value for each is an int. In the end, _internal_generate_diskimage does str(int(capacity)) (where capacity is just size in this case), which forcibly gets rid of the ambiguity I guess, but it seems a bit inelegant.
  2. AFAICT, test-60-command-mix-positions-and-not.tdl, test-61-command-duplicate-position.tdl and test-62-repository-localhost.tdl are not currently run and have never been run. I left this as-is too, but it might want fixing.

AdamWill added a commit to AdamWill/imagefactory that referenced this pull request Jan 23, 2024
In clalancette/oz#310 I'm proposing we
change the unit of the `generate_diskimage` method's size arg
from gibibytes to bytes. This needs adjusting to match that.
Don't merge this unless and until that PR is merged.

Signed-off-by: Adam Williamson <awilliam@redhat.com>
@AdamWill
Copy link
Contributor Author

ImageFactory PR: redhat-imaging/imagefactory#458

@AdamWill
Copy link
Contributor Author

FWIW, I'm also working on a PR that fixes a few problems, gets the test suite (and hence oz itself) working on Python 3.12, and - hopefully - a github action to run it automatically, in containers (I have the test suite working in containers locally, just need to wrap it up).

@AdamWill AdamWill force-pushed the image-size-bytes branch 5 times, most recently from edcc28a to 3e95bad Compare January 25, 2024 17:00
@AdamWill AdamWill changed the title WIP DO NOT MERGE Allow image size specification using any SI or IEC unit Allow image size specification using any SI or IEC unit Jan 25, 2024
@AdamWill
Copy link
Contributor Author

hmm, I think this failure might actually be a real one! looking into it.

@AdamWill
Copy link
Contributor Author

yaaay.

The change to the XML schema is probably a bit more permissive than it should be, but, you know, regexes. i don't want to make it too complicated.

It turns out we do need this, because an attempt to build a current Fedora Workstation aarch64 image with size 14 gibibytes - the largest currently-possible size that's less than 16 gigabytes - failed because it was 331 MB short. This should let us specify the size as 16 gigabytes and get a successful, not-too-large image. I will do a scratch build of oz with this patch and get Kevin to do a test image build.

@AdamWill
Copy link
Contributor Author

This scratch build was done with size set to 16GB and worked. We did realize we need a change to Koji too, though.

Copy link

@keszybz keszybz left a comment

Choose a reason for hiding this comment

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

I tested both functions manually, and they work as expected.

oz/ozutil.py Show resolved Hide resolved
@AdamWill
Copy link
Contributor Author

BTW, to resolve the licensing concern, Richard said the function is too trivial to be copyrightable.

Currently, oz only allows image sizes to be specified as integer
amounts of gibibytes or tebibytes (that's IEC power-of-two units).
This is unfortunately inflexible. Consider that storage media are
typically specified in SI power-of-ten sizes, so a USB stick may
be 16 gigabytes (SI power-of-ten GB) in size - that's
16,000,000,000 bytes. Let's say we want to build an image that
will fit on such a USB stick. oz will only allow us to build an
image of 15,032,385,536 bytes (14 gibibytes) or 16,106,127,360
bytes (15 gibibytes). So we're either slightly too big, or leaving
nearly a gigabyte on the table.

This allows the image size to be specified in the TDL with most
any IEC or SI unit suffix, from B (bytes) all the way up to YiB
(yobibytes). A size with no suffix or the suffix "G" is taken as
gibibytes and a size with the suffix "T" is taken as tebibytes,
as before, but other ambiguous suffixes are not accepted. All
casing is accepted. Behind the scenes, we convert the size to
bytes and specify it that way in the libvirt XML when creating
the image in _internal_generate_diskimage.

This does change the interface of generate_diskimage(), by making
the unit for the size argument bytes instead of gibibytes. I can't
see a clean way to avoid this while allowing flexibility. I have
checked, and AFAICT, among active projects, only oz itself and
the ImageFactory TinMan plugin call this function. The TinMan
plugin will need a trivial change to its fallback default value.

Signed-off-by: Adam Williamson <awilliam@redhat.com>
@AdamWill
Copy link
Contributor Author

just to make things clearer, I've moved the CI fixes to #314 . that means CI fails on this PR now, but with 314 merged it would pass again.

Copy link

@keszybz keszybz left a comment

Choose a reason for hiding this comment

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

LGTM.

@keszybz
Copy link

keszybz commented Feb 11, 2024

It's been two weeks. This is a blocker for Fedora 40 Beta (https://bugzilla.redhat.com/show_bug.cgi?id=2263771), so it'd be nice to get this resolved…

@AdamWill
Copy link
Contributor Author

We don't necessarily need it merged upstream, though. ;) In fact, the builders have it already, as part of this update.

What we don't have yet is a patched Koji, Kevin and I can work on that next week I guess.

bella485 pushed a commit to bella485/koji that referenced this pull request May 11, 2024
This has never been necessary, because oz has always treated a
size with no unit as being in gibibytes. After
clalancette/oz#310 it will be actively
harmful, because it will prevent us from using that new ability
of oz to specify a size using any other unit, as we have a
specific reason to want:
https://bugzilla.redhat.com/show_bug.cgi?id=2247611

Signed-off-by: Adam Williamson <awilliam@redhat.com>
mikem23 pushed a commit to koji-project/koji that referenced this pull request Aug 14, 2024
This has never been necessary, because oz has always treated a
size with no unit as being in gibibytes. After
clalancette/oz#310 it will be actively
harmful, because it will prevent us from using that new ability
of oz to specify a size using any other unit, as we have a
specific reason to want:
https://bugzilla.redhat.com/show_bug.cgi?id=2247611

Signed-off-by: Adam Williamson <awilliam@redhat.com>
@nullr0ute nullr0ute merged commit cf69701 into clalancette:master Aug 21, 2024
0 of 3 checks passed
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