Run user-provided command as part of build flow #6715

rhcarvalho · 2016-01-19T14:18:59Z

This is meant to be used for running project unit tests as part of a build.

Trello card: https://trello.com/c/I1v1YeOf
Closes #6758

This PR/card won't introduce changes to oc new-build nor oc new-app nor the Web Console.
For now, the feature can be used via oc edit bc.

Pending work:

agreement on API changes
tests
clean up
maybe move some code to source-to-image
~~if build fails because of failure in the post commit hook, there should be a clear error message to reflect that (see pkg/build/builder/common.go#L56 and pkg/build/registry/build/strategy.go#L103)~~ trello: https://trello.com/c/nAPDRELK/852-update-build-status-when-post-commit-hook-fails

rhcarvalho · 2016-01-19T14:20:41Z

@bparees @PI-Victor PTAL

rhcarvalho · 2016-01-19T14:30:46Z

pkg/build/builder/common.go

+ cpuShares := r.read("/sys/fs/cgroup/cpu,cpuacct/cpu.shares")
+ cpuPeriod := r.read("/sys/fs/cgroup/cpu,cpuacct/cpu.cfs_period_us")
+ cpuQuota := r.read("/sys/fs/cgroup/cpu,cpuacct/cpu.cfs_quota_us")
+ memory := r.read("/sys/fs/cgroup/memory/memory.limit_in_bytes")


@bparees this is the part you will need in the card Enforce CGroup constraints on build containers.

Worth noting that we need better error handling / implementation here. E.g.:

the memory limit read from /sys/fs/cgroup might overflow int64. The Docker client uses int64, so we can't really send anything > 2^63-1.

~~these values might be incorrectly set or not available with older versions of the Docker daemon.~~ edit: we require Docker 1.8.2, which will place the files as expected.

bparees · 2016-01-19T19:22:20Z

pkg/build/builder/sti.go

 glog.Flush()
 }
 return nil
 }

+func (s *S2IBuilder) runPostCommitHook(imageID string) error {
+ glog.Infof("Running post commit hook with image %s ...", imageID)


also log the command being run.

also not sure the point of this function. whoever is calling it should just call runScriptInContainer directly. (and then it should do the glog.Info)

rhcarvalho · 2016-01-22T10:40:44Z

pkg/build/builder/sti.go

@@ -204,19 +215,42 @@ func (s *S2IBuilder) Build() error {
 return err
 }

+ // TODO: why in docker builds we defer removing the image, while here we don't?!


@bparees @mfojtik may I ask you why we don't defer removing the built image in Docker builds, while we do it in S2I?

Feature or bug?

probably a bug, I don't see any reason to keep the image cached on the node locally when the image is pushed to remote registry (other than a performance issue when running all-in-one ;-) @csrwng any ideas?

See my comment on the pull to remove. We stopped removing the image on purpose.

rhcarvalho · 2016-01-22T11:13:08Z

I'm working with multiple commits now, as it makes my life easier.
We can squash it down to one when we're done with reviewing.

smarterclayton · 2016-01-22T16:07:27Z

pkg/build/api/types.go

@@ -59,6 +59,10 @@ type BuildSpec struct {
 // Compute resource requirements to execute the build
 Resources kapi.ResourceRequirements

+ // PostCommit is a build hook executed after the build output image is
+ // committed, before it is pushed to a registry.
+ PostCommit BuildPostCommitSpec


Why is this not just post like deployments? Will we ever add postPush? What other possible posts are there.

strictly naming wise as i remember it's trying to outline more the fact that it takes place between the commit and before the push.

I would recommend we just use Post, and only have one of them ever. The
alignment with deployments matters to me.

On Fri, Jan 22, 2016 at 11:14 AM, Ionut Palade notifications@github.com
wrote:

In pkg/build/api/types.go
#6715 (comment):

@@ -59,6 +59,10 @@ type BuildSpec struct {
// Compute resource requirements to execute the build
Resources kapi.ResourceRequirements

// PostCommit is a build hook executed after the build output image is

// committed, before it is pushed to a registry.

PostCommit BuildPostCommitSpec

strictly naming wise as i remember it's trying to outline more the fact
that it takes place between the commit and before the push.

—
Reply to this email directly or view it on GitHub
https://github.com/openshift/origin/pull/6715/files#r50555320.

It is postCommit because not long ago we we were about to add post, which meant postPush and would trigger downstream builds. And longer ago, we were about to add "post build hooks" which were also semantically after push. Since the "push" part of the build can fail, I see value in being able to have post push actions at some point.

bump for interest.

I don't really see that being likely (adding more hooks), and if they are, they wouldn't be the 80% case anyway. However, this is fine for now.

rhcarvalho · 2016-01-25T13:41:45Z

pkg/build/builder/common.go

+func runPostCommitHook(dockerClient DockerClient, build *api.Build, strategyName, imageID string) error {
+ glog.Infof("Running post commit hook with image %s ...", imageID)
+ containerName := fmt.Sprintf("openshift_%s-build_%s_%s_post-commit_%08x", strategyName, build.Name, build.Namespace, rand.Uint32())
+ return runScriptInContainer(dockerClient, build.Spec.PostCommit.RunBash, imageID, containerName)


@bparees @mfojtik @smarterclayton please help me sanity checking here.

Is the only reason not to run the post commit script as a POD the fact that we need to reference a specific image that only exists in the node where it has been built?

This is one more difference between Build Post Commit and Deployment Lifecycle Hooks.

According to a conversation with @bparees, there's also the problem that starting a pod would consume more of the users quota.

smarterclayton · 2016-02-11T20:14:55Z

EventHistory is cleared when 1000 events have been written to etcd since
the last time you did a list - you can't ask for a change older than 1000
etcd indices.

On Thu, Feb 11, 2016 at 3:08 PM, Rodolfo Carvalho notifications@github.com
wrote:

@smarterclayton https://github.com/smarterclayton @bparees
https://github.com/bparees even after adding the retry logic (
rhcarvalho/origin@dbdb6a7
rhcarvalho@dbdb6a7),
the test still fails in Jenkins because of a timeout: we wait at most 10s
for a build to start.

https://ci.openshift.redhat.com/jenkins/job/test_pull_requests_origin/10671/console

I0211 12:25:59.513781 11193 rest.go:57] ---> event: watch.Event{Type:"ERROR", Object:(*unversioned.Status)(0xc20f87f480)}
I0211 12:25:59.513799 11193 rest.go:75] ---> the watcher expired, trying again with a new one
I0211 12:25:59.513805 11193 rest.go:35] ---> watcherLoop #31158
I0211 12:25:59.513824 11193 rest.go:46] ---> eventLoop #1
I0211 12:25:59.513835 11193 rest.go:84] ---> timed out waiting for build "busybox-1"

I cannot reproduce this locally, and would like to get some ideas on how
to proceed.
Simply increasing the timeout might remedy the situation for now, but it
is not a definitive solution.

I 'd like to know what causes the EventHistory to be cleared / watcher to
be expired so that I could reproduce it locally and propose a better fix...

—
Reply to this email directly or view it on GitHub
#6715 (comment).

rhcarvalho · 2016-02-11T20:16:53Z

@smarterclayton is the etcd instance in Jenkins shared? Why would it get 1000 events in a "short" time frame?

rhcarvalho · 2016-02-11T20:19:02Z

I'm now forcing a timeout of 5 min waiting for the build, let's see what happens in Jenkins...

smarterclayton · 2016-02-11T20:21:01Z

Events, controllers out of control, bad loop somewhere, etc.

On Thu, Feb 11, 2016 at 3:16 PM, Rodolfo Carvalho notifications@github.com
wrote:

@smarterclayton https://github.com/smarterclayton is the etcd instance
in Jenkins shared? Why would it get 1000 events in a "short" time frame?

—
Reply to this email directly or view it on GitHub
#6715 (comment).

rhcarvalho · 2016-02-12T17:35:19Z

Rebased against latest master
Added tests in test/cmd/builds.sh to make sure all variations of the build post commit hook are persisted
Commented out oc start-build tests (will uncomment them in a follow up with a fix to Watching logs fail when etcd event history is cleared #7174)

@bparees PTAL

rhcarvalho · 2016-02-12T17:37:45Z

test/cmd/builds.sh

+os::cmd::expect_success 'oc new-build -D "FROM busybox:1"'
+os::cmd::try_until_text 'oc get istag busybox:1' 'busybox@sha256:'
+os::cmd::expect_success 'oc patch bc/busybox -p '\''{"spec":{"postCommit":{"script":"echo hello $1","args":["world"],"command":null}}}'\'
+os::cmd::expect_success_and_text 'oc get bc/busybox -o=jsonpath="{.spec.postCommit['\''script'\'','\''args'\'','\''command'\'']}"' '^echo hello \$1 \[world\] \[\]$'


This test makes sure we persist the post commit hook correctly.

The line below is a test that ensures the hook is actually executed as part of the build, pending a fix to #7174.

bparees · 2016-02-12T17:49:03Z

@rhcarvalho squash and lgtm

rhcarvalho · 2016-02-15T09:46:27Z

[test]

@PI-Victor

This is meant to be used for running project unit tests as part of a build. Changes: - Add utility to run containers and stream logs - Add build spec API field for post commit actions Includes a common-case API for running a shell script and an kapi.Container-like API for more sophisticated use cases. A good deal of this was done pair programming with @PI-Victor.

openshift-bot · 2016-02-15T16:00:31Z

Evaluated for origin test up to 3e5a859

rhcarvalho · 2016-02-15T16:03:16Z

@bparees squashed.

bparees · 2016-02-15T16:57:47Z

[merge]

openshift-bot · 2016-02-15T17:00:38Z

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/merge_pull_requests_origin/4962/) (Image: devenv-rhel7_3430)

openshift-bot · 2016-02-15T17:00:39Z

Evaluated for origin merge up to 3e5a859

openshift-bot · 2016-02-15T17:29:19Z

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

…sr-cmd Merged by openshift-bot

smarterclayton · 2016-02-15T19:52:05Z

Also, can you guys make sure you have a blurb for me for the release notes
that describes how this works in a few sentences (if we have a CLI command,
even better).

On Mon, Feb 15, 2016 at 2:31 PM, OpenShift Bot notifications@github.com
wrote:

Merged #6715 #6715.

—
Reply to this email directly or view it on GitHub
#6715 (comment).

rhcarvalho · 2016-02-16T15:28:41Z

@smarterclayton as @bparees noted in the issue, the CLI for this is unlike to land soon.

The easiest way to use this right now is with oc patch as in the test/cmd tests, or using oc edit / edit YAML in Web Console.

$ oc patch bc/my-build-config -p '{"spec":{"postCommit":{"script":"rake test"}}}'

Full docs is coming at openshift/openshift-docs#1448

For release notes:

Build hooks can be used to execute commands in a temporary container running a freshly built image before it is pushed to a registry. This is useful to validate images by, e.g., running unit tests.

smarterclayton · 2016-02-16T16:38:23Z

Please note in the cards for CLI work for this - #7331 will be the reference for "mutate a pod template command".

rhcarvalho · 2016-02-16T16:56:27Z

noted, thanks!

rhcarvalho reviewed Jan 19, 2016
View reviewed changes

rhcarvalho force-pushed the build-post-commit-pre-push-usr-cmd branch 2 times, most recently from 199a073 to 027404c Compare January 19, 2016 16:07

bparees reviewed Jan 19, 2016
View reviewed changes

rhcarvalho force-pushed the build-post-commit-pre-push-usr-cmd branch from 027404c to a8b0fc9 Compare January 21, 2016 12:03

rhcarvalho mentioned this pull request Jan 21, 2016

Run unit tests as part of build flow #6758

Closed

rhcarvalho reviewed Jan 22, 2016
View reviewed changes

rhcarvalho force-pushed the build-post-commit-pre-push-usr-cmd branch from a8b0fc9 to 4d0798c Compare January 22, 2016 11:10

rhcarvalho force-pushed the build-post-commit-pre-push-usr-cmd branch from 4d0798c to 151ad0f Compare January 22, 2016 11:38

smarterclayton reviewed Jan 22, 2016
View reviewed changes

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

rhcarvalho mentioned this pull request Jan 25, 2016

Sanitize S2I Builder tests #6810

Merged

rhcarvalho force-pushed the build-post-commit-pre-push-usr-cmd branch 2 times, most recently from 6f98eb4 to ac08605 Compare January 25, 2016 13:25

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

rhcarvalho reviewed Jan 25, 2016
View reviewed changes

rhcarvalho force-pushed the build-post-commit-pre-push-usr-cmd branch from ac08605 to 3460048 Compare January 25, 2016 15:12

rhcarvalho force-pushed the build-post-commit-pre-push-usr-cmd branch from 9efce2d to 0fefe49 Compare February 11, 2016 20:18

rhcarvalho force-pushed the build-post-commit-pre-push-usr-cmd branch from 0fefe49 to 10c7245 Compare February 12, 2016 17:32

rhcarvalho reviewed Feb 12, 2016
View reviewed changes

rhcarvalho force-pushed the build-post-commit-pre-push-usr-cmd branch from 10c7245 to 3e5a859 Compare February 15, 2016 15:59

openshift-bot pushed a commit that referenced this pull request Feb 15, 2016

Merge pull request #6715 from rhcarvalho/build-post-commit-pre-push-u…

a722cb8

…sr-cmd Merged by openshift-bot

openshift-bot merged commit a722cb8 into openshift:master Feb 15, 2016

rhcarvalho deleted the build-post-commit-pre-push-usr-cmd branch February 16, 2016 12:54

rhcarvalho mentioned this pull request Feb 17, 2016

Run post build hook with /bin/sh -ic #7379

Merged

rhcarvalho mentioned this pull request Mar 1, 2016

Incremental builds with S2I builders don't appear to work. #7708

Closed

This was referenced Mar 29, 2016

build post-commit container names are not unique #8266

Closed

Add unique suffix to build post-hook containers #8316

Merged

rhcarvalho mentioned this pull request Jun 28, 2016

[WIP] Build logs retry when etcd watcher expires #7697

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Run user-provided command as part of build flow #6715

Run user-provided command as part of build flow #6715

rhcarvalho commented Jan 19, 2016

rhcarvalho commented Jan 19, 2016

rhcarvalho Jan 19, 2016

bparees Jan 19, 2016

bparees Jan 19, 2016

rhcarvalho Jan 22, 2016

mfojtik Jan 22, 2016

csrwng Jan 22, 2016

rhcarvalho commented Jan 22, 2016

smarterclayton Jan 22, 2016

PI-Victor Jan 22, 2016

smarterclayton Jan 22, 2016

rhcarvalho Jan 25, 2016

PI-Victor Jan 28, 2016

smarterclayton Jan 28, 2016

rhcarvalho Jan 25, 2016

rhcarvalho Jan 25, 2016

smarterclayton commented Feb 11, 2016

rhcarvalho commented Feb 11, 2016

rhcarvalho commented Feb 11, 2016

smarterclayton commented Feb 11, 2016

rhcarvalho commented Feb 12, 2016

rhcarvalho Feb 12, 2016

bparees commented Feb 12, 2016

rhcarvalho commented Feb 15, 2016

openshift-bot commented Feb 15, 2016

rhcarvalho commented Feb 15, 2016

bparees commented Feb 15, 2016

openshift-bot commented Feb 15, 2016

openshift-bot commented Feb 15, 2016

openshift-bot commented Feb 15, 2016

smarterclayton commented Feb 15, 2016

rhcarvalho commented Feb 16, 2016

smarterclayton commented Feb 16, 2016 via email

rhcarvalho commented Feb 16, 2016

Run user-provided command as part of build flow #6715

Run user-provided command as part of build flow #6715

Conversation

rhcarvalho commented Jan 19, 2016

rhcarvalho commented Jan 19, 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

rhcarvalho commented Jan 22, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smarterclayton commented Feb 11, 2016

rhcarvalho commented Feb 11, 2016

rhcarvalho commented Feb 11, 2016

smarterclayton commented Feb 11, 2016

rhcarvalho commented Feb 12, 2016

Choose a reason for hiding this comment

bparees commented Feb 12, 2016

rhcarvalho commented Feb 15, 2016

openshift-bot commented Feb 15, 2016

rhcarvalho commented Feb 15, 2016

bparees commented Feb 15, 2016

openshift-bot commented Feb 15, 2016

openshift-bot commented Feb 15, 2016

openshift-bot commented Feb 15, 2016

smarterclayton commented Feb 15, 2016

rhcarvalho commented Feb 16, 2016

smarterclayton commented Feb 16, 2016 via email

rhcarvalho commented Feb 16, 2016