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

Run integration tests against dockerd #1164

Open
wants to merge 2 commits into
base: master
from

Conversation

@SamWhited
Copy link
Contributor

SamWhited commented Sep 9, 2019

Adds a dockerd worker to integration tests when an environment variable is set.

@GordonTheTurtle

This comment has been minimized.

Copy link
Collaborator

GordonTheTurtle commented Sep 9, 2019

Please sign your commits following these rules:
https://github.com/moby/moby/blob/master/CONTRIBUTING.md#sign-your-work
The easiest way to do this is to amend the last commit:

$ git clone -b "test_dockerd" git@github.com:SamWhited/buildkit.git somewhere
$ cd somewhere
$ git commit --amend -s --no-edit
$ git push -f

Amending updates the existing PR. You DO NOT need to open a new one.

@SamWhited SamWhited force-pushed the SamWhited:test_dockerd branch from 213c657 to b15f7f6 Sep 9, 2019
@GordonTheTurtle GordonTheTurtle removed the dco/no label Sep 9, 2019
@SamWhited SamWhited force-pushed the SamWhited:test_dockerd branch from b15f7f6 to 89a8c8e Sep 9, 2019
@tonistiigi

This comment has been minimized.

Copy link
Member

tonistiigi commented Sep 9, 2019

Buildkitd should not start dockerd. Dockerd sandbox implementation starts dockerd and gets a client through Docker API instead of calling runBuildkitd.

@SamWhited SamWhited force-pushed the SamWhited:test_dockerd branch 9 times, most recently from b8d5d6e to 79e2000 Sep 9, 2019
@SamWhited

This comment has been minimized.

Copy link
Contributor Author

SamWhited commented Sep 10, 2019

Buildkitd should not start dockerd. Dockerd sandbox implementation starts dockerd and gets a client through Docker API instead of calling runBuildkitd.

@tonistiigi we still have to run buildkitd, no? As far as I can tell buildkitdclient.New doesn't launch buildkitd itself which is why everything else calls runbuildkitd.

@tonistiigi

This comment has been minimized.

Copy link
Member

tonistiigi commented Sep 10, 2019

we still have to run buildkitd, no?

No. Need to run docker daemon, get docker client for that daemon, upgrade that client to buildkit client like https://github.com/docker/buildx/blob/master/driver/docker/driver.go#L42-L44 and run tests on that buildkit client.

@SamWhited

This comment has been minimized.

Copy link
Contributor Author

SamWhited commented Sep 10, 2019

Okay, I've been digging around all day to figure out how best to do this and I think we'll still need a bigger restructure than the sandbox/backend changes that have already been made, but we'll see. I'll have to tweak sandbox to include some client options or a new client method since right now the tests themselves each create their buildkit clients. Since we create the sandbox implementation with newSandbox, we can add whatever we need there but it will end up being a bigger change than I thought was necessary. I may also have to figure out a place to move some things to avoid import loops, but we'll see. I'm a bit nervous moving things around since I still don't really understand what everything in here does or why it's necessary, as I discover that I may start documenting them as well.

@tonistiigi

This comment has been minimized.

Copy link
Member

tonistiigi commented Sep 10, 2019

Shouldn't be a need for big refactorings afaics and lets make sure to avoid changes to existing test (instead we can listen on a temporary socket for example). It is expected that not all tests will work this way(and not all features are actually supported in docker yet) and some need to be skipped.

@SamWhited SamWhited force-pushed the SamWhited:test_dockerd branch 2 times, most recently from 055e815 to 2928d9e Sep 10, 2019
@AkihiroSuda

This comment has been minimized.

Copy link
Member

AkihiroSuda commented Sep 11, 2019

Is the dockerd worker expected to be used only for testing? Or will it replace Moby's builder-next pkg?

@SamWhited

This comment has been minimized.

Copy link
Contributor Author

SamWhited commented Sep 11, 2019

@AkihiroSuda this is just to run the integration tests.

@SamWhited SamWhited force-pushed the SamWhited:test_dockerd branch 6 times, most recently from 5bca241 to 7e49ae4 Sep 11, 2019
@SamWhited SamWhited force-pushed the SamWhited:test_dockerd branch from b4e903d to 13edfa2 Nov 5, 2019
@SamWhited SamWhited force-pushed the SamWhited:test_dockerd branch from 13edfa2 to 3202f17 Nov 5, 2019
SamWhited added a commit to SamWhited/buildkit that referenced this pull request Nov 6, 2019
This pulls in
moby/moby@b96a0c7
which is needed by moby#1164.
It is submitted as a separate PR to keep the diff on moby#1164 small.
SamWhited added a commit to SamWhited/buildkit that referenced this pull request Nov 6, 2019
This pulls in
moby/moby@b96a0c7
which is needed by moby#1164.
It is submitted as a separate PR to keep the diff on moby#1164 small.

Signed-off-by: Sam Whited <sam@samwhited.com>
SamWhited added a commit to SamWhited/buildkit that referenced this pull request Nov 6, 2019
This pulls in
moby/moby@b96a0c7
which is needed by moby#1164.
It is submitted as a separate PR to keep the diff on moby#1164 small.

Signed-off-by: Sam Whited <sam@samwhited.com>
@SamWhited SamWhited force-pushed the SamWhited:test_dockerd branch from 3202f17 to 180fc4c Nov 6, 2019
SamWhited added a commit to SamWhited/buildkit that referenced this pull request Nov 7, 2019
This pulls in
moby/moby@b96a0c7
which is needed by moby#1164.
It is submitted as a separate PR to keep the diff on moby#1164 small.

Signed-off-by: Sam Whited <sam@samwhited.com>
SamWhited added a commit to SamWhited/buildkit that referenced this pull request Nov 7, 2019
This pulls in
moby/moby@b96a0c7
which is needed by moby#1164.
It is submitted as a separate PR to keep the diff on moby#1164 small.

Signed-off-by: Sam Whited <sam@samwhited.com>
SamWhited added a commit to SamWhited/buildkit that referenced this pull request Nov 7, 2019
This pulls in
moby/moby@b96a0c7
which is needed by moby#1164.
It is submitted as a separate PR to keep the diff on moby#1164 small.

Signed-off-by: Sam Whited <sam@samwhited.com>
@SamWhited SamWhited force-pushed the SamWhited:test_dockerd branch from 180fc4c to 4da68c5 Nov 7, 2019
@SamWhited

This comment has been minimized.

Copy link
Contributor Author

SamWhited commented Nov 7, 2019

Once #1245 is merged this PR should also be ready, then I can update moby/moby#40023 to stop using my fork.

SamWhited added a commit to SamWhited/buildkit that referenced this pull request Nov 7, 2019
This pulls in moby/moby@b96a0c7 which is needed by moby#1164.
It is submitted as a separate PR to keep the diff on moby#1164 small.

Signed-off-by: Sam Whited <sam@samwhited.com>
@SamWhited SamWhited force-pushed the SamWhited:test_dockerd branch from 4da68c5 to f2b44ac Nov 7, 2019
SamWhited added a commit to SamWhited/buildkit that referenced this pull request Nov 7, 2019
This pulls in moby/moby@b96a0c7 which is needed by moby#1164.
It is submitted as a separate PR to keep the diff on moby#1164 small.

Signed-off-by: Sam Whited <sam@samwhited.com>
@SamWhited SamWhited force-pushed the SamWhited:test_dockerd branch from f2b44ac to 2d22e77 Nov 7, 2019
This pulls in moby/moby@b96a0c7 which is needed by #1164.
It is submitted as a separate PR to keep the diff on #1164 small.

Signed-off-by: Sam Whited <sam@samwhited.com>
@SamWhited SamWhited force-pushed the SamWhited:test_dockerd branch from 2d22e77 to 3412fcf Nov 7, 2019
@SamWhited

This comment has been minimized.

Copy link
Contributor Author

SamWhited commented Nov 7, 2019

💚 tests; PTAL, thanks! A complete run with green tests has also been run on the docker side of things.

@SamWhited SamWhited referenced this pull request Nov 7, 2019
@SamWhited SamWhited force-pushed the SamWhited:test_dockerd branch 2 times, most recently from 6a513c8 to 32d075c Nov 7, 2019
client/client_test.go Outdated Show resolved Hide resolved
client/client_test.go Show resolved Hide resolved
util/testutil/integration/dockerd.go Outdated Show resolved Hide resolved
util/testutil/integration/dockerd.go Outdated Show resolved Hide resolved
frontend/dockerfile/dockerfile_test.go Show resolved Hide resolved
@SamWhited SamWhited force-pushed the SamWhited:test_dockerd branch 3 times, most recently from 552289a to 17e3dfd Nov 7, 2019
Signed-off-by: Sam Whited <sam@samwhited.com>
@SamWhited SamWhited force-pushed the SamWhited:test_dockerd branch from 17e3dfd to 58cf258 Nov 8, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.