Skip to content
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.

Sign up for GitHub

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

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

Merged
merged 19 commits into from
Sep 19, 2022
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
3caa0ab
WIP: add check that all image layers exist in cache
emmileaf Sep 1, 2022
c1d1d4d
WIP: remove unused methods from previous commit
emmileaf Sep 1, 2022
ecb4575
WIP: fix broken unit tests
emmileaf Sep 1, 2022
0d70c29
WIP: formatting, renaming, docs
emmileaf Sep 2, 2022
c9a51ec
WIP: add unit tests
emmileaf Sep 2, 2022
755dcfc
WIP: refactor allLayersCached
emmileaf Sep 2, 2022
b231d79
Alternative: try retrieving layer (redundantly) instead of just check…
emmileaf Sep 2, 2022
6ee16e0
Revert "Alternative: try retrieving layer (redundantly) instead of ju…
emmileaf Sep 9, 2022
ac8a2c7
Renaming fix
emmileaf Sep 9, 2022
cd2070f
Rename to areAllLayersCached
emmileaf Sep 12, 2022
7e89ea7
Fix formatting
emmileaf Sep 12, 2022
3445f77
Move verifyNotNull check to be reused
emmileaf Sep 12, 2022
1456e95
Test renaming and whitespace suggestions
emmileaf Sep 12, 2022
099c0ce
Switch to Truth.assertThat and clean up added unit tests
emmileaf Sep 12, 2022
ab53ec2
Refactor common code in getCachedBaseImages to helper method
emmileaf Sep 12, 2022
05097fb
Update unit tests to separate action and assertion statements
emmileaf Sep 13, 2022
d891281
Minor: clean up unused change after refactor
emmileaf Sep 13, 2022
4971ee9
Remove redundant else blocks
emmileaf Sep 13, 2022
4ecb92f
Update CHANGELOG.md
emmileaf Sep 16, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions jib-core/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ All notable changes to this project will be documented in this file.
### Changed

### Fixed
- Fixed partially cached base image authorization issue by adding check for existence of layers in cache ([#3767](https://github.com/GoogleContainerTools/jib/pull/3767))

## 0.22.0

Expand Down
68 changes: 50 additions & 18 deletions jib-core/src/main/java/com/google/cloud/tools/jib/builder/steps/PullBaseImageStep.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -456,19 +456,12 @@ List<Image> getCachedBaseImages()

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));
ManifestAndConfigTemplate manifestAndConfig = manifestsAndConfigs.get(0);
Optional<Image> cachedImage = getBaseImageIfAllLayersCached(manifestAndConfig, true);
if (!cachedImage.isPresent()) {
return Collections.emptyList();
}

ContainerConfigurationTemplate containerConfig =
Verify.verifyNotNull(manifestsAndConfigs.get(0).getConfig());
PlatformChecker.checkManifestPlatform(buildContext, containerConfig);

return Collections.singletonList(
JsonToImageTranslator.toImage(
(BuildableManifestTemplate) Verify.verifyNotNull(manifest), containerConfig));
return Collections.singletonList(cachedImage.get());
}

// Manifest list cached. Identify matching platforms and check if all of them are cached.
Expand All @@ -484,13 +477,52 @@ List<Image> getCachedBaseImages()
if (!manifestAndConfigFound.isPresent()) {
return Collections.emptyList();
}

ManifestTemplate manifest = Verify.verifyNotNull(manifestAndConfigFound.get().getManifest());
ContainerConfigurationTemplate containerConfig =
Verify.verifyNotNull(manifestAndConfigFound.get().getConfig());
images.add(
JsonToImageTranslator.toImage((BuildableManifestTemplate) manifest, containerConfig));
Optional<Image> cachedImage =
getBaseImageIfAllLayersCached(manifestAndConfigFound.get(), false);
if (!cachedImage.isPresent()) {
return Collections.emptyList();
}
images.add(cachedImage.get());
}
return images.build();
}

/**
* Helper method to retrieve a base image from cache given manifest and container config. Does not
* return image if any layers of base image are missing in cache.
*
* @param manifestAndConfig stores an image manifest and a container config
* @param isSingleManifest true if base image is not a manifest list
* @return the single cached {@link Image} found, or {@code Optional#empty()} if the base image
* not found in cache with all layers present.
* @throws BadContainerConfigurationFormatException if the container configuration is in a bad
* format
* @throws PlatformNotFoundInBaseImageException if build target platform is not found in the base
* image
* @throws LayerCountMismatchException LayerCountMismatchException if the manifest and
* configuration contain conflicting layer information
*/
private Optional<Image> getBaseImageIfAllLayersCached(
ManifestAndConfigTemplate manifestAndConfig, boolean isSingleManifest)
throws BadContainerConfigurationFormatException, PlatformNotFoundInBaseImageException,
LayerCountMismatchException {
Cache baseImageLayersCache = buildContext.getBaseImageLayersCache();
ManifestTemplate manifest = Verify.verifyNotNull(manifestAndConfig.getManifest());

// Verify all layers described in manifest are present in cache
if (!baseImageLayersCache.areAllLayersCached(manifest)) {
return Optional.empty();
}
if (manifest instanceof V21ManifestTemplate) {
return Optional.of(JsonToImageTranslator.toImage((V21ManifestTemplate) manifest));
}
ContainerConfigurationTemplate containerConfig =
Verify.verifyNotNull(manifestAndConfig.getConfig());
if (isSingleManifest) {
// If base image is not a manifest list, check and warn misconfigured platforms.
PlatformChecker.checkManifestPlatform(buildContext, containerConfig);
}
return Optional.of(
JsonToImageTranslator.toImage((BuildableManifestTemplate) manifest, containerConfig));
}
}
11 changes: 11 additions & 0 deletions jib-core/src/main/java/com/google/cloud/tools/jib/cache/Cache.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
import com.google.cloud.tools.jib.image.json.ContainerConfigurationTemplate;
import com.google.cloud.tools.jib.image.json.ImageMetadataTemplate;
import com.google.cloud.tools.jib.image.json.ManifestAndConfigTemplate;
import com.google.cloud.tools.jib.image.json.ManifestTemplate;
import com.google.cloud.tools.jib.image.json.V21ManifestTemplate;
import com.google.common.collect.ImmutableList;
import java.io.IOException;
Expand Down Expand Up @@ -192,6 +193,16 @@ public Optional<ImageMetadataTemplate> retrieveMetadata(ImageReference imageRefe
return cacheStorageReader.retrieveMetadata(imageReference);
}

/**
* Returns {@code true} if all image layers described in a manifest exist in the cache.
*
* @param manifest the image manifest
* @return a boolean
*/
public boolean areAllLayersCached(ManifestTemplate manifest) {
return cacheStorageReader.areAllLayersCached(manifest);
}

/**
* Retrieves the {@link CachedLayer} that was built from the {@code layerEntries}.
*
Expand Down
32 changes: 32 additions & 0 deletions jib-core/src/main/java/com/google/cloud/tools/jib/cache/CacheStorageReader.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,38 @@ static void verifyImageMetadata(ImageMetadataTemplate metadata, Path metadataCac
this.cacheStorageFiles = cacheStorageFiles;
}

/**
* Returns {@code true} if all image layers described in a manifest have a corresponding file
* entry in the cache.
*
* @param manifest the image manifest
* @return a boolean
*/
boolean areAllLayersCached(ManifestTemplate manifest) {
elefeint marked this conversation as resolved.
Show resolved Hide resolved

List<DescriptorDigest> layerDigests;

if (manifest instanceof V21ManifestTemplate) {
layerDigests = ((V21ManifestTemplate) manifest).getLayerDigests();
} else if (manifest instanceof BuildableManifestTemplate) {
layerDigests =
((BuildableManifestTemplate) manifest)
.getLayers().stream()
.map(BuildableManifestTemplate.ContentDescriptorTemplate::getDigest)
.collect(Collectors.toList());
} else {
throw new IllegalArgumentException("Unknown manifest type: " + manifest);
}
Comment on lines +103 to +105
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good!


for (DescriptorDigest layerDigest : layerDigests) {
Path layerDirectory = cacheStorageFiles.getLayerDirectory(layerDigest);
if (!Files.exists(layerDirectory)) {
return false;
}
}
return true;
}

/**
* Retrieves the cached image metadata (a manifest list and a list of manifest/container
* configuration pairs) for an image reference.
Expand Down
34 changes: 34 additions & 0 deletions jib-core/src/test/java/com/google/cloud/tools/jib/builder/steps/PullBaseImageStepTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@

package com.google.cloud.tools.jib.builder.steps;

import static com.google.common.truth.Truth.assertThat;
import static org.mockito.ArgumentMatchers.eq;

import com.google.cloud.tools.jib.api.DescriptorDigest;
Expand All @@ -40,6 +41,7 @@
import com.google.cloud.tools.jib.image.json.ContainerConfigurationTemplate;
import com.google.cloud.tools.jib.image.json.ImageMetadataTemplate;
import com.google.cloud.tools.jib.image.json.ManifestAndConfigTemplate;
import com.google.cloud.tools.jib.image.json.ManifestTemplate;
import com.google.cloud.tools.jib.image.json.OciIndexTemplate;
import com.google.cloud.tools.jib.image.json.PlatformNotFoundInBaseImageException;
import com.google.cloud.tools.jib.image.json.UnlistedPlatformInManifestListException;
Expand Down Expand Up @@ -155,6 +157,7 @@ public void testCall_digestBaseImage()
ImageMetadataTemplate imageMetadata =
new ImageMetadataTemplate(null, Arrays.asList(manifestAndConfig));
Mockito.when(cache.retrieveMetadata(imageReference)).thenReturn(Optional.of(imageMetadata));
Mockito.when(cache.areAllLayersCached(manifestAndConfig.getManifest())).thenReturn(true);

ImagesAndRegistryClient result = pullBaseImageStep.call();
Assert.assertEquals("fat system", result.images.get(0).getOs());
Expand Down Expand Up @@ -196,6 +199,7 @@ public void testCall_offlineMode_cached()
ImageMetadataTemplate imageMetadata =
new ImageMetadataTemplate(null, Arrays.asList(manifestAndConfig));
Mockito.when(cache.retrieveMetadata(imageReference)).thenReturn(Optional.of(imageMetadata));
Mockito.when(cache.areAllLayersCached(manifestAndConfig.getManifest())).thenReturn(true);

ImagesAndRegistryClient result = pullBaseImageStep.call();
Assert.assertEquals("fat system", result.images.get(0).getOs());
Expand Down Expand Up @@ -297,6 +301,24 @@ public void testGetCachedBaseImages_emptyCache()
Assert.assertEquals(Arrays.asList(), pullBaseImageStep.getCachedBaseImages());
}

@Test
public void testGetCachedBaseImages_partiallyCached_emptyListReturned()
throws InvalidImageReferenceException, CacheCorruptedException, IOException,
LayerCountMismatchException, PlatformNotFoundInBaseImageException,
BadContainerConfigurationFormatException, UnlistedPlatformInManifestListException {
ImageReference imageReference = ImageReference.parse("cat");
Mockito.when(buildContext.getBaseImageConfiguration())
.thenReturn(ImageConfiguration.builder(imageReference).build());
ManifestTemplate manifest = Mockito.mock(ManifestTemplate.class);
ImageMetadataTemplate imageMetadata =
new ImageMetadataTemplate(
null, Arrays.asList(new ManifestAndConfigTemplate(manifest, null)));
Mockito.when(cache.retrieveMetadata(imageReference)).thenReturn(Optional.of(imageMetadata));
Mockito.when(cache.areAllLayersCached(manifest)).thenReturn(false);

assertThat(pullBaseImageStep.getCachedBaseImages()).isEmpty();
}

@Test
public void testGetCachedBaseImages_v21ManifestCached()
throws InvalidImageReferenceException, IOException, CacheCorruptedException,
Expand All @@ -316,6 +338,7 @@ public void testGetCachedBaseImages_v21ManifestCached()
null, Arrays.asList(new ManifestAndConfigTemplate(v21Manifest, null)));

Mockito.when(cache.retrieveMetadata(imageReference)).thenReturn(Optional.of(imageMetadata));
Mockito.when(cache.areAllLayersCached(v21Manifest)).thenReturn(true);

List<Image> images = pullBaseImageStep.getCachedBaseImages();

Expand Down Expand Up @@ -344,6 +367,7 @@ public void testGetCachedBaseImages_v22ManifestCached()
ImageMetadataTemplate imageMetadata =
new ImageMetadataTemplate(null, Arrays.asList(manifestAndConfig));
Mockito.when(cache.retrieveMetadata(imageReference)).thenReturn(Optional.of(imageMetadata));
Mockito.when(cache.areAllLayersCached(manifestAndConfig.getManifest())).thenReturn(true);

List<Image> images = pullBaseImageStep.getCachedBaseImages();

Expand Down Expand Up @@ -381,6 +405,12 @@ public void testGetCachedBaseImages_v22ManifestListCached()
new ManifestAndConfigTemplate(
new V22ManifestTemplate(), containerConfigJson2, "sha256:digest2")));
Mockito.when(cache.retrieveMetadata(imageReference)).thenReturn(Optional.of(imageMetadata));
Mockito.when(
cache.areAllLayersCached(imageMetadata.getManifestsAndConfigs().get(0).getManifest()))
.thenReturn(true);
Mockito.when(
cache.areAllLayersCached(imageMetadata.getManifestsAndConfigs().get(1).getManifest()))
.thenReturn(true);

Mockito.when(containerConfig.getPlatforms())
.thenReturn(ImmutableSet.of(new Platform("arch1", "os1"), new Platform("arch2", "os2")));
Expand Down Expand Up @@ -416,6 +446,9 @@ public void testGetCachedBaseImages_v22ManifestListCached_partialMatches()
new ContainerConfigurationTemplate(),
"sha256:digest1")));
Mockito.when(cache.retrieveMetadata(imageReference)).thenReturn(Optional.of(imageMetadata));
Mockito.when(
cache.areAllLayersCached(imageMetadata.getManifestsAndConfigs().get(0).getManifest()))
.thenReturn(true);

Mockito.when(containerConfig.getPlatforms())
.thenReturn(ImmutableSet.of(new Platform("arch1", "os1"), new Platform("arch2", "os2")));
Expand Down Expand Up @@ -454,6 +487,7 @@ public void testGetCachedBaseImages_v22ManifestListCached_onlyPlatforms()
Arrays.asList(
unrelatedManifestAndConfig, targetManifestAndConfig, unrelatedManifestAndConfig));
Mockito.when(cache.retrieveMetadata(imageReference)).thenReturn(Optional.of(imageMetadata));
Mockito.when(cache.areAllLayersCached(targetManifestAndConfig.getManifest())).thenReturn(true);

Mockito.when(containerConfig.getPlatforms())
.thenReturn(ImmutableSet.of(new Platform("target-arch", "target-os")));
Expand Down
43 changes: 43 additions & 0 deletions jib-core/src/test/java/com/google/cloud/tools/jib/cache/CacheStorageReaderTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,8 @@

package com.google.cloud.tools.jib.cache;

import static com.google.common.truth.Truth.assertThat;

import com.google.cloud.tools.jib.api.DescriptorDigest;
import com.google.cloud.tools.jib.api.ImageReference;
import com.google.cloud.tools.jib.blob.Blobs;
Expand Down Expand Up @@ -638,4 +640,45 @@ public void testVerifyImageMetadata_validOciImageIndex() throws CacheCorruptedEx
CacheStorageReader.verifyImageMetadata(metadata, Paths.get("/cache/dir"));
// should pass without CacheCorruptedException
}

@Test
public void testAllLayersCached_v21SingleManifest()
throws IOException, CacheCorruptedException, DigestException, URISyntaxException {
setupCachedMetadataV21(cacheDirectory);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@emmileaf emmileaf Sep 12, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 😃

ImageMetadataTemplate metadata =
cacheStorageReader.retrieveMetadata(ImageReference.of("test", "image", "tag")).get();
V21ManifestTemplate manifest =
(V21ManifestTemplate) metadata.getManifestsAndConfigs().get(0).getManifest();
DescriptorDigest firstLayerDigest = manifest.getLayerDigests().get(0);
DescriptorDigest secondLayerDigest = manifest.getLayerDigests().get(1);

// Create only one of the layer directories so that layers are only partially cached.
Files.createDirectories(cacheStorageFiles.getLayerDirectory(firstLayerDigest));
boolean checkWithPartialLayersCached = cacheStorageReader.areAllLayersCached(manifest);
// Create the other layer directory so that all layers are cached.
Files.createDirectories(cacheStorageFiles.getLayerDirectory(secondLayerDigest));
boolean checkWithAllLayersCached = cacheStorageReader.areAllLayersCached(manifest);

assertThat(checkWithPartialLayersCached).isFalse();
assertThat(checkWithAllLayersCached).isTrue();
}

@Test
public void testAllLayersCached_v22SingleManifest()
throws IOException, CacheCorruptedException, DigestException, URISyntaxException {
setupCachedMetadataV22(cacheDirectory);
ImageMetadataTemplate metadata =
cacheStorageReader.retrieveMetadata(ImageReference.of("test", "image", "tag")).get();
V22ManifestTemplate manifest =
(V22ManifestTemplate) metadata.getManifestsAndConfigs().get(0).getManifest();
DescriptorDigest layerDigest = manifest.getLayers().get(0).getDigest();

boolean checkBeforeLayerCached = cacheStorageReader.areAllLayersCached(manifest);
// Create the single layer directory so that all layers are cached.
Files.createDirectories(cacheStorageFiles.getLayerDirectory(layerDigest));
boolean checkAfterLayerCached = cacheStorageReader.areAllLayersCached(manifest);

assertThat(checkBeforeLayerCached).isFalse();
assertThat(checkAfterLayerCached).isTrue();
}
}