-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
tests: set --iptables=false
for dockerd tests
#4007
base: master
Are you sure you want to change the base?
Conversation
--iptables=false
for dockerd tests
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SGTM
I guess tests would show if this didn't work, so 😅
@@ -12,7 +12,7 @@ on: | |||
env: | |||
SETUP_BUILDX_VERSION: "latest" | |||
SETUP_BUILDKIT_IMAGE: "moby/buildkit:latest" | |||
TESTFLAGS: "-v --parallel=1 --timeout=30m" | |||
TESTFLAGS: "-v --parallel=6 --timeout=30m" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I looked at your pipeline run on your fork https://github.com/jedevc/buildkit/actions/runs/5520028773
Seems many tests failed but not sure if linked to //. Triggered one on this repo to look at the state: https://github.com/moby/buildkit/actions/runs/5521394187
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you also create a new branch from v0.11
and apply this change, vendor in moby and open draft PR there to see if everything looks good?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See moby/moby#45936.
Why isn't this needed for |
7e42800
to
f3d2cb3
Compare
Without this option, all tests will attempt to launch a daemon that all attempt to modify the same iptables chain which will fail. Signed-off-by: Justin Chadwell <[email protected]>
Signed-off-by: Justin Chadwell <[email protected]>
f3d2cb3
to
5a3a81c
Compare
🤦 so this doesn't work for a singular test in moby: This fails because (surprisingly) it seems to be our only buildkit test that requires container network access. Disabling iptables means we get no external network access. I think what we'd want is an approach that lets us disable parallel tests and requires iptables for a set of tests? That way we can run most tests in parallel... except for this one, and probably only apply this for dockerd tests. Alternatively... we could manually create the bridge interface, setup masquerading, and ensure that each instance of docker has a unique IP range, so we could run multiple networked tests in parallel. |
Similar in style to moby/moby#45891.
When attempting to run dockerd tests in parallel, it's easy to get the following error:
(compare before and after)
By disabling iptables for parallel tests, we don't hit this problem. I'm fairly sure we shouldn't need this for any of the buildkit tests? We're not exposing any ports on the host, and I don't think we use anything but the default bridge network for our tests, which ticks the requirement for the flag under https://docs.docker.com/engine/reference/commandline/dockerd/#run-multiple-daemons.