Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update bats tests to run in parallel #2191

Merged
merged 1 commit into from
Apr 23, 2019

Conversation

saschagrunert
Copy link
Member

@saschagrunert saschagrunert commented Mar 29, 2019

This PR utilizes the parallelism support of bats and applies them to the integration tests. All integration tests should now run int ~15min instead of >30min.

@k8s-ci-robot k8s-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 Mar 29, 2019
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: saschagrunert

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

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Mar 29, 2019
@vrothberg
Copy link
Member

The hacky modification to vendor/github.com/containers/image/copy/copy.go needs either to be fixed within containers/image or cheggaaa/pb#148 before merging this.

Why is this modification needed? Note that containers/image is using a new library now. Vendoring the latest release of containers/image should help.

@saschagrunert
Copy link
Member Author

Why is this modification needed?

Basically because of this behavior: cheggaaa/pb#148 When running dedicated applications in parallel they will lock each other out and never finish.

Note that containers/image is using a new library now. Vendoring the latest release of containers/image should help.

Ah thanks for the hint, I will check that out.

@vrothberg
Copy link
Member

vrothberg commented Mar 29, 2019

Why is this modification needed?

Basically because of this behavior: cheggaaa/pb#148 When running dedicated applications in parallel they will lock each other out and never finish.

Ewww. The new library does not suffer from this issue, so updating should be safe. Heads up: updating image is likely to require updating storage, buildah AND libpod as well. Just in case you run into incompatibility issues.

@@ -184,7 +184,7 @@ function teardown() {

# 6. test running with ctr docker/default
# test that we cannot run with a syscall blocked by the default seccomp profile
@test "ctr seccomp profiles runtime/default" {
@test "ctr seccomp profiles docker/default" {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why did you have to change the name of the test here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test name is a duplicate with test number 2 and bats now complains about this.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok thanks for the clarification!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should be noted that this is intended to be fixed at some point in the future, but it has never actually worked as-expected (the latest-defined test would always be executed).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The fun part is that this test has overwritten the above one which has never been executed therefore, but that should be fixed now.

@k8s-ci-robot k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Mar 29, 2019
@saschagrunert
Copy link
Member Author

Okay this topic is much larger than I initially thought. There are several API changes which break up tests so I will see next week if I split this PR into several pieces.

@codecov-io
Copy link

codecov-io commented Mar 29, 2019

Codecov Report

Merging #2191 into master will decrease coverage by 0.02%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #2191      +/-   ##
==========================================
- Coverage   31.87%   31.85%   -0.03%     
==========================================
  Files          76       76              
  Lines        6708     6706       -2     
==========================================
- Hits         2138     2136       -2     
  Misses       4444     4444              
  Partials      126      126

@vrothberg
Copy link
Member

@saschagrunert I suggest waiting with updating containers/image until buildah and libpod have the new releases. There is a whole bunch of transitive dependencies that must be changed as well. I can spin that up on Monday.

@umohnani8
Copy link
Member

/retest

@k8s-ci-robot k8s-ci-robot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Apr 3, 2019
@k8s-ci-robot k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Apr 3, 2019
@saschagrunert saschagrunert changed the title WIP: Update bats tests to run in parallel Update bats tests to run in parallel Apr 4, 2019
@openshift-ci-robot openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 4, 2019
@saschagrunert
Copy link
Member Author

@chavafg I'm wondering if you had any problems with make install of GNU parallel on RHEL? I was not able to install it the usual way because there was some strange error:

TASK [install parallel] ********************************************************
task path: /go/src/github.com/cri-o/cri-o/contrib/test/integration/build/parallel.yml:9
changed: [localhost] => {
    "changed": true, 
    "cmd": "./configure\n make install", 
    "delta": "0:00:01.303754", 
    "end": "2019-04-10 09:22:44.107113", 
    "rc": 0, 
    "start": "2019-04-10 09:22:42.803359"
}

STDOUT:
---
checking for a BSD-compatible install... /bin/install -c
checking whether build environment is sane... yes
checking for a thread-safe mkdir -p... /bin/mkdir -p
checking for gawk... gawk
checking whether make sets $(MAKE)... yes
checking whether make supports nested variables... yes
checking whether ln -s works... yes
checking that generated files are newer than configure... done
configure: creating ./config.status
config.status: creating Makefile
config.status: creating src/Makefile
config.status: creating config.h
Making install in src
make[1]: Entering directory `/root/parallel-20190222/src'
make[2]: Entering directory `/root/parallel-20190222/src'
 /bin/mkdir -p '/usr/local/bin'
 /bin/install -c parallel sql niceload parcat parset env_parallel env_parallel.ash env_parallel.bash env_parallel.csh env_parallel.dash env_parallel.fish env_parallel.ksh env_parallel.mksh env_parallel.pdksh env_parallel.sh env_parallel.tcsh env_parallel.zsh '/usr/local/bin'
make  install-exec-hook
make[3]: Entering directory `/root/parallel-20190222/src'
rm /usr/local/bin/sem || true
ln -s parallel /usr/local/bin/sem
make[3]: Leaving directory `/root/parallel-20190222/src'
 /bin/mkdir -p '/usr/local/share/doc/parallel'
 /bin/install -c -m 644 parallel.html env_parallel.html sem.html sql.html niceload.html parallel_tutorial.html parallel_book.html parallel_design.html parallel_alternatives.html parcat.html parset.html parallel.texi env_parallel.texi sem.texi sql.texi niceload.texi parallel_tutorial.texi parallel_book.texi parallel_design.texi parallel_alternatives.texi parcat.texi parset.texi parallel.pdf env_parallel.pdf sem.pdf sql.pdf niceload.pdf parallel_tutorial.pdf parallel_book.pdf parallel_design.pdf parallel_alternatives.pdf parcat.pdf parset.pdf '/usr/local/share/doc/parallel'
 /bin/mkdir -p '/usr/local/share/man/man1'
 /bin/install -c -m 644 parallel.1 env_parallel.1 sem.1 sql.1 niceload.1 parcat.1 parset.1 '/usr/local/share/man/man1'
 /bin/mkdir -p '/usr/local/share/man/man7'
 /bin/install -c -m 644 parallel_tutorial.7 parallel_book.7 parallel_design.7 parallel_alternatives.7 '/usr/local/share/man/man7'
make[2]: Leaving directory `/root/parallel-20190222/src'
make[1]: Leaving directory `/root/parallel-20190222/src'
make[1]: Entering directory `/root/parallel-20190222'
make[2]: Entering directory `/root/parallel-20190222'
make[2]: Nothing to be done for `install-exec-am'.
make[2]: Nothing to be done for `install-data-am'.
make[2]: Leaving directory `/root/parallel-20190222'
make[1]: Leaving directory `/root/parallel-20190222'
---

STDERR:
---
rm: cannot remove ‘/usr/local/bin/sem’: No such file or directory
---

@saschagrunert saschagrunert force-pushed the parallel-bats branch 2 times, most recently from 0c7fa7a to ed939fc Compare April 10, 2019 13:46
@chavafg
Copy link
Contributor

chavafg commented Apr 10, 2019

@saschagrunert Didn't had any issues with rhel, but we use our own scripts to install parallel:
https://github.com/kata-containers/tests/blob/master/.ci/lib.sh#L331

@saschagrunert
Copy link
Member Author

/retest

@chavafg
Copy link
Contributor

chavafg commented Apr 10, 2019

kata uses devicemapper driver for running the tests. seems like all tests are trying to get the same physical device (/dev/sdb) at the same time.

@saschagrunert
Copy link
Member Author

kata uses devicemapper driver for running the tests. seems like all tests are trying to get the same physical device (/dev/sdb) at the same time.

Hm, this makes it a bit problematic right now. Since it is configured outside of the tests I would have no chance to fix it. WDYT? I mean we could set JOBS=1 in crio.sh, right?

@chavafg
Copy link
Contributor

chavafg commented Apr 11, 2019

I have already added the JOBS=1 on our jenkins job configuration. Can you please send a restart?

@saschagrunert
Copy link
Member Author

/test kata-containers

@saschagrunert
Copy link
Member Author

I mean it works, but the added GNU parallel dependency seems obsolete now. Do you have any idea how we could solve it @chavafg. I'm running a bit out of ideas.

@chavafg
Copy link
Contributor

chavafg commented Apr 11, 2019

sorry, didn't catch your question... can you elaborate?

@saschagrunert
Copy link
Member Author

/test kata-containers

@saschagrunert
Copy link
Member Author

saschagrunert commented Apr 11, 2019

sorry, didn't catch your question... can you elaborate?

Yep, you added GNU parallel to the kata toolchain. But parallel will not be used at all If you run bats with --jobs 1. I mean the dependency is not necessary any more in the kata CI.

@chavafg
Copy link
Contributor

chavafg commented Apr 11, 2019

ohh ok, got it. Yeah, I think we would still want to continue testing with devicemapper as it seems that we are the only job that do this... so for this testing we would not need it for the moment. But I am also trying to parallelize our k8s test suite which uses bats, so we are fine to have it.

@saschagrunert
Copy link
Member Author

/check-dco

@openshift-ci-robot openshift-ci-robot added the dco-signoff: yes Indicates the PR's author has DCO signed all their commits. label Apr 13, 2019
This commit utilizes the parallelism support of bats and applies them to
the integration tests.

Signed-off-by: Sascha Grunert <sgrunert@suse.com>
@rhatdan
Copy link
Contributor

rhatdan commented Apr 19, 2019

@umohnani8
Copy link
Member

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Apr 22, 2019
@saschagrunert
Copy link
Member Author

/test kata-containers

1 similar comment
@saschagrunert
Copy link
Member Author

/test kata-containers

@mrunalp
Copy link
Member

mrunalp commented Apr 23, 2019

/lgtm

@openshift-merge-robot openshift-merge-robot merged commit 110a2d0 into cri-o:master Apr 23, 2019
@saschagrunert saschagrunert deleted the parallel-bats branch April 24, 2019 05:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. dco-signoff: yes Indicates the PR's author has DCO signed all their commits. lgtm Indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet