-
Notifications
You must be signed in to change notification settings - Fork 574
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
Slim down docker cache size #3190
Conversation
b427837
to
4d56f0a
Compare
4d56f0a
to
e08df0f
Compare
// TODO: ignoring the `ref` field though does create stable results to compare, but the SBOM is fundamentally gutted and not worth comparing (find a better redaction or compare method) | ||
//{ | ||
// name: cyclonedxjson.ID.String(), | ||
// redactor: func(in []byte) []byte { | ||
// // unstable values | ||
// in = regexp.MustCompile(`"(timestamp|serialNumber|bom-ref|ref)":\s*"(\n|[^"])+"`).ReplaceAll(in, []byte(`"$1": "redacted"`)) | ||
// in = regexp.MustCompile(`"(dependsOn)":\s*\[(?:\s|[^]])+]`).ReplaceAll(in, []byte(`"$1": []`)) | ||
// return in | ||
// }, | ||
// json: true, | ||
//}, | ||
//{ | ||
// name: cyclonedxxml.ID.String(), | ||
// redactor: func(in []byte) []byte { | ||
// // unstable values | ||
// in = regexp.MustCompile(`(serialNumber|bom-ref|ref)="[^"]+"`).ReplaceAll(in, []byte{}) | ||
// in = regexp.MustCompile(`<timestamp>[^<]+</timestamp>`).ReplaceAll(in, []byte{}) | ||
// | ||
// return in | ||
// }, | ||
//}, |
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 worth calling out -- I noticed that when redacting the ref
field the SBOM generated is ultimately wrong (several empty dependency fields created). This makes the test fundamentally wrong, and needs to be rethought for this format.
Why was this dealt with on this PR? Changing the image test fixture lead to test breakage, which is how I found the problem.
@@ -1,4 +1,4 @@ | |||
FROM centos:7.9.2009@sha256:dead07b4d8ed7e29e98de0f4504d87e8880d4347859d839686a31da35a3b532f |
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 image was so old that I was having a hard time replicating the state, so I updated the test fixture + test
@@ -1,6 +1,27 @@ | |||
FROM centos:7.9.2009@sha256:be65f488b7764ad3638f236b7b515b3678369a5124c47b8d32916d6487418ea4 |
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 image was so old that I was having a hard time replicating the state, so I updated the test fixture + test
8496a0a
to
0ecfe1a
Compare
Taskfile.yaml
Outdated
@@ -493,4 +506,4 @@ tasks: | |||
desc: Remove all docker cache and local image tar cache | |||
cmds: | |||
- 'find . -type f -wholename "**/test-fixtures/cache/stereoscope-fixture-*.tar" -delete' | |||
- "docker images --format '{{`{{.ID}}`}} {{`{{.Repository}}`}}' | grep stereoscope-fixture- | awk '{print $$1}' | uniq | xargs -r docker rmi --force" | |||
- "docker images --format '{{`{{.ID}}`}} {{`{{.Repository}}`}}' | grep stereoscope-fixture- | awk '{print $1}' | uniq | xargs -r docker rmi --force" |
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 was broken when ported from Makefile to Taskfile
@@ -313,6 +314,18 @@ tasks: | |||
- "echo '\nTar cache:'" | |||
- 'find . -type f -wholename "**/test-fixtures/snapshot/*" | sort' | |||
|
|||
check-docker-cache: |
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 to help regress in terms of how large our docker image fixture caches are collectively
0ecfe1a
to
96675da
Compare
@@ -1,13 +1,10 @@ | |||
//go:build !arm64 |
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 was able to make this cross-platform, which makes the fixture build/sync logic a little simpler (it's the same for all platforms)
Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com>
Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com>
Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com>
Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com>
Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com>
Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com>
732fb89
to
5a05bca
Compare
Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com>
0070827
to
0d857f9
Compare
@@ -85,6 +85,7 @@ from-images: | |||
paths: | |||
- /usr/local/go/bin/go | |||
|
|||
# TODO: this is no longer available from dockerhub! (the snippet is vital) |
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.
calling this out, but I have no action to take here
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 file is in the PR twice, because there are unrelated test fixtures that use this. Since I didn't want the docker contexts to overlap, it was easier to copy/paste the code than to orchestrate reusing it
Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com>
Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com>
Fixes #3181
We use docker images and other ephemeral data as a basis for test fixtures in unit/integration/cli tests...
...but we have over `5 GB` in cache!
There is what is listed below + a few-hundred MB centos 7 image that was removed.
In CI today we fetch and load the caches directly so we don't need to re pull all images. However, as mentioned in #3181 , without those caches, forks begin to see issues with the default runner space.
This PR makes our existing image caches smaller, a step in the right direction here...
... down to ~ `300 MB`
Overall changes:
test-fixtures/place
with gitignored data should have an accompanyingtest-fixtures/place.fingerprint
file with checksums of the input material used to build the fixtures in that directory. This is all governed by the newly refactoredmake fingerprints
target.make build-fixtures
that knows how to find**/test-fixtures/Makefile
files and run thefixtures
andclean
targets to manage creating/purging test fixtures. This makes it much easier to simply add new fixtures and not need to remember all of the places to add references too (there were some fixtures that we never had wired up into CI actions/cache calls, which means we never leveraged them in CI -- we continually rebuilt them! 😮💨 )ghcr.io/anchore/syft/test-fixture-cache
image. This is periodically refreshed (pushed) based off of the a 4am cron job running from the main branch -- no previous fixture state is used when building/pushing a new test fixture image.oras
is used to deal with pushing/pulling fixtures from an OCI registry, we additionally keepORAS_CACHE
pointed to.tmp/oras_cache
to prevent the need to pull the same data again and again. This also self-purges itself when it gets larger than 4 GB.make refresh-fixtures
that dynamically figures if cache should be pulled or rebuilt manuallyThis has resulted in improved performance in CI:
integration
step is down from 6 minute runs to 1.5 minute runs 🎉unit
step is down from 15 minute runs to 3 minute runs 🍰cli
step is down from 3 minutes to 2 minute runs 🌮Caveats:
oras
cache locally (to save on unnecessary network transfers) as well as the installed fixtures, we're storing all test fixtures twice! (three times if you count the potentially larger container images when built from scratch. However, these changes aggressively clear any existing cache when downloading, so we shouldn't be accumulating unused cache like we had been before. This also includes deleting stereoscope images from the docker daemon, so it shouldn't be all that common to have the duplicate docker image except for one-off instances when you're adding that image during development. All this being said... I think we're making better tradeoffs in this PR than we are today.From a developer perspective this should be transparent, no action should be needed. The next time you run
make unit
the fixture cache should be downloaded and installed in the right spots.For the future: