Tailor CI for ARM, skip legacy integration test. by michael2012z · Pull Request #39595 · moby/moby

michael2012z · 2019-07-23T05:01:26Z

Signed-off-by: Michael Zhao michael.zhao@arm.com

- What I did
This PR is trying to make a minimal test set (without legacy integration test cases) for ARM.
Introduce a new environment variable SKIP_LEGACY_INTEGRATION_TEST to control whether legacy integration test cases are ignored or not.

- How I did it

Introduce a new environment variable SKIP_LEGACY_INTEGRATION_TEST, default value is "no".
If it's set to "yes" or anything rather than "no", legacy integration suites will be skipped.
In arm CI, SKIP_LEGACY_INTEGRATION_TEST is set to "yes".
CI for other architecture's are not impacted.

- How to verify it
Run "make test-integration" on an ARM machine.

- Description for the changelog

See the discussion in issue: #39479
Backgroud: CI on arm (32 and 64 bit) architecture took too long time to finish. So the CI for it was stopped. Now I am trying to bring it back, and try to set a minimal set in it to save time.

- A picture of a cute animal (not mandatory but encouraged)

michael2012z · 2019-07-26T01:37:13Z

Could anybody give some comments? Thanks.

psftw

A couple of nitpicks in the Jenkinsfile, but overall looks good to me. Note: we are turning on the Jenkinsfile integration today, which will have two key implications for this PR: 1. A new GitHub PR status with context continuous-integration/jenkins/pr-merge will replace the handful of existing statuses (which will continue for a short time as we transition). 2. In order for your changes to the Jenkinsfile to be reflected in your PR build job, you will need to have Collaborator status in the moby/moby repository (if not already) otherwise Jenkins will ignore your Jenkinsfile changes (for security).

psftw · 2019-07-29T16:17:20Z

Jenkinsfile

we've recently removed usage of withGithubStatus, so it can be dropped here as well.

psftw · 2019-07-29T16:21:25Z

Jenkinsfile

suggest aarch64 && packet to run on our powerful packet hosts and avoid accidentally running some tasks on ec2 instances due to ambiguity in label expression. Later if we parallelize the ARM tasks, we could swap this out for aarch64 && ubuntu-1804 && overlay2 to run on many a1.xlarge ec2 agents at once if that improves performance overall.

michael2012z · 2019-07-30T03:12:20Z

Thank you for reviewing the PR and sharing the update of CI.

New status continuous-integration/jenkins/pr-merge looks cool.
I am not a Collaborator now, hope I can become one in future. :)

michael2012z · 2019-08-01T01:06:39Z

Hi, is there any more comments on this?
Ping @tianon , would you like to give some feedback?

tianon · 2019-08-01T22:16:33Z

Seems reasonable to me, but I haven't been involved in Docker's CI setup much (so I'd defer to @psftw's opinion here). 👍

tianon · 2019-08-01T22:17:43Z

I think this also has some overlap with #39628?

Signed-off-by: Michael Zhao <michael.zhao@arm.com>

michael2012z · 2019-08-05T11:37:52Z

@tianon, thank you for reviewing.
Indeed, there was overlap, I updated this PR to use one of the environment variables introduced by #39628. This PR was simplified.

tianon · 2019-08-05T14:11:47Z

LGTM 👍

michael2012z · 2019-08-06T07:16:59Z

Hi, @thaJeztah , @psftw ,
Can we merge this PR to bring back arm64 CI?
I think running the CI on one more architecture would be helpful to guarantee the quality of the project, although the test cases included is a small portion of all.

thaJeztah

Thanks! I left some comments inline; I created a branch with those suggested changes, and will open a PR (that way Jenkins should also actually try to run the new version, so we can verify if it works 👍)

thaJeztah · 2019-08-06T09:24:45Z

Jenkinsfile

+            beforeAgent true
+            expression { params.aarch64 }
+          }
+          agent { label 'aarch64 && packet' }


@psftw wondering if we need the packet label for filtering, or if just aarch64 would be enough

thaJeztah · 2019-08-06T09:27:13Z

Jenkinsfile

+          }
+          agent { label 'aarch64 && packet' }
+          steps {
+            sh '''


I made some enhancements in #39656 and #39661; we can probably make the same/similar changes here;

use the GIT_COMMIT environment variable (instead of manually getting it through git rev-parse)

split the build step to a separate stage

add a stage to print docker version, docker info, and run the contrib/check-config.sh script (helps when debugging issues as it would provide details about the machine that CI is running on)

inline the steps that are executed (instead of using hack/ci/arm - I'm trying to get rid of the hack/ci/* scripts because those can now be described in the Jenkinsfiles itself (which makes it more transparent)

thaJeztah · 2019-08-06T09:31:52Z

Jenkinsfile

+                docker rm -vf docker-pr-aarch64$BUILD_NUMBER || true
+
+                echo "Chowning /workspace to jenkins user"
+                docker run --rm -v "$WORKSPACE:/workspace" aarch64/busybox chown -R "$(id -u):$(id -g)" /workspace


the busybox image is multi-arch, so could be just busybox now (also see my other comment)

thaJeztah · 2019-08-06T09:32:08Z

Jenkinsfile

+                -v "$WORKSPACE/bundles:/go/src/github.com/docker/docker/bundles" \
+                --name docker-pr-aarch64$BUILD_NUMBER \
+                -e DOCKER_GRAPHDRIVER=vfs \
+                -e DOCKER_EXECDRIVER=native \


This environment variable is no longer needed and can be removed

Yes.
A lot of improvements had been made on master branch since I wrote the script in the new stage "aarch64".
Thanks for correcting them in the new PR.

thaJeztah · 2019-08-06T09:33:21Z

Jenkinsfile

+            sh '''
+              GITCOMMIT=$(git rev-parse --short HEAD)
+
+              docker build --rm --force-rm --build-arg APT_MIRROR=cdn-fastly.deb.debian.org -t docker-aarch64:$GITCOMMIT -f Dockerfile .


given that we don't run multiple architectures on a single machine, there's no chance on collision with a container from another test, so the -arch64 suffix can be removed

thaJeztah · 2019-08-06T09:34:53Z

Jenkinsfile

+              docker run --rm -t --privileged \
+                -v "$WORKSPACE/bundles:/go/src/github.com/docker/docker/bundles" \
+                --name docker-pr-aarch64$BUILD_NUMBER \
+                -e DOCKER_GRAPHDRIVER=vfs \


vfs is probably ok for now (we can check what those machines support, and switch to (e.g.) overlay2 if supported

thaJeztah · 2019-08-06T09:40:34Z

Jenkinsfile

                }
            }
        }
+        stage('aarch64') {


Looks like this stage is not in the right place (it's currently at the same level as the top-level Build stage, which means it won't be run in parallel with the other tests, but after all the other tests are completed)

thaJeztah · 2019-08-06T09:47:57Z

Jenkinsfile

+            }
+          }
+        }
+      }


Looks like there's too many closing brackets here

thaJeztah · 2019-08-06T10:57:45Z

carrying in #39678

michael2012z · 2019-08-06T14:09:08Z

@thaJeztah Thank you for improving this PR and carrying it in #39678 so the modification can be verified.
I will close this PR.

thaJeztah · 2019-08-06T14:38:07Z

You're welcome! I was reviewing, and thought; might as well just make the changes (as it would be difficult to write them all down, haha); CI is running on that PR, and so far, it looks good! (even just enabled the legacy integration-cli tests to see how long they would take on those workers.

michael2012z requested a review from tianon as a code owner July 23, 2019 05:01

GordonTheTurtle added the status/0-triage label Jul 23, 2019

michael2012z mentioned this pull request Jul 23, 2019

Jenkins CI for arm64 #39479

Closed

michael2012z force-pushed the tailor_arm_ci branch from a6cc778 to 3d1c4a4 Compare July 24, 2019 04:47

michael2012z changed the title ~~[DISCUSSING - DO NOT MERGE] Tailor CI for ARM, skip legacy integration test.~~ Tailor CI for ARM, skip legacy integration test. Jul 24, 2019

psftw reviewed Jul 29, 2019

View reviewed changes

michael2012z force-pushed the tailor_arm_ci branch from 3d1c4a4 to dbe6fe4 Compare July 30, 2019 03:12

michael2012z added 2 commits August 5, 2019 18:59

Test to enable CI on aarch64.

2c3b3c2

Signed-off-by: Michael Zhao <michael.zhao@arm.com>

Tailor CI for ARM, skip legacy integration test.

283bfc7

Signed-off-by: Michael Zhao <michael.zhao@arm.com>

michael2012z force-pushed the tailor_arm_ci branch from dbe6fe4 to 283bfc7 Compare August 5, 2019 11:33

thaJeztah added area/testing process/cherry-pick status/2-code-review and removed status/0-triage labels Aug 6, 2019

thaJeztah requested changes Aug 6, 2019

View reviewed changes

thaJeztah mentioned this pull request Aug 6, 2019

[Carry 39595] Tailor CI for ARM, skip legacy integration test. #39678

Merged

michael2012z closed this Aug 6, 2019

thaJeztah added process/cherry-picked and removed process/cherry-pick labels Oct 8, 2019

thaJeztah mentioned this pull request Jan 25, 2020

[19.03 backport][Carry 39595] Tailor CI for ARM, skip legacy integration test #40414

Closed

Conversation

michael2012z commented Jul 23, 2019

Uh oh!

michael2012z commented Jul 26, 2019

Uh oh!

psftw 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!

michael2012z commented Jul 30, 2019

Uh oh!

michael2012z commented Aug 1, 2019

Uh oh!

tianon commented Aug 1, 2019

Uh oh!

tianon commented Aug 1, 2019

Uh oh!

michael2012z commented Aug 5, 2019

Uh oh!

tianon commented Aug 5, 2019

Uh oh!

michael2012z commented Aug 6, 2019

Uh oh!

thaJeztah 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!

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!

thaJeztah commented Aug 6, 2019

Uh oh!

michael2012z commented Aug 6, 2019

Uh oh!

thaJeztah commented Aug 6, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants