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.

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
Show file tree
Hide file tree
Changes from 16 commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -445,8 +445,8 @@ List<Image> getCachedBaseImages()
LayerCountMismatchException, UnlistedPlatformInManifestListException,
PlatformNotFoundInBaseImageException {
ImageReference baseImage = buildContext.getBaseImageConfiguration().getImage();
Optional<ImageMetadataTemplate> metadata =
buildContext.getBaseImageLayersCache().retrieveMetadata(baseImage);
Cache baseImageLayersCache = buildContext.getBaseImageLayersCache();
Optional<ImageMetadataTemplate> metadata = baseImageLayersCache.retrieveMetadata(baseImage);
if (!metadata.isPresent()) {
return Collections.emptyList();
}
Expand All @@ -456,19 +456,13 @@ 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();
} else {
return Collections.singletonList(cachedImage.get());
}
Copy link
Contributor

Choose a reason for hiding this comment

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

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


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

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

// Manifest list cached. Identify matching platforms and check if all of them are cached.
Expand All @@ -484,13 +478,53 @@ 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();
} else {
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
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
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
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
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

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();
}
}