-
Notifications
You must be signed in to change notification settings - Fork 18
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
build-image enhancements #191
base: master
Are you sure you want to change the base?
build-image enhancements #191
Conversation
Example of the manifests pushed by this script at https://quay.io/repository/phlogistonjohn/samba-server?tab=tags |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Few comments on commits excluding hack/build-image: add --combined option for building indexes
.
a85ee31
to
0d70903
Compare
Signed-off-by: John Mulligan <[email protected]>
Some `black` and `flake8` fixes. Signed-off-by: John Mulligan <[email protected]>
Signed-off-by: John Mulligan <[email protected]>
Don't call the same function many times when that function's value is never expected to change at this scope. Signed-off-by: John Mulligan <[email protected]>
I find lambda to be a bit of an unpleasant thing and try to remove it when possible. Since all uses of lambda in this code are calling the same function we can remove the function (and unchanging) argument and have the variable section map to the run function's remaining kwargs. Signed-off-by: John Mulligan <[email protected]>
Add an --archive=LOCATION command line argument that is somewhat like push but instead uses the archive tarballs. This could be use in a situation where you are building on a fast machine or different arch than the machine you will ultimately be pushing from. Signed-off-by: John Mulligan <[email protected]>
Add a --load=LOCATION command line argument that mirrors the new --archive command but imports images from the archive dir. This could be used in a situation where you are building images on one machine but then need to update or push images from a different machine. Signed-off-by: John Mulligan <[email protected]>
Split the existing function used to break up a container name into parts into a smaller reusable function. Signed-off-by: John Mulligan <[email protected]>
Indexes (aka manifests) allow for images to be built for different architectures and the associated with one tag (or set of shared tags). The build script enables indexes/manifests when supplied with the --combined flag. The `build` function is changed a bit and no longer always builds an image. If the image is already present locally, the build will be skipped - this is done because (re)building an image of a different architecture can be very slow. This new behavior tries to avoid costly unintentional rebuilds. A future change will add a new command to force a rebuild. Signed-off-by: John Mulligan <[email protected]>
Add a --rebuild command to force building of images that already exist in local storage. Signed-off-by: John Mulligan <[email protected]>
Fix/improve the behavior of the script when --push is combined with --combined. Signed-off-by: John Mulligan <[email protected]>
The description of `exists` didn't match the code and now we can extend and clean things up in the presence of indexes. Signed-off-by: John Mulligan <[email protected]>
Ignore index targets if `--combined` is passed along with `--archive=<dir>`. Signed-off-by: John Mulligan <[email protected]>
Allow the script to skip creating unqualified tags on images (like 'nightly' or 'latest'). Signed-off-by: John Mulligan <[email protected]>
Signed-off-by: John Mulligan <[email protected]>
0d70903
to
df64528
Compare
# apply additional tag names | ||
for tname in target.all_names(baseless=cli.without_repo_bases): | ||
tag_args = [eng, "tag", target.image_name(), tname] | ||
if target.image_name() != tname: | ||
run(cli, tag_args, check=True) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As per your example from description, is this the place which decides on the tag under which manifests are grouped? In the light of your example how do we arrive at the tag name default-fedora
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not really, that is now handled by the BuildRequest class - in particular the _expand_special_tags (and add_special_tags) and _build_indexes methods.
The tag default-fedora
for an image is considered a distro qualified tag. For an index it's the fully qualified tag (as the arch belongs w/in the index). The fully qualified tags are determined when each target class is constructed. So if you pass --combined, we walk through the list of requested images and if FQINs exist we'll derive the index tags from those (in _build_indexes and TargetIndex.from_image).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not really, that is now handled by the BuildRequest class - in particular the _expand_special_tags (and add_special_tags) and _build_indexes methods.
Ok.
The tag
default-fedora
for an image is considered a distro qualified tag. For an index it's the fully qualified tag (as the arch belongs w/in the index). The fully qualified tags are determined when each target class is constructed. So if you pass --combined, we walk through the list of requested images and if FQINs exist we'll derive the index tags from those (in _build_indexes and TargetIndex.from_image).
But isn't default-fedora
different from what we had earlier categorized as distro qualified tag? add_special_tags() used to create fedora-latest
for default package source and fedora-nightly
for nightly package source as distro qualified tags. With this change tag_name() kind of reverses the tag name parts and doesn't use latest
for default package source.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm probably not explaining it very well. I request that you please try it out in practice rather than me going back and forth trying to poorly explain it. If you think something is wrong, please do speak up, but I fear the more I try to explain the code rather than just read it - I will confuse things more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I went through the steps to create samba-server:default-fedora-amd64
and samba-server:default-fedora-arm64
:
. . .
Successfully tagged localhost/samba-server:fedora-latest
Successfully tagged localhost/samba-server:default-fedora-amd64
Successfully tagged quay.io/samba.org/samba-server:fedora-latest
Successfully tagged quay.io/samba.org/samba-server:latest
Successfully tagged quay.io/samba.org/samba-server:default-fedora-amd64
46c6d74fa62c487e96f1c3685e453539b8771d722378b6a34731c4c4351ec4bf
. . .
Successfully tagged localhost/samba-server:fedora-latest
Successfully tagged localhost/samba-server:default-fedora-arm64
Successfully tagged quay.io/samba.org/samba-server:fedora-latest
Successfully tagged quay.io/samba.org/samba-server:latest
Successfully tagged quay.io/samba.org/samba-server:default-fedora-arm64
480500c271df8b226022ab125eff1f4bd8e0d6985c147a38ea6531ec5eb80494
. . .
and then executed the following command:
$ hack/build-image -i quay.io/anoopcs/samba-server:default-fedora-arm64 -i quay.io/anoopcs/samba-server:default-fedora-amd64 --combined --push --push-selected-tags=mixed --push-state=auto-index
The above command resulted in two different tags with 2 manifests each as follows:
$ podman image ls | grep anoopcs
quay.io/anoopcs/samba-server default-fedora 20191b9fb0da 6 minutes ago 1.05 kB
quay.io/anoopcs/samba-server latest 20191b9fb0da 6 minutes ago 1.05 kB
quay.io/anoopcs/samba-server default-fedora-amd64 46c6d74fa62c 56 minutes ago 472 MB
quay.io/anoopcs/samba-server default-fedora-arm64 480500c271df 57 minutes ago 581 MB
Earlier we created the distro qualified tag as fedora-latest
and now it gets tagged as default-fedora
. I hope this new naming happens within tag_names().
# apply additional tag names | ||
for tname in target.all_names(baseless=cli.without_repo_bases): | ||
tag_args = [eng, "tag", target.image_name(), tname] | ||
if target.image_name() != tname: | ||
run(cli, tag_args, check=True) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not really, that is now handled by the BuildRequest class - in particular the _expand_special_tags (and add_special_tags) and _build_indexes methods.
Ok.
The tag
default-fedora
for an image is considered a distro qualified tag. For an index it's the fully qualified tag (as the arch belongs w/in the index). The fully qualified tags are determined when each target class is constructed. So if you pass --combined, we walk through the list of requested images and if FQINs exist we'll derive the index tags from those (in _build_indexes and TargetIndex.from_image).
But isn't default-fedora
different from what we had earlier categorized as distro qualified tag? add_special_tags() used to create fedora-latest
for default package source and fedora-nightly
for nightly package source as distro qualified tags. With this change tag_name() kind of reverses the tag name parts and doesn't use latest
for default package source.
New features for hack/build-image:
This is fairly large group of changes to prepare the build-image script for multi-arch management.
It tries to enable new workflows such as building images on disparate hosts with varying architectures, sharing those builds with --archive and gathering them with --load, before pushing them with --combined to create multi-arch indexes.
One workflow example goes a bit like: