Enable blob caching and blobInfo caching #34

nalind · 2018-12-12T16:09:32Z

This patch set updates the versions of the storage, image, and buildah libraries and enables blob and blobInfo caching. We switch back to the main containers/image repository, since it now supports registry names specified as IP addresses, and drop logic for setting the InsecureSkipTLSVerify flag, which the image library's registry client logic now sets automatically if we don't.

nalind · 2018-12-12T16:32:49Z

/test all

nalind · 2018-12-13T14:14:55Z

/retest

adambkaplan

A few questions (otherwise lgtm)

Dockerfile

adambkaplan · 2018-12-13T14:25:08Z

pkg/build/builder/cmd/builder.go

+		// By default, enable a blob cache, but allow it to be moved or
+		// disabled via the environment.
+		cfg.blobCache = "/var/cache/blobs"
+		if dir, isSet := os.LookupEnv("BUILD_BLOBCACHE"); isSet {


How will this be disabled - empty env valule?

please call this BUILD_ENABLE_BLOBCACHE or something that makes it clear it's an enablement (and then make it enable when the value is set to true (case insensitive))

Okay, changing it.

To be sure, this means we're going to need to update the build controller to enable the blob cache. By default, it isn't enabled, but we want to use it here. Blob info caching is enabled by default.

sorry I screwed this up. but i think BUILD_BLOBCACHE_DIR would be a better name.

(and ignore my comments about expecting a value of true)

adambkaplan · 2018-12-13T14:30:30Z

pkg/build/builder/daemonless.go

-	} else {
-		glog.V(2).Infof("Registry for %q is not present in the registries configuration, assuming it is secure.", imageName)
-	}
-


I assume the designation of secure/insecure registries will be taken care of by copying in /etc/containers/registries.conf?

See openshift/origin#21653

@nalind did something get fixed in buildah that we don't need this any more?

Yes, containers/image#468 was merged.

@adambkaplan Yup, the way the builder expects the configuration to be provided to it hasn't changed, we just don't need to do this bit of work ourselves any more.

pkg/build/builder/cmd/builder.go

nalind · 2018-12-14T16:36:01Z

Backing out the changes to pay attention to the ImageOptimizationPolicy, which can be done in a different PR after this one.

nalind · 2018-12-14T16:36:13Z

/retest

runcom · 2018-12-19T10:16:11Z

This still needs a corresponding origin PR for the controller (e.g. openshift/origin#21613)

bparees · 2019-01-02T16:04:56Z

/retest

runcom · 2019-01-05T11:51:10Z

picked up this PR to bring in containers/buildah#1244 for testing till it gets merged and I'm seeing errors coming from pkg/errors and raven-go. I've been able to overcome that by sticking the lib in glide with:

+- package: github.com/pkg/errors
+  version: 645ef00459ed84a119197bfb8d8205042c6df63d
+- package: github.com/getsentry/raven-go
+  version: 32a13797442ccb601b11761d74232773c1402d14

it is related to getsentry/raven-go#225

runcom · 2019-01-05T18:42:25Z

The parallel pull + pgzip PR in Buildah has been merged. We could grab it here, I'm already testing with it.

Signed-off-by: Nalin Dahyabhai <nalin@redhat.com>

Switch back to upstream containers/image, since it handles IP addresses as registries. Note that matching for entries in the insecure registries list does not treat entries with no port number part as matching any port, so it must often be specified. This version also adds blob info caching by default. Signed-off-by: Nalin Dahyabhai <nalin@redhat.com>

Signed-off-by: Nalin Dahyabhai <nalin@redhat.com>

The image library's registry client now handles setting the DockerInsecureSkipTLSVerify flag based on the registries configuration, so we no longer need to do so ourselves. Signed-off-by: Nalin Dahyabhai <nalin@redhat.com>

Signed-off-by: Nalin Dahyabhai <nalin@redhat.com>

Use /var/cache/blobs, by default, for caching blobs. Blob caching is useful for avoiding the work of recompressing layers that we inherit from a base image when we go to push the newly-built image. If $BUILD_BLOBCACHE_DIR is set, we its value as the location, instead of /var/cache/blobs. Setting the variable to an empty value will disable the cache. Signed-off-by: Nalin Dahyabhai <nalin@redhat.com>

openshift-ci-robot · 2019-01-08T16:55:25Z

@nalind: The following tests failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
ci/prow/e2e-gcp-image-ecosystem	`c7567ef`	link	`/test e2e-gcp-image-ecosystem`
ci/prow/e2e-gcp	`c7567ef`	link	`/test e2e-gcp`
ci/prow/e2e-gcp-builds	`c7567ef`	link	`/test e2e-gcp-builds`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

mrunalp · 2019-01-08T17:27:59Z

We need containers/buildah#1265 as well

mrunalp · 2019-01-08T17:38:18Z

/test e2e-aws

nalind · 2019-01-08T17:43:17Z

/test e2e-aws-builds

nalind · 2019-01-08T17:48:09Z

We need containers/buildah#1265 as well

Included as of #34 (comment).

Dockerfile

bparees · 2019-01-08T18:09:36Z

one nit on the dir name, otherwise lgtm pending passing tests.

pkg/build/builder/cmd/builder.go

bparees · 2019-01-08T18:57:19Z

/lgtm

openshift-ci-robot · 2019-01-08T19:02:00Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bparees, nalind

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [bparees]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

openshift-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Dec 12, 2018

openshift-ci-robot requested review from bparees and gabemontero December 12, 2018 16:09

nalind force-pushed the vendor-update branch 3 times, most recently from 00524e0 to 8db526a Compare December 12, 2018 16:32

nalind force-pushed the vendor-update branch from 8db526a to 199b5fb Compare December 12, 2018 20:13

adambkaplan reviewed Dec 13, 2018

View reviewed changes

bparees reviewed Dec 13, 2018

View reviewed changes

pkg/build/builder/cmd/builder.go Outdated Show resolved Hide resolved

bparees self-assigned this Dec 13, 2018

nalind force-pushed the vendor-update branch 3 times, most recently from b757d4d to c7567ef Compare December 14, 2018 16:35

nalind force-pushed the vendor-update branch from c7567ef to 4318ebe Compare January 7, 2019 16:18

nalind added 7 commits January 7, 2019 13:07

bump(github.com/containers/storage)

6a11dbe

Signed-off-by: Nalin Dahyabhai <nalin@redhat.com>

bump(github.com/containers/buildah)

bbab78b

Signed-off-by: Nalin Dahyabhai <nalin@redhat.com>

Update for containers/image API changes

fdc34e5

The image library's registry client now handles setting the DockerInsecureSkipTLSVerify flag based on the registries configuration, so we no longer need to do so ourselves. Signed-off-by: Nalin Dahyabhai <nalin@redhat.com>

bump(github.com/gorilla/mux)

94e4ded

Signed-off-by: Nalin Dahyabhai <nalin@redhat.com>

bump(github.com/opencontainers/selinux)

60fc859

Signed-off-by: Nalin Dahyabhai <nalin@redhat.com>

nalind force-pushed the vendor-update branch from 4318ebe to 1c03321 Compare January 8, 2019 15:01

bparees reviewed Jan 8, 2019

View reviewed changes

Dockerfile Show resolved Hide resolved

bparees reviewed Jan 8, 2019

View reviewed changes

pkg/build/builder/cmd/builder.go Show resolved Hide resolved

bparees reviewed Jan 8, 2019

View reviewed changes

pkg/build/builder/cmd/builder.go Show resolved Hide resolved

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jan 8, 2019

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 8, 2019

openshift-merge-robot merged commit 3dc11b0 into openshift:master Jan 8, 2019

nalind deleted the vendor-update branch January 8, 2019 19:16

Enable blob caching and blobInfo caching #34

Enable blob caching and blobInfo caching #34

Uh oh!

Conversation

nalind commented Dec 12, 2018

Uh oh!

nalind commented Dec 12, 2018

Uh oh!

nalind commented Dec 13, 2018

Uh oh!

adambkaplan left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

nalind commented Dec 14, 2018

Uh oh!

nalind commented Dec 14, 2018

Uh oh!

runcom commented Dec 19, 2018

Uh oh!

bparees commented Jan 2, 2019

Uh oh!

runcom commented Jan 5, 2019

Uh oh!

runcom commented Jan 5, 2019

Uh oh!

openshift-ci-robot commented Jan 8, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mrunalp commented Jan 8, 2019

Uh oh!

mrunalp commented Jan 8, 2019

Uh oh!

nalind commented Jan 8, 2019

Uh oh!

nalind commented Jan 8, 2019

Uh oh!

Uh oh!

bparees commented Jan 8, 2019

Uh oh!

Uh oh!

Uh oh!

bparees commented Jan 8, 2019

Uh oh!

openshift-ci-robot commented Jan 8, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

openshift-ci-robot commented Jan 8, 2019 •

edited

Loading