Differential D687

rewrite createhdds in python, make it shinier
ClosedPublic
Actions

Authored by adamwill on Dec 11 2015, 6:48 AM.

Details

Reviewers
garretraziel
jskladan
Commits
rOPENQA078ec0138bf3: rewrite createhdds in python, make it shinier
rOPENQA574eb3720f53: rewrite createhdds in python, make it shinier
rOPENQA4223b4e029e8: rewrite createhdds in python, make it shinier
Summary

createhdds.sh was just too damn simple and understandable, so
I thought I'd make it three times longer, object oriented,
and hard to understand!

OK, OK, that's not great sales. Alright. The main thing was to
make it smarter. This rewrite lets it do these things:

  • Only create the images that are missing (not rebuild all)
  • Work out the releases to build images for
  • Rename images when appropriate
  • Rebuild images when they need rebuilding
  • Remove old / abandoned images

It can figure out what images ought to be present - including
working out the 'next' release and figuring out from that what
releases it needs images for - and build only the missing ones.
There's a 'version' concept for images; if the existing image
is older than the version given in the data file, it'll be
rebuilt. The data file can list 'rename' pairs, allowing images
to be renamed (like when we move from a single image to multiple
label/filesystem variants). This code uses fedfind's ability
to find the current release version to figure out what releases
we need virtbuilder images for (so you don't have to pass it
in). And it can find image files that aren't in the 'currently
expected' set and wipe them. Images can also have a 'maxage',
triggering a rebuild when they exceed it - this is intended
for the virtbuilder images, so we get a rebuild with the
latest updates every so often (default is two weeks).

The point of all this is to help with unattended deployment/
maintenance, i.e. the ansible deployment we have in infra;
the idea is that we can just set that up to run the 'all'
subcommand every so often, and it'll remove old images, create
new ones, and rebuild ones that are outdated.

I kept the ability to build a single image (or a whole image
'group'), and included the ability to just run a check without
actually doing a rebuild. There's a few little weird things
and holes here as it's not really the focus of the tool.

Test Plan

Build all images, do a full test run, and see if
it works OK. Test out all the variations of building single
images / image groups, and using the 'check' command.

Diff Detail

Repository
rOPENQA fedora_openqa
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
adamwill retitled this revision from to rewrite createhdds in python, make it shinier.Dec 11 2015, 6:48 AM
adamwill updated this object.
adamwill edited the test plan for this revision. (Show Details)
adamwill added reviewers: jskladan, garretraziel.
adamwill added a comment.EditedDec 11 2015, 6:50 AM

I should probably add a little readme or something explaining hdds.json and the .commands files - for now they're documented in the main script's docstrings, I'll throw something together tomorrow if I can. Hopefully you can follow how it works from the docstrings.

The disk size handling is a bit half-assed; I was using blivet's cool Size class for it, which works great, but blivet's deps are pretty heavy to pull in just for this use, so I hacked together something good enough.

adamwill updated this revision to Diff 1773.Dec 11 2015, 6:59 AM

rename 'apiver' to 'imgver', it's really not an api

adamwill updated this revision to Diff 1774.Dec 11 2015, 7:02 AM

more 'API' renaming

adamwill updated this revision to Diff 1775.Dec 11 2015, 7:13 AM

sigh, i screwed up the apiver/imgver sed, fixing

jskladan added a comment.Dec 11 2015, 8:37 AM

Overall, it is probably OK, although it is overly-complicated for what it does.

Thorough documentation of the hdds.json is a must, though - I could probably get to the gist of it by following the code carefully, but that is not the right way (but I guess you'll be adding the docs soon anyway).

Some comments for the first-pass of the code in-line

tools/createhdds.py
489

Why return current, when it's never really used in any of the func-calls?

543

hdds is not used at all in the method, but the comments mention the hdds.json on multiple occasions - this is my first pass of the code, so take it with a grain of salt, but it sometimes does not make sense to me. For example:

If the user passed in label or filesystem, we pass them on
to get_guestfs_images as single-item lists, otherwise we
​just pass 'None', causing it to use the values from hdds.json

How is it overriden by hdds.json, when the hdds value (the parsed hdds.json file) is not used at all?

683–691

This could IMHO be embedded in the main() directly, the run() is (AFAIK) not called from anywhere else, anyway...

691

Since hdds is not used at all in cli_image(), how about changing the order of the arguments (args.func(args, hdds))? This would then allow the cli_image() to have def cli_image(args, *_) signature.
Absolutely not necessary, just a food for thought.

tools/hdds.json
5

Why imgver instead of imgver?

garretraziel accepted this revision.Dec 11 2015, 11:38 AM

Other than my and Josef's concerns, looks good to me.

tools/createhdds.py
254

It's not necessary ATM, but could you make root password also configurable?

266–278

I was hoping that expect is only temporary solution, but oh well, I guess that there is still no better way how to do this.

This revision is now accepted and ready to land.Dec 11 2015, 11:38 AM
jskladan requested changes to this revision.Dec 11 2015, 11:59 AM

Nacking just so the 'Ack' status from @garretraziel does not make it seem like it's ready to land

This revision now requires changes to proceed.Dec 11 2015, 11:59 AM
adamwill updated this revision to Diff 1776.Dec 11 2015, 3:01 PM

fix up ' imgver' in hdds.json too

adamwill added inline comments.Dec 11 2015, 3:23 PM
tools/createhdds.py
490

because I wrote check() quite early - before I had the actual CLI code in place - and was just printing everything it returned to make sure it was sane :) i.e., no particularly great reason. I can change it.

544

so what happens here is that we actually pulled a dict out of hdds *in parse_args()*, and stuffed it into args; note where this function reads args.imggrp. I originally had it set up so parse_args() would just send the image 'type' and the name of the image group, and this function would find the appropriate group in hdds itself, then thought 'wait, that's silly, since parse_args() already got the dict it can just return the whole thing'. This is part of the 'clever clever' trick to set up a named subcommand parser for each individual image group. It may be that there's a cleaner/more obvious way to set this up, I'll think about it a bit, and comment it if not.

684–692

first comment - yeah, probably, I just boilerplate this stuff around from script to script (it's all there in relval and fedfind too). second comment - sure, it's a tiny thing but it'd give me more pylint points, I guess I'll do it :)

tools/hdds.json
6

Because I did a bad sed at midnight after a few drinks :) I fixed it up in createhdds.py but forgot to fix the json too until this morning.

adamwill added a comment.Dec 11 2015, 3:33 PM

On complexity - so I was trying to strike a balance between a somewhat-sensible and extensible design, and simplicity. I could write it much simpler just keeping all the image definitions in-line in the python and not worry so much about genericness, but I kinda hate having a bunch of what's essentially configuration in-line in code. I actually have an impulse to make it much *more* generic - right now there's a decent amount of special-casing in terms of how the 'grouped' things in the image groups are handled (filesystems and labels and release and arch), if we suddenly decide we want some other form of image group variation I'll have to make quite a few tweaks.

What I like about this design is that you can add a new image or image group just by editing hdds.json (and adding a .commands file if appropriate) - you don't have to touch the code at all.

tools/createhdds.py
255

could do, sure.

267–279

yeah, I couldn't think of any. I tried without it and found the same problem you did - virt-builder can't do the selinux relabel itself and just dumps a .autorelabel to make it happen on first boot. rwmjones pointed me to https://bugzilla.redhat.com/show_bug.cgi?id=1049656 , which is in the general neighbourhood but not exactly what I saw when I ran it with -x -v - when I did that I saw it complaining about the policy version (which is why I wrote that in my comment).

Given the requirement 'boot the image, make sure the relabel finished, and shut it down cleanly' I couldn't think of any better option than expect :/ I did at least move all other stuff out of expect so this is the only thing we're using it for now. if anyone has any ideas I'm happy to try...

adamwill updated this revision to Diff 1777.Dec 12 2015, 6:35 AM

address review concerns, add README, do more stuff in commands

So this mostly addresses the issues raised in review, and adds
a README file explaining various stuff including the JSON file
format.

Note that instead of making the root password configurable in
the JSON (as suggested), I decided to be more consistent:
update, root-password, and selinux-relabel are all actually
virt-builder customization commands we were passing in as
arguments, so I went ahead and moved them into the .commands
files instead (adding a minimal.commands). This achieves the
same goal and is consistent - we only handle non-customization
arguments in the JSON / python.

adamwill updated this revision to Diff 1779.Dec 12 2015, 8:22 PM

document 'maxage'

adamwill updated this revision to Diff 1780.Dec 12 2015, 8:26 PM

comment the 'imggrp' argparse stuff a bit more

as this threw jskladan off, let's document it as hard as
possible.

adamwill updated this revision to Diff 1781.Dec 12 2015, 10:11 PM

drop the 'image version' files, simplify some other stuff

in the shower this morning I realized a way to simplify this
quite a bit. We don't need special 'image version' files -
we can just stuff the image version into the filename, and
then everything we want to happen will just happen magically.
The old files will now be considered 'unknown' (so all will
clean them up) and the new files 'missing' (so all will build
them). Simples! As a special case, the version isn't included
if it's < 2, so the current filenames are still 'valid'.

That kinda led to a few other re-arranagements: I prefer this
approach to mutating the guestfs image file names (providing
an argument that specifies image attributes that should be
appended to the file name) as it's more generic and it all
happens at instantiation, it doesn't need special methods.
And quite a bit of the stuff on the top-level Image class
just didn't really seem necessary any more. That whole class
could actually just go and be replaced with a size-setting
function now I guess...

adamwill updated this revision to Diff 1782.Dec 12 2015, 10:14 PM

clean up guestfs image temp files if user ctrl-c's

adamwill updated this revision to Diff 1783.Dec 13 2015, 5:03 AM

drop Image class in favour of a simple size string function

about the only useful thing the Image class did any more was
handle size strings, so let's ditch it and just have a simple
function for the size strings. (I actually would like to use
blivet's awesome Size class for this, but it's a heavy dep
just for this purpose).

jskladan requested changes to this revision.Dec 15 2015, 12:02 PM

Definitelly much better than the first pass. I still have some food for thought though.

tools/createhdds.py
99

This does not really make much sense. First of all, why not store the 1 in the first place? Next up, the content of the imgver does not really need to be integer at all. The way your code works now, the images get rebuilt if imgver changes to any kind of new value. For example if I had a json file with imgver set to 20, and "updated" the imgver to 3, the code would still say that the said image needs to be rebuilt, as there is no file with _v3_ in the directory.

So it would IMHO only make sense to remove the int() check, and just plain store whatever is imgver set to, into the filename.
Change the documentation to reflect the fact, that the code does not at all care about it being an integer, or an ascending number, and just say something in the lines of "When imgver changes, the image gets rebuilt".

Ditto for the VirtBuilderImage.

101–104

see comment on get_guestfs_images

195–196

See comment on GuestfsImage.

287–293

This could really be replaced with:

name_extras = []
if len(imggrp.get('filesystems', [])) > 1 or len(filesystems) > 1:
    name_extras.append(filesystem)
if len(imggrp.get('labels', [])) > 1 or len(labels) > 1:
    name_extras.append(label)
img = GuestfsImage(​name, imgver, size, filesystem, label, parts, writes, uploads, name_extras)

and then in GuestfsImage one could easily replace the:

if name_extras:
    for item in name_extras:
        getter = attrgetter(item)
        self.filename = "{0}_{1}".format(self.filename, getter(self))

with much simpler:

if name_extras:
    for item in name_extras:
        self.filename = "{0}_{1}".format(self.filename, item)

and get rid of the (very hard to read, and unnecessary) attrgetter "magic".

I get why it was written this way, but since the default values of filesystem and label are never really used, as this is the only place where the GuestfsImage is instantiated, and the filesystem and label parameters are passed to the constructor.

This revision now requires changes to proceed.Dec 15 2015, 12:02 PM
adamwill added inline comments.Dec 15 2015, 5:24 PM
tools/createhdds.py
99

Why not store the 1 in the first place - so *currently existing* images aren't considered invalid, i.e. we can switch over to this new script without configuring renames for every existing image or renaming them manually. We can do that, though, if you'd prefer.

Integer comparison stuff - yeah, you're right, we can ditch that now.

287–293

yeah, in fact I actually meant to change that already but got sidetracked into something else (I tried ditching the classes entirely, but that didn't work out well and I gave up on it) and forgot it...

adamwill updated this revision to Diff 1790.Dec 15 2015, 5:58 PM

drop imgver concept entirely, simplify name_extras

So how about this: we don't really need imgver at all. We
can just do it with 'name': if we change an image group
incompatibly, just change its name to 'desktop_v2' or
whatever. The only slightly odd thing about that is that
it will also become the subcommand name, but meh, that doesn't
seem like a very big deal. I wasn't planning to automate use
of the subcommand names or anything, only 'check' and 'all'.

Also this simplifies name_extras as suggested by jskladan.

garretraziel requested changes to this revision.Dec 16 2015, 1:38 PM

Looks good in overall, name_extras is much more readable now, but I would (and I think that @jskladan would agree) leave imgver in there - just drop int() type conversion and append it every time. Although you could control it with name, changing programs' subcommand feels weird :-).

This revision now requires changes to proceed.Dec 16 2015, 1:38 PM
adamwill added a comment.Dec 16 2015, 5:22 PM

Well, I honestly don't really love the design of an 'imgver' that's actually just an arbitrary string that gets appended to the image name, but if you really want that, fine. I think it's icky.

adamwill updated this revision to Diff 1804.Dec 16 2015, 10:32 PM

restore imgver, by popular demand

it works a bit more simply now; it's just a string that gets
included in the filename (with the usual _ separators) if
specified, if not specified it is entirely omitted (this
preserves the current names).

jskladan accepted this revision.Dec 17 2015, 7:40 AM

Great, thanks!

garretraziel accepted this revision.Dec 17 2015, 12:11 PM

Yup, seems fine to me.

This revision is now accepted and ready to land.Dec 17 2015, 12:11 PM
Closed by commit rOPENQA574eb3720f53: rewrite createhdds in python, make it shinier (authored by adamwill). · Explain WhyDec 17 2015, 9:25 PM
This revision was automatically updated to reflect the committed changes.

Revision Contents

PathPackages
MInstallGuide.md (2 lines)
Mdocker/README.md (4 lines)
AMtools/README.createhdds (110 lines)
AMtools/createhdds.py (656 lines)
DMtools/createhdds.sh (218 lines)
AMtools/desktop.commands (7 lines)
AMtools/hdds.json (132 lines)
AMtools/minimal.commands (3 lines)

Diff 1811

View Options

InstallGuide.md

Show First 20 Lines • Show All 72 Lines • ▼ Show 20 Line(s)718. set up fedora tests from repo
73 zypper in guestfs-tools 73​ zypper in guestfs-tools
74 git clone http://bitbucket.org/rajcze/openqa_fedora.git /var/lib/openqa/tests/fedora 74​ git clone http://bitbucket.org/rajcze/openqa_fedora.git /var/lib/openqa/tests/fedora
75 chown -R geekotest /var/lib/openqa/tests/ 75​ chown -R geekotest /var/lib/openqa/tests/
76 76​
779. set up disks 77​9. set up disks
78 78​
79 git clone http://bitbucket.org/rajcze/openqa_fedora_tools.git ~/openqa_fedora_tools 79​ git clone http://bitbucket.org/rajcze/openqa_fedora_tools.git ~/openqa_fedora_tools
80 cd /var/lib/openqa/factory/hdd/ 80​ cd /var/lib/openqa/factory/hdd/
81 bash ~/openqa_fedora_tools/tools/createhdds.sh VERSION # version is current stable Fedora version 81​ ~/openqa_fedora_tools/tools/createhdds.py all
82 chown geekotest * 82​ chown geekotest *
83 83​
8410. setup services 84​10. setup services
85 85​
86 systemctl enable apache2 86​ systemctl enable apache2
87 systemctl restart apache2 87​ systemctl restart apache2
88 systemctl enable openqa-webui 88​ systemctl enable openqa-webui
89 systemctl restart openqa-webui 89​ systemctl restart openqa-webui
▲ Show 20 Lines • Show All 41 Lines • Show Last 20 Lines
View Options

docker/README.md

Show First 20 Lines • Show All 98 Lines • ▼ Show 20 Line(s)
99Populate the OpenQA's database: 99​Populate the OpenQA's database:
100 100​
101 docker exec openqa_webui /var/lib/openqa/tests/fedora/templates 101​ docker exec openqa_webui /var/lib/openqa/tests/fedora/templates
102 102​
103Create all necessary disk images: 103​Create all necessary disk images:
104 104​
105 clone https://bitbucket.org/rajcze/openqa_fedora_tools 105​ clone https://bitbucket.org/rajcze/openqa_fedora_tools
106 dnf install libguestfs-tools libguestfs-xfs expect 106​ dnf install libguestfs-tools libguestfs-xfs expect
107 cd data/factory/hdd && /PATH/TO/openqa_fedora_tools/tools/createhdds.sh VERSION 107​ cd data/factory/hdd && /PATH/TO/openqa_fedora_tools/tools/createhdds.py
108 108​
109where `VERSION` is current stable Fedora version (its images will be created for upgrade tests) and createhdds.sh is in `openqa_fedora_tools` repository in `/tools` directory. 109​where createhdds.py is in `openqa_fedora_tools` repository in `/tools` directory.
110 110​
111 111​
112# Running jobs 112​# Running jobs
113 113​
114After performing the "setup" tasks above - do not forget about tests and ISOs - you can schedule a test like this: 114​After performing the "setup" tasks above - do not forget about tests and ISOs - you can schedule a test like this:
115 115​
116 docker exec openqa_webui /var/lib/openqa/script/client isos post ISO=Fedora-Server-netinst-x86_64-22_Beta_RC3.iso DISTRI=fedora VERSION=rawhide FLAVOR=generic_boot ARCH=x86_64 BUILD=22_Beta_RC3 116​ docker exec openqa_webui /var/lib/openqa/script/client isos post ISO=Fedora-Server-netinst-x86_64-22_Beta_RC3.iso DISTRI=fedora VERSION=rawhide FLAVOR=generic_boot ARCH=x86_64 BUILD=22_Beta_RC3
117 117​
▲ Show 20 Lines • Show All 59 Lines • Show Last 20 Lines
View Options

tools/README.createhdds

  • This file was added.
1​# createhdds.py
2​
3​createhdds.py creates and maintains the set of pre-rolled hard disk images needed for some of the Fedora openQA tests.
4​
5​## Usage
6​
7​Most usage information can be seen in the help text - just run `createhdds.py -h` for an overview of the subcommands available, and `createhdds.py (subcommand) -h` for help on a subcommand. To put it simply, the most common usage is simply to run `createhdds.py all -c`. This will create all the currently-expected images that have not already been created, and recreate any that need recreating (images can have a 'maximum age' causing them to be rebuilt by `all` when they're older than that age, and images also have a 'version' - if the image's 'version' is bumped by the maintainers, `all` will rebuild it). It will also remove any image files that are present that aren't expected to be present - usually images for old releases that are no longer tested, or images we've simply stopped using. In a typical deployment of a Fedora openQA instance, the admin should set things up so the `openqa_fedora_tools` git checkout is updated and `createhdds.py all -c` is run regularly - say, once a day (and probably not while tests are being run).
8​
9​`createhdds.py check` will just check whether all expected images are present and up-to-date; if they are, it will exit 0, if they are not, it will exit 1 and print a message. This can be handy for use with things like Ansible (so you can run the check to decide whether you need to run the creation, and thus avoid spurious 'changed' statuses).
10​
11​There are also individual subcommands for each of the named 'image groups', allowing you to create just the image(s) from that group. For image groups which usually generate multiple images, the subcommand will have arguments that let you restrict creation to just a subset of those images (and for virt-builder type images, you can create the image(s) for a different release than would usually be the case, too).
12​
13​In `all` mode, and in single-image mode if you do not pass `--release`, createhdds can decide what releases to build images for, for those image groups that include an installed Fedora release (the virt-builder type images). A virt-builder type image group can specify the releases to build images for absolutely (by giving the release numbers as positive integers), or relative to the next pending release (by giving the release numbers as negative integers). When it encounters one of these 'relative' release numbers, `createhdds` uses [fedfind](https://www.happyassassin.net/fedfind) to discover the 'current' release, and adds 1 to that (to find the 'pending' release). Just in case anything goes wrong with this, or you need to override it for some reason, the `--nextrel` argument is available for relevant subcommands to explicitly specify the 'next release'.
14​
15​## Specifying images / 'image groups': `hdds.json` and `.commands` files
16​
17​All the information on what images can/should be created comes from the `hdds.json` file and some `virt-builder` commands files. You can add, modify and remove image definitions without touching `createhdds.py`. `hdds.json` should define a single dictionary with three keys: `guestfs`, `virtbuilder`, and `renames`. The meat is `guestfs` and `virtbuilder`, which define 'image groups': for each image group, `createhdds all` will create one or more images (multiple images produced from a single group are referred to as 'variants'). Groups and variants are provided for so that if you want to create, say, three images that are identical but for their disk label, you don't have to create a whole new almost-identical entry for each one. The rules about what particular attributes of an image can be implemented as 'variants' are somewhat arbitrary and actually just taken from the old `createhdds.sh`; each function in that implementation became an 'image group' in this rewrite, and the attributes that can vary between variants are the same ones that could be set as function arguments in `createhdds.sh`. The `.commands` files allow for customization of `virt-builder` images; more on this later. `hdds.json` and any `.commands` files must always be in the same folder as `createhdds.py` (not necessarily the same folder the disk images reside in).
18​
19​### renames
20​
21​The value of `renames` is a simple list-of-lists. Each item is a pair of strings. In `all` mode, and optionally in `check` mode, createhdds will read in all the items from the `renames` list. For each item it will look for a file (in the working directory - createhdds always works on the working directory) named the same as the first string, and if it finds such a file, change its name to the second string. So for instance, the value of `renames` could be `[['disk_foo.img', 'disk_bar.img'], ['disk_monkey.img', 'disk_fish.img']]`; that would result in createhdds renaming `disk_foo.img` to `disk_bar.img` and `disk_monkey.img` to `disk_fish.img`. This mechanism is provided to aid in the situation where an image's expected name changes, but the existing image file is still valid; instead of forcing the user to rename it manually or re-generate it, we can list it in `renames` and it will get renamed automatically. Of course, if the image's content changes too, we shouldn't use this mechanism.
22​
23​### guestfs and virtbuilder
24​
25​The value for both `guestfs` and `virtbuilder` is a list of dicts. Each dict defines a single 'image group'; an image group can produce just a single image, or several variants - we will learn what determines the possible variants for an image group below. The images in `guestfs` are produced using libguestfs - these are images that just contain a particular partition layout and perhaps some files we seed directly, but no installed operating system. The images in `virtbuilder` are produced using `virt-builder` - these are images containing an installed operating system. Some keys are common to both image group types:
26​
27​#### `imgver`
28​
29​This is the 'image version'. It is **optional** for both image types. By convention, it should be an integer digit string, but its practical effect is simply to be included in the image filename(s), so it *can* be any string valid in a filename. If omitted or set to the empty string, no imgver component will be included in the filename. This means that by changing the version you can change the expected name - which will cause the `check` mode to report the old file as 'unknown' and the new file as 'missing', and will cause the `all` mode to build the new file. Thus if you're maintaining the image set, and you make some change to an image group which would mean that existing image files for that group can no longer be used, you can change `imgver` to cause the images to be rebuilt.
30​
31​#### `name`
32​
33​The image group's name (a string). **Required** for both image types. This is included in the image file names, of course, and it will also be the subcommand to create image(s) from this group.
34​
35​#### `size`
36​
37​This key is **required** for `guestfs`, **optional** for `virtbuilder`. It is the size of the image. A plain digit string is a size in bytes. A digit string followed by 'M', 'MB' or 'MiB' is a size in megabytes (power-of-2). A digit string followed by 'G', 'GB' or 'GiB' is a size in gigabytes (power-of-2). For `virtbuilder`, if `size` is not set, the image will be whatever size the `virt-builder` base image it's created from is.
38​
39​Some keys are specific to each type of image. These are the `guestfs`-specific keys:
40​
41​#### `parts`
42​
43​This key is **required**. This key's value is a list of dicts. Each dict represents a single partition that should be created on the disk. Required keys are:
44​
45​* `type` - the partition type: 'p' for primary, 'l' for logical, 'e' for extended
46​* `start` - start sector
47​* `end` - end sector
48​
49​These values are just passed straight to libguestfs, so you can find further info on them in the libguestfs documentation, especially on various special values for `start` and `end` (negative values are relative to the end of the disk, for e.g.)
50​
51​Optional keys are:
52​
53​* `filesystem` - if set, this partition will *always* have this filesystem. If not set, the filesystem will be determined according to the image group's `filesystems` value (see below).
54​* `label` - if set, this partition will have this label
55​
56​#### `writes`
57​
58​This key is **optional**. Its value is a list of dicts. Each dict represents a single file that should be created on one of the partitions in the image. There are exactly three required keys for the dict:
59​
60​* `part` - the number of the partition the file should be written to, starting at 1
61​* `path` - the path where the file should be written (root is the root of the partition it's being written to)
62​* `content` - the actual content to write to the file (a string)
63​
64​#### `uploads`
65​
66​This key is **optional**. Its value is a list of dicts. Each dict represents a single file that should be retrieved from a web site and copied to a partition in the image. There are exactly three required keys for the dict:
67​
68​* `part` - the number of the partition the file should be written to, starting at 1
69​* `target` - the path where the file should be written (root is the root of the partition it's being written to)
70​* `source` - the URL of the file to download
71​
72​There's currently no provision for uploading a *local* file, or any protocol besides http/https (you can use either).
73​
74​#### `labels` and `filesystems`
75​
76​These keys are **optional**. Each one's value is a list of strings. These keys together determine how many image variants are expected to be produced from the image group. If not set, the default value of `labels` is ['mbr'], and the default value of `filesystems` is ['ext4'] (both single-item lists). For each `guestfs` image group, the expected images will be the combinations of `labels` and `filesystems`. This means that if you don't set either key, or you set either key to a single item list, only a single image will be expected. If you set `labels` to a two-item list and `filesystems` to a single-item list, two images will be expected. If you set both keys to a two-item list, four images will be expected...and so on.
77​
78​When multiple combinations are in play, the names of the images will include the relevant values. So if an image group has multiple entries in the `labels` list but not the `filesystems` list, the filenames will be `disk_(name)_(label1).img`, `disk_(name)_(label2).img`, and so on. If there are multiple entries in both lists, you'll get `disk_(name)_(filesystem1)_(label1).img`, `disk_(name)_(filesystem1)_(label2).img`, etc etc. The `all` subcommand will always create all the expected images; the image group subcommand will create all the expected images by default, but will have `--label` and `--filesystem` arguments allowing the user to restrict creation to a single item from either or both lists.
79​
80​For `labels`, the values represent disk label types, and are passed to guestfs. The only values used at present are `gpt` and `mbr`. Obviously, `gpt` formats the disk with a GPT disk label, `mbr` formats the disk with an MBR label.
81​
82​For `filesystems`, the values represent...filesystems. Any of the partitions defined in `parts` (see above) which does not specify a `filesystem` will be formatted with this filesystem.
83​
84​Let's consider some examples!
85​
86​Say an image group specifies `name : "blank"`, `labels : ["mbr", "gpt"]` and does not specify `filesystems`. There will be two expected images: `disk_blank_mbr.img` and `disk_blank_gpt.img`. The former will have an MBR disk label, the latter a GPT disk label. In all other respects, the images will be identical. If any partition does not specify a filesystem, it will be formatted as `ext4` (as the default for `filesystems` is `['ext4']`).
87​
88​Say an image group specifies `name : "blank"`, `filesystems : ["ext4", "ntfs"]` and does not specify `labels`. There will be two expected images: `disk_blank_ext4.img` and `disk_blank_ntfs.img`. Both will have an MBR disk label (as the default for `labels` is `['mbr']`). On the former, all partitions which do not specify a `filesystem` will be formatted as ext4; on the latter, all partitions which do not specify a `filesystem` will be formatted as ntfs. In all other respects the images will be identical. As a special note: it is nonsense to specify multiple `filesystems`, but also explicitly specify a `filesystem` for each partition in `parts`. This will result in the creation of multiple identical images, because none of the values from `filesystems` will actually do anything. However, it's perfectly reasonable to have an image group with *some* partitions that explicitly specify a filesystem and *some* that do not, and then have multiple filesystem variants - say, you want multiple variant images with the data partitions formatted using different filesystems, but you want the `/boot` partition in each variant to be ext4.
89​
90​Finally, say an image group specifies `name : "blank"`, `filesystems : ["ext4", "ntfs"]` and `labels : ["mbr", "gpt"]`. There will be *four* expected images, `disk_blank_ext4_mbr.img`, `disk_blank_ntfs_mbr.img`, `disk_blank_ext4_gpt.img`, `disk_blank_ntfs_gpt.img`. The `mbr` images will have MBR disk labels, the `gpt` images will have GPT disk labels, the `ext4` images will have partitions that don't explicitly specify a filesystem formatted as ext4, and the `ntfs` images will have partitions that don't explicitly specify a filesystem formatted as NTFS.
91​
92​Whew! That was a lot of explanation, but it's not really a super-complicated concept, you'll get it easy. OK, let's move on to the `virtbuilder`-specific keys:
93​
94​#### `releases`
95​
96​This key is **required**. It defines the releases and arches for which images are expected; thus it determines the number of images that will be expected for this group. The value is a dict. Each key in the dict represents a release; the value for each key is a list of the arches for which images should be built for that release. The keys should be integer digit strings. **Positive** values indicate absolute release numbers. **Negative** values are relative to whatever is the pending release at the time the images are created. So a release number `-1` means 'the release one before the pending release at the time the images are built'. So if the next Fedora release will be Fedora 24 at the time the images are created, and one of the dict keys is `-1`, an image will be expected for Fedora 23.
97​
98​The filename for a virt-builder type image always includes the release number and arch it's built for - `disk_f(release)_(name)_(arch).img`.
99​
100​Let's look at an example! Say the `name` is `minimal` and the `releases` dict is `{ "-1" : ["i686", "x86_64"], "-2" : ["x86_64"] }`. Three images will be expected, and the expected releases will be relevant to the pending release. Say the pending release is Fedora 24, the expected images will be `disk_f23_minimal_i686.img` (Fedora 23 for i686), `disk_f23_minimal_x86_64.img` (Fedora 23 for x86_64), and `disk_f22_minimal_x86_64.img` (Fedora 22 for x86_64). When time moves on and the next pending release is F25, images will be expected for Fedora 23 and Fedora 24, and the Fedora 22 images will be considered obsolete and deleted by cleanup modes of `createhdds`.
101​
102​As with the `guestfs` case, the single image group subcommand will have parameters to limit creation. So in our example, the `minimal` subcommand will have `--release` and `--arch` parameters, each allowing just a single value. For coding simplicity, passing `--arch` alone is ignored (this may be fixed later) and will just result in the 'expected' images being created. If `--release` is passed, only a single image will be created, for whatever release is specified; by default it will be the x86_64 image, you may pass `--arch (arch)` to build another arch instead.
103​
104​#### `maxage`
105​
106​This key is **optional**. If not set, it defaults to 14. Its value should be an integer string. This basically indicates how often (as a number of days) the image should be rebuilt. virt-builder images can go 'stale' - at build time we update the installed OS to the latest packages, but of course by the time the image is used, later updates may be available. If the test the image is used for needs all the latest packages to be installed, the test will have to install the later updates, and it's inefficient to have one or more tests doing that every day. So it makes sense to re-generate the images periodically so that the tests only have to install few if any updates. For any image group with a non-0 maxage, createhdds `check` and `any` modes will check any existing image file's age against maxage; if it exceeds the maxage `check` will consider it 'outdated', and `all` will rebuild it. To disable maxage checks for an image group, set `maxage` to 0.
107​
108​### `commands` files
109​
110​Customization of virt-builder type images, beyond the Fedora release/arch combination, is done with `.commands` files. virt-builder allows you to pass in any set of customization commands you like in a text file. It seemed easier to simply let maintainers create a `.commands` file for each virt-builder image group than to come up with a syntax for specifying customizations in the JSON. The logic is simple: for each virt-builder image group, if there is a file named `(name).commands`, that file will be passed to virt-builder as the customization command file. For instance, the `desktop.commands` file contains the customization commands for the 'desktop' virt-builder image group; it installs the Workstation package group, creates a regular user, and does a few other things. The virt-builder documentation explains all the customization commands. The `.commands` file is technically optional, but in most cases you will want to include at least the commands from `minimal.commands` - to set a root password, update packages, and schedule an SELinux relabel (as files touched by other customization commands will have incorrect labels).
View Options

tools/createhdds.py

  • This file was added.
PropertyOld ValueNew Value
File Modenull100755
1​#!/usr/bin/env python3
2​
3​# Copyright (C) 2015 Red Hat
4​#
5​# createhdds is free software; you can redistribute it and/or modify
6​# it under the terms of the GNU General Public License as published by
7​# the Free Software Foundation, either version 3 of the License, or
8​# (at your option) any later version.
9​#
10​# This program is distributed in the hope that it will be useful,
11​# but WITHOUT ANY WARRANTY; without even the implied warranty of
12​# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
13​# GNU General Public License for more details.
14​#
15​# You should have received a copy of the GNU General Public License
16​# along with this program. If not, see <http://www.gnu.org/licenses/>.
17​#
18​# Author: Adam Williamson <awilliam@redhat.com>
19​
20​"""Tool for creating hard disk images for Fedora openQA."""
21​
22​import argparse
23​import logging
24​import json
25​import os
26​try:
27​ import subprocess32 as subprocess
28​except ImportError:
29​ import subprocess
30​import sys
31​import time
32​import tempfile
33​
34​import fedfind.helpers
35​import guestfs
36​import pexpect
37​
38​from six.moves.urllib.request import urlopen
39​
40​# this is a bit icky, but it means you can run the script from
41​# anywhere - we use this to locate hdds.json and the virtbuilder
42​# image command files, so they must always be in the same place
43​# as the script itself. images are checked/created in the working
44​# directory.
45​SCRIPTDIR = os.path.abspath(os.path.dirname(sys.argv[0]))
46​logger = logging.getLogger('createhdds')
47​
48​def handle_size(size):
49​ """Simple function to handle sizes like '10G' or '100MB', returns
50​ the size in bytes as an int. Used by both image classes.
51​ """
52​ size = str(size)
53​ if size.endswith('G') or size.endswith('GB') or size.endswith('GiB'):
54​ return int(size.split('G')[0]) * 1024 * 1024 * 1024
55​ elif size.endswith('M') or size.endswith('MB') or size.endswith('MiB'):
56​ return int(size.split('M')[0]) * 1024 * 1024
57​ else:
58​ return int(size)
59​
60​class GuestfsImage(object):
61​ """Class representing an image created by guestfs. 'size' is the
62​ desired image size, valid formats are a digit string (size in
63​ bytes, digit string plus 'M', 'MB' or 'MiB' (size in power of two
64​ megabytes), or digit string plus 'G', 'GB' or 'GiB' (size in power
65​ of two gigabytes). 'imgver' is the image 'version' - in practice
66​ it's simply a string that gets included in the image file name
67​ if specified. 'filesystem' is the default filesystem for the image
68​ - it will be used for parts that don't explicitly specify a
69​ filesystem. 'label' is the disk label format to be used. parts,
70​ writes, and uploads are lists of dicts that specify the partitions
71​ that should be created and files that should be written or copied
72​ ('uploaded') to them. These are read in from hdds.json by
73​ get_guestfs_images and passed here. 'name_extras' is an iterable
74​ of strings which should be appended to the image file name, with _
75​ separators (this provides a mechanism for get_guestfs_images to
76​ include the label and/or filesystem in the image name when
77​ creating images from a group with variants).
78​ """
79​ def __init__(self, name, size, imgver='', filesystem='ext4', label='mbr', parts=None,
80​ writes=None, uploads=None, name_extras=None):
81​ self.name = name
82​ self.size = handle_size(size)
83​ # guestfs images are never outdated
84​ self.outdated = False
85​ self.filesystem = filesystem
86​ self.label = label
87​ self.parts = []
88​ self.writes = []
89​ self.uploads = []
90​ if parts:
91​ self.parts = parts
92​ if writes:
93​ self.writes = writes
94​ if uploads:
95​ self.uploads = uploads
96​
97​ self.filename = "disk_{0}".format(name)
98​ if imgver:
99​ self.filename = "{0}_{1}".format(self.filename, imgver)
jskladanUnsubmitted
Not Done

This does not really make much sense. First of all, why not store the 1 in the first place? Next up, the content of the imgver does not really need to be integer at all. The way your code works now, the images get rebuilt if imgver changes to any kind of new value. For example if I had a json file with imgver set to 20, and "updated" the imgver to 3, the code would still say that the said image needs to be rebuilt, as there is no file with _v3_ in the directory.

So it would IMHO only make sense to remove the int() check, and just plain store whatever is imgver set to, into the filename.
Change the documentation to reflect the fact, that the code does not at all care about it being an integer, or an ascending number, and just say something in the lines of "When imgver changes, the image gets rebuilt".

Ditto for the VirtBuilderImage.

adamwillAuthorUnsubmitted
Not Done

Why not store the 1 in the first place - so *currently existing* images aren't considered invalid, i.e. we can switch over to this new script without configuring renames for every existing image or renaming them manually. We can do that, though, if you'd prefer.

Integer comparison stuff - yeah, you're right, we can ditch that now.

100​ if name_extras:
101​ for item in name_extras:
102​ self.filename = "{0}_{1}".format(self.filename, item)
103​ self.filename = "{0}.img".format(self.filename)
104​
jskladanUnsubmitted
Not Done

see comment on get_guestfs_images

105​ def create(self):
106​ """Create the image."""
107​ gfs = guestfs.GuestFS(python_return_dict=True)
108​ try:
109​ # Create the disk image with a temporary name
110​ tmpfile = "{0}.tmp".format(self.filename)
111​ gfs.disk_create(tmpfile, "raw", int(self.size))
112​ # 'launch' guestfs with the disk attached
113​ gfs.add_drive_opts(tmpfile, format="raw", readonly=0)
114​ gfs.launch()
115​ # identify the disk and create a disk label
116​ disk = gfs.list_devices()[0]
117​ gfs.part_init(disk, self.label)
118​ # create and format the partitions
119​ for part in self.parts:
120​ # each partition can specify a filesystem, if it doesn't,
121​ # we use the image default
122​ if 'filesystem' not in part:
123​ part['filesystem'] = self.filesystem
124​ # create the partition: the dict must specify type ('p'
125​ # for primary, 'l' for logical, 'e' for extended), and
126​ # start and end sector numbers - more details in
127​ # guestfs docs
128​ gfs.part_add(disk, part['type'], int(part['start']), int(part['end']))
129​ # identify the partition and format it
130​ partn = gfs.list_partitions()[-1]
131​ gfs.mkfs(part['filesystem'], partn, label=part.get('label'))
132​ # do file 'writes' (create a file with a given string as
133​ # its content)
134​ for write in self.writes:
135​ # the write dict must specify the partition to be
136​ # written to, numbered from 1 as humans usually do;
137​ # find that part and mount it
138​ partn = gfs.list_partitions()[int(write['part'])-1]
139​ gfs.mount(partn, "/")
140​ # do the write: the dict must specify the target path
141​ # and the string to be written ('content')
142​ gfs.write(write['path'], write['content'])
143​ # do file 'uploads'. in guestfs-speak that means transfer
144​ # a file from the host to the image, we use it to mean
145​ # download a file from an http server and transfer that
146​ # to the image
147​ for upload in self.uploads:
148​ # download the file to a temp file - borrowed from
149​ # fedora_openqa_schedule (which stole it from SO)
150​ with tempfile.NamedTemporaryFile(prefix="createhdds") as dltmpfile:
151​ resp = urlopen(upload['source'])
152​ while True:
153​ # This is the number of bytes to read between buffer
154​ # flushes. Value taken from the SO example.
155​ buff = resp.read(8192)
156​ if not buff:
157​ break
158​ dltmpfile.write(buff)
159​ # as with write, the dict must specify a target
160​ # partition and location ('target')
161​ partn = gfs.list_partitions()[int(upload['part'])-1]
162​ gfs.mount(partn, "/")
163​ gfs.upload(dltmpfile.name, upload['target'])
164​ # we're all done! rename to the correct name
165​ os.rename(tmpfile, self.filename)
166​ except:
167​ # if anything went wrong, we want to wipe the temp file
168​ # then raise
169​ os.remove(tmpfile)
170​ raise
171​ finally:
172​ # whether things go right or wrong, we want to close the
173​ # gfs instance
174​ gfs.close()
175​
176​
177​class VirtBuilderImage(object):
178​ """Class representing an image created by virt-builder. 'release'
179​ is the release the image will be built for. 'arch' is the arch.
180​ 'size' is the desired image size, valid formats are a digit string
181​ (size in bytes, digit string plus 'M', 'MB' or 'MiB' (size in
182​ power of two megabytes), or digit string plus 'G', 'GB' or 'GiB'
183​ (size in power of two gigabytes). 0 (or any false-y value) means
184​ the image will be the size of the upstream base image. 'imgver' is
185​ the image 'version' - in practice it's simply a string that gets
186​ included in the image file name if specified. 'maxage' is the
187​ maximum age of the image file (in days) - if the image is older
188​ than this, 'check' will report it as 'outdated' and 'all' will
189​ rebuild it.
190​ """
191​ def __init__(self, name, release, arch, size=0, imgver='', maxage=14):
192​ self.name = name
193​ self.size = handle_size(size)
194​ self.filename = "disk_f{0}_{1}".format(str(release), name)
195​ if imgver:
196​ self.filename = "{0}_{1}".format(self.filename, imgver)
jskladanUnsubmitted
Not Done

See comment on GuestfsImage.

197​ self.filename = "{0}_{1}.img".format(self.filename, arch)
198​ self.release = release
199​ self.arch = arch
200​ self.maxage = maxage
201​
202​ def create(self):
203​ """Create the image."""
204​ # Basic creation command with standard params
205​ args = ["virt-builder", "fedora-{0}".format(str(self.release)), "-o", self.filename,
206​ "--arch", self.arch]
207​ if self.size:
208​ args.extend(["--size", "{0}b".format(str(int(self.size)))])
209​ # We use guestfs's ability to read customization commands from
210​ # a file. The convention is to have a file 'name.commands' in
211​ # SCRIPTDIR; if this file exists, we pass it to virt-builder.
212​ if os.path.isfile("{0}/{1}.commands".format(SCRIPTDIR, self.name)):
213​ args.extend(["--commands-from-file", "{0}/{1}.commands".format(SCRIPTDIR, self.name)])
214​ ret = subprocess.call(args)
215​ if ret > 0:
216​ sys.exit("virt-builder command {0} failed!".format(' '.join(args)))
217​ # We have to boot the disk to make SELinux relabelling happen;
218​ # virt-builder can't do it unless the policy version on the host
219​ # is the same as the guest(?) There are lots of bad ways to do
220​ # this, expect is the one we're using.
221​ child = pexpect.spawnu("qemu-kvm -m 2G -nographic {0}".format(self.filename), timeout=None)
222​ child.expect(u"localhost login:")
223​ child.sendline(u"root")
224​ child.expect(u"Password:")
225​ child.sendline(u"weakpassword")
226​ child.expect(u"~]#")
227​ child.sendline(u"poweroff")
228​ child.expect(u"reboot: Power down")
229​ child.close()
230​
231​ @property
232​ def outdated(self):
233​ """Whether the image is outdated - if self.maxage is set and
234​ it's older than that.
235​ """
236​ if not os.path.isfile(self.filename):
237​ return False
238​
239​ if self.maxage:
240​ age = int(time.time()) - int(os.path.getmtime(self.filename))
241​ # maxage is in days
242​ if age > int(self.maxage) * 24 * 60 * 60:
243​ return True
244​
245​ return False
246​
247​
248​def get_guestfs_images(imggrp, labels=None, filesystems=None):
249​ """Passed a single 'image group' dict (usually read out of hdds.
250​ json), returns a list of GuestfsImage instances. labels and
251​ filesystems act as overrides to the values specified in hdds.json
252​ and the defaults in this function; multiple images will be created
253​ for whatever combinations of labels and filesystems are specified.
254​ If they're not passed, then we use the values from the dict; if
garretrazielUnsubmitted
Not Done

It's not necessary ATM, but could you make root password also configurable?

255​ the dict doesn't specify, we just create a single image with the
adamwillAuthorUnsubmitted
Not Done

could do, sure.

256​ label set to 'mbr' and the filesystem to 'ext4'. The filesystem
257​ setting is itself a default - it's only used for 'part' entries
258​ which don't specify a filesystem (see the GuestfsImage docs).
259​ """
260​ imgs = []
261​ # Read in the various dict values
262​ name = imggrp['name']
263​ size = imggrp['size']
264​ parts = imggrp['parts']
265​ # These are optional
266​ writes = imggrp.get('writes')
267​ uploads = imggrp.get('uploads')
268​ imgver = imggrp.get('imgver')
269​
270​ # Here we implement the 'labels / filesystems' behaviour explained
271​ # in the docstring
272​ if not labels:
273​ labels = imggrp.get('labels', ['mbr'])
274​ if not filesystems:
275​ filesystems = imggrp.get('filesystems', ['ext4'])
276​
277​ # Now we've sorted out all our settings, instantiate the images
278​ for label in labels:
garretrazielUnsubmitted
Not Done

I was hoping that expect is only temporary solution, but oh well, I guess that there is still no better way how to do this.

279​ for filesystem in filesystems:
adamwillAuthorUnsubmitted
Not Done

yeah, I couldn't think of any. I tried without it and found the same problem you did - virt-builder can't do the selinux relabel itself and just dumps a .autorelabel to make it happen on first boot. rwmjones pointed me to https://bugzilla.redhat.com/show_bug.cgi?id=1049656 , which is in the general neighbourhood but not exactly what I saw when I ran it with -x -v - when I did that I saw it complaining about the policy version (which is why I wrote that in my comment).

Given the requirement 'boot the image, make sure the relabel finished, and shut it down cleanly' I couldn't think of any better option than expect :/ I did at least move all other stuff out of expect so this is the only thing we're using it for now. if anyone has any ideas I'm happy to try...

280​ # We want to indicate filesystem/label in the filename
281​ # when we're creating images with more than one. We check
282​ # both the passed 'filesystems' value and the imggrp
283​ # value, so if the caller overrode the imggrp value we
284​ # still get the the long name (happens when using the
285​ # single-image CLI subcommand and restricting the label/
286​ # filesystem)
287​ name_extras = []
288​ if len(imggrp.get('filesystems', [])) > 1 or len(filesystems) > 1:
289​ name_extras.append(filesystem)
290​ if len(imggrp.get('labels', [])) > 1 or len(labels) > 1:
291​ name_extras.append(label)
292​ img = GuestfsImage(
293​ name, size, imgver, filesystem, label, parts, writes, uploads, name_extras)
jskladanUnsubmitted
Not Done

This could really be replaced with:

name_extras = []
if len(imggrp.get('filesystems', [])) > 1 or len(filesystems) > 1:
    name_extras.append(filesystem)
if len(imggrp.get('labels', [])) > 1 or len(labels) > 1:
    name_extras.append(label)
img = GuestfsImage(​name, imgver, size, filesystem, label, parts, writes, uploads, name_extras)

and then in GuestfsImage one could easily replace the:

if name_extras:
    for item in name_extras:
        getter = attrgetter(item)
        self.filename = "{0}_{1}".format(self.filename, getter(self))

with much simpler:

if name_extras:
    for item in name_extras:
        self.filename = "{0}_{1}".format(self.filename, item)

and get rid of the (very hard to read, and unnecessary) attrgetter "magic".

I get why it was written this way, but since the default values of filesystem and label are never really used, as this is the only place where the GuestfsImage is instantiated, and the filesystem and label parameters are passed to the constructor.

adamwillAuthorUnsubmitted
Not Done

yeah, in fact I actually meant to change that already but got sidetracked into something else (I tried ditching the classes entirely, but that didn't work out well and I gave up on it) and forgot it...

294​ imgs.append(img)
295​ return imgs
296​
297​def get_virtbuilder_images(imggrp, nextrel=None, releases=None):
298​ """Passed a single 'image group' dict (usually read out of hdds.
299​ json), returns a list of VirtBuilderImage instances. 'nextrel'
300​ indicates the 'next' release of Fedora: sometimes we determine the
301​ release to build image(s) for in relation to this, so we need to
302​ know it. If it's not specified, we ask fedfind to figure it out
303​ for us (this is the usual case, we just allow specifying it
304​ in case there's an issue with fedfind or you want to test image
305​ creation for the next-next release ahead of time or something).
306​ The image group dict must include a dict named 'releases' which
307​ indicates what releases to build images for and what arches to
308​ build for each release; 'releases' can be used to override that. If
309​ set, the image group 'releases' dict is ignored, and this dict is
310​ used instead. The dict's keys must be release numbers or negative
311​ integers: -1 means 'one release lower than the "next" release',
312​ -2 means 'two releases lower than the "next" release', and so on.
313​ The values are the arches to build for that release.
314​ """
315​ imgs = []
316​ # Set this here so if we need to calculate it, we only do it once
317​ if not nextrel:
318​ nextrel = 0
319​ name = imggrp['name']
320​ # this is the second place we set a default for maxage - bit ugly
321​ maxage = int(imggrp.get('maxage', 14))
322​ if not releases:
323​ releases = imggrp['releases']
324​ size = imggrp.get('size', 0)
325​ imgver = imggrp.get('imgver')
326​ # add an image for each release/arch combination
327​ for (relnum, arches) in releases.items():
328​ if int(relnum) < 0:
329​ # negative relnum indicates 'relative to next release'
330​ if not nextrel:
331​ nextrel = fedfind.helpers.get_current_release() + 1
332​ relnum = int(nextrel) + int(relnum)
333​ for arch in arches:
334​ imgs.append(
335​ VirtBuilderImage(name, relnum, arch, size=size, imgver=imgver, maxage=maxage))
336​ return imgs
337​
338​def get_all_images(hdds, nextrel=None):
339​ """Simply iterates over the 'image group' dicts in hdds.json and
340​ calls the appropriate get_foo_images() function for each, then
341​ returns the list of all images. No overrides for labels,
342​ filesystems, releases or arches are provided here, this function
343​ is just for painting inside the lines - it's used to determine
344​ what images are 'expected' to exist according to hdds.json. We
345​ do allow passing of 'nextrel' just in case there's some issue
346​ with the auto-discovery.
347​ """
348​ imgs = []
349​ for imggrp in hdds['guestfs']:
350​ imgs.extend(get_guestfs_images(imggrp))
351​
352​ for imggrp in hdds['virtbuilder']:
353​ imgs.extend(get_virtbuilder_images(imggrp, nextrel=nextrel))
354​ return imgs
355​
356​def do_renames(hdds):
357​ """Rename files according to the 'renames' list in hdds.json,
358​ which is just a list of 'old name, new name' pairs. Say there's
359​ an image we used to only create an mbr version of, but now we add
360​ a gpt version; that means the expected name of the mbr version
361​ changes. This function allows us to specify in hdds.json that the
362​ mbr image should be renamed, which saves having to rebuild it.
363​ 'all' always does renames, 'check' can be told to do renames.
364​ """
365​ for (orig, new) in hdds['renames']:
366​ if os.path.isfile(orig) and not os.path.exists(new):
367​ logger.info("Renaming %s to %s...", orig, new)
368​ os.rename(orig, new)
369​
370​def delete_all():
371​ """Remove absolutely all createhdds-controlled files; we assume
372​ anything in the working directory starting with 'disk' and ending
373​ with 'img' is one of our files.
374​ """
375​ files = [fl for fl in os.listdir('.') if fl.startswith('disk') and fl.endswith('img')]
376​ for _file in files:
377​ os.remove(_file)
378​
379​def clean(unknown):
380​ """This simply removes all the files in the list. The list is
381​ expected to be the list of 'unknown' files returned by check();
382​ both 'all' and 'clean' can delete 'unknown' files if requested.
383​ 'unknown' files are usually images for old releases we're no
384​ longer testing, images we've simply stopped using, or old image
385​ files when the image group name has been changed to indicate an
386​ incompatible change to the image(s).
387​ """
388​ for filename in unknown:
389​ try:
390​ logger.info("Removing unknown image %s", filename)
391​ os.remove(filename)
392​ except OSError:
393​ # We don't really care if the file didn't exist for some
394​ # reason, so just pass.
395​ pass
396​
397​def check(hdds, nextrel=None):
398​ """This calls get_all_images() to find out what images are expected
399​ to exist, then compares that to the images that are actually
400​ present and returns three lists. The first two are lists of Image
401​ subclass instances, the last is a list of filenames. The first is a
402​ list of the images that just aren't there at all. The second is a
403​ list of images that are present but 'outdated', because they've
404​ exceeded their maxage. The last is a list of image files that are
405​ present but don't match any of the expected images - 'unknown'
406​ images. The 'clean()' function can be used to remove these.
407​ 'nextrel' is passed through to get_all_images(), and is available
408​ in case auto-detection via fedfind fails.
409​ """
410​ current = []
411​ missing = []
412​ outdated = []
413​ unknown = []
414​ # Get the list of all 'expected' images
415​ expected = get_all_images(hdds, nextrel=nextrel)
416​ # Get the list of all present image files
417​ files = set(fl for fl in os.listdir('.') if fl.startswith('disk') and fl.endswith('img'))
418​ # Compare present images vs. expected images to produce 'unknown'
419​ expnames = set(img.filename for img in expected)
420​ unknown = list(files.difference(expnames))
421​
422​ # Now determine if images are absent or outdated
423​ for img in expected:
424​ if not os.path.isfile(img.filename):
425​ missing.append(img)
426​ continue
427​ if img.outdated:
428​ outdated.append(img)
429​ continue
430​ current.append(img)
431​
432​ logger.debug("Current images: %s", ', '.join([img.filename for img in current]))
433​ logger.debug("Missing images: %s", ', '.join([img.filename for img in missing]))
434​ logger.debug("Outdated images: %s", ', '.join([img.filename for img in outdated]))
435​ logger.debug("Unknown images: %s", ', '.join(unknown))
436​ return (missing, outdated, unknown)
437​
438​def cli_all(args, hdds):
439​ """Function for the CLI 'all' subcommand. Creates all images. If
440​ args.delete is set, blows all existing images away and recreates
441​ them; otherwise it will just fill in missing or outdated images.
442​ If args.clean is set, also wipes 'unknown' images.
443​ """
444​ if args.delete:
445​ logger.info("Removing all images...")
446​ delete_all()
447​
448​ # handle renamed images (see do_renames docstring)
449​ do_renames(hdds)
450​
451​ # call check() to find out what we need to do
452​ (missing, outdated, unknown) = check(hdds, nextrel=args.nextrel)
453​
454​ # wipe 'unknown' images if requested
455​ if args.clean:
456​ clean(unknown)
457​
458​ # 'missing' plus 'outdated' is all the images we need to build; if
459​ # args.delete was set, all images will be in this list
460​ missing.extend(outdated)
461​ for (num, img) in enumerate(missing, 1):
462​ logger.info("Creating image %s...[%s/%s]", img.filename, str(num), str(len(missing)))
463​ img.create()
464​
465​def cli_check(args, hdds):
466​ """Function for the CLI 'check' subcommand. Basically just calls
467​ check() and prints the results. Does renames before checking if
468​ args.rename was set, and wipes 'unknown' images after checking if
469​ args.clean was set. Exits with status 1 if any missing and/or
470​ outdated files are found (handy for scripting/automation).
471​ """
472​ if args.rename:
473​ do_renames(hdds)
474​
475​ (missing, outdated, unknown) = check(hdds, nextrel=args.nextrel)
476​ if missing:
477​ print("Missing images: {0}".format(', '.join([img.filename for img in missing])))
478​ if outdated:
479​ print("Outdated images: {0}".format(', '.join([img.filename for img in outdated])))
480​ if unknown:
481​ print("Unknown images: {0}".format(', '.join(unknown)))
482​ if args.clean:
483​ clean(unknown)
484​
485​ if missing or outdated:
486​ sys.exit("Missing and/or outdated images found!")
487​ else:
488​ sys.exit()
489​
jskladanUnsubmitted
Not Done

Why return current, when it's never really used in any of the func-calls?

490​def cli_image(args, *_):
adamwillAuthorUnsubmitted
Not Done

because I wrote check() quite early - before I had the actual CLI code in place - and was just printing everything it returned to make sure it was sane :) i.e., no particularly great reason. I can change it.

491​ """Function for CLI image group subcommands (a subcommand is added
492​ for each image group in hdds.json). Will create the image(s) from
493​ the specified group. For guestfs image groups with multiple labels
494​ and/or filesystems, the user can pass args.label and/or args.
495​ filesystem to limit creation to a single label and/or filesystem.
496​ Note this function does no checking; the image will always be
497​ recreated, even if it already exists and is current.
498​ """
499​ # Note that on this path, the parsing of hdds is done by
500​ # parse_args(). It passes us the image type and the image group
501​ # dict from hdds as a tuple; we need to know the type so we know
502​ # what fiddling to do with the args and what function to call
503​ imggrp = args.imggrp[1]
504​ imgtype = args.imggrp[0]
505​
506​ if imgtype == 'guestfs':
507​ # If the user passed in label or filesystem, we pass them on
508​ # to get_guestfs_images as single-item lists, otherwise we
509​ # just pass 'None', causing it to use the values from imggrp
510​ # (which come from hdds.json)
511​ labels = None
512​ filesystems = None
513​ if args.label:
514​ labels = [args.label]
515​ if args.filesystem:
516​ filesystems = [args.filesystem]
517​ imgs = get_guestfs_images(imggrp, labels=labels, filesystems=filesystems)
518​
519​ elif imgtype == 'virtbuilder':
520​ # If the user passed args.release, we construct a releases
521​ # dict to pass to get_virtbuilder_images to override the dict
522​ # from imggrp. If they passed args.arch, we use that arch,
523​ # otherwise we default to x86_64. If args.release isn't set,
524​ # we just pass None as release, and the releases dict from
525​ # imggrp will be used, and images created for all release/
526​ # arch combinations listed there. FIXME: if the user passes
527​ # args.arch but not args.release, we just ignore it...
528​ releases = None
529​ if args.release:
530​ if args.arch:
531​ arches = [args.arch]
532​ else:
533​ arches = ['x86_64']
534​ releases = {args.release: arches}
535​ imgs = get_virtbuilder_images(imggrp, releases=releases)
536​
537​ for (num, img) in enumerate(imgs, 1):
538​ logger.info("Creating image %s...[%s/%s]", img.filename, str(num), str(len(imgs)))
539​ img.create()
540​
541​def parse_args(hdds):
542​ """Parse arguments with argparse."""
543​ parser = argparse.ArgumentParser(description=(
jskladanUnsubmitted
Not Done

hdds is not used at all in the method, but the comments mention the hdds.json on multiple occasions - this is my first pass of the code, so take it with a grain of salt, but it sometimes does not make sense to me. For example:

If the user passed in label or filesystem, we pass them on
to get_guestfs_images as single-item lists, otherwise we
​just pass 'None', causing it to use the values from hdds.json

How is it overriden by hdds.json, when the hdds value (the parsed hdds.json file) is not used at all?

544​ "Tool for creating hard disk images for Fedora openQA."))
adamwillAuthorUnsubmitted
Not Done

so what happens here is that we actually pulled a dict out of hdds *in parse_args()*, and stuffed it into args; note where this function reads args.imggrp. I originally had it set up so parse_args() would just send the image 'type' and the name of the image group, and this function would find the appropriate group in hdds itself, then thought 'wait, that's silly, since parse_args() already got the dict it can just return the whole thing'. This is part of the 'clever clever' trick to set up a named subcommand parser for each individual image group. It may be that there's a cleaner/more obvious way to set this up, I'll think about it a bit, and comment it if not.

545​ parser.add_argument(
546​ '-l', '--loglevel', help="The level of log messages to show",
547​ choices=('debug', 'info', 'warning', 'error', 'critical'),
548​ default='info')
549​
550​ # This is a workaround for a somewhat infamous argparse bug
551​ # in Python 3. See:
552​ # https://stackoverflow.com/questions/23349349/argparse-with-required-subparser
553​ # http://bugs.python.org/issue16308
554​ subparsers = parser.add_subparsers(dest='subcommand')
555​ subparsers.required = True
556​
557​ parser_all = subparsers.add_parser(
558​ 'all', description="Ensure all images are present and up-to-date.")
559​ parser_all.add_argument(
560​ '-d', '--delete', help="Delete and re-build all images",
561​ action='store_true')
562​ parser_all.add_argument(
563​ '-c', '--clean', help="Remove unknown (usually old) images",
564​ action='store_true')
565​ parser_all.add_argument(
566​ '-n', '--nextrel', help="The release to treat as the 'next' release "
567​ "- this determines what releases some images will be built for. If "
568​ "not set or set to 0, createhdds will try to discover it when needed",
569​ type=int, default=0)
570​ parser_all.set_defaults(func=cli_all)
571​
572​ parser_check = subparsers.add_parser(
573​ 'check', description="Check status of existing image files.")
574​ parser_check.add_argument(
575​ '-n', '--nextrel', help="The release to treat as the 'next' release "
576​ "- this determines what releases some images are expected to exist for. If "
577​ "not set or set to 0, createhdds will try to discover it when needed",
578​ type=int, default=0)
579​ parser_check.add_argument(
580​ '-r', '--rename', help="Whether to rename images when the expected name "
581​ "has changed or not", action="store_true")
582​ parser_check.add_argument(
583​ '-c', '--clean', help="Remove unknown (usually old) images",
584​ action='store_true')
585​ parser_check.set_defaults(func=cli_check)
586​
587​ # This here is somewhat clever-clever: we generate a subcommand for
588​ # each image group listed in hdds.json, which can be used to build
589​ # image(s) from that group. For guestfs image groups, we also check
590​ # if the group has multiple labels and/or filesystems by default,
591​ # and add arguments to limit image creation to just a single label
592​ # and/or filesystem.
593​ for imggrp in hdds['guestfs']:
594​ imgparser = subparsers.add_parser(
595​ imggrp['name'], description="Create {0} image(s)".format(imggrp['name']))
596​ labels = imggrp.get('labels', [])
597​ if len(labels) > 1:
598​ imgparser.add_argument(
599​ '-l', '--label', help="Create only images with this disk label",
600​ choices=labels)
601​ filesystems = imggrp.get('filesystems', [])
602​ if len(filesystems) > 1:
603​ imgparser.add_argument(
604​ '-f', '--filesystem', help="Create only images with this filesystem",
605​ choices=filesystems)
606​ imgparser.set_defaults(func=cli_image, label=None, filesystem=None)
607​ # Here we're stuffing the type of the image and the dict from
608​ # hdds into args for cli_image() to use.
609​ imgparser.set_defaults(imggrp=('guestfs', imggrp))
610​
611​ # For libvirt images, we provide args to override the release/arch
612​ # combination; using args.release will always result in just a
613​ # single image being built, for x86_64 unless args.arch is set to
614​ # i686.
615​ for imggrp in hdds['virtbuilder']:
616​ imgparser = subparsers.add_parser(
617​ imggrp['name'], description="Create {0} image(s)".format(imggrp['name']))
618​ imgparser.add_argument(
619​ '-r', '--release', help="The release to build the image(s) for. If not "
620​ "set or set to 0, createhdds will attempt to determine the current "
621​ "release and build for appropriate releases relative to that",
622​ type=int, default=0)
623​ imgparser.add_argument(
624​ '-a', '--arch', help="The arch to build the image(s) for. If neither "
625​ "this nor --release is set, createhdds will decide the appropriate "
626​ "arch(es) to build for each release. If this is not set but --release "
627​ "is set, only x86_64 image(s) will be built.",
628​ choices=('x86_64', 'i686'))
629​ imgparser.set_defaults(func=cli_image)
630​ # Here we're stuffing the type of the image and the dict from
631​ # hdds into args for cli_image() to use.
632​ imgparser.set_defaults(imggrp=('virtbuilder', imggrp))
633​ return parser.parse_args()
634​
635​def main():
636​ """Main loop - set up logging, parse args, run subcommand
637​ function.
638​ """
639​ try:
640​ with open('{0}/hdds.json'.format(SCRIPTDIR), 'r') as fout:
641​ hdds = json.load(fout)
642​ args = parse_args(hdds)
643​ loglevel = getattr(
644​ logging, args.loglevel.upper(), logging.INFO)
645​ logging.basicConfig(level=loglevel)
646​ args.func(args, hdds)
647​ except KeyboardInterrupt:
648​ sys.stderr.write("Interrupted, exiting...\n")
649​ # there may be a guestfs temp image file lying around...
650​ tmps = [fl for fl in os.listdir('.') if fl.startswith('disk') and fl.endswith('.tmp')]
651​ for tmp in tmps:
652​ os.remove(tmp)
653​ sys.exit(1)
654​
655​if __name__ == '__main__':
656​ main()
jskladanUnsubmitted
Not Done

This could IMHO be embedded in the main() directly, the run() is (AFAIK) not called from anywhere else, anyway...

adamwillAuthorUnsubmitted
Not Done

first comment - yeah, probably, I just boilerplate this stuff around from script to script (it's all there in relval and fedfind too). second comment - sure, it's a tiny thing but it'd give me more pylint points, I guess I'll do it :)

jskladanUnsubmitted
Not Done

Since hdds is not used at all in cli_image(), how about changing the order of the arguments (args.func(args, hdds))? This would then allow the cli_image() to have def cli_image(args, *_) signature.
Absolutely not necessary, just a food for thought.

View Options

tools/createhdds.sh

  • This file was deleted.
PropertyOld ValueNew Value
File Mode100755null
This file was completely deleted. Show File Contents
View Options

tools/desktop.commands

  • This file was added.
1​root-password password:weakpassword
2​update
3​selinux-relabel
4​install @workstation-product-environment
5​link /usr/lib/systemd/system/graphical.target:/etc/systemd/system/default.target
6​firstboot-command useradd -m -p '' ejohn
7​firstboot-command echo 'ejohn:weakpassword' | chpasswd
View Options

tools/hdds.json

  • This file was added.
1​{
2​ "guestfs" : [
3​ {
4​ "name" : "full",
5​ "size" : "10G",
jskladanUnsubmitted
Not Done

Why imgver instead of imgver?

6​ "labels" : ["mbr", "gpt"],
adamwillAuthorUnsubmitted
Not Done

Because I did a bad sed at midnight after a few drinks :) I fixed it up in createhdds.py but forgot to fix the json too until this morning.

7​ "parts" : [
8​ {
9​ "filesystem" : "ext4",
10​ "type" : "p",
11​ "start" : "4096",
12​ "end" : "10485760"
13​ },
14​ {
15​ "filesystem" : "ext4",
16​ "type" : "p",
17​ "start" : "10485761",
18​ "end" : "-4097"
19​ }
20​ ],
21​ "writes" : [
22​ {
23​ "part" : "1",
24​ "path" : "/testfile",
25​ "content" : "Hello, world!"
26​ },
27​ {
28​ "part" : "2",
29​ "path" : "/testfile",
30​ "content" : "Oh, hi Mark"
31​ }
32​ ]
33​ },
34​ {
35​ "name" : "freespace",
36​ "size" : "10G",
37​ "labels" : ["mbr", "gpt"],
38​ "parts" : [
39​ {
40​ "filesystem" : "ext4",
41​ "type" : "p",
42​ "start" : "4096",
43​ "end" : "2097152"
44​ }
45​ ],
46​ "writes" : [
47​ {
48​ "part" : "1",
49​ "path" : "/testfile",
50​ "content" : "Hello, world!"
51​ }
52​ ]
53​ },
54​ {
55​ "name" : "ks",
56​ "size" : "100M",
57​ "parts" : [
58​ {
59​ "filesystem" : "ext4",
60​ "type" : "p",
61​ "start" : "4096",
62​ "end" : "-1"
63​ }
64​ ],
65​ "uploads" : [
66​ {
67​ "part" : "1",
68​ "target" : "/root-user-crypted-net.ks",
69​ "source" : "https://jskladan.fedorapeople.org/kickstarts/root-user-crypted-net.ks"
70​ }
71​ ]
72​ },
73​ {
74​ "name" : "updates_img",
75​ "size" : "100M",
76​ "parts" : [
77​ {
78​ "filesystem" : "ext4",
79​ "label" : "UPDATES_IMG",
80​ "type" : "p",
81​ "start" : "4096",
82​ "end" : "-1"
83​ }
84​ ],
85​ "uploads" : [
86​ {
87​ "part" : "1",
88​ "target" : "/updates.img",
89​ "source" : "https://fedorapeople.org/groups/qa/updates/updates-unipony.img"
90​ }
91​ ]
92​ },
93​ {
94​ "name" : "shrink",
95​ "size" : "10G",
96​ "labels" : ["mbr", "gpt"],
97​ "filesystems" : ["ext4", "ntfs"],
98​ "parts" : [
99​ {
100​ "type" : "p",
101​ "start" : "4096",
102​ "end" : "-4097"
103​ }
104​ ],
105​ "writes" : [
106​ {
107​ "part" : "1",
108​ "path" : "/testfile",
109​ "content" : "Hello, world!"
110​ }
111​ ]
112​ }
113​ ],
114​ "virtbuilder" : [
115​ {
116​ "name" : "minimal",
117​ "releases" : {
118​ "-1" : ["x86_64"],
119​ "-2" : ["x86_64"]
120​ }
121​ },
122​ {
123​ "name" : "desktop",
124​ "releases" : {
125​ "-1" : ["x86_64", "i686"],
126​ "-2" : ["x86_64", "i686"]
127​ },
128​ "size" : "20G"
129​ }
130​ ],
131​ "renames" : []
132​}
View Options

tools/minimal.commands

  • This file was added.
1​root-password password:weakpassword
2​update
3​selinux-relabel