-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Implement new manifest cache design: cache manifest lists and manifest+config pairs #2711
Conversation
…into manifest-cache
Will update CHANGELOG and do some more manual end-to-end tests. |
jib-core/src/main/java/com/google/cloud/tools/jib/builder/steps/PullBaseImageStep.java
Show resolved
Hide resolved
jib-core/src/main/java/com/google/cloud/tools/jib/builder/steps/PullBaseImageStep.java
Show resolved
Hide resolved
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 know if I know enough of this part of the code to review can we do in person on monday morning?
jib-core/src/main/java/com/google/cloud/tools/jib/builder/steps/PullBaseImageStep.java
Show resolved
Hide resolved
jib-core/src/main/java/com/google/cloud/tools/jib/cache/CacheStorageWriter.java
Outdated
Show resolved
Hide resolved
jib-core/src/main/java/com/google/cloud/tools/jib/builder/steps/PullBaseImageStep.java
Show resolved
Hide resolved
jib-core/src/main/java/com/google/cloud/tools/jib/builder/steps/PullBaseImageStep.java
Outdated
Show resolved
Hide resolved
ae9f3fa
to
b92f1ff
Compare
b92f1ff
to
8a57ce2
Compare
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.
Thanks for refactoring this, I had a much easier time following the logic. Some simple questions, but otherwise lgtm.
} | ||
|
||
// TODO: support OciIndexTemplate once AbstractManifestPuller starts to accept it. | ||
Verify.verify(manifestTemplate instanceof V22ManifestListTemplate); |
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.
Does this then throw an error for OciIndexTemplate
? Perhaps we can give the user a better error message (I thought Verify
is for catching programming/integration issues?)
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.
Basically the refactored code doesn't change the current behavior in this regard. The current code would simply break with ClassCastException
if manifestTemplate
could ever be of OciIndexTemplate
, as it can't cast OciInexTemplate
into BuildableManifestTemplate
in jsonManifestToImage()
. That is, it's an impossible condition; we never see OciIndexTemplate
, as the manifest puller doesn't tell a registry that it can accept an OCI image index. All the registries in practice so far have been returning a Docker manifest list.
throws IOException, LayerPropertyNotFoundException, UnknownManifestFormatException { | ||
BuildableManifestTemplate manifest = | ||
(BuildableManifestTemplate) manifestAndDigest.getManifest(); | ||
Preconditions.checkArgument(manifest.getSchemaVersion() == 2); |
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.
Similarly here, what happens when schema version is not 2
, does this just kill our build with a confusing error message?
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.
Will kill the build. It's an impossible condition the user should never hit. If we see it, it's a bug for us to fix. That is, we should pull a container config only when it's a schema 2 (i.e., when OciImageTemplate
or V22ManifestTemplate
), because the V2.1 manifest embeds a container config.
But actually, BuildableManifestTemplate
is always schema version 2. The check line is only to help us locate places to update more easily in the future when a schema version 3 is introduced in the spec.
I will add CHANGELOG once this is fully implemented. |
(See #2707 about the new JSON data structure (
ImageMetadataTemplate
) for the cache filemanifests_configs.json
.)The new cache supports storing and retrieving both OCI and Docker manifests and manifest lists.
ImageMetadataTemplate.manifestsAndConfigs
without caching a manifest list (nullImageMetadataTemplate.manifestList
).Cache API Changes
Current manifest cache API
ManifestAndConfigTemplate
).New API
ImageMetadataTemplate
).Updating Manifest Cache File
The implementation replaces
manifests_configs.json
whenever running Jib online. Jib will store only those manifests and configs Jib downloaded and used in the last build–no accumulation of past manifests and configs.Cache Invalidation in the New Release
The new cache file is
manifests_configs.json
. Existingmanifest.json
andconfig.json
will not be used. The user upgrading Jib will lose previous manifest and config caches.