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

Refactor configuration build/push/tar build steps to reduce duplicate code #269

Merged
merged 7 commits into from
May 14, 2018
Merged

Refactor configuration build/push/tar build steps to reduce duplicate code #269

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
0a6d4d1
Reduce duplicate code in BuildContainerConfigurationStep, BuildTarbal…
TadCordle May 11, 2018
1535b0f
template -> manifestTemplate
TadCordle May 11, 2018
9f16937
Feedback + refactor further
TadCordle May 11, 2018
7e315ca
Feedback
TadCordle May 14, 2018
610d840
Rename everything
TadCordle May 14, 2018
200d42c
Add tests for TarStreamBuilder.addEntry(String, String)
TadCordle May 14, 2018
428f565
More tests
TadCordle May 14, 2018
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
5 changes: 2 additions & 3 deletions jib-core/src/main/java/com/google/cloud/tools/jib/builder/BuildDockerSteps.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@
package com.google.cloud.tools.jib.builder;

import com.google.cloud.tools.jib.Timer;
import com.google.cloud.tools.jib.blob.Blob;
import com.google.cloud.tools.jib.cache.Cache;
import com.google.cloud.tools.jib.cache.CacheDirectoryNotOwnedException;
import com.google.cloud.tools.jib.cache.CacheMetadataCorruptedException;
Expand Down Expand Up @@ -124,10 +123,10 @@ public void run()

timer2.lap("Setting up build container configuration");
// Builds the container configuration.
ListenableFuture<ListenableFuture<Blob>> buildContainerConfigurationFutureFuture =
ListenableFuture<ListenableFuture<Image>> buildContainerConfigurationFutureFuture =
Copy link
Contributor

Choose a reason for hiding this comment

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

buildImageFutureFuture

Futures.whenAllSucceed(pullBaseImageLayerFuturesFuture)
.call(
new BuildContainerConfigurationStep(
new BuildImageStep(
buildConfiguration,
listeningExecutorService,
pullBaseImageLayerFuturesFuture,
Expand Down
14 changes: 6 additions & 8 deletions ...lder/BuildContainerConfigurationStep.java → ...oud/tools/jib/builder/BuildImageStep.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,11 +17,9 @@
package com.google.cloud.tools.jib.builder;

import com.google.cloud.tools.jib.Timer;
import com.google.cloud.tools.jib.blob.Blob;
import com.google.cloud.tools.jib.cache.CachedLayer;
import com.google.cloud.tools.jib.image.Image;
import com.google.cloud.tools.jib.image.LayerPropertyNotFoundException;
import com.google.cloud.tools.jib.image.json.ImageToJsonTranslator;
import com.google.common.util.concurrent.Futures;
import com.google.common.util.concurrent.ListenableFuture;
import com.google.common.util.concurrent.ListeningExecutorService;
Expand All @@ -31,8 +29,8 @@
import java.util.concurrent.ExecutionException;
import java.util.concurrent.Future;

/** Builds the container configuration. */
class BuildContainerConfigurationStep implements Callable<ListenableFuture<Blob>> {
/** Builds a model {@link Image}. */
class BuildImageStep implements Callable<ListenableFuture<Image>> {

private static final String DESCRIPTION = "Building container configuration";

Expand All @@ -43,7 +41,7 @@ class BuildContainerConfigurationStep implements Callable<ListenableFuture<Blob>
private final List<ListenableFuture<CachedLayer>> buildApplicationLayerFutures;
private final List<String> entrypoint;

BuildContainerConfigurationStep(
BuildImageStep(
BuildConfiguration buildConfiguration,
ListeningExecutorService listeningExecutorService,
ListenableFuture<List<ListenableFuture<CachedLayer>>> pullBaseImageLayerFuturesFuture,
Expand All @@ -58,7 +56,7 @@ class BuildContainerConfigurationStep implements Callable<ListenableFuture<Blob>

/** Depends on {@code pullBaseImageLayerFuturesFuture}. */
@Override
public ListenableFuture<Blob> call() throws ExecutionException, InterruptedException {
public ListenableFuture<Image> call() throws ExecutionException, InterruptedException {
// TODO: This might need to belong in BuildImageSteps.
List<ListenableFuture<?>> afterImageLayerFuturesFutureDependencies = new ArrayList<>();
afterImageLayerFuturesFutureDependencies.addAll(
Expand All @@ -72,7 +70,7 @@ public ListenableFuture<Blob> call() throws ExecutionException, InterruptedExcep
* Depends on {@code pushAuthorizationFuture}, {@code pullBaseImageLayerFuturesFuture.get()}, and
* {@code buildApplicationLayerFutures}.
*/
private Blob afterImageLayerFuturesFuture()
private Image afterImageLayerFuturesFuture()
throws ExecutionException, InterruptedException, LayerPropertyNotFoundException {
try (Timer ignored = new Timer(buildConfiguration.getBuildLogger(), DESCRIPTION)) {
// Constructs the image.
Expand All @@ -88,7 +86,7 @@ private Blob afterImageLayerFuturesFuture()
image.setEntrypoint(entrypoint);

// Gets the container configuration content descriptor.
return new ImageToJsonTranslator(image).getContainerConfigurationBlob();
return image;
}
}
}
10 changes: 4 additions & 6 deletions jib-core/src/main/java/com/google/cloud/tools/jib/builder/BuildImageSteps.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@
package com.google.cloud.tools.jib.builder;

import com.google.cloud.tools.jib.Timer;
import com.google.cloud.tools.jib.blob.Blob;
import com.google.cloud.tools.jib.blob.BlobDescriptor;
import com.google.cloud.tools.jib.cache.Cache;
import com.google.cloud.tools.jib.cache.CacheDirectoryNotOwnedException;
Expand Down Expand Up @@ -145,10 +144,10 @@ public void run()

timer2.lap("Setting up build container configuration");
// Builds the container configuration.
ListenableFuture<ListenableFuture<Blob>> buildContainerConfigurationFutureFuture =
ListenableFuture<ListenableFuture<Image>> buildContainerConfigurationFutureFuture =
Copy link
Contributor

Choose a reason for hiding this comment

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

s/buildContainerConfigurationFutureFuture/buildImageFutureFuture

Futures.whenAllSucceed(pullBaseImageLayerFuturesFuture)
.call(
new BuildContainerConfigurationStep(
new BuildImageStep(
buildConfiguration,
listeningExecutorService,
pullBaseImageLayerFuturesFuture,
Expand Down Expand Up @@ -189,11 +188,10 @@ public void run()
buildConfiguration,
listeningExecutorService,
authenticatePushFuture,
pullBaseImageLayerFuturesFuture,
buildAndCacheApplicationLayerFutures,
pushBaseImageLayerFuturesFuture,
pushApplicationLayersFuture,
pushContainerConfigurationFutureFuture),
pushContainerConfigurationFutureFuture,
buildContainerConfigurationFutureFuture),
listeningExecutorService);

timer2.lap("Running push new image");
Expand Down
77 changes: 20 additions & 57 deletions jib-core/src/main/java/com/google/cloud/tools/jib/builder/BuildTarballAndLoadDockerStep.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,56 +21,39 @@
import com.google.cloud.tools.jib.blob.Blobs;
import com.google.cloud.tools.jib.cache.CachedLayer;
import com.google.cloud.tools.jib.docker.json.DockerLoadManifestTemplate;
import com.google.cloud.tools.jib.image.ImageReference;
import com.google.cloud.tools.jib.image.Image;
import com.google.cloud.tools.jib.image.Layer;
import com.google.cloud.tools.jib.image.LayerPropertyNotFoundException;
import com.google.cloud.tools.jib.image.json.ImageToJsonTranslator;
import com.google.cloud.tools.jib.json.JsonTemplateMapper;
import com.google.cloud.tools.jib.tar.TarStreamBuilder;
import com.google.common.annotations.VisibleForTesting;
import com.google.common.util.concurrent.Futures;
import com.google.common.util.concurrent.ListenableFuture;
import com.google.common.util.concurrent.ListeningExecutorService;
import java.io.BufferedOutputStream;
import java.io.IOException;
import java.io.OutputStream;
import java.nio.file.Files;
import java.nio.file.Path;
import java.util.ArrayList;
import java.util.List;
import java.util.concurrent.Callable;
import java.util.concurrent.ExecutionException;
import java.util.concurrent.Future;
import org.apache.commons.compress.archivers.tar.TarArchiveEntry;

/** Adds image layers to a tarball and loads into Docker daemon. */
class BuildTarballAndLoadDockerStep implements Callable<Void> {

/**
* Builds a {@link DockerLoadManifestTemplate} from image parameters and returns the result as a
* blob.
*/
@VisibleForTesting
static Blob getManifestBlob(ImageReference imageReference, List<String> layerFiles) {
// Set up the JSON template.
DockerLoadManifestTemplate template = new DockerLoadManifestTemplate();
template.setRepoTags(imageReference.toStringWithTag());
template.addLayerFiles(layerFiles);

// Serializes into JSON.
return JsonTemplateMapper.toBlob(template);
}

private final BuildConfiguration buildConfiguration;
private final ListeningExecutorService listeningExecutorService;
private final ListenableFuture<List<ListenableFuture<CachedLayer>>>
pullBaseImageLayerFuturesFuture;
private final List<ListenableFuture<CachedLayer>> buildApplicationLayerFutures;
private final ListenableFuture<ListenableFuture<Blob>> buildConfigurationFutureFuture;
private final ListenableFuture<ListenableFuture<Image>> buildConfigurationFutureFuture;
Copy link
Contributor

Choose a reason for hiding this comment

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

buildImageFutureFuture


BuildTarballAndLoadDockerStep(
BuildConfiguration buildConfiguration,
ListeningExecutorService listeningExecutorService,
ListenableFuture<List<ListenableFuture<CachedLayer>>> pullBaseImageLayerFuturesFuture,
List<ListenableFuture<CachedLayer>> buildApplicationLayerFutures,
ListenableFuture<ListenableFuture<Blob>> buildConfigurationFutureFuture) {
ListenableFuture<ListenableFuture<Image>> buildConfigurationFutureFuture) {
Copy link
Contributor

Choose a reason for hiding this comment

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

buildImageFutureFuture

this.buildConfiguration = buildConfiguration;
this.listeningExecutorService = listeningExecutorService;
this.pullBaseImageLayerFuturesFuture = pullBaseImageLayerFuturesFuture;
Expand Down Expand Up @@ -99,48 +82,28 @@ public Void call() throws ExecutionException, InterruptedException {
* <p>TODO: Refactor into testable components
*/
private Void afterPushBaseImageLayerFuturesFuture()
throws ExecutionException, InterruptedException, IOException {
throws ExecutionException, InterruptedException, IOException, LayerPropertyNotFoundException {
// Add layers to image tarball
Image image = NonBlockingFutures.get(NonBlockingFutures.get(buildConfigurationFutureFuture));
TarStreamBuilder tarStreamBuilder = new TarStreamBuilder();
List<String> layerFiles = new ArrayList<>();
for (Future<CachedLayer> cachedLayerFuture :
NonBlockingFutures.get(pullBaseImageLayerFuturesFuture)) {
Path layerFile = NonBlockingFutures.get(cachedLayerFuture).getContentFile();
layerFiles.add(layerFile.getFileName().toString());
tarStreamBuilder.addEntry(
new TarArchiveEntry(layerFile.toFile(), layerFile.getFileName().toString()));
}
for (Future<CachedLayer> cachedLayerFuture : buildApplicationLayerFutures) {
Path layerFile = NonBlockingFutures.get(cachedLayerFuture).getContentFile();
// TODO: Consolidate with build configuration step so we don't have to rebuild the image
if (!layerFiles.contains(layerFile.getFileName().toString())) {
layerFiles.add(layerFile.getFileName().toString());
tarStreamBuilder.addEntry(
new TarArchiveEntry(layerFile.toFile(), layerFile.getFileName().toString()));
}
DockerLoadManifestTemplate manifestTemplate = new DockerLoadManifestTemplate();
Copy link
Contributor

Choose a reason for hiding this comment

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

Prob make a TODO/file an issue to refactor this tar building to a separate class specifically for building a Docker image tar so that it is unit-testable. Generally, these async steps should be kept as minimal as possible.

Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like #261 is this.


for (Layer layer : image.getLayers()) {
Path cachedFile = ((CachedLayer) layer).getContentFile();
Copy link
Contributor

Choose a reason for hiding this comment

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

I feel like we should actually change Image to only hold and return CachedLayers so that we don't have to do the cast here, but that change could prob go in a TODO/separate issue/PR.

String layerName = cachedFile.getFileName().toString();
tarStreamBuilder.addEntry(new TarArchiveEntry(cachedFile.toFile(), layerName));
manifestTemplate.addLayerFile(layerName);
}

// Add config to tarball
// TODO: Add ability to add blobs as entries to TarStreamBuilder without using temp files
Blob containerConfigurationBlob =
NonBlockingFutures.get(NonBlockingFutures.get(buildConfigurationFutureFuture));
Path tempConfig = Files.createTempFile(null, null);
tempConfig.toFile().deleteOnExit();
try (OutputStream bufferedOutputStream =
new BufferedOutputStream(Files.newOutputStream(tempConfig))) {
containerConfigurationBlob.writeTo(bufferedOutputStream);
}
tarStreamBuilder.addEntry(new TarArchiveEntry(tempConfig.toFile(), "config.json"));
new ImageToJsonTranslator(image).getContainerConfigurationBlob();
tarStreamBuilder.addEntry(Blobs.writeToString(containerConfigurationBlob), "config.json");

// Add manifest to tarball
Blob manifestBlob = getManifestBlob(buildConfiguration.getTargetImageReference(), layerFiles);
Path tempManifest = Files.createTempFile(null, null);
tempManifest.toFile().deleteOnExit();
try (OutputStream bufferedOutputStream =
new BufferedOutputStream(Files.newOutputStream(tempManifest))) {
manifestBlob.writeTo(bufferedOutputStream);
}
tarStreamBuilder.addEntry(new TarArchiveEntry(tempManifest.toFile(), "manifest.json"));
manifestTemplate.setRepoTags(buildConfiguration.getTargetImageReference().toStringWithTag());
tarStreamBuilder.addEntry(
Blobs.writeToString(JsonTemplateMapper.toBlob(manifestTemplate)), "manifest.json");

// Load the image to docker daemon
// TODO: Command is untested/not very robust
Expand Down
15 changes: 10 additions & 5 deletions ...core/src/main/java/com/google/cloud/tools/jib/builder/PushContainerConfigurationStep.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,9 @@
import com.google.cloud.tools.jib.blob.BlobDescriptor;
import com.google.cloud.tools.jib.hash.CountingDigestOutputStream;
import com.google.cloud.tools.jib.http.Authorization;
import com.google.cloud.tools.jib.image.Image;
import com.google.cloud.tools.jib.image.LayerPropertyNotFoundException;
import com.google.cloud.tools.jib.image.json.ImageToJsonTranslator;
import com.google.cloud.tools.jib.registry.RegistryClient;
import com.google.cloud.tools.jib.registry.RegistryException;
import com.google.common.io.ByteStreams;
Expand All @@ -40,13 +43,13 @@ class PushContainerConfigurationStep implements Callable<ListenableFuture<BlobDe

private final BuildConfiguration buildConfiguration;
private final ListenableFuture<Authorization> pushAuthorizationFuture;
private final ListenableFuture<ListenableFuture<Blob>> buildConfigurationFutureFuture;
private final ListenableFuture<ListenableFuture<Image>> buildConfigurationFutureFuture;
Copy link
Contributor

Choose a reason for hiding this comment

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

buildImageFutureFuture

private final ListeningExecutorService listeningExecutorService;

PushContainerConfigurationStep(
BuildConfiguration buildConfiguration,
ListenableFuture<Authorization> pushAuthorizationFuture,
ListenableFuture<ListenableFuture<Blob>> buildConfigurationFutureFuture,
ListenableFuture<ListenableFuture<Image>> buildConfigurationFutureFuture,
Copy link
Contributor

Choose a reason for hiding this comment

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

ditto

Copy link
Contributor

Choose a reason for hiding this comment

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

And L63

ListeningExecutorService listeningExecutorService) {
this.buildConfiguration = buildConfiguration;
this.pushAuthorizationFuture = pushAuthorizationFuture;
Expand All @@ -69,7 +72,8 @@ public ListenableFuture<BlobDescriptor> call() throws ExecutionException, Interr
* Depends on {@code buildConfigurationFutureFuture.get()} and {@code pushAuthorizationFuture}.
Copy link
Contributor

Choose a reason for hiding this comment

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

ditto

*/
private BlobDescriptor afterBuildConfigurationFutureFuture()
throws ExecutionException, InterruptedException, IOException, RegistryException {
throws ExecutionException, InterruptedException, IOException, RegistryException,
LayerPropertyNotFoundException {
try (Timer timer = new Timer(buildConfiguration.getBuildLogger(), DESCRIPTION)) {
// TODO: Use PushBlobStep.
// Pushes the container configuration.
Expand All @@ -80,10 +84,11 @@ private BlobDescriptor afterBuildConfigurationFutureFuture()
buildConfiguration.getTargetImageRepository())
.setTimer(timer);

Image image = NonBlockingFutures.get(NonBlockingFutures.get(buildConfigurationFutureFuture));
Blob containerConfigurationBlob =
new ImageToJsonTranslator(image).getContainerConfigurationBlob();
CountingDigestOutputStream digestOutputStream =
new CountingDigestOutputStream(ByteStreams.nullOutputStream());
Blob containerConfigurationBlob =
NonBlockingFutures.get(NonBlockingFutures.get(buildConfigurationFutureFuture));
containerConfigurationBlob.writeTo(digestOutputStream);

BlobDescriptor descriptor = digestOutputStream.toBlobDescriptor();
Expand Down
Loading