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: don't run tests on overlayfs/tmpfs #1199

Closed
wants to merge 6 commits into from

Conversation

jlebon
Copy link
Member

@jlebon jlebon commented Sep 20, 2017

It occurred to me that a simple way of getting coverage for tests that don't work on overlayfs/tmpfs is to simply run them in a directory mounted from the host.

To do this, we switch f26-primary to execute on a test node rather than in a container, and make use of the new TEST_TMPDIR var to point it at the mounted /srv dir.

We also have to tweak the overlayfs detection logic only check the filesystem in which we're actually running the test, rather than just /.

@rh-atomic-bot
Copy link

☔ The latest upstream changes (presumably 75150fe) made this pull request unmergeable. Please resolve the merge conflicts.

Allow developers to override the default /var/tmp dir, which e.g. might
be on overlayfs and thus produces reduced coverage.
We use `setfattr` to determine whether the filesystem we're on supports
xattrs, but we need to check that `setfattr` itself is available. We
just make it a hard requirement but only if trying to run tests that ask
about xattr support.
It occurred to me that a simple way of getting coverage for tests that
don't work on overlayfs/tmpfs is to simply run them in a directory
mounted from the host.

To do this, we switch `f26-primary` to execute on a test node rather
than in a container, and make use of the new `TEST_TMPDIR` var to point
it at the mounted /srv dir.

We also have to tweak the overlayfs detection logic only check the
filesystem in which we're actually running the test, rather than just /.
Copy link
Member

@cgwalters cgwalters left a comment

Choose a reason for hiding this comment

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

Maybe split off the first two changes again while we discuss this (sorry to do it again but this is quite a notable change and I think worth some bikeshedding)

.papr.yml Outdated
- ci/build-check.sh
- ci/ci-release-build.sh
- docker run -e TEST_TMPDIR -e CFLAGS -e GI_SCANNERFLAGS -e ASAN_OPTIONS
--privileged -v $PWD:$PWD --workdir $PWD -v /srv:/srv
Copy link
Member

Choose a reason for hiding this comment

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

There's nothing for this change that requires --privileged right? I suspect it's just SELinux labeling that we'd hit; relabeling with chcon -R -h -t container_file_t $PWD and doing the same with TEST_TMPDIR would let us avoid being privileged, and that in turn would help guarantee these tests can be run in Kube for example.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point, I added a fixup to try that out. ⬇️

Copy link
Member Author

Choose a reason for hiding this comment

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

Ahh, hmm, we're still hitting

error: lsetxattr: Permission denied

when trying to commit files (with a relabelfrom SELinux violation).

I suspect it's just SELinux labeling that we'd hit; relabeling with chcon -R -h -t container_file_t $PWD and doing the same with TEST_TMPDIR would let us avoid being privileged,

But we'll still be trying to re-apply all the labels when committing to a bare repo, right? Unless we teach libglnx to only update security.selinux if the label isn't already set to the wanted value, though that's quite a performance hit. We could hide the behaviour behind an env var, I guess? We could also put back --privileged (or at least --security-opt label:disable) and add another testsuite to make sure the in-container path doesn't break?

@jlebon
Copy link
Member Author

jlebon commented Sep 21, 2017

Woohoo!

Before:

+ gnome-desktop-testing-runner -p 0 libostree/
Running test: libostree/test-pull-contenturl.sh.test
Copying gpghome to /tmp/test-tmp-libostree_test-pull-contenturl.sh.test-7NW96Y
evaluating for overlayfs...
overlayfs found; enabling OSTREE_NO_XATTRS
done

After:

+ gnome-desktop-testing-runner -p 0 libostree/
Running test: libostree/test-refs.sh.test
Copying gpghome to /srv/test-tmp-libostree_test-refs.sh.test-L1CX6Y
evaluating for overlayfs...
done

@jlebon
Copy link
Member Author

jlebon commented Sep 21, 2017

Sure, split out #1207.

@cgwalters
Copy link
Member

I'd say we should unconditionally disable xattrs for bare mode unit tests, and do the wiring to change test-basic.sh to run fully as an installed test (as root).

@cgwalters
Copy link
Member

To elaborate more concretely, I propose: Our unit-style tests i.e. make check which is currently also supported as "InstalledTestsSpec" style assume non-root and focus on testing bare-user-only and archive, and optionally bare-user if supported. (Mostly the status quo, minus bare).

Retain executing those tests in containers (which we should also make fully unprivileged and not just the docker default).

The tests that use bare (which is a lot of the key ones like test-basic.sh and test-pull-bare.sh) are migrated to instead be run via tests/installed/test-bare-unit.sh).

@cgwalters
Copy link
Member

If you agree, I can take care of doing this.

@jlebon
Copy link
Member Author

jlebon commented Sep 26, 2017

Agreed, that's a logical way to split the tests.

If you agree, I can take care of doing this.

Sounds good!

@cgwalters
Copy link
Member

Moved to #1217

@cgwalters cgwalters closed this Sep 26, 2017
jlebon added a commit to jlebon/ostree that referenced this pull request Sep 28, 2017
This was originally part of ostreedev#1199, but never got in. The work in ostreedev#1218
is awesome for CI, though for the developer workflow, I find it much
easier to just set $TEST_TMPDIR to a non-overlayfs mount point. Teach
the test libraries to just look at $PWD rather than /.

Also export the function that does the overlayfs checking. Prep for a
future patch.
rh-atomic-bot pushed a commit that referenced this pull request Sep 28, 2017
This was originally part of #1199, but never got in. The work in #1218
is awesome for CI, though for the developer workflow, I find it much
easier to just set $TEST_TMPDIR to a non-overlayfs mount point. Teach
the test libraries to just look at $PWD rather than /.

Also export the function that does the overlayfs checking. Prep for a
future patch.

Closes: #1170
Approved by: cgwalters
@jlebon jlebon deleted the pr/test-tmpdir branch June 14, 2018 01:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants