Skip export of caches with no layers to OCI structures #4336
Conversation
| @@ -5437,6 +5438,70 @@ func testBasicGhaCacheImportExport(t *testing.T, sb integration.Sandbox) { | |||
| testBasicCacheImportExport(t, sb, []CacheOptionsEntry{im}, []CacheOptionsEntry{ex}) | |||
| } | |||
|
|
|||
| func testRegistryEmptyCacheExport(t *testing.T, sb integration.Sandbox) { | |||
There was a problem hiding this comment.
6 lines of code, 65 lines of tests. That seems about right.
That said, I'm not sure if we need a similar test for the local exporter, since the change was in shared code.
| @@ -185,6 +186,11 @@ func (ce *contentCacheExporter) Finalize(ctx context.Context) (map[string]string | |||
| return nil, err | |||
| } | |||
|
|
|||
| if len(config.Layers) == 0 { | |||
| bklog.G(ctx).Warn("failed to match any cache with layers") | |||
There was a problem hiding this comment.
I trivially copied this warning from the equivalent code in the inline exporter. It's not super-expressive, which suggests no one really cares about the warning, or is even looking for it, in the inline exporter. (Unless bklog.G(ctx) is doing more than I think it is)
If we really do want a warning or similar, it might make sense to define an error type for ErrNoCacheLayersToExport or something, and log that occurance with relevant details at a higher level (common to all exporters) and/or fail if that is an unexpected/undesirable case by that logic.
There was a problem hiding this comment.
I think we should also do a progress.OneOff call in here with "skipping cache export for empty result" message so the user sees what is happening on the progress output.
There was a problem hiding this comment.
I guess this is what you intended? The API's a little weird for this use-case, perhaps this function should have a single progress writer it uses rather than a bunch of (Edit: Looking again, the other uses here are more idiomatic, and a single progress tracker would fold down the progress of the different cache-push actions into one, and that's probably not a desired change)OneOffs?
(Note, GitHub did not expand the comment context to include the newly-added line below the comment. -_-)
TBBle
force-pushed
the
skip_export_of_empty_caches
branch
from
e52abf0
to
08f3f4f
Compare
These happen when a container image is built using only metadata actions, and hence does not generate any filesystem changes. Such a degenerate cache is not interesting, and some OCI registries reject OCI images with no layers. Specifically, this only affects the `registry` and `local` exporters. The `inline` exporter already early-outs in this case with the same warning, and the `gha`, `s3`, and `azblob` exporters are unchanged. Signed-off-by: Paul "TBBle" Hampson <Paul.Hampson@Pobox.com>
TBBle
force-pushed
the
skip_export_of_empty_caches
branch
from
08f3f4f
to
bf211cf
Compare
|
@tonistiigi @jedevc are we open to backporting this to 0.12.3? |
|
This doesn't look anything critical as there is no practical reason to use |
|
Hmm, I'd argue it's a bug/correctness issue and it's hard to know how you're using I'd also say that the spec not being final is a non-issue; not having a way to store "not-images" formalized has never stopped BuildKit in the past. TL;DR I'd like to have this in Moby sooner than later, but I won't protest too loudly if you don't want to backport/consider it a bug in the initial implementation. |
These happen when a container image is built using only metadata actions, and hence does not generate any filesystem changes.
Such a degenerate cache is not interesting, and some OCI registries reject OCI images with no layers.
Specifically, this only affects the
registryandlocalexporters.The
inlineexporter already early-outs in this case with the same warning, and thegha,s3, andazblobexporters are unchanged.This should finally resolve the last vestiges of negative user experience in aws/containers-roadmap#876.