Some additional Jenkinsfile improvements by thaJeztah · Pull Request #39661 · moby/moby

thaJeztah · 2019-08-03T15:26:41Z

playing with some changes to simplify stages, make the steps more consistent, and to move some steps around to reduce time and optimise for parallel stages

thaJeztah · 2019-08-03T18:18:22Z

Jenkinsfile

Hm, so looks like "validate" needs to have privileged;

[2019-08-03T18:16:40.092Z] + docker run --rm -t -v /home/ubuntu/workspace/moby_PR-39661-3CKP7XCGKUQMONDQU2RSP42V5SYBDIVL7PRW2C4KIVVRW2B5LUCQ/bundles:/go/src/github.com/docker/docker/bundles -v /home/ubuntu/workspace/moby_PR-39661-3CKP7XCGKUQMONDQU2RSP42V5SYBDIVL7PRW2C4KIVVRW2B5LUCQ/.git:/go/src/github.com/docker/docker/.git --name docker-pr9 -e DOCKER_GITCOMMIT=9b16baed380e8bf01675742438cb957733c0501f -e DOCKER_GRAPHDRIVER docker:9b16baed380e8bf01675742438cb957733c0501f hack/validate/default [2019-08-03T18:16:40.656Z] mount: permission denied [2019-08-03T18:16:40.657Z] Could not mount /sys/kernel/security. [2019-08-03T18:16:40.657Z] AppArmor detection and --privileged mode might break. [2019-08-03T18:16:40.657Z] mount: permission denied script returned exit code 32

thaJeztah · 2019-08-03T18:32:58Z

dang; unit-tests also require privileged; we should check if we really need that;

[2019-08-03T18:28:45.994Z] + docker run --rm -t -v /home/jenkins/workspace/moby_PR-39661-3CKP7XCGKUQMONDQU2RSP42V5SYBDIVL7PRW2C4KIVVRW2B5LUCQ/bundles:/go/src/github.com/docker/docker/bundles --name docker-pr10 -e DOCKER_GITCOMMIT=2c137b8bdba4a02500b2c2fe536ef677264cfd2c -e DOCKER_GRAPHDRIVER docker:2c137b8bdba4a02500b2c2fe536ef677264cfd2c hack/test/unit
[2019-08-03T18:28:46.356Z] mount: permission denied
[2019-08-03T18:28:46.356Z] Could not mount /sys/kernel/security.
[2019-08-03T18:28:46.356Z] AppArmor detection and --privileged mode might break.
[2019-08-03T18:28:46.356Z] mount: permission denied
script returned exit code 32

thaJeztah · 2019-08-03T18:39:27Z

Argh.. jenkins is acting up


[2019-08-03T18:36:41.582Z] using credential docker-jenkins-github-credentials
[2019-08-03T18:36:41.587Z] Cloning the remote Git repository
[2019-08-03T18:36:41.587Z] Cloning with configured refspecs honoured and without tags
[2019-08-03T18:36:41.638Z] Cloning repository https://github.com/moby/moby.git
[2019-08-03T18:36:41.845Z] ERROR: Failed to clean the workspace
[2019-08-03T18:36:41.845Z] jenkins.util.io.CompositeIOException: Unable to delete '/home/ubuntu/workspace/moby_PR-39661-3CKP7XCGKUQMONDQU2RSP42V5SYBDIVL7PRW2C4KIVVRW2B5LUCQ'. Tried 3 times (of a maximum of 3) waiting 0.1 sec between attempts.
[2019-08-03T18:36:41.845Z] 	at jenkins.util.io.PathRemover.forceRemoveDirectoryContents(PathRemover.java:86)

This patch removes the manual steps to resolve the Git commit, and instead, uses the `GIT_COMMIT` that's set by Jenkins's Git plugin. Behavior changes slightly, because `GIT_PLUGIN` contains the full commit-sha, not the short one. Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

Both of these tests are fairly short, and shouldn't interfer with eachother, so we can combine them and re-use the same dev-image (so that it'll only be built once). Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

Having this information can help debugging issues in CI (which could be caused by missing/incorrect configuration of the machines). We ping to a fixed version of the script, because this script is ran directly on the host, and we don't want pull-requests modifying this script to have direct access to the machines. Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

The .git mount is only needed for the DCO check, and for building the binaries if `DOCKER_GITCOMMIT` is not set. Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

thaJeztah · 2019-08-03T21:09:16Z

Jenkinsfile

                            }
                        }
-                        stage("Run tests") {
+                        stage("Docker-py") {


I was a bit cautious with the order of these test, because they all bind-mount the bundles directory, and some of the tests will clean that up, which makes archiving artefacts from previous stages complicated (not sure if there's a recommended approach for that?)

thaJeztah · 2019-08-03T21:09:54Z

Jenkinsfile

+                                    binary-daemon \
+                                    dynbinary-daemon \
+                                    test-integration-flaky \
+                                    test-integration \


Should probably split this up into separate stages as well, but here I was also cautious because we want to archive artifacts

thaJeztah · 2019-08-03T22:20:22Z

Okay, this is all green now; we can do some further splitting-up and organising after this. We should also look at storing the docker-py junit.xml, so that it can be shown in the Jenkins UI

This is what it looks like with these changes:

ping @zelahi @tiborvass @seemethere @andrewhsu PTAL

zelahi

LGTM, I left one comment. But really liking how the different stages are looking

zelahi · 2019-08-04T17:35:23Z

Jenkinsfile

@@ -36,35 +37,82 @@ pipeline {
                            steps {
                                sh 'docker version'


Was wondering, does the docker version and docker info need to be printed out?

it helps to find out which version and graph driver has been configured for the node.

unless your question is more along the lines of, "why does docker info have the server version info but not the client version info in the output?"

Yes, the idea of printing these is to help with debugging (e.g. why does something fail on X, but pass on Y, which could be due to a different version of docker installed, or a different configuration.

Ahh thanks for clearing that up! I was wondering if it was left here for debugging purposes. It does make sense to have both since docker info doesn't show the client info

andrewhsu · 2019-08-05T17:21:33Z

I don't think locking down check-config.sh to a fixed version in a2ad56d will help that much in preventing a person creating a PR to run arbitrary code. The PR author has access to the other Jenkinsfile step entry points.

As long as there is nothing interesting on the node that the user can get access to and as long as the admins of jenkins control the resources, the Jenkinsfile entry points should not have to be constructed much different than circleci and travis entry points.

andrewhsu

LGTM

can remove a2ad56d in a separate PR in order to get these improvements in first

seemethere · 2019-08-05T17:25:04Z

@andrewhsu more-so we shouldn't have the Jenkinsfile run any arbitrary shell scripts anyways.

thaJeztah · 2019-08-05T17:27:50Z

I don't think locking down check-config.sh to a fixed version in a2ad56d will help that much in preventing a person creating a PR to run arbitrary code. The PR author has access to the other Jenkinsfile step entry points.

True, but the other ones are running inside the container, not on the host (although I spotted the make clean, which could be abused)

zelahi · 2019-08-05T17:35:08Z

Jenkinsfile

    parameters {
-        booleanParam(name: 'unit', defaultValue: true, description: 'x86 unit tests')
+        booleanParam(name: 'unit_validate', defaultValue: true, description: 'x86 unit tests and vendor check')
        booleanParam(name: 'janky', defaultValue: true, description: 'x86 Build/Test')


Also this is more of a knitpick but would it be useful to modify the description of janky. The reason is what's the difference between the tests being ran in unit_validate and janky?

It really should be named integration-cli although ~~I have no idea what the difference now between janky and experimental is now~~

nvm, I can see that experimental sets DOCKER_EXPERIMENTAL=y, but still not entirely sure what functionality is still hidden behind experimental

tiborvass · 2019-08-05T20:41:44Z

@andrewhsu @seemethere I don't understand #39661 (comment) but @thaJeztah made sure that only the people authorized modifying the Jenkinsfile can modify what check-config shell script to run, as the commit is hardcoded in the Jenkinsfile.

tiborvass · 2019-08-05T20:42:21Z

Jenkinsfile

+                                    dynbinary-daemon \
+                                    test-integration-flaky \
+                                    test-integration \
+                                    cross


this should be in an other stage. We can do that in a followup

🤦‍♂️ ah, we actually discussed that, forgot doing it in the end.

Yes, some more improvements can be made overall (wrt artifacts as mentioned as well). Thought this was a good starting point to split up some steps

tiborvass · 2019-08-05T20:43:15Z

Ping @cpuguy83 @vdemeester

vdemeester

LGTM 🐯

thaJeztah added status/2-code-review area/testing process/cherry-pick labels Aug 3, 2019

GordonTheTurtle added the status/0-triage label Aug 3, 2019

thaJeztah force-pushed the jenkinsfile_improvements branch 4 times, most recently from dfb2a7f to b588501 Compare August 3, 2019 18:16

thaJeztah commented Aug 3, 2019

View reviewed changes

thaJeztah force-pushed the jenkinsfile_improvements branch from b588501 to 5a42229 Compare August 3, 2019 18:22

thaJeztah force-pushed the jenkinsfile_improvements branch 2 times, most recently from 95a684e to e1a280d Compare August 3, 2019 18:36

thaJeztah removed the status/0-triage label Aug 3, 2019

thaJeztah force-pushed the jenkinsfile_improvements branch 2 times, most recently from a4eaa11 to f461996 Compare August 3, 2019 18:43

thaJeztah added 10 commits August 3, 2019 20:49

Jenkinsfile: combine "vendor" and "unit tests"

f51c139

Both of these tests are fairly short, and shouldn't interfer with eachother, so we can combine them and re-use the same dev-image (so that it'll only be built once). Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

Jenkinsfile: standardize cointainer names and fix s390x cleanup

f2e09af

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

Jenkinsfile: remove ip_vs modprobe for unit/vendor stage

6523ced

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

Jenkinsfile: use overlay2 for Power and s390x as well

c75d7e0

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

Jenkinsfile: extract DOCKER_GRAPHDRIVER as environment variable

781e79d

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

Jenkinsfile: move building e2e image to "unit-vendor" stage

efacee1

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

Jenkinsfile: remove unused GIT_SHA1 env-var

0634816

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

Jenkinsfile: consistent indentation and order of env-vars

f814e04

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

thaJeztah force-pushed the jenkinsfile_improvements branch from f461996 to 1f8ac35 Compare August 3, 2019 18:50

thaJeztah added 2 commits August 3, 2019 21:49

Jenkinsfile: remove .git mount in stages that don't use it

47ac8a9

The .git mount is only needed for the DCO check, and for building the binaries if `DOCKER_GITCOMMIT` is not set. Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

Jenkinsfile: inline janky steps, and move validate to separate stage

f411be2

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

thaJeztah added 2 commits August 3, 2019 21:49

Jenkinsfile: inline experimental, power, z steps, and split Unit test

1e8ede5

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

Jenkinsfile: move docker-py to separate stage

ad28fec

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

thaJeztah force-pushed the jenkinsfile_improvements branch from 1f8ac35 to ad28fec Compare August 3, 2019 19:50

thaJeztah commented Aug 3, 2019

View reviewed changes

zelahi reviewed Aug 5, 2019

View reviewed changes

seemethere approved these changes Aug 5, 2019

View reviewed changes

zelahi approved these changes Aug 5, 2019

View reviewed changes

andrewhsu approved these changes Aug 5, 2019

View reviewed changes

zelahi reviewed Aug 5, 2019

View reviewed changes

tiborvass reviewed Aug 5, 2019

View reviewed changes

tiborvass approved these changes Aug 5, 2019

View reviewed changes

vdemeester approved these changes Aug 6, 2019

View reviewed changes

vdemeester merged commit 2eaf35c into moby:master Aug 6, 2019

thaJeztah deleted the jenkinsfile_improvements branch August 6, 2019 09:18

thaJeztah mentioned this pull request Aug 6, 2019

Tailor CI for ARM, skip legacy integration test. #39595

Closed

This was referenced Aug 19, 2019

[19.03 backport] Jenkinsfile and related test-changes docker-archive/engine#328

Merged

[18.09 backport] Jenkinsfile and related test-changes docker-archive/engine#329

Merged

thaJeztah added process/cherry-picked and removed process/cherry-pick labels Aug 21, 2019

thaJeztah added this to the 20.03.0 milestone Apr 2, 2020

Conversation

thaJeztah commented Aug 3, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thaJeztah commented Aug 3, 2019

Uh oh!

thaJeztah commented Aug 3, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thaJeztah commented Aug 3, 2019

Uh oh!

zelahi left a comment

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!

andrewhsu commented Aug 5, 2019

Uh oh!

andrewhsu left a comment

Choose a reason for hiding this comment

Uh oh!

seemethere commented Aug 5, 2019

Uh oh!

thaJeztah commented Aug 5, 2019

Uh oh!

zelahi Aug 5, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

seemethere Aug 5, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tiborvass commented Aug 5, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tiborvass commented Aug 5, 2019

Uh oh!

vdemeester left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

zelahi Aug 5, 2019 •

edited

Loading

seemethere Aug 5, 2019 •

edited

Loading