clalancette / oz

Automated installation for guest images
GNU Lesser General Public License v2.1
311 stars 130 forks source link

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

Open AdamWill opened 5 months ago

AdamWill commented 5 months ago

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 commented 5 months ago

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 commented 5 months ago

ImageFactory PR: https://github.com/redhat-imaging/imagefactory/pull/458

AdamWill commented 5 months ago

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 commented 5 months ago

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

AdamWill commented 5 months ago

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 commented 5 months ago

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

AdamWill commented 5 months ago

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

AdamWill commented 5 months ago

just to make things clearer, I've moved the CI fixes to https://github.com/clalancette/oz/pull/314 . that means CI fails on this PR now, but with 314 merged it would pass again.

keszybz commented 4 months ago

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 commented 4 months ago

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.