Implement new manifest cache fully capable of multi-platform image building #2730

chanseokoh · 2020-08-26T21:08:40Z

Follow-up to #2711 to complete the new cache design to support multi-platform image building.

Adds UnlistedPlatformInManifestListException.
No platform verification of a cached manifest any more. getCachedBaseImages() returns cached images only when Jib has cached all the manifests matching the configured platforms.
When caching a manifest list and manifests, PullBaseImageStep also stores additional information of manifest digests. This is to enable looking up a manifest from a manifest list by digest. For example,

{
  "manifestList": {  # manifest list content
    "manifests": [
      {  # manifest descriptor 1
        "digest": "sha256:a157906...",
        "platform": { ... },
        ...
      },
      {
        "digest": "sha256:...",
        "platform": { ... },
        ...
      }
    ]
  },

  "manifestsAndConfigs": [
    {
      "manifestDigest": "sha256:a1579064....",  <-- manifest digest for lookup
      "manifest": { ... },
      "config": { ... }
    },
    { ... },
    ...
  ]

…into manifest-cache

…ols/jib into manifest-cache

chanseokoh · 2020-08-27T16:39:08Z

Ready for review. See the PR description for high-level changes.

loosebazooka · 2020-08-27T15:06:08Z

jib-core/src/main/java/com/google/cloud/tools/jib/builder/steps/PullBaseImageStep.java

- + "Jib online once to re-cache the right image manifest.");
- }
- return new ImagesAndRegistryClient(Collections.singletonList(image.get()), null);
+ List<Image> images = getCachedBaseImages();


I thought we could still run into an issue where we don't have all the necessary platform information for an image/platform that wasn't previously referenced?

I assume downloading all the image layers for all the images is impractical?

where we don't have all the necessary platform information for an image/platform that wasn't previously referenced?

In this case, the getCachedBaseImages() in this PR returns an empty list (i.e., cache miss). Offline building will fail and suggest to re-run online once. The digest reference case will fall through to normal operation (contacting the base image repository).

jib-core/src/main/java/com/google/cloud/tools/jib/image/json/ManifestAndConfigTemplate.java

mpeddada1 · 2020-08-27T19:26:06Z

jib-core/src/main/java/com/google/cloud/tools/jib/builder/steps/PullBaseImageStep.java

+ if (manifestList == null) {
+ Verify.verify(manifestsAndConfigs.size() == 1);
+ ManifestTemplate manifest = manifestsAndConfigs.get(0).getManifest();
+ if (manifest instanceof V21ManifestTemplate) {
+ return Collections.singletonList(
+ JsonToImageTranslator.toImage((V21ManifestTemplate) manifest));
+ }
+ return Collections.singletonList(
+ JsonToImageTranslator.toImage(
+ (BuildableManifestTemplate) Verify.verifyNotNull(manifest),
+ Verify.verifyNotNull(manifestsAndConfigs.get(0).getConfig())));


So this section is for when the base image reference is a single manifest?

Exactly. When the image reference is an image manifest and not a manifest list. In the cache metadata, manifestList is null in this case.

Ah gotcha, is there a particular reason why we chose null instead of an empty list for manifestList?

Good question. A "manifest list" is not a List in Java but a class called ManifestTemplate.

Right my bad, thanks for clarifying:)

mpeddada1 · 2020-08-27T19:30:33Z

jib-core/src/main/java/com/google/cloud/tools/jib/builder/steps/PullBaseImageStep.java

+ * single image (if cached). If a manifest list, returns all the images matching the configured
+ * platforms in the manifest list but only when all of the images are cached.


Why do we make sure to return the images matching the configured platforms in the manifest list only if all the images are cached?

If we have cached only a subset of what we really need (i.e., Jib needs to build, say, three Images, but the cache has only two Images), then it's not a cache hit as a whole, thus a cache miss.

Theoretically you could say, Jib could re-download only the missing Images, but note that performance is not the reason for caching. It's about whether we can completely avoid accessing the Internet or not. So, if any image is missing in the cache, I am saying "cache miss" for simplicity.

mpeddada1 · 2020-08-27T19:40:57Z

jib-core/src/main/java/com/google/cloud/tools/jib/builder/steps/PullBaseImageStep.java

+ ManifestTemplate manifest = Verify.verifyNotNull(manifestAndConfigFound.get().getManifest());
+ ContainerConfigurationTemplate containerConfig =
+ Verify.verifyNotNull(manifestAndConfigFound.get().getConfig());


I may be missing something here but what's the reasoning behind checking if these values are null?/ When would they be null?

It's our assumption and how we designed the cache to work. If any ManifestAndConfig entry (which is just a pair of a manifest and a config) is saved (cached), their values should never be null; otherwise, something got terribly wrong.

Now, we then use Verify.verifyNotNull() to signal the NullAway check (it's a check we set up in our repo to catch potential NPE) that manifest and containerConfig are not supposed to be null. Otherwise, because JsonToImageTranslator.toImage() doesn't accept @Nullable parameters, NullAway will complain that you're passing potentially null parameters.

Oh I see! Makes sense

chanseokoh added 30 commits August 13, 2020 15:13

Implement new manifests_configs.json cache

4f9c1bc

Merge branch 'master' into manifest-cache

f480cf1

wip

abeb080

Implement new manfiest+container config cache

e643c54

Revert unnecessary code

1682e3b

Revert filter(...) to original code to avoid false NullAway positives

999125e

Convert to polymorphic JSON de-/serialization

2166d2b

Add manifest+config cache JSON template

595bd19

Fix tests

5e2c6bc

Update code comment

a53417d

Merge branch 'prepare-manifest-cache' into manifest-cache

126d30d

Fix tests

be8d91f

Fix wrong cache validation

c6bada4

Simplify test code

38e00c9

Add CacheStorageReader tests

ca69545

Add tests

badb776

Revert unintended debug code

c8ad480

Fix bug in writing metadata cache

e20a836

Add tests

4bfe4ef

Add tests

5cadf8c

Update comments

bcd17f4

Update Javadocs

dfab7ce

Rename variables

c52ebb8

Merge branch 'master' of https:/GoogleContainerTools/jib …

f0a9203

…into manifest-cache

Refactor code

bb0c901

Refactor code

1c5c7a3

Merge branch 'manifest-cache' of https:/GoogleContainerTo…

0908e25

…ols/jib into manifest-cache

Decrease log level (lifecycle -> info)

b3a0c89

New cache API / refactor code

8a57ce2

Add unlisted manfiest excception

783c5bf

chanseokoh added 4 commits August 24, 2020 15:35

Refactor code

064bb85

Merge branch 'master' into manifest-cache-multi-platform-part1

a1e6b9d

Add tests

17282e1

Merge branch 'master' into manifest-cache-multi-platform-part1

3d7ffb8

googlebot added the cla: yes label Aug 26, 2020

chanseokoh added 7 commits August 26, 2020 17:10

Javadoc placeholder

a9d44b8

Fix Javadoc

1464c0e

Add and fix tests

27e620b

Fix tests

0cb21d2

Fix tests

c01c470

Clean up code

cfeefb2

Update Javadoc

6426994

chanseokoh requested a review from a team August 27, 2020 16:38

chanseokoh marked this pull request as ready for review August 27, 2020 16:38

loosebazooka reviewed Aug 27, 2020

View reviewed changes

Update Javadocs

e170f76

mpeddada1 reviewed Aug 27, 2020

View reviewed changes

loosebazooka approved these changes Aug 28, 2020

View reviewed changes

chanseokoh merged commit a4028d5 into master Aug 31, 2020

chanseokoh deleted the manifest-cache-multi-platform-part1 branch August 31, 2020 20:11

chanseokoh mentioned this pull request Sep 3, 2020

Update CHANGELOG about manifest cache being invalidated #2740

Merged

chanseokoh mentioned this pull request Sep 23, 2020

When jib.from.platforms is configured, build may fail due to mismatching platform from previously cached manifest #2655

Closed

chanseokoh added this to the v2.6.0 milestone Sep 29, 2020

mpeddada1 mentioned this pull request Sep 9, 2022

Check for existence of layers in cache before returning cached base image #3767

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement new manifest cache fully capable of multi-platform image building #2730

Implement new manifest cache fully capable of multi-platform image building #2730

chanseokoh commented Aug 26, 2020 •

edited

Loading

chanseokoh commented Aug 27, 2020

loosebazooka Aug 27, 2020

chanseokoh Aug 27, 2020

mpeddada1 Aug 27, 2020

chanseokoh Aug 27, 2020

mpeddada1 Aug 27, 2020

chanseokoh Aug 27, 2020

mpeddada1 Aug 28, 2020

mpeddada1 Aug 27, 2020

chanseokoh Aug 27, 2020 •

edited

Loading

mpeddada1 Aug 27, 2020 •

edited

Loading

chanseokoh Aug 27, 2020 •

edited

Loading

mpeddada1 Aug 28, 2020

		* single image (if cached). If a manifest list, returns all the images matching the configured
		* platforms in the manifest list but only when all of the images are cached.

Implement new manifest cache fully capable of multi-platform image building #2730

Implement new manifest cache fully capable of multi-platform image building #2730

Conversation

chanseokoh commented Aug 26, 2020 • edited Loading

chanseokoh commented Aug 27, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chanseokoh Aug 27, 2020 • edited Loading

Choose a reason for hiding this comment

mpeddada1 Aug 27, 2020 • edited Loading

Choose a reason for hiding this comment

chanseokoh Aug 27, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chanseokoh commented Aug 26, 2020 •

edited

Loading

chanseokoh Aug 27, 2020 •

edited

Loading

mpeddada1 Aug 27, 2020 •

edited

Loading

chanseokoh Aug 27, 2020 •

edited

Loading