build/strategy: wire in blob mount-from functionality #21613

runcom · 2018-12-04T21:08:06Z

@mrunalp @mtrmac @bparees @smarterclayton

This is just a wip to test things out as we need the resulting hypershift image from this PR to substitute the original one in a cluster and test this workflow, so, do not merge

Signed-off-by: Antonio Murdaca runcom@linux.com

pkg/build/controller/strategy/docker.go

pkg/build/controller/strategy/sti.go

bparees · 2018-12-04T21:15:03Z

pkg/build/controller/strategy/docker.go

+						{
+							Name:      "buildcachedir",
+							MountPath: "/var/lib/containers/cache/",
+						},


gonna need to fix unit tests:
https://github.com/openshift/origin/blob/master/pkg/build/controller/strategy/sti_test.go#L105-L134
https://github.com/openshift/origin/blob/master/pkg/build/controller/strategy/docker_test.go#L65-L97

bparees · 2018-12-04T21:15:36Z

pkg/build/controller/strategy/docker.go

 					},
 				},
+				{Name: "buildcachedir", VolumeSource: v1.VolumeSource{
+					HostPath: &v1.HostPathVolumeSource{Path: "/miloslavcache/"},


this needs to exist on the host, right? or will k8s create it?

cri-o/docker should create it if it doesn't exist on mount time right? or does kube check that?

i don't know, easy enough to test (create a pod that mounts a non-existent hostdir)

We will need to check if we can create this directory on RHCOS.

sh-4.2# id uid=0(root) gid=0(root) groups=0(root) context=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 sh-4.2# sh-4.2# sh-4.2# id uid=0(root) gid=0(root) groups=0(root) context=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 sh-4.2# pwd / sh-4.2# mkdir miloslavcache mkdir: cannot create directory ‘miloslavcache’: Permission denied sh-4.2# mkdir mycache mkdir: cannot create directory ‘mycache’: Permission denied

I was able to create /var/lib/containers/.cache

bparees · 2018-12-04T21:16:00Z

pkg/build/controller/strategy/docker.go

+				{
+					Name:      "buildcachedir",
+					MountPath: "/var/lib/containers/cache/",
+				},


you don't need this in the git-clone container.

bparees · 2018-12-04T21:16:16Z

pkg/build/controller/strategy/docker.go

+				{
+					Name:      "buildcachedir",
+					MountPath: "/var/lib/containers/cache/",
+				},


you don't need this in the manage-dockerfile container.

bparees · 2018-12-04T21:16:37Z

pkg/build/controller/strategy/sti.go

+				{
+					Name:      "buildcachedir",
+					MountPath: "/var/lib/containers/cache/",
+				},


bparees · 2018-12-04T21:17:32Z

pkg/build/controller/strategy/sti.go

+				{
+					Name:      "buildcachedir",
+					MountPath: "/var/lib/containers/cache/",
+				},


this one is needed (we will pull images from within this container). And docker.go should have it too.

assuming you care about this cache for pulling images (not pushing them).

bparees · 2018-12-04T21:17:42Z

pkg/build/controller/strategy/sti.go

+				{
+					Name:      "buildcachedir",
+					MountPath: "/var/lib/containers/cache/",
+				},


not needed.

bparees · 2018-12-04T21:18:50Z

pkg/build/controller/strategy/docker.go

+				{
+					Name:      "buildcachedir",
+					MountPath: "/var/lib/containers/cache/",
+				},


this one is correct/needed.

smarterclayton · 2018-12-05T03:51:14Z

We’re going back to a shared cache? Doesn’t that cance the security benefits we got?

Wasn’t clear to me why shared cache is required for mount from - mount from should work even for a local user?

bparees · 2018-12-05T04:10:16Z

We’re going back to a shared cache? Doesn’t that cance the security benefits we got?

as @mtrmac explained it to me, it's not a blob cache, it's just metadata/ids about the blobs.

Wasn’t clear to me why shared cache is required for mount from - mount from should work even for a local user?

the issue here is subsequent builds. We need some minimal cached information so a subsequent push can benefit from previous pushes that have happened.

@mtrmac gave me a nice explanation but that's the extent of what i understood of it. Maybe he can put it in writing here to help.

mtrmac · 2018-12-05T13:33:12Z

We’re going back to a shared cache?

Wasn’t clear to me why shared cache is required for mount from - mount from should work even for a local user?

(Mounting itself does not require extra privileges.)

This is just a metadata cache.

For builds that have a base image pulled from the same registry that the build results are pushed to (e.g. if both the base image and the build result are stored in the internal registry), we don’t need a long-term metadata cache: we just need the information from the pull to be available to the push, and that will allow mounting the layers from the base image repo to the build result image repo.

We do need a long-term metadata cache if the base image is located in a different registry than we are pushing the build result to, e.g. if the base image is located at registry.access.redhat.com or the Docker Hub, and the destination is the internal registry (I was told this is very common.)

In that case, during the very first build, we necessarily pull all the layers of the base image from the external registry, and push all of them again (+ the new ones created during the build) to the internal one; that is unavoidable. On subsequent builds, though, we can avoid re-pushing the base layers to the internal registry again, if:

We have a record of the compressed digest used when pushing the layer to the internal registry (which is not necessarily the same as the digest used on the external registry, if the compression implementation differs); this needs to be recorded from the previous build (re-computing the value by compressing the layers would negate most of the benefit, because the compression is usually what takes the majority of the time during pushes).
For builds that share the base image with the previous build, but use a different destination repo, we are aware of the repo used for the initial build, so that we can mount from it. This also needs to be recorded from the previous build.

(Reordered)

Doesn’t that cance the security benefits we got?

Yes, the shared state does add a bit of risk, if the code running within one build somehow broke out (AFAICS the build commands are isolated from the builder in a separate OCI/runc context) and were able to write to the long-term cache: it could cause subsequent builds to incorrectly substitute malicious blobs in build results.

With schema2 / OCI images, this would only be a DoS because the image configuration contains DiffID (uncompressed digest) values, and is not affected by the blob substitution: so, the substituted blobs would be downloaded and rejected as non-matching (both by Docker and CRI-O), making the corrupted build result unusable.

With schema1 (e.g. when pushing to a schema1-only registry), the substitutions would not be noticed, though.

runcom · 2019-01-05T11:47:33Z

/retest

bparees · 2019-01-05T16:31:08Z

looks like you broke the formatting:

[WARNING] !!! gofmt needs to be run on the listed files
./pkg/build/controller/strategy/docker.go
./pkg/build/controller/strategy/sti.go

Looks like there are also a few more places where you've changed the expected number of volumes. (unit test failures)

otherwise it lgtm.

bparees · 2019-01-08T18:44:31Z

/lgtm
/hold cancel

Signed-off-by: Antonio Murdaca <runcom@linux.com>

bparees · 2019-01-08T18:53:12Z

/lgtm

openshift-ci-robot · 2019-01-08T18:53:36Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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:

~~pkg/build/OWNERS~~ [bparees]

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

mrunalp · 2019-01-08T20:31:28Z

/retest

openshift-bot · 2019-01-08T21:37:40Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2019-01-08T23:40:28Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2019-01-09T01:39:02Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2019-01-09T05:41:06Z

/retest

Please review the full test history for this PR and help us cut down flakes.

runcom · 2019-01-09T10:35:03Z

/retest

openshift-bot · 2019-01-09T11:44:28Z

/retest

Please review the full test history for this PR and help us cut down flakes.

mrunalp · 2019-01-09T17:12:26Z

/test unit

mrunalp · 2019-01-09T17:41:29Z

/test e2e-aws

mrunalp · 2019-01-09T17:41:39Z

/test e2e-aws-serial

openshift-bot · 2019-01-09T19:47:29Z

/retest

Please review the full test history for this PR and help us cut down flakes.

runcom · 2019-01-09T22:41:28Z

/retest

mrunalp · 2019-01-10T01:04:44Z

/retest

openshift-bot · 2019-01-10T05:52:55Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2019-01-10T07:53:00Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2019-01-10T09:54:05Z

/retest

Please review the full test history for this PR and help us cut down flakes.

runcom · 2019-01-10T12:16:04Z

this is green now 🤔

openshift-ci-robot · 2019-01-10T15:08:28Z

@runcom: The following test failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
ci/prow/e2e-aws-builds	`36017bf`	link	`/test e2e-aws-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.

openshift-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Dec 4, 2018

openshift-ci-robot requested review from bparees and mfojtik December 4, 2018 21:08

mtrmac reviewed Dec 4, 2018

View reviewed changes

pkg/build/controller/strategy/docker.go Outdated Show resolved Hide resolved

pkg/build/controller/strategy/sti.go Outdated Show resolved Hide resolved

bparees reviewed Dec 4, 2018

View reviewed changes

pkg/build/controller/strategy/sti.go Outdated

{

Name: "buildcachedir",

MountPath: "/var/lib/containers/cache/",

},

Copy link

Contributor

bparees Dec 4, 2018

Choose a reason for hiding this comment

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

not needed

bparees reviewed Dec 4, 2018

View reviewed changes

pkg/build/controller/strategy/sti.go Outdated

{

Name: "buildcachedir",

MountPath: "/var/lib/containers/cache/",

},

Copy link

Contributor

bparees Dec 4, 2018

Choose a reason for hiding this comment

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

not needed.

bparees reviewed Dec 4, 2018

View reviewed changes

runcom force-pushed the blob-mount-from-cache branch 4 times, most recently from ed07245 to 1f00740 Compare December 5, 2018 02:42

runcom mentioned this pull request Dec 19, 2018

Enable blob caching and blobInfo caching openshift/builder#34

Merged

runcom force-pushed the blob-mount-from-cache branch from 1f00740 to 4a3c9bf Compare December 19, 2018 11:15

runcom force-pushed the blob-mount-from-cache branch 2 times, most recently from bbaeb4c to aa3139d Compare January 5, 2019 11:41

runcom force-pushed the blob-mount-from-cache branch from aa3139d to 541f2f4 Compare January 5, 2019 14:28

runcom force-pushed the blob-mount-from-cache branch from 541f2f4 to b94a8c8 Compare January 5, 2019 23:41

runcom changed the title ~~WIP: wire in blob mount-from functionality~~ wire in blob mount-from functionality Jan 5, 2019

openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jan 5, 2019

runcom force-pushed the blob-mount-from-cache branch from 377f127 to 5cc1bac Compare January 8, 2019 18:32

openshift-ci-robot added lgtm Indicates that a PR is ready to be merged. and removed do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels Jan 8, 2019

build: strategy: add blob mount-from cache dir to docker and sti

36017bf

Signed-off-by: Antonio Murdaca <runcom@linux.com>

runcom force-pushed the blob-mount-from-cache branch from 5cc1bac to 36017bf Compare January 8, 2019 18:52

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

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

openshift-merge-robot merged commit b4b6535 into openshift:master Jan 10, 2019

runcom deleted the blob-mount-from-cache branch January 10, 2019 14:21

build/strategy: wire in blob mount-from functionality #21613

build/strategy: wire in blob mount-from functionality #21613

Uh oh!

Conversation

runcom commented Dec 4, 2018

Uh oh!

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!

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!

smarterclayton commented Dec 5, 2018

Uh oh!

bparees commented Dec 5, 2018

Uh oh!

mtrmac commented Dec 5, 2018

Uh oh!

runcom commented Jan 5, 2019

Uh oh!

bparees commented Jan 5, 2019

Uh oh!

bparees commented Jan 8, 2019

Uh oh!

bparees commented Jan 8, 2019

Uh oh!

openshift-ci-robot commented Jan 8, 2019

Uh oh!

mrunalp commented Jan 8, 2019

Uh oh!

openshift-bot commented Jan 8, 2019

Uh oh!

openshift-bot commented Jan 8, 2019

Uh oh!

openshift-bot commented Jan 9, 2019

Uh oh!

openshift-bot commented Jan 9, 2019

Uh oh!

runcom commented Jan 9, 2019

Uh oh!

openshift-bot commented Jan 9, 2019

Uh oh!

mrunalp commented Jan 9, 2019

Uh oh!

mrunalp commented Jan 9, 2019

Uh oh!

mrunalp commented Jan 9, 2019

Uh oh!

openshift-bot commented Jan 9, 2019

Uh oh!

runcom commented Jan 9, 2019

Uh oh!

mrunalp commented Jan 10, 2019

Uh oh!

openshift-bot commented Jan 10, 2019

Uh oh!