-
Notifications
You must be signed in to change notification settings - Fork 17
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
[CI:TOOLING] Track image IDs instead of tar exports #380
Conversation
87a7115
to
a0f22cc
Compare
4c546a1
to
12b728a
Compare
I think this is ready for review. Container image build times with these changes: imgts 06:47 Compared to a past build: IMGTS 08:13 Likely the biggest build-time boost is the result of pushing/pulling the imgts, as it's used as a base-image for most of the others. |
Ed and/or Paul, if you have time would you mind giving this a look-through? |
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.
This is completely over my head, but LGTM in theory with one question
Makefile
Outdated
@@ -150,8 +153,14 @@ timebomb-check: | |||
false; \ | |||
fi | |||
|
|||
# Given the path to a file containing 'sha245:<image id>' return <image id> |
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 worth a repush on its own
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.
Oh whups. I can fix that NP, this PR builds quickly.
@@ -163,19 +172,18 @@ ci_debug: $(_TEMPDIR)/ci_debug.tar ## Build and enter container for local develo | |||
-e GAC_FILEPATH=$(GAC_FILEPATH) \ | |||
-e AWS_SHARED_CREDENTIALS_FILE=$(AWS_SHARED_CREDENTIALS_FILE) \ | |||
-e TEMPDIR=$(_TEMPDIR) \ | |||
docker-archive:$< | |||
$(call imageid,$<) $(if $(DBG_TEST_CMD),$(DBG_TEST_CMD),) |
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 don't understand this. Does this magic image have an entrypoint or command defined for when it's invoked without args? If so, did you confirm that the true
here actually overrides the default? Because sometimes you can override via command-line, and sometimes you need to specify --entrypoint
. (I could figure this out myself given enough time but my brain hurts from this). Anyhow, CI got skipped for this PR so I can't verify tests.
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.
This is intended for interactive-use by humans, and should probably be documented in the README.md. The entrypoint is simply the default 'bash'. It's for developing/debugging the dockerfile-based Cirrus-CI tasks, since the image-build happens w/in Cirrus (i.e. it's never pushed anywhere):
(.cirrus.yml)
...cut...
image_builder_task:
...cut...
container:
dockerfile: "image_builder/Containerfile"
...cut...
...cut...
Anyhow, CI got skipped
Careful, CI wasn't entirely skipped. The builds and new test tasks did run 😉
I'll add some quick docs for this.
FWIW, the image_builder_debug
target (also intended for humans) is already documented.
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 don't feel qualified to review this. There is a lot of makefile logic that I cannot follow right now. Anyhow using image ids to pass the images around to other tasks seems better
Previously all container builds run by the Makefile managed them based on presence/absence of a docker-archive tar file. Producing these exports is time-consuming and ultimately unnecessary extra work. The tar files are never actually consumed in a meaningful way by any other targets. Further, most of the container builds in CI run in parallel, simply throwing away the tar when finished. Fix this by switching to management based on image-ID files instead. The only exception is the `imgts` image and images which are based on it. For those, some special handling is required (already done by the CI build script), so some comments were added to assist. Also, remove the `bench_stuff` target entirely as this has long since been retired. Signed-off-by: Chris Evich <cevich@redhat.com>
Previously if either debugging targets broke in some way, nobody would know. Fix this by adding simple CI tests that confirm they build and run a basic command. Also, quiet down the unzipping of AWS cli tools. Signed-off-by: Chris Evich <cevich@redhat.com>
Clarify the difference between `ci_debug` and `image_builder_debug`. Signed-off-by: Chris Evich <cevich@redhat.com>
12b728a
to
8b60787
Compare
Thanks guys. I understand |
Previously all container builds run by the Makefile managed them based
on presence/absence of a docker-archive tar file. Producing these
exports is time-consuming and ultimately unnecessary extra work. The
tar files are never actually consumed in a meaningful way by any other
targets. Further, most of the container builds in CI run in parallel,
simply throwing away the tar when finished.
Fix this by switching to management based on image-ID files instead.
The only exception is the
imgts
image and images which are based onit. For those, some special handling is required (already done by the
CI build script), so some comments were added to assist.
Also, remove the
bench_stuff
target entirely as this has long sincebeen retired.