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

integration: Make sure pivot_root() works even if parent mount is shared #1151

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

rhvgoyal
Copy link
Contributor

We recently had regression where parent mount of rootfs is "shared" and
rootfsPropataion=private. Write an integration test to test this
configuration, to make avoid future regressions.

Signed-off-by: Vivek Goyal vgoyal@redhat.com

@mrunalp
Copy link
Contributor

mrunalp commented Oct 26, 2016

@rhvgoyal Did you run this on RHEL 7?

# Create a "shared" parent mount point for container
# rootfs and make sure pivot_root() still works. This
cd ${BUSYBOX_BUNDLE}
mount --bind . .
Copy link
Member

Choose a reason for hiding this comment

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

If you put this inside @test and you use unshare -m to run a shell command, that might work a bit nicer since it means we won't be messing with host mountpoints while testing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@cyphar, not sure how to use unshare -m in this script. IIUC, I will have to have another script to run actual test and do unshare -m <new-script>. If I just call unshare -m it will just launch another bash in new mount namespace but that does not run my rest of the test code.

Copy link
Member

@cyphar cyphar Oct 28, 2016

Choose a reason for hiding this comment

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

Yes, what I'm saying is to remove this from the setup code (where it doesn't belong because it's changing how other tests are running) and modify your @test so that it does something like:

% tmpscript=$(mktemp)
% cat >$tmpscript <<EOF
#!/bin/sh

cd ${BUSYBOX_BUNDLE}
mount --bind . .
mount --make-private .
mount --make-shared .
runc ...
EOF
% unshare -m sh $tmpscript
% rm $tmpscript

Which might work (though it means we couldn't use the runc aliases I have in helpers.bash. You could also try making bats run bats inside an unshare -m but that feels like a bad idea.

@hqhq
Copy link
Contributor

hqhq commented Oct 27, 2016

@rhvgoyal Sorry I messed the commits, can you please rebase?

@rhvgoyal
Copy link
Contributor Author

@mrunalp no, I have not run this on rhel7. Only on fedora24.

@rhvgoyal
Copy link
Contributor Author

@mrunalp I tested on rhel7 just now. This particular individual test passes.

bats tests/integration/mount_propagation.bats

But, if I try to run all the integration tests locally, it hangs. And this happens on master branch too. So that seems to be a separate problem, independent of this PR.

@cyphar
Copy link
Member

cyphar commented Oct 28, 2016

@rhvgoyal Have you tried rebooting your machine and then running all the tests of master -- your current tests will modify the host's mountpoints. Which is why I want to use unshare -m because the current tests are dangerous to say the least.

We recently had regression where parent mount of rootfs is "shared" and
rootfsPropataion=private. Write an integration test to test this
configuration, to make avoid future regressions.

Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
@rhvgoyal
Copy link
Contributor Author

@cyphar, I understand that why do you want to use separate mount namespace. It makes it harder to break host setup. If running test in separate mount namespace makes sense for this test, then I think it makes sense for all the test? Any test can decide to create a mount and we probably would want to make sure it does not spoil host environment.

How about, running all integration tests in a separate mount namespace? I have modifed Makefile to call bats in a separate mount namespace. unshare -m bats -t tests/integration. Will this work?

@cyphar
Copy link
Member

cyphar commented Oct 29, 2016

@rhvgoyal The problem is that I'd like a new mount namespace for each test. I guarantee there's a way to hack this into bats (I'll take a look at this later), but the main issue is that I don't want to have explicit changes of the mount namespace in one test affect other tests. mount --bind . . and other things affect how other tests run (even though they really shouldn't).

@crosbymichael
Copy link
Member

Did you two find any solution for running the tests in a separate mountns?

@cyphar
Copy link
Member

cyphar commented Dec 3, 2016

@crosbymichael Not really, though I was looking at the bats source code recently and it might be possible to pull it off if you mess around with how the functions are generated. Almost certainly not worth the effort though. The only really viable method is this:

@test "some test" {
    script="$(mktemp --tmpdir=$BATS_TMPDIR)"
    cat >$script <<EOF
        the test function
    EOF

    # Then run bats inside unshare. Not sure how to get the script arguments from a function though.
    unshare -m "$0" $script
}

Which /could/ work. The big thing is that you need to source the bats helpers in order for the errors to propagate.

Overall, it's probably not worth it.

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

Successfully merging this pull request may close these issues.

None yet

6 participants