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

ci: untangle getting test images #2741

Merged
merged 4 commits into from
Feb 5, 2021

Conversation

kolyshkin
Copy link
Contributor

@kolyshkin kolyshkin commented Jan 15, 2021

TL;DR: this optimizes, simplifies and unifies the process of getting container images
used for tests, resulting in less code, less chances to have broken CI, easier way to
run libcontainer/integration tests, and easier way to support more architectures.

Currently, we have three different ways of getting images:

  1. (for hello-world) the image is in this repo under tests/integration/testdata.

  2. (for busybox) download it from github (the repo that is used for
    preparing official Docker image) using curl.

  3. (for debian) download from Docker hub, using skopeo and umoci.

To further complicate things, we have to do this downloading in multiple
scenarios (at least 4): locally, in github CI, from Dockefile, inside a
Vagrant VM. For each scenario, we have to install skopeo and umoci, and
those two are not yet universally available for all the distros that we
use.

Yet another complication is those images are used for tests/integration
(bats-driven tests) as well as for libcontainer/integration (go tests).
The tests in libcontainer/integration rely on busybox being available
from /busybox, and the bats tests just download the images to a
temporary location during every run.

It is also hard to support CI for other architectures, because all
the machinery for preparing images is so complicated.

This commit is an attempt to simplify and optimize getting images,
mostly by getting rid of skopeo and umoci dependencies, but also
by moving the download logic into one small shell script, which
is used from all the places.

Benefits:

  • images (if not present) are only downloaded once;
  • same images are used for both kind of tests (go and bats);
  • same images are used for local and inside-docker tests
    (because source directory is mounted into container);
  • the download logic is located within 1 simple shell script.

Side benefits:

  • tests won't try to use amd64 image on an unknown architecture;
  • some other arches (like mips64le or s390) might already work
    (except for hello-world but it is trivial to fix);
  • I can finally remove /busybox directory from my laptop
    (cd libcontainer/integration && go test will download images if needed);
  • I can run any bats tests locally without having to install skopeo/umoci.

Caveats:

  • currently the script downloads all (busybox and debian) images at once. Unless we'll have more than 5 images I want to keep it that way (for simplicity);
  • the latest versions of images are downloaded (e.g. those are not pinned to a specific sha). Pinning would require a per-image per-arch sha map, and I'd like to keep it simple while it works.

While at it:

  • make rootless.sh more robust (set -e -u -o pipefail) and shellcheck-valid.
    This does not really belong here but I don't want to have too many small PRs.
  • use "slim" debian variant (thanks to @tianon)

Previous work related to this:

// init makes sure the container images are downloaded,
// and initializes busyboxTar. If images can't be downloaded,
// we are unable to run any tests, so panic.
func init() {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This function looks a tad complicated, but all it does is calls get-images.sh
and parses its output to get the value of BUSYBOX_IMAGE. This way we
don't have to worry about doing some extra steps before running go test.

@kolyshkin
Copy link
Contributor Author

@tianon could you please review this (at least the shell part, i.e. get-images.sh)?

tests/integration/helpers.bash Outdated Show resolved Hide resolved
tests/integration/get-images.sh Outdated Show resolved Hide resolved
@kolyshkin
Copy link
Contributor Author

@AkihiroSuda @mrunalp PTAL

tests/rootless.sh Outdated Show resolved Hide resolved
AkihiroSuda
AkihiroSuda previously approved these changes Jan 27, 2021
@kolyshkin
Copy link
Contributor Author

Rebased; addressed review comment.

@kolyshkin kolyshkin added this to the 1.0.0-rc94 milestone Feb 2, 2021
It is not used anywhere since commit 30601ef.

Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
@kolyshkin kolyshkin force-pushed the test-images branch 2 times, most recently from 5cb591b to 96b29fa Compare February 2, 2021 20:01
This simplifies and optimizes getting container images used for tests.

Currently, we have three different ways of getting images:

1. (for hello-world) the image is in this repo under tests/integration/testdata.

2. (for busybox) download it from github (the repo that is used for
   preparing official Docker image) using curl.

3. (for debian) download from Docker hub, using skopeo and umoci.

To further complicate things, we have to do this downloading in multiple
scenarios (at least 4): locally, in github CI, from Dockefile, inside a
Vagrant VM. For each scenario, we have to install skopeo and umoci, and
those two are not yet universally available for all the distros that we
use.

Yet another complication is those images are used for tests/integration
(bats-driven tests) as well as for libcontainer/integration (go tests).
The tests in libcontainer/integration rely on busybox being available
from /busybox, and the bats tests just download the images to a
temporary location during every run.

It is also hard to support CI for other architectures, because all
the machinery for preparing images is so complicated.

This commit is an attempt to simplify and optimize getting images,
mostly by getting rid of skopeo and umoci dependencies, but also
by moving the download logic into one small shell script, which
is used from all the places.

Benefits:

 - images (if not present) are only downloaded once;
 - same images are used for both kind of tests (go and bats);
 - same images are used for local and inside-docker tests
   (because source directory is mounted into container);
 - the download logic is located within 1 simple shell script.

[v2: fix eval; more doc to get-images; print URL if curl failed]
[v3: use "slim" debian, twice as small]
[v4: fix not using $image in setup_bundle]
[v5: don't remove TESTDATA from helpers.bash]
[v6: add i386 support]

Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Currently, set -e ("exit on error") is only set before running tests.
The bad consequence is the script may fail to set up test environment
but will still run the tests.

Use "set -e -u -o pipefail" for the whole script to catch potential
issues with test setup and/or the script itself.

A few caveats:

1. We have to set RUNC_USE_SYSTEMD to an empty value if it is unset, so
   that the subsequent checks like [ -z "$RUNC_USE_SYSTEMD" ] won't
   trigger a "unbound variable" error. Same for ROOTLESS_TESTPATH.

2. Functions that have code like [ -f $file ] && do_something towards
   the end may return 1 in case $file does not exist (as test aka [
   was the last operation performed, and its exit code is returned.
   This is why we had to add return 0.

Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
@cyphar
Copy link
Member

cyphar commented Feb 4, 2021

I would prefer that we kept using umoci+skopeo for this, since having our own scripts for fetching images (especially tying it to GitHub blob URLs) makes me a little bit nervous, but if this does fix CI issues then I guess it's a net positive. Hopefully umoci works its way into more distributions so we don't have to do this forever.

@kolyshkin
Copy link
Contributor Author

@cyphar note we are only using umoci+skopeo for fetching Debian, busybox is (and was always) done by curl.

I agree using proper tools (rather than relying on some URLs) is better in theory, but practically that this PR does simplifies things a lot from a perspective of CI maintainer, makes the code smaller, and solves a few dangling issues (like inability to run go test or bats tests/integration/whatever.bats locally without doing some complex setup first) along the way).

Once skopeo and umoci are universally available we'll modify the tests/integration/get-images.sh script to use them.

@cyphar
Copy link
Member

cyphar commented Feb 5, 2021

Yeah, agreed. Sorry the above was intended to be an LGTM. Lemme do that now.

Copy link
Member

@cyphar cyphar left a comment

Choose a reason for hiding this comment

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

LGTM.

_, ex, _, _ := runtime.Caller(0)
getImages, err := filepath.Abs(filepath.Join(filepath.Dir(ex), "..", "..", "tests", "integration", "get-images.sh"))
if err != nil {
panic(err)
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this cause t.Skip rather than panic ?

Copy link
Member

Choose a reason for hiding this comment

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

You can't cause t.Skip because it's done in init().

Copy link
Member

Choose a reason for hiding this comment

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

We can, by defining a global variable

Copy link
Member

Choose a reason for hiding this comment

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

You would want it to be t.Fatal because the images are needed for tests (and skipping them would hide test failures), and t.Fatal will kill the test anyway. I guess it is slightly nicer to do that than panic, but this really shouldn't happen often enough to be an issue (not to mention you'd need to add t.Fatal to some helper function and I can't see an obvious candidate in utils_test.go -- none of the functions take *testing.T).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants