Test dockerfile in gateway mode #552

ijc · 2018-08-01T13:04:18Z

This arose as a possibility in #533 (comment). I have a few concerns though:

This pulls tonistiigi/dockerfile:latest, which I suspect (I'm not entirely sure, but I am pretty confident) isn't always going to work e.g. when adding a new dockerfile feature and testing it. Need to find a mechanism to instead inject a locally freshly built image.
When run independently the builtin tests take 3m51 (wallclock) and the gateway ones 5m55 (wallclock).
- a) gateway is a surprising amount slower (nb: this was run on my laptop, so not exactly controlled conditions)
- b) when adding client (i.e. the work in access gateway API from client #533) as a third mode the overall test breaches the 10min limit imposed by go test on a single test (even though it is 3 separate top level TestFoo functions). I couldn't see a programmatic way to get around that (could use go test -timeout but that's not ideal).
I marked a bunch of tests as t.Skip/TBD in gateway mode since they were failing for me, need to investigate why.

Also:

Dropped a duplicated testNoSnapshotLeak

ijc · 2018-08-01T13:23:54Z

Pushed a bit more than I intended, now dropped.

I guess frontend-opt=gateway-devel is waaaay too expensive to use here, would make an already slow test unbearable (I don't think there is any cache sharing between sandboxes even).

ijc · 2018-08-01T15:25:30Z

Re time taken, perhaps TestDockerfileMasterGatewayIntegration should just be gated on a tag? So not run in CI etc but runnable manually?

tonistiigi · 2018-08-01T17:31:28Z

frontend/dockerfile/dockerfile_test.go

+			if opt.FrontendAttrs == nil {
+				opt.FrontendAttrs = make(map[string]string)
+			}
+			opt.FrontendAttrs["source"] = "tonistiigi/dockerfile"


tonistiigi/dockerfile:master

Done, I had misread https://hub.docker.com/r/tonistiigi/dockerfile/tags/. I will also revisit "dockerfile: skip some tests which appear to fail in gateway mode" since this may have changed the results.

BTW, I still have this concern from the intro:

which I suspect (I'm not entirely sure, but I am pretty confident) isn't always going to work e.g. when adding a new dockerfile feature and testing it. Need to find a mechanism to instead inject a locally freshly built image.

will also revisit "dockerfile: skip some tests which appear to fail in gateway mode"

All apart from TestDockerfileInvalidCommand now pass and I can see the issue with that one so I updated the skip reason and commit message to reflect that.

It does imply that image skew is a real problem though.

tonistiigi · 2018-08-01T17:42:20Z

I'd assume the performance is from pulling from registry for every test. We could set up a shared local registry and use that in the tests (and later for busybox/alpine as well).

For the setup I think it might make sense to have a list/map like allTests and then if we want to add a new configuration it is enabled with this list. Then we can easily control with env when some configs are disabled and add specific tests that only run on a specific configuration with experimental features. It may also make sense to not run all the workers configuration with the external dockerfile configs by default.

ijc · 2018-08-02T09:23:52Z

I'd assume the performance is from pulling from registry for every test.

Ah, yes, that's plausible.

For the setup I think it might make sense to have a list/map like allTests and then if we want to add a new configuration it is enabled with this list.

You intend this to be a list/map of that matrix of runtime and frontend mode eg. "oci + gateway"/ "containerd + builtin", as opposed to a list of tests like the one in dispatchAll, right?

It may also make sense to not run all the workers configuration with the external dockerfile configs by default.

Indeed, it's complete overkill right now.

ijc · 2018-08-02T13:25:31Z

~~What about instead of a local registry we produce (once at the start) something which can be used with buildctl build --import-cache?~~ Nevermind, that goes via a registry too...

tonistiigi · 2018-08-02T17:43:12Z

You intend this to be a list/map of that matrix of runtime and frontend mode eg. "oci + gateway"/ "containerd + builtin", as opposed to a list of tests like the one in dispatchAll, right?

I'm not 100% sure how to tie it with specific workers but the issue with dispatchAll is that it can only run everything together by only configuring one predefined variable (mode). I think it would be more flexible if the instead of the mode what you would configure would be the list of tests.

ijc · 2018-08-03T09:22:48Z

I'd been toying with an idea in my head of something like:

type C struct { // a (C)ell in a test Matrix
    mode M
    worker W // TBD the real type
}

const (
    BuiltinOCI = C{Builtin, OCI},
    BuiltinContainerd = C{Builtin, Containerd},
    GatewayOCI = C{Gateway, OCI},
    ...etc
}

type T struct { // a (T)est
    func(*testing.T, integration.Sandbox, *frontend)
    []C
}

Then have a list in dispatchAll

cases := []T{
    {testNoLeakSnapshot,  []C{BuiltinOCI, GatewayOCI, ... },
    {testSomethingElse,   []C{BuiltinOCI, GatewayContainerd, ...},
...
}

With the idea that TestingWithMode.Call would be responsible for filtering on the runtime (since integration.Dispatch handles iterating over those already) and dispatchAll is the one iterating over the modes to make specific lists of tests for each mode.

It's a bit horribly verbose (although shorter aliases would help). Sadly I think gofmt would prevent the most readable layout:

cases := []T{
    {testNoLeakSnapshot,  []C{BuiltinOCI,                    GatewayOCI, ... },
    {testSomethingElse,   []C{BuiltinOCI,                                GatewayContainerd, ...},
    {testAnother,         []C{BuiltinOCI, BuiltinContainerd, 
...
}

Which exposes the matrix visually. If gofmt could be coerced into cooperation then []C could instead be a bunch of named bool and you would have:

cases := []T{              // BuiltinOCI BuildingContainerd GatewayOCI, GatewayContainerd 
    {testNoLeakSnapshot,  []C{true,      false,             true,       false },
    {testSomethingElse,   []C{true,      true,              false,      false },
...
}

It doesn't really scale to any additional dimensions over {test×runtime×mode} though. I'm also a bit wary of slicing the matrix since it's non-obvious which mode/runtime combinations might expose various types of bugs reliably.

Another way would be to repeat the test functions, so

cases := []T{
    {testNoLeakSnapshot,  Builtin, OCI },
    {testNoLeakSnapshot,  Gateway, OCI },
    {testSomethingElse,   Builtin, OCI },
    {testSomethingElse,   Gateway, Containerd },
    {testAnother,         Builtin, OCI },
    {testAnother,         Builtin, Containerd },

Doesn't give you quite the same overview of the matrix, but does scale more easily to other dimensions (are there other dimensions we might care about here?)

ijc · 2018-08-03T10:38:10Z

frontend/dockerfile/dockerfile_test.go

+	case ModeBuiltin:
+		cmd += " --frontend dockerfile.v0"
+	case ModeMasterGateway:
+		cmd += " --frontend gateway.v0 --frontend-opt=source=tonistiigi/dockerfile"


This one should have :master on it too. This also explained the failure of TestDockerfileInvalidCommand so that commit can now be dropped too.

ijc · 2018-08-07T09:33:15Z

@tonistiigi Rereading #552 (comment) perhaps I overthought it and all you meant was that dispatch all should be:

dispatchAll(t *testing.T, m Mode, []func(*testing.T, integration.Sandbox, *frontend))

So that each of TestDockerfiler*Integeration could pass a specific list of tests (perhaps utilizing some helper slices of common ones).

ijc · 2018-08-08T08:23:28Z

Related to the overall time taken to test 3 modes, what do you think about dropping the builtin mode altogether and just having client side gateway and frontend modes?

tonistiigi · 2018-08-08T21:09:58Z

@ijc Builtin is the default on moby so I'm hesitant to drop it atm.

ijc · 2018-08-09T09:19:41Z

Ah, I had just assumed it was via the gateway, nm.

ijc · 2018-08-17T15:20:05Z

Rebased and added the Client variant to the mix, but didn't address any of the other stuff yet since I'm not sure which path to take.

Looks like Client mode is leaking some snapshots (checkAllRemoved is failing in a bunch of cases). Delving into testNoSnapshotLeak it looks like it has leaked an extra snapshot for each of the two local sources. Haven't figured out where/why or what it has to do with client mode though.

ijc · 2018-08-21T09:05:18Z

It's a bit of a hack but I added a commit which causes go test ./frontend/dockerfile to only run the integration tests in builtin mode by default and to support a $BUILDKIT_DOCKERFILE_INTEGRATION_MODE envvar to override, plus added code to hack/test to run the other two modes separately if the envvar is not set.

ijc · 2018-08-21T09:06:29Z

(nb: this will still fail until #582 is merged)

There is no need to repeat this test. Signed-off-by: Ian Campbell <ijc@docker.com>

This adds the `TestDispatch` interface, which gives the caller the opportunity to nominate a function to use to calculate the name. This is useful if the real test function takes arguments other than `*testing.T, Sandbox` since a wrapper (possibly anonymous) can then be used to set things up and produce the extra arguments etc. The existing `Test` type implements the new interface and a simple helper function means existing code continues to work. Signed-off-by: Ian Campbell <ijc@docker.com>

…uiltin` To achive this each test case now takes an additional `frontend` parameter which provides an appropriate `solver` and `dfCmdArgs` which are the only places so far that need to differ based on the way the frontend is run. Uses the new `TestDispatcher` interface to allow this new option to be added. Signed-off-by: Ian Campbell <ijc@docker.com>

…a time Cummulatively running in all modes (or even two of them) hits the 10min default maximum time for a test. Instead only run in builtin mode (the previous sole mode) by default and require an environment variable (`BUILDKIT_DOCKERFILE_INTEGRATION_MODE`) to test the other modes. Update `hack/test` to run the other modes after the main tests iff the envvar was not set. Signed-off-by: Ian Campbell <ijc@docker.com>

ijc · 2018-08-31T12:42:40Z

I rebased. In theory this should now all pass. The two new tests picked up by the rebase (testQuotedMetaArgs and testIgnoreEntrypoint) are now both failing the TestDockerfileMasterGatewayIntegration case in ways which suggest that the image they are using has not picked up the corresponding fixes -- that's the first bullet in the PR description and I think it's a blocker.

I've dropped the WIP since I think that's the last remaining issue.

ijc · 2018-08-31T12:56:21Z

I'm going to see if I can convince TestDockerfileMasterGatewayIntegration to use gateway-devel=true mode to build the local gateway code ...

ijc · 2018-08-31T16:09:12Z

gateway-devel mode makes just the TestDockerfileMasterGatewayIntegration by itself exceed the 10min barrier.

tonistiigi · 2018-09-12T05:47:33Z

@ijc added local mirror support in #618 that should make this much more usable. I'll carry this after it gets merged with some tweaks

ijc · 2018-09-12T10:57:21Z

Cool thanks, will watch out for it.

ijc · 2018-09-13T09:51:35Z

Closing in favour of #622

ijc force-pushed the test-dockerfile-in-gateway-mode branch 2 times, most recently from ddd98b7 to f6c8bea Compare August 1, 2018 13:22

tonistiigi reviewed Aug 1, 2018

View reviewed changes

ijc force-pushed the test-dockerfile-in-gateway-mode branch from f6c8bea to 15186ca Compare August 2, 2018 09:17

ijc force-pushed the test-dockerfile-in-gateway-mode branch from 15186ca to 93156ac Compare August 2, 2018 10:07

ijc commented Aug 3, 2018

View reviewed changes

ijc force-pushed the test-dockerfile-in-gateway-mode branch 2 times, most recently from e29fb59 to 2972ec8 Compare August 3, 2018 13:56

ijc mentioned this pull request Aug 7, 2018

access gateway API from client #533

Merged

ijc force-pushed the test-dockerfile-in-gateway-mode branch from 2972ec8 to f420d15 Compare August 17, 2018 15:17

ijc mentioned this pull request Aug 20, 2018

Client-side frontend leaks local source snapshots. #581

Closed

Ian Campbell added 4 commits August 31, 2018 11:20

dockerfile: remove duplicate testNoSnapshotLeak

59d67db

There is no need to repeat this test. Signed-off-by: Ian Campbell <ijc@docker.com>

ijc force-pushed the test-dockerfile-in-gateway-mode branch from 69f418f to 2d5eacc Compare August 31, 2018 12:39

ijc changed the title ~~[WIP] Test dockerfile in gateway mode~~ Test dockerfile in gateway mode Aug 31, 2018

tonistiigi mentioned this pull request Sep 13, 2018

dockerfile: enable frontend selection in integration tests #622

Merged

ijc closed this Sep 13, 2018

ijc deleted the test-dockerfile-in-gateway-mode branch September 13, 2018 09:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Test dockerfile in gateway mode #552

Test dockerfile in gateway mode #552

ijc commented Aug 1, 2018

ijc commented Aug 1, 2018

ijc commented Aug 1, 2018

tonistiigi Aug 1, 2018

ijc Aug 2, 2018

ijc Aug 2, 2018

ijc Aug 2, 2018

tonistiigi commented Aug 1, 2018

ijc commented Aug 2, 2018

ijc commented Aug 2, 2018 •

edited

Loading

tonistiigi commented Aug 2, 2018

ijc commented Aug 3, 2018

ijc Aug 3, 2018

ijc commented Aug 7, 2018

ijc commented Aug 8, 2018

tonistiigi commented Aug 8, 2018

ijc commented Aug 9, 2018

ijc commented Aug 17, 2018

ijc commented Aug 21, 2018

ijc commented Aug 21, 2018

ijc commented Aug 31, 2018

ijc commented Aug 31, 2018

ijc commented Aug 31, 2018

tonistiigi commented Sep 12, 2018

ijc commented Sep 12, 2018

ijc commented Sep 13, 2018

Test dockerfile in gateway mode #552

Test dockerfile in gateway mode #552

Conversation

ijc commented Aug 1, 2018

ijc commented Aug 1, 2018

ijc commented Aug 1, 2018

tonistiigi Aug 1, 2018

Choose a reason for hiding this comment

ijc Aug 2, 2018

Choose a reason for hiding this comment

ijc Aug 2, 2018

Choose a reason for hiding this comment

ijc Aug 2, 2018

Choose a reason for hiding this comment

tonistiigi commented Aug 1, 2018

ijc commented Aug 2, 2018

ijc commented Aug 2, 2018 • edited Loading

tonistiigi commented Aug 2, 2018

ijc commented Aug 3, 2018

ijc Aug 3, 2018

Choose a reason for hiding this comment

ijc commented Aug 7, 2018

ijc commented Aug 8, 2018

tonistiigi commented Aug 8, 2018

ijc commented Aug 9, 2018

ijc commented Aug 17, 2018

ijc commented Aug 21, 2018

ijc commented Aug 21, 2018

ijc commented Aug 31, 2018

ijc commented Aug 31, 2018

ijc commented Aug 31, 2018

tonistiigi commented Sep 12, 2018

ijc commented Sep 12, 2018

ijc commented Sep 13, 2018

ijc commented Aug 2, 2018 •

edited

Loading