enable build pods to tolerate running on crio while talking to docker #16491

bparees · 2017-09-21T19:44:49Z

No description provided.

openshift-merge-robot · 2017-09-21T19:45:13Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bparees

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

Needs approval from an approver in each of these OWNERS Files:

~~OWNERS~~ [bparees]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

bparees · 2017-09-21T19:51:32Z

@mrunalp @openshift/devex ptal

the s2i vendor changes will be replaced with a bump commit shortly.

smarterclayton · 2017-09-21T21:10:46Z

pkg/build/builder/util.go

+func readResolvConfHostPath() (string, error) {
+	// find the /etc/resolv.conf host path based on what is mounted into this
+	// container.
+	resolvConf, err := os.Open("/proc/1/mountinfo")


This is going to fail when we turn on shared pid namespace, isn't it?

no idea. will containers no longer have their own /proc filesystem with their own pid 1 at that point?

Yeah, this won't work with pid namespace sharing. Cleaner way is using crio inspect endpoint but hopefully, we can move away from docker before we have to do that.

smarterclayton · 2017-09-21T21:11:12Z

pkg/build/builder/util.go

+// readPid determines the actual host pid of the pid 1 process in this container
+func readPid() (string, error) {
+	// get pid from /proc/1/sched , e.g.: "java (8151, #threads: 53)"
+	pidFile, err := os.Open("/proc/1/sched")


Wow... that's hacky.

and, let me emphasize, temporary until we move to pure crio support, however long that takes.

Cleaner way for getting pid as well as resolv path is from crio inspect endpoint but we didn't want that.. and yes this doesn't work with pid namespaces but hopefully we can migrate away from docker for builds by then or switch to using crio inspect endpoint.

smarterclayton · 2017-09-21T21:12:04Z

pkg/build/builder/util.go

+// readResolveConfHostPath determines the path to the resolv.conf file for this
+// container, as it exists on the host machine.  (/etc/resolv.conf is mounted
+// into the container from the host path).
+func readResolvConfHostPath() (string, error) {


This stuff needs to go into its own package. It's getting pretty ugly in here. These aren't build utils, they're build environment / os coupling.

do you really want to encourage other people to consume this stuff? again it should all go away in the future.

smarterclayton · 2017-09-21T21:12:34Z

pkg/build/builder/docker.go

@@ -305,7 +310,8 @@ func (d *DockerBuilder) dockerBuild(dir string, tag string, secrets []buildapi.S
 		NoCache:        noCache,
 		Pull:           forcePull,
 		BuildArgs:      buildArgs,
-		NetworkMode:    string(getDockerNetworkMode()),
+		NetworkMode:    network,
+		BuildBinds:     fmt.Sprintf("[\"%s:/etc/resolv.conf\"]", resolvConfHostPath),


If I can inject : into the resolve conf host path, can I break this?

it'll break, i don't know that it'll get you anywhere.

if you can inject some quotes and a comma along with the colon you could presumably do pretty interesting things in terms of bindmounting host content into your build container.

I'm not sure how a user would have any control over that path though.

Regardless, we can certainly audit/strip those characters from the resolvConfHostPath.

smarterclayton · 2017-09-21T21:13:00Z

pkg/build/builder/docker.go

@@ -297,6 +297,11 @@ func (d *DockerBuilder) dockerBuild(dir string, tag string, secrets []buildapi.S
 		return err
 	}

+	network, resolvConfHostPath, err := getContainerNetworkMode()


It would be a lot better if this is an interface passed into docker builder, and none of this code had any idea of what this stuff was.

what exactly are you proposing be an interface? A ContainerNetworkModeGetter interface?

bparees · 2017-09-21T22:09:19Z

/retest

bparees · 2017-09-22T00:01:41Z

/retest

runcom · 2017-09-22T10:15:37Z

/test crio

bparees · 2017-09-22T22:16:57Z

/retest

runcom · 2017-09-25T10:39:45Z

/test crio

mrunalp · 2017-09-27T20:29:29Z

pkg/build/controller/strategy/sti.go

@@ -178,6 +178,7 @@ func (bs *SourceBuildStrategy) CreateBuildPod(build *buildapi.Build) (*v1.Pod, e

 	setOwnerReference(pod, build)
 	setupDockerSocket(pod)
+	setupCrioSocket(pod)


Should there be a check to add this only if the socket source path exists?

I didn't see a good way to do that(after all this logic is running in a controller that doesn't know anything about the target node for the build pod) and it doesn't do any harm to do the mount even if the source path doesn't exist.

So, the controller doesn't add the mount to the pod if it can't find the source? If the mount is passed down to the runc config.json, it will fail to start the container if it can't find the source.

the controller adds the mount. if runc is the container engine, then the source will be there.

and docker seemingly does not care if it does not exist, because this is working when i run using docker as the container engine.

…0682a92635ba9f7

bparees · 2017-09-27T21:17:45Z

@mrunalp comments addressed

mrunalp · 2017-09-27T21:39:48Z

LGTM

bparees · 2017-09-27T21:48:11Z

(3) [online-crio] Build toleration for CRI-O/Docker hybrid mode

bparees · 2017-09-27T22:46:08Z

/retest

openshift-ci-robot · 2017-09-28T01:33:54Z

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

Test name	Commit	Details	Rerun command
ci/openshift-jenkins/extended_conformance_crio	`d94ceaf`	link	`/test crio`

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.

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.

bparees · 2017-09-28T03:14:00Z

/retest

openshift-merge-robot · 2017-09-28T07:42:50Z

Automatic merge from submit-queue

smarterclayton · 2018-02-24T21:50:46Z

I can't find any reference to buildbinds in projectatomic/docker - where is the code that actually implements that located?

smarterclayton · 2018-02-24T21:52:20Z

Also, why did we expand the docker api?

bparees · 2018-02-25T20:32:21Z

docker build -v provides this on the cli, I don't remember what the issue was w/ the docker api itself not exposing it (or how docker build handles it if the api doesn't provide it)

smarterclayton · 2018-02-26T04:29:51Z

It does not. Not on regular docker.

…

On Sun, Feb 25, 2018 at 3:32 PM, Ben Parees ***@***.***> wrote: docker build -v provides this on the cli, I don't remember what the issue was w/ the docker api itself not exposing it (or how docker build handles it if the api doesn't provide it) — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#16491 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABG_p5sPcab3cagZ9Ng6Prqu9E1xDMEmks5tYcNXgaJpZM4Pf0g_> .

bparees · 2018-02-26T04:30:53Z

It does not. Not on regular docker.

you mean "docker build -v" isn't even an option in regular docker?

smarterclayton · 2018-02-26T05:49:41Z

Correct

…

On Sun, Feb 25, 2018 at 11:30 PM, Ben Parees ***@***.***> wrote: It does not. Not on regular docker. you mean "docker build -v" isn't even an option in regular docker? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#16491 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABG_p5jkqjvPgN6TcbdqE7iayJHvGRRNks5tYjN_gaJpZM4Pf0g_> .

runcom · 2018-02-26T08:26:01Z

It's not indeed, we've added that long time ago @rhatdan

bparees · 2018-02-26T16:53:00Z

guessing it got added as a "might as well" when the logic was being added to bindmount subscription secrets into the build container.

smarterclayton · 2018-02-27T03:01:26Z

Where's the implementation for this? I can't find it in projectatomic/docker for some reason.

runcom · 2018-02-27T08:37:04Z

It's in branches 1.12.6, 1.13.1

smarterclayton · 2018-02-27T15:45:26Z

I see ImageBuildOptions having "binds". It doesn't have "buildbinds".

…

On Tue, Feb 27, 2018 at 3:37 AM, Antonio Murdaca ***@***.***> wrote: It's in branches 1.12.6, 1.13.1 — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#16491 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABG_pw-cxR3mbNRo0ThAqLDY1LgSRhx_ks5tY76ygaJpZM4Pf0g_> .

runcom · 2018-02-27T16:45:41Z

"buildbinds" comes from the patched library, I think it's just the struct in the go client library, not in projectatomic/docker, That is translated to json as "buildbinds"

api/server/router/build/build_routes.go
97:     var buildBinds = []string{}
98:     buildBindsJSON := r.FormValue("buildbinds")
99:     if buildBindsJSON != "" {
100:            if err := json.NewDecoder(strings.NewReader(buildBindsJSON)).Decode(&buildBinds); err != nil {
103:            options.Binds = buildBinds

openshift-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Sep 21, 2017

openshift-merge-robot assigned soltysh and smarterclayton Sep 21, 2017

openshift-merge-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. needs-api-review labels Sep 21, 2017

bparees force-pushed the crio_networking branch from 15bee68 to 9eb991d Compare September 21, 2017 19:50

bparees mentioned this pull request Sep 21, 2017

CRI-O integration [9 failures] #16403

Closed

smarterclayton reviewed Sep 21, 2017

View reviewed changes

bparees force-pushed the crio_networking branch 2 times, most recently from 1199dd6 to 57b6f4f Compare September 21, 2017 21:42

bparees force-pushed the crio_networking branch from 57b6f4f to 5e88801 Compare September 22, 2017 03:56

bparees force-pushed the crio_networking branch 2 times, most recently from 7aa8ccd to 902e5ec Compare September 22, 2017 19:30

bparees changed the title ~~enable build pods to tolerate running on crio while talking to docker~~ [WIP] enable build pods to tolerate running on crio while talking to docker Sep 23, 2017

openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 23, 2017

bparees force-pushed the crio_networking branch from 999ac00 to c52a663 Compare September 24, 2017 01:34

openshift-merge-robot removed the needs-api-review label Sep 24, 2017

bparees force-pushed the crio_networking branch from c43829a to c8f320a Compare September 26, 2017 17:40

bparees changed the title ~~[WIP] enable build pods to tolerate running on crio while talking to docker~~ enable build pods to tolerate running on crio while talking to docker Sep 26, 2017

mrunalp reviewed Sep 27, 2017

View reviewed changes

bparees force-pushed the crio_networking branch from 5d6d55c to 33e17c1 Compare September 27, 2017 20:38

bparees added 2 commits September 27, 2017 16:42

bump(github.com/kubernetes-incubator/crio): a8ee86b1cce0c13bd541a9914…

c2d44ec

…0682a92635ba9f7

setup crio networking for build containers

57a4f0d

bparees force-pushed the crio_networking branch from 33e17c1 to 57a4f0d Compare September 27, 2017 20:42

bparees added the lgtm Indicates that a PR is ready to be merged. label Sep 27, 2017

openshift-merge-robot removed the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 28, 2017

bparees added approved Indicates a PR has been approved by an approver from all required OWNERS files. kind/bug Categorizes issue or PR as related to a bug. labels Sep 28, 2017

openshift-merge-robot merged commit a8ac70a into openshift:master Sep 28, 2017

bparees deleted the crio_networking branch September 29, 2017 17:11

enable build pods to tolerate running on crio while talking to docker #16491

enable build pods to tolerate running on crio while talking to docker #16491

Conversation

bparees commented Sep 21, 2017

openshift-merge-robot commented Sep 21, 2017

bparees commented Sep 21, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bparees commented Sep 21, 2017

bparees commented Sep 22, 2017

runcom commented Sep 22, 2017

bparees commented Sep 22, 2017

runcom commented Sep 25, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bparees commented Sep 27, 2017

mrunalp commented Sep 27, 2017

bparees commented Sep 27, 2017

bparees commented Sep 27, 2017

openshift-ci-robot commented Sep 28, 2017 • edited Loading

bparees commented Sep 28, 2017

openshift-merge-robot commented Sep 28, 2017

smarterclayton commented Feb 24, 2018

smarterclayton commented Feb 24, 2018

bparees commented Feb 25, 2018

smarterclayton commented Feb 26, 2018 via email

bparees commented Feb 26, 2018

smarterclayton commented Feb 26, 2018 via email

runcom commented Feb 26, 2018

bparees commented Feb 26, 2018

smarterclayton commented Feb 27, 2018

runcom commented Feb 27, 2018

smarterclayton commented Feb 27, 2018 via email

runcom commented Feb 27, 2018

openshift-ci-robot commented Sep 28, 2017 •

edited

Loading