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

Jump to bottom

Parallelize local base image compression #2022

Merged
merged 2 commits into from
Sep 25, 2019
Merged

Parallelize local base image compression #2022

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
fcd1ef2
Parallelize base image compression
TadCordle Sep 25, 2019
457b81b
Changelog
TadCordle Sep 25, 2019
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
2 changes: 2 additions & 0 deletions jib-core/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,8 @@ All notable changes to this project will be documented in this file.

### Changed

- Local base image layers are now processed in parallel, speeding up builds using large local base images. ([#1913](https:/GoogleContainerTools/jib/issues/1913))

### Fixed

- Fixed temporary directory cleanup during builds using local base images. ([#2016](https:/GoogleContainerTools/jib/issues/2016))
Expand Down
32 changes: 24 additions & 8 deletions jib-core/src/main/java/com/google/cloud/tools/jib/builder/steps/ExtractTarStep.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,9 @@
import java.util.List;
import java.util.Optional;
import java.util.concurrent.Callable;
import java.util.concurrent.ExecutionException;
import java.util.concurrent.ExecutorService;
import java.util.concurrent.Future;
import java.util.zip.GZIPInputStream;
import java.util.zip.GZIPOutputStream;

Expand Down Expand Up @@ -85,16 +88,19 @@ static boolean isGzipped(Path path) throws IOException {
}
}

private final ExecutorService executorService;
private final BuildConfiguration buildConfiguration;
private final Path tarPath;
private final ProgressEventDispatcher.Factory progressEventDispatcherFactory;
private final TempDirectoryProvider tempDirectoryProvider;

ExtractTarStep(
ExecutorService executorService,
BuildConfiguration buildConfiguration,
Path tarPath,
ProgressEventDispatcher.Factory progressEventDispatcherFactory,
TempDirectoryProvider tempDirectoryProvider) {
this.executorService = executorService;
this.buildConfiguration = buildConfiguration;
this.tarPath = tarPath;
this.progressEventDispatcherFactory = progressEventDispatcherFactory;
Expand All @@ -104,7 +110,7 @@ static boolean isGzipped(Path path) throws IOException {
@Override
public LocalImage call()
throws IOException, LayerCountMismatchException, BadContainerConfigurationFormatException,
CacheCorruptedException {
ExecutionException, InterruptedException {
Path destination = tempDirectoryProvider.newDirectory();
try (TimerEventDispatcher ignored =
new TimerEventDispatcher(
Expand Down Expand Up @@ -139,21 +145,31 @@ public LocalImage call()

// Process layer blobs
// TODO: Optimize; compressing/calculating layer digests is slow
// e.g. parallelize, faster compression method
// e.g. faster compression method
try (ProgressEventDispatcher progressEventDispatcher =
progressEventDispatcherFactory.create(
"processing base image layers", layerFiles.size())) {
List<PreparedLayer> layers = new ArrayList<>(layerFiles.size());
V22ManifestTemplate v22Manifest = new V22ManifestTemplate();

// Start compressing layers in parallel
List<Future<CachedLayer>> cachedLayers = new ArrayList<>();
for (int index = 0; index < layerFiles.size(); index++) {
Path layerFile = destination.resolve(layerFiles.get(index));
CachedLayer layer =
getCachedTarLayer(
configurationTemplate.getLayerDiffId(index),
layerFile,
layersAreCompressed,
progressEventDispatcher.newChildProducer());
DescriptorDigest diffId = configurationTemplate.getLayerDiffId(index);
ProgressEventDispatcher.Factory progressEventDispatcherFactory =
progressEventDispatcher.newChildProducer();

cachedLayers.add(
executorService.submit(
Copy link
Member

Choose a reason for hiding this comment

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

Ideally, the code here won't deal with concurrency and StepsRunner should be able to maximize concurrency by individually compressing and caching each layer. For example, right now it is possible that some registry base image layers are being pulled in while some base layers can be pushed simultaneously. (I am sure constructing a base image manifest can be delayed, since I believe the reason we need them at an early stage is to be able to download the layers from a registry.) Also this would be nice for the consistent async framework design where each step doesn't get an ExecutorService.

But I think this is OK for short-term simple improvement for a low-hanging fruit. We should continue to track for the ideal async handling.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I agree this should be iterated on to be more consistent/less self-contained. But I figured it'd be nice to at least get the small performance gain in without adding too much complexity at the moment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also if we change the build steps more it'd be nice to keep the second bullet in #1913 in mind.

Instead of computing the layer digest and then pushing to a registry, push base image layers while compressing on-the-fly (#1906 (comment))

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I'm already getting a headache over all these. Sounds like a fun challenge!

() ->
getCachedTarLayer(
diffId, layerFile, layersAreCompressed, progressEventDispatcherFactory)));
}

// Collect compressed layers and add to manifest
for (Future<CachedLayer> layerFuture : cachedLayers) {
CachedLayer layer = layerFuture.get();
layers.add(new PreparedLayer.Builder(layer).build());
v22Manifest.addLayer(layer.getSize(), layer.getDigest());
}
Expand Down
2 changes: 1 addition & 1 deletion ...core/src/main/java/com/google/cloud/tools/jib/builder/steps/ObtainBaseImageLayerStep.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -116,7 +116,7 @@ private static ImmutableList<ObtainBaseImageLayerStep> makeList(
private final @Nullable Authorization pullAuthorization;
private final BlobExistenceChecker blobExistenceChecker;

ObtainBaseImageLayerStep(
private ObtainBaseImageLayerStep(
BuildConfiguration buildConfiguration,
ProgressEventDispatcher.Factory progressEventDispatcherFactory,
Layer layer,
Expand Down
1 change: 1 addition & 0 deletions jib-core/src/main/java/com/google/cloud/tools/jib/builder/steps/StepsRunner.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -257,6 +257,7 @@ private void extractTar() {
executorService.submit(
() ->
new ExtractTarStep(
executorService,
buildConfiguration,
results.tarPath.get(),
childProgressDispatcherFactory,
Expand Down
11 changes: 8 additions & 3 deletions jib-core/src/test/java/com/google/cloud/tools/jib/builder/steps/ExtractTarStepTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,17 +19,18 @@
import com.google.cloud.tools.jib.builder.ProgressEventDispatcher;
import com.google.cloud.tools.jib.builder.steps.ExtractTarStep.LocalImage;
import com.google.cloud.tools.jib.cache.Cache;
import com.google.cloud.tools.jib.cache.CacheCorruptedException;
import com.google.cloud.tools.jib.configuration.BuildConfiguration;
import com.google.cloud.tools.jib.event.EventHandlers;
import com.google.cloud.tools.jib.filesystem.TempDirectoryProvider;
import com.google.cloud.tools.jib.image.LayerCountMismatchException;
import com.google.cloud.tools.jib.image.json.BadContainerConfigurationFormatException;
import com.google.common.io.Resources;
import com.google.common.util.concurrent.MoreExecutors;
import java.io.IOException;
import java.net.URISyntaxException;
import java.nio.file.Path;
import java.nio.file.Paths;
import java.util.concurrent.ExecutionException;
import org.junit.Assert;
import org.junit.Before;
import org.junit.Rule;
Expand Down Expand Up @@ -71,10 +72,12 @@ public void setup() throws IOException {
@Test
public void testCall_validDocker()
throws URISyntaxException, LayerCountMismatchException,
BadContainerConfigurationFormatException, IOException, CacheCorruptedException {
BadContainerConfigurationFormatException, IOException, ExecutionException,
InterruptedException {
Path dockerBuild = getResource("core/extraction/docker-save.tar");
LocalImage result =
new ExtractTarStep(
MoreExecutors.newDirectExecutorService(),
buildConfiguration,
dockerBuild,
progressEventDispatcherFactory,
Expand All @@ -101,10 +104,12 @@ public void testCall_validDocker()
@Test
public void testCall_validTar()
throws URISyntaxException, LayerCountMismatchException,
BadContainerConfigurationFormatException, IOException, CacheCorruptedException {
BadContainerConfigurationFormatException, IOException, ExecutionException,
InterruptedException {
Path tarBuild = getResource("core/extraction/jib-image.tar");
LocalImage result =
new ExtractTarStep(
MoreExecutors.newDirectExecutorService(),
buildConfiguration,
tarBuild,
progressEventDispatcherFactory,
Expand Down
2 changes: 2 additions & 0 deletions jib-gradle-plugin/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,8 @@ All notable changes to this project will be documented in this file.

### Changed

- Local base image layers are now processed in parallel, speeding up builds using large local base images. ([#1913](https:/GoogleContainerTools/jib/issues/1913))

### Fixed

- Fixed temporary directory cleanup during builds using local base images. ([#2016](https:/GoogleContainerTools/jib/issues/2016))
Expand Down
2 changes: 2 additions & 0 deletions jib-maven-plugin/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,8 @@ All notable changes to this project will be documented in this file.

### Changed

- Local base image layers are now processed in parallel, speeding up builds using large local base images. ([#1913](https:/GoogleContainerTools/jib/issues/1913))

### Fixed

- Fixed temporary directory cleanup during builds using local base images. ([#2016](https:/GoogleContainerTools/jib/issues/2016))
Expand Down