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

emmileaf · 2022-09-06T15:43:31Z

In this PR:

Implementation of a proposed solution in #3733’s discussions, to check for presence of layer files in cache in addition to the manifest json before returning the cached base image(s). This would also address the unauthorized error described in #2220 and also reported in #2007’s comments.

Details:

Adds allLayersCached() method to Cache and CacheStorageReader that checks for existence of cached layers for all layers described in the image manifest.
Updates PullBaseImageStep.getCachedBaseImages() to invoke this check, and returns no cached images if layers are missing.

Two approaches for layer check:

Changes up to 755dcfc is a simpler check which verifies that the corresponding layer directory exists in the cache (but not that its contents are valid)
b231d79 checks layers by making a call to retrieve the layer, though the retrieved layer is unused and the later ObtainBaseImageLayerStep would retrieve it again

Open to suggestions on what the better approach might be here - the second option does redundant work, but is a more rigorous validation.

…ing for layer directory presence

mpeddada1 · 2022-09-08T19:57:15Z

@emmileaf Took a first look at the changes! To make sure that I'm understanding the two approaches correctly:

Approach 1 checks for the existence of {cacheDirectory}/layers/{layerDigest} where the layer digests are read from the manifest.
Approach 2 actually tries to grab the cached layer given the digests and if this act results in some sort of an IO issue then the layer is deemed to not be present in the cached?

Both approaches are valid but I'm a little concerned about the performance overhead approach 2 might add especially since the change will result in the cached layer being retrieved twice. Approach 1 might be a sufficient check. In terms of readability it also makes it more explicit that we are checking for the presence of the layer. Catching exceptions also works but there is usually a risk involved with it if we're not as specific as possible since methods can throw an exception for a number of reasons and it is usually recommended to let exceptions propagate to the caller or be handled by surrounding frameworks (testing frameworks, for example). Lmk what you think.

emmileaf · 2022-09-08T21:02:33Z

@mpeddada1 Thanks for the review! Your understanding of the two approaches is spot on.

I also agree with the concerns here around approach 2’s redundant work overhead and relying on exceptions. The downside of approach 1's check I was trying to work around is that, CacheStorageWriter.writeCompressed() creates the {cacheDirectory}/layers/{layerDigest} directory first, so it is possible for this check to succeed but the layer partially written (or not valid for retrieval in some other way). Not sure how unlikely that race condition would be though.

Maybe there is some way to make this check more rigorous without fully retrieving the layer? I'll revert the last commit to switch back to approach 1, and see if I can make any improvements from there.

…st checking for layer directory presence" This reverts commit b231d79.

emmileaf · 2022-09-09T13:53:06Z

The downside of approach 1's check I was trying to work around is that, CacheStorageWriter.writeCompressed() creates the {cacheDirectory}/layers/{layerDigest} directory first

Just realized this statement isn't true and I had misunderstood this cache write logic earlier - it looks like a temp folder is created for writing the layer first, and then the contents get moved into {cacheDirectory}/layers/{layerDigest} (ref: #879 (comment)). This addresses the concern with partial layer directories, and I'll go ahead with the switch to approach 1 in this PR. Apologies for the confusion!

mpeddada1 · 2022-09-09T14:56:59Z

Just realized this statement isn't true and I had misunderstood this cache write logic earlier - it looks like a temp folder is created for writing the layer first, and then the contents get moved into {cacheDirectory}/layers/{layerDigest} (ref: #879 (comment)). This addresses the concern with partial layer directories, and I'll go ahead with the switch to approach 1 in this PR. Apologies for the confusion!

Ah This changes the write to only produce a layer directory at the intended location in full. This way, cache reads never see an incomplete layer directory. That is reassuring, thank you for verifying this!

jib-core/src/main/java/com/google/cloud/tools/jib/cache/CacheStorageReader.java

mpeddada1 · 2022-09-09T20:06:20Z

jib-core/src/main/java/com/google/cloud/tools/jib/cache/CacheStorageReader.java

+   * @param manifest the image manifest
+   * @return a boolean
+   */
+  boolean allLayersCached(ManifestTemplate manifest) {


Would it possible to move this method directly to PullBaseImageStep? Since that is the only class where this is used and it could help us avoid having to go through an extra layer of classes to reach this method. Or does this method require any variables that are specific to CacheStorageReader?

Yeah, I was thinking about this too - this method goes through a few layers because it depends on CacheStorageReader’s cacheStorageFiles variable here.

Got it, thanks! Hm it looks like Cache takes in cacheStorageFiles as a parameter to it's constructor, could we use that maybe?

jib/jib-core/src/main/java/com/google/cloud/tools/jib/cache/Cache.java

Line 66 in c5936be

private Cache(CacheStorageFiles cacheStorageFiles) {

Ah you’re right - I missed that we could just add cacheStorageFiles as a field and move this logic to Cache.

I went ahead and tried to make this change, but noticed that the Cache class is set up in a way that many of its methods are wrappers around calls to either CacheStorageReader or CacheStorageWriter. Looking at the existing test suites, CacheStorageReaderTest is also more straightforward to add to than CacheTest for unit testing the new check.

I am tempted to leave this logic in CacheStorageReader just to stay consistent with the existing setup here - lmk what you think!

mpeddada1 · 2022-09-09T20:20:12Z

jib-core/src/main/java/com/google/cloud/tools/jib/cache/CacheStorageReader.java

+    } else {
+      throw new IllegalArgumentException("Unknown manifest type: " + manifest);
+    }


I wonder if this would break current behavior for those doing multi-platform image building? For context #2730, we also do caching for manifest lists in addition to single manifests.

That’s a good callout, and please let me know if I'm misunderstanding anything here!

Right now the added logic in PullBaseImageStep.getCachedBaseImages() never passes anything of V22ManifestList type into areAllLayersCached() calls explicitly. In the case of manifest lists, this check is made individually when looping over the platform-specific manifests, and returns an overall cache miss if any of the platform-specific manifests has incomplete layers.

But, do you think there is value here to have CacheStorageReader.areAllLayersCached() itself handle the manifest list type, rather than rely on the code calling it? Perhaps instead of throwing an exception here, it can also just return false, and leave the rest of the behavior to existing logic?

Ah thanks for the detailed explanation! You're right, it is called for the individual manifests in the manifest list.

But, do you think there is value here to have CacheStorageReader.areAllLayersCached() itself handle the manifest list type, rather than rely on the code calling it?

Hm that's a good question. I was initially thinking about this too but looking at the code for getCachedBaseImage, we would probably still have to iterate through the manifests in the manifest list again to retrieve the collection of images? What you have currently (which a more fail-fast approach) seems like a better choice.

Sounds good!

mpeddada1

Thanks for the thorough explanations. We're getting close!

mpeddada1 · 2022-09-12T17:34:06Z

jib-core/src/main/java/com/google/cloud/tools/jib/cache/CacheStorageReader.java

+    } else {
+      throw new IllegalArgumentException("Unknown manifest type: " + manifest);
+    }


Ah thanks for the detailed explanation! You're right, it is called for the individual manifests in the manifest list.

But, do you think there is value here to have CacheStorageReader.areAllLayersCached() itself handle the manifest list type, rather than rely on the code calling it?

Hm that's a good question. I was initially thinking about this too but looking at the code for getCachedBaseImage, we would probably still have to iterate through the manifests in the manifest list again to retrieve the collection of images? What you have currently (which a more fail-fast approach) seems like a better choice.

mpeddada1 · 2022-09-12T17:40:38Z

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

+      if (!baseImageLayersCache.allLayersCached(Verify.verifyNotNull(manifest))) {
+        return Collections.emptyList();
+      }


How about we move the Verify.verifyNotNull condition to allLayersCached?

Moved this to a line above, since this condition is applied again to the manifest in toImage() a few lines down

mpeddada1 · 2022-09-12T17:45:12Z

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

      }

      ManifestTemplate manifest = Verify.verifyNotNull(manifestAndConfigFound.get().getManifest());
+      // Verify all layers described in manifest are present in cache
+      if (!baseImageLayersCache.allLayersCached(manifest)) {
+        return Collections.emptyList();
+      }
+


Is it possible to combine !manifestAndConfigFound.isPresent() and areAllLayersCached with an &&?

It gets messier here to combine them I think? (Since grabbing the manifest is conditional on manifestAndConfigFound.isPresent(), plus it also needs to perform the verifyNotNull check)

Ah I see! thanks for trying this out. Hm thinking of a way in which we could remove the number of if statements since they are all testing similar things but just at different levels of granularity. Is it possible to put them in a helper method with a descriptive name?

You're right, the many if statements did get more cumbersome with the changes added in this PR. I think the refactoring suggestion from @elefeint will help with this here!

Excellent refactoring!

mpeddada1 · 2022-09-12T17:48:09Z

jib-core/src/main/java/com/google/cloud/tools/jib/cache/CacheStorageReader.java

+   * @param manifest the image manifest
+   * @return a boolean
+   */
+  boolean allLayersCached(ManifestTemplate manifest) {


Got it, thanks! Hm it looks like Cache takes in cacheStorageFiles as a parameter to it's constructor, could we use that maybe?

jib/jib-core/src/main/java/com/google/cloud/tools/jib/cache/Cache.java

Line 66 in c5936be

private Cache(CacheStorageFiles cacheStorageFiles) {

jib-core/src/test/java/com/google/cloud/tools/jib/builder/steps/PullBaseImageStepTest.java

jib-core/src/test/java/com/google/cloud/tools/jib/cache/CacheStorageReaderTest.java

mpeddada1 · 2022-09-12T18:09:31Z

jib-core/src/test/java/com/google/cloud/tools/jib/cache/CacheStorageReaderTest.java

+  public void testAllLayersCached_v21SingleManifest()
+      throws IOException, CacheCorruptedException, DigestException, URISyntaxException {
+
+    setupCachedMetadataV21(cacheDirectory);


Minor suggestion: If possible, let's format the test into three blocks: arrange, act and assert, with a space between each of these blocks. The arrange block will take care of all setup, the act block will call the method we're testing and the assert block will do all the verification.

Thanks for the suggestion, this makes a lot of sense! Tried to follow this idea though with the act and assert block more or less combined, since for a few of the tests had asserts both before and after certain actions.

How about we store these calls in variables? For example:

cacheAfterFirstLayerDirectory = Files.createDirectories(cacheStorageFiles.getLayerDirectory(firstLayerDigest)); areAllLayersCachedAfterFirstLayer = cacheStorageReader.areAllLayersCached(manifest) cacheAfterSecondLayerDirectory = ... areAllLayersCachedAfterSecondLayer= .. //Assert block

This is just an example so you can pick a name you think works better.

Ohh apologies I completely misunderstood earlier! Thank you for the explanation, I see what is meant by having separate act and assert blocks now. Will update the tests 😃

jib-core/src/test/java/com/google/cloud/tools/jib/cache/CacheStorageReaderTest.java

elefeint

Broadly LGTM with my limited knowledge. A couple of random questions inline.

jib-core/src/main/java/com/google/cloud/tools/jib/cache/CacheStorageReader.java

elefeint · 2022-09-12T19:14:06Z

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

@@ -486,6 +492,11 @@ List<Image> getCachedBaseImages()
      }

      ManifestTemplate manifest = Verify.verifyNotNull(manifestAndConfigFound.get().getManifest());
+      // Verify all layers described in manifest are present in cache


(optional) there was quite a bit of code duplication in this method even in the past between the section that deals with general and platform-based manifest processing. Could there be an opportunity for refactoring common logic out into a helper method?

Ah this is a great suggestion - will refactor this and see if I can make the methods cleaner to read.

mpeddada1

LGTM. This fix required in-depth knowledge into Jib's caching mechanism and you really hit it out of the park! Also made great refactorings along the way.

mpeddada1 · 2022-09-13T15:39:45Z

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

+      } else {
+        return Collections.singletonList(cachedImage.get());
      }


super nit: we can do away with the else here and return Collections.singletonList(...) outside the if.

mpeddada1 · 2022-09-13T15:40:27Z

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

      }

      ManifestTemplate manifest = Verify.verifyNotNull(manifestAndConfigFound.get().getManifest());
+      // Verify all layers described in manifest are present in cache
+      if (!baseImageLayersCache.allLayersCached(manifest)) {
+        return Collections.emptyList();
+      }
+


Excellent refactoring!

emmileaf · 2022-09-13T15:57:07Z

@mpeddada1 @elefeint @chanseokoh Thank you so much for all the help here with this PR!

elefeint

Nice refactoring!

emmileaf · 2022-09-16T14:22:40Z

Thank you! Will update changelog and merge.
(Edit: will merge once main branch's CI status is back in the clear, to avoid more confusion)

sonarcloud · 2022-09-16T14:29:38Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

90.9% Coverage
0.0% Duplication

…cached base image (#3767)" This reverts commit 7aa1110.

herder · 2022-10-25T11:27:37Z

Hi @emmileaf @elefeint - thank you for this fix!

We'd love to get this available for quite a few of our builds that fail from this, so I was wondering if there is a release with this fix planned soon?

emmileaf · 2022-10-25T15:57:39Z

Hi @herder - we currently have some work in progress on compatibility in our release build environment, but can plan for a release later this week including this change.

emmileaf · 2022-10-28T20:26:52Z

@herder jib-core 0.23.0, jib-maven-plugin 3.3.1, and jib-gradle-plugin 3.3.1 have been released with this fix.

herder · 2022-10-31T20:23:43Z

Great, thank you so much @emmileaf !

emmileaf added 7 commits September 1, 2022 14:35

WIP: add check that all image layers exist in cache

3caa0ab

WIP: remove unused methods from previous commit

c1d1d4d

WIP: fix broken unit tests

ecb4575

WIP: formatting, renaming, docs

0d70c29

WIP: add unit tests

c9a51ec

WIP: refactor allLayersCached

755dcfc

Alternative: try retrieving layer (redundantly) instead of just check…

b231d79

…ing for layer directory presence

emmileaf requested review from elefeint and mpeddada1 September 6, 2022 15:51

emmileaf marked this pull request as ready for review September 6, 2022 16:00

emmileaf added 2 commits September 9, 2022 09:23

Revert "Alternative: try retrieving layer (redundantly) instead of ju…

6ee16e0

…st checking for layer directory presence" This reverts commit b231d79.

Renaming fix

ac8a2c7

mpeddada1 reviewed Sep 9, 2022

View reviewed changes

emmileaf added 2 commits September 12, 2022 09:53

Rename to areAllLayersCached

cd2070f

Fix formatting

7e89ea7

mpeddada1 reviewed Sep 12, 2022

View reviewed changes

elefeint suggested changes Sep 12, 2022

View reviewed changes

emmileaf added 6 commits September 12, 2022 15:31

Move verifyNotNull check to be reused

3445f77

Test renaming and whitespace suggestions

1456e95

Switch to Truth.assertThat and clean up added unit tests

099c0ce

Refactor common code in getCachedBaseImages to helper method

ab53ec2

Update unit tests to separate action and assertion statements

05097fb

Minor: clean up unused change after refactor

d891281

mpeddada1 approved these changes Sep 13, 2022

View reviewed changes

Remove redundant else blocks

4971ee9

emmileaf requested a review from elefeint September 13, 2022 15:58

elefeint approved these changes Sep 16, 2022

View reviewed changes

Update CHANGELOG.md

4ecb92f

emmileaf merged commit 7aa1110 into master Sep 19, 2022

emmileaf deleted the base-image-caching branch September 19, 2022 19:44

emmileaf added a commit that referenced this pull request Sep 19, 2022

Revert "fix: Check for existence of layers in cache before returning …

3899fde

…cached base image (#3767)" This reverts commit 7aa1110.

emmileaf mentioned this pull request Sep 19, 2022

Revert "fix: Check for existence of layers in cache before returning cached base image" #3782

Closed

emmileaf restored the base-image-caching branch September 19, 2022 20:24

This was referenced Sep 20, 2022

Possible 401 Unauthorized when base image is referred to by a digest and manifest is cached #2220

Closed

Multi-module maven project with parallel builds fail with UNAUTHORIZED. #3733

Closed

emmileaf deleted the base-image-caching branch November 21, 2022 20:42

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

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

Conversation

emmileaf commented Sep 6, 2022 • edited Loading

mpeddada1 commented Sep 8, 2022

emmileaf commented Sep 8, 2022

emmileaf commented Sep 9, 2022

mpeddada1 commented Sep 9, 2022

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

mpeddada1 left a 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

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

emmileaf Sep 12, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elefeint left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mpeddada1 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emmileaf commented Sep 13, 2022

elefeint left a comment

Choose a reason for hiding this comment

emmileaf commented Sep 16, 2022 • edited Loading

sonarcloud bot commented Sep 16, 2022

herder commented Oct 25, 2022

emmileaf commented Oct 25, 2022

emmileaf commented Oct 28, 2022

herder commented Oct 31, 2022

emmileaf commented Sep 6, 2022 •

edited

Loading

emmileaf Sep 12, 2022 •

edited

Loading

emmileaf commented Sep 16, 2022 •

edited

Loading