Extended builds #9813

PI-Victor · 2016-07-12T23:08:33Z

OLD PR: #9596 (had to close it)

Trello: https://trello.com/c/MWUAjl6k/237-13-extended-builds-separate-mvn-vs-jboss-evg
This is dependent on: openshift/source-to-image#505
To make this work, in your bc

   strategy:
      sourceStrategy:
        env:
        - name: EXAMPLE
          value: sample-app
        from:
          kind: ImageStreamTag
          name: wildfly:latest
        runtimeArtifacts:
        - sourcePath: /path/to/1
          destinationDir: path/to/1
        - sourcePath: /path/to/2
          destinationDir: path/to/2
        - sourcePath: /path/to/3
          destinationDir: path/to/3
        - sourcePath: /path/to/4
          destinationDir: path/to/4
        runtimeImage:
          kind: ImageStreamTag
          name: openjdk:latest

cc: @php-coder @bparees
NOTE: sourcePath needs to be a full path insider your builder image that points to the file/dir that you want to be copied, while destinationDir is the path relative to your runtimeImage WORKDIR where your files will be copied

NOTE:
In order to run this, you need to update your s2i builder image with the current compiled binary.

either using bindmountproxy this works with oc cluster up instructions are in the repo.

or by using openshift-devtools

extras/rebuild-s2i-builder

this will rebuild the s2i-builder image and copy your openshift locally compiled binary inside it (you need to run openshift with --latest-images for openshift to use that rebuilt image)

bparees · 2016-07-14T05:31:29Z

pkg/build/api/v1/types.go

+	// runtimePullSecret is the name of a Secret that would be used for setting up
+	// the authentication for pulling the Docker runtime images from private
+	// Docker registries.
+	RuntimePullSecret *kapi.LocalObjectReference `json:"runtimePullSecret,omitempty"`


i think we decided we don't need this since you can have multiple auths in the existing pullsecret, right?

errr... i might have missed that discussion, but if so i will remove it

well as it is i don't think your code is actually using it, either.

see build/controller/strategy/util.go:setupDockerSecrets() to see how user-provided secrets get setup as mount points and an env variable gets defined that points to the mount point, and then later we reference the env variable to find the mount point and locate the secret to do the actual pull.

ah, you meant this conversation #9596 (comment)
sorry i didn't get it from your comment that we don't want to use it, my bad. i see now what you mean.

the discussion definitely wasn't conclusive, but i'm going to propose we go with the single field. note that as @liggitt pointed out, this means we need to be a little more sophisticated in the logic that picks the secret in the first place, since we need to find a secret that matches both image pullspecs.

bparees · 2016-07-14T05:40:06Z

these commits look mixed up, the bump commit is not just a bump.

PI-Victor · 2016-07-14T07:32:29Z

these commits look mixed up, the bump commit is not just a bump.

@bparees thanks for catching that, i think i squashed wrongly or something, will fix.

bparees · 2016-07-14T14:13:08Z

i realize it's a work in progress but just a heads up that i'm going to want to see extended tests for this.

php-coder · 2016-07-14T16:02:08Z

pkg/build/builder/sti.go

@@ -105,6 +105,9 @@ func (s *S2IBuilder) Build() error {
 	if runtimeImage != nil {
 		runtimeImageName = s.build.Spec.Strategy.SourceStrategy.RuntimeImage.Name
 		runtimeDockerAuth, _ = dockercfg.NewHelper().GetDockerAuth(runtimeImageName, dockercfg.PullAuthType)
+		// FIXME: do we need to explicitly turn off the incremental flag or make it
+		// part of the validation? or do we need to touch it at all?
+		s.build.Spec.Strategy.SourceStrategy.Incremental = false


As far I understand it should be a part of validation logic. (S2I also checks this case but only when it's executed as a binary.)

+1, should be part of the validation.

PI-Victor · 2016-07-19T13:09:14Z

api/swagger-spec/oapi-v1.json

@@ -20355,17 +20373,6 @@
     "forcePull": {
      "type": "boolean",
      "description": "forcePull describes if the builder should pull the images from registry prior to building."
-     },
-     "runtimeImage": {
-      "$ref": "v1.ObjectReference",


@php-coder don't forget when you have time to add this back so i don't fetch this 👯

Thanks, I've just pushed with -f to fix it.

php-coder · 2016-07-20T16:02:35Z

pkg/build/builder/cmd/dockercfg/cfg.go

+func (h *Helper) GetDockerAuth(imageName, runtimeImageName, authType string) (docker.AuthConfiguration, bool) {
+	if runtimeImageName != "" {
+		glog.V(3).Infof("Runtime image detected %s", runtimeImageName)
+	}


@PI-Victor Why do we need to add this argument? I see that we're just printing it.

@php-coder i don't think this will stay, just wanted to get some input about it.
maybe the user wants to know that we detected a runtime image?

But maybe we could put it inside of if s.build.Spec.Strategy.SourceStrategy.RuntimeImage != nil { ? And I see that the similar message already here, so I'm not sure should we show to a user runtime image detected message twice.

ok, fair point.

PI-Victor · 2016-07-25T13:23:23Z

i'm having trouble understanding what should happen if we don't find the runtime docker credentials (dockerAuth). first thing that comes to mind is error out, but the getdockerauth helper function is not thought of in such a way.
on which image should we run the post commit hook, my first thought is the builder image and not the runtime image, since the builder image contains the artifacts, however, the post commit hook is executed in an ephemeral container, so nothing makes it in the runtime image. (i think i answered my own question, but feedback is greatly appreciated)
if the tests in the post commit hook fail, then what? we don't push the runtime image to the registry? and what about the builder image. because right now it just keeps adding on top of that with every build.
@bparees @php-coder

bparees · 2016-07-25T13:31:12Z

if we don't find an auth associated w/ the docker repo in question, we just attempt to pull w/ no creds. So there is no immediate error, there may be an error when we go to pull the image if the repo requires creds.

As for the image to run tests in, that is a good question. Logically they should be run in the runtime image since that is the image that "matters", but from a technical perspective it's very likely the runtime image doesn't have the tools (eg rake) needed to run the tests. I guess we stand by the logic that they can install whatever tools they need to run the tests, as part of the test script. So runtime image. But I'd be interested in additional opinions from @smarterclayton.

smarterclayton · 2016-07-25T17:05:31Z

As per discussion with ben this footprint is acceptable for now.

php-coder · 2016-07-26T12:07:02Z

pkg/build/builder/sti.go

+		config.RuntimeAuthentication = dockerAuth
+		config.PullAuthentication = dockerAuth
+		config.RuntimeArtifacts = copyToVolumeList(s.build.Spec.Strategy.SourceStrategy.RuntimeArtifacts)
+		glog.V(0).Infof("Runtime Image detected: pushTag: %#v, buildTag: %#v, dockerAuth: %#v", pushTag, buildTag, dockerAuth)


@PI-Victor @bparees

I have questions:

should we log it as V(0)?

should we log auth info? Is this information sensitive?

Right now I see in the console message like this:

Runtime Image detected: pushTag: "java-extended-build-from-repo-1", buildTag: "extended-test-extended-build-p2s69-4kb13/java-extended-build-from-repo-1:e3cd7fa9", dockerAuth: docker.AuthConfiguration{Username:"", Password:"", Email:"", ServerAddress:""}

no, you're right. it's ok, that was just for debugging. i'll push an update

PI-Victor · 2016-07-26T12:29:55Z

@bparees i assume, since we keep an eye out only for the image that gets pushed to the registry, (runtime image in this case) that imagechange triggers will trigger a new build only in the case where the runtime image gets updated and not the builder. since we treat the builder as a discardable image?

PI-Victor · 2016-07-26T12:44:18Z

p.s.: still waiting on a small comment regarding the postcommit hook?

bparees · 2016-07-26T13:01:51Z

@bparees i assume, since we keep an eye out only for the image that gets pushed to the registry, (runtime image in this case) that imagechange triggers will trigger a new build only in the case where the runtime image gets updated and not the builder. since we treat the builder as a discardable image?

You can add imagechangetriggers on any image you want, so you can have an imagechangetrigger on both images. The real question is what the "default" imagechangetrigger (the one where you don't specify the image explicitly) should be looking at, and I think it should be looking at the builder image, not the runtime image. What is it doing currently?

PI-Victor · 2016-07-26T16:25:33Z

@bparees i hate to ask this but can you give me some pointers where to find the monitoring logic for imagechange triggers. i assume it's a controller of some sort, but i can't find it.
and also object validation logic?

PI-Victor · 2016-07-26T16:25:50Z

@php-coder maybe you know? ^^

PI-Victor · 2016-07-26T16:29:37Z

nvm about the imagechange controller. found it.

PI-Victor · 2016-07-27T11:47:43Z

pkg/build/api/validation/validation.go

@@ -503,6 +503,7 @@ func validateSourceStrategy(strategy *buildapi.SourceBuildStrategy, fldPath *fie
 	allErrs = append(allErrs, validateFromImageReference(&strategy.From, fldPath.Child("from"))...)
 	allErrs = append(allErrs, validateSecretRef(strategy.PullSecret, fldPath.Child("pullSecret"))...)
 	allErrs = append(allErrs, ValidateStrategyEnv(strategy.Env, fldPath.Child("env"))...)
+	allErrs = append(allErrs, validateRuntimeImage(strategy, fldPath.Child("runtimeImage"))...)


@bparees the validation that i've added here applies to the the build object, so you cannot do oc create -f build.json with both incremental=true and a runtime image defined.
a bc that can test this is here https://gist.githubusercontent.com/PI-Victor/16f030f52ef6127149a8f1a12d7cc0b9/raw/a62bda283f6e6d0f84d7eb6ac8eee2fc968d2b31/gistfile1.txt

both validateBuild and validateBuildConfig run the validateCommonSpec which runs the below code, so it's covered.

PI-Victor · 2016-07-28T19:08:10Z

@bparees yes, thank you! that's what i'm seeing. (when i tested previously, i thought it was running on the builder image for some reason.)
gonna check/fix the round trip conversions and update the API docs.

bparees · 2016-07-28T19:23:36Z

test/extended/builds/s2i_extended_build.go

+			o.Expect(err).NotTo(o.HaveOccurred())
+
+			g.By("running the build")
+			err = oc.Run("start-build").Args(buildConfigName).Execute()


we should always record and print the output from start-build as demonstrated here:
https://github.com/openshift/origin/pull/9592/files#diff-f8c83618d04227c926ba1eed1330b6faR37

(same comment for the other start-build invocations)

Ok, I added. But I decided to output these logs only when start-build fails and not always as in the example.

i believe that output (in the example) only shows up if the test fails anyway, though i'm not certain.

bparees · 2016-07-28T20:36:37Z

@PI-Victor looks like you need to run hack/update-generated-protobuf.sh to fix the serialization test issue. That is a new step in our api update process.

PI-Victor · 2016-07-28T20:40:29Z

@bparees talk about syncs, i just finished compiling the objective-c protobuf bin and was generating them.

bparees · 2016-07-28T20:41:19Z

@PI-Victor ah, well then you don't need the PR I just opened :)

PI-Victor · 2016-07-28T20:46:12Z

@bparees it's more than welcome, thanks!

php-coder · 2016-07-29T11:20:35Z

Now it fails because of this flake: #9938

PI-Victor · 2016-07-29T14:08:13Z

re[test]

php-coder · 2016-07-29T20:37:30Z

Fails because of this:

---> Running in 391bd955abee
F0729 16:07:48.745444 1 helpers.go:108] error: build error: open /dev/mapper/docker-202:2-50512749-475af41a36b175505d6e542638ba684e5be8f10d2ea00d43d3468458bc6a36fc-init: no such file or directory

Don't know what does it mean and how to fix it...

re[test]

PI-Victor · 2016-07-29T21:29:56Z

@php-coder looks like a docker flake to me

PI-Victor · 2016-07-30T10:40:31Z

[test]

bparees · 2016-08-01T03:19:34Z

flake
#9959

and probably flake
#9548

[test]

bparees · 2016-08-01T03:20:38Z

(or maybe flake #9490)

PI-Victor · 2016-08-01T07:58:11Z

re[test] for the love of god.

php-coder · 2016-08-01T12:30:30Z

[testextended][extended:core(s2i extended build)]

stevekuznetsov · 2016-08-01T17:28:51Z

re[test]

bparees · 2016-08-01T20:20:47Z

[test]

bparees · 2016-08-02T15:42:52Z

[merge]

openshift-bot · 2016-08-02T17:12:42Z

Evaluated for origin test up to 5da464c

openshift-bot · 2016-08-02T17:18:39Z

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/7398/) (Image: devenv-rhel7_4730)

openshift-bot · 2016-08-02T17:18:41Z

Evaluated for origin merge up to 5da464c

openshift-bot · 2016-08-02T17:18:43Z

Evaluated for origin testextended up to 5da464c

openshift-bot · 2016-08-02T18:16:33Z

continuous-integration/openshift-jenkins/testextended SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin_extended/388/) (Extended Tests: core(s2i extended build))

openshift-bot · 2016-08-02T18:35:16Z

continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/7398/)

PI-Victor mentioned this pull request Jul 12, 2016

[WIP] Extended builds #9596

Closed

bparees added the needs-api-review label Jul 13, 2016

bparees reviewed Jul 14, 2016
View reviewed changes

php-coder reviewed Jul 14, 2016
View reviewed changes

openshift-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 19, 2016

PI-Victor reviewed Jul 19, 2016
View reviewed changes

openshift-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 20, 2016

php-coder mentioned this pull request Jul 20, 2016

bump(github.com/openshift/source-to-image): 9350cd1ef8afc9c91dcdc96e1d972cbcc6f36181 #9957

Merged

php-coder reviewed Jul 20, 2016
View reviewed changes

php-coder changed the title ~~[WIP] Extended builds and s2i bump~~ [WIP] Extended builds Jul 25, 2016

smarterclayton added api-approved and removed needs-api-review labels Jul 25, 2016

php-coder reviewed Jul 26, 2016
View reviewed changes

PI-Victor reviewed Jul 27, 2016
View reviewed changes

bparees reviewed Jul 28, 2016
View reviewed changes

add extended builds functionality

5da464c

openshift-bot merged commit fd653af into openshift:master Aug 2, 2016

php-coder mentioned this pull request Aug 8, 2016

Improve tests for extended build #10270

Merged

Extended builds #9813

Extended builds #9813

Conversation

PI-Victor commented Jul 12, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bparees Jul 14, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bparees commented Jul 14, 2016

PI-Victor commented Jul 14, 2016

bparees commented Jul 14, 2016

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

php-coder Jul 20, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PI-Victor commented Jul 25, 2016

bparees commented Jul 25, 2016

smarterclayton commented Jul 25, 2016

php-coder Jul 26, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PI-Victor commented Jul 26, 2016

PI-Victor commented Jul 26, 2016

bparees commented Jul 26, 2016

PI-Victor commented Jul 26, 2016 • edited Loading

PI-Victor commented Jul 26, 2016

PI-Victor commented Jul 26, 2016

PI-Victor Jul 27, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PI-Victor commented Jul 28, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

php-coder Jul 29, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bparees commented Jul 28, 2016

PI-Victor commented Jul 28, 2016

bparees commented Jul 28, 2016

PI-Victor commented Jul 28, 2016

php-coder commented Jul 29, 2016 • edited Loading

PI-Victor commented Jul 29, 2016

php-coder commented Jul 29, 2016 • edited Loading

PI-Victor commented Jul 29, 2016

PI-Victor commented Jul 30, 2016

bparees commented Aug 1, 2016

bparees commented Aug 1, 2016

PI-Victor commented Aug 1, 2016

php-coder commented Aug 1, 2016

stevekuznetsov commented Aug 1, 2016

bparees commented Aug 1, 2016

bparees commented Aug 2, 2016

openshift-bot commented Aug 2, 2016

openshift-bot commented Aug 2, 2016 • edited Loading

openshift-bot commented Aug 2, 2016

openshift-bot commented Aug 2, 2016

openshift-bot commented Aug 2, 2016

openshift-bot commented Aug 2, 2016

PI-Victor commented Jul 12, 2016 •

edited

Loading

bparees Jul 14, 2016 •

edited

Loading

php-coder Jul 20, 2016 •

edited

Loading

php-coder Jul 26, 2016 •

edited

Loading

PI-Victor commented Jul 26, 2016 •

edited

Loading

PI-Victor Jul 27, 2016 •

edited

Loading

php-coder Jul 29, 2016 •

edited

Loading

php-coder commented Jul 29, 2016 •

edited

Loading

php-coder commented Jul 29, 2016 •

edited

Loading

openshift-bot commented Aug 2, 2016 •

edited

Loading