Skip to content

Misc test improvements#35840

Merged
cpuguy83 merged 4 commits intomoby:masterfrom
kolyshkin:misc-test
Jan 2, 2018
Merged

Misc test improvements#35840
cpuguy83 merged 4 commits intomoby:masterfrom
kolyshkin:misc-test

Conversation

@kolyshkin
Copy link
Contributor

@kolyshkin kolyshkin commented Dec 20, 2017

Those are some test fixes I made while working on other stuff. Those are largely unrelated, so please see descriptions in individual commit messages (for the same reason, please review commits individually so you won't miss those descriptions).

@AkihiroSuda AkihiroSuda added the status/failing-ci Indicates that the PR in its current state fails the test suite label Dec 20, 2017
@thaJeztah
Copy link
Member

Windows failure is a test that will be removed; #35844

03:09:41 ----------------------------------------------------------------------
03:09:41 FAIL: docker_cli_events_test.go:84: DockerSuite.TestEventsLimit
03:09:41 
03:09:41 docker_cli_events_test.go:119:
03:09:41     c.Assert(err, checker.IsNil, check.Commentf("%q failed with error", strings.Join(args, " ")))
03:09:41 ... value *errors.errorString = &errors.errorString{s:"exit status 125: d:\\CI\\CI-f5a5c119a\\binary\\docker.exe: Error response from daemon: hcsshim::PrepareLayer failed in Win32: This operation returned because the timeout period expired. (0x5b4) layerId=ebd699ccff4ea5dc7bcd6a1a358e62c9b3220534ca404b9df21f2233f27cb2a8 flavour=1.\n"} ("exit status 125: d:\\CI\\CI-f5a5c119a\\binary\\docker.exe: Error response from daemon: hcsshim::PrepareLayer failed in Win32: This operation returned because the timeout period expired. (0x5b4) layerId=ebd699ccff4ea5dc7bcd6a1a358e62c9b3220534ca404b9df21f2233f27cb2a8 flavour=1.\n")
03:09:41 ... "run --rm busybox true" failed with error
03:09:41 

TestDaemonRestartKillContainers looks to be broken on PowerPC (saw it fail on other PR's)

02:37:57 --- FAIL: TestDaemonRestartKillContainers (0.00s)
02:37:57     --- FAIL: TestDaemonRestartKillContainers/live-restore=true/container_without_restart_policy/stop-daemon (3.30s)
02:37:57     	daemon.go:285: [d8d26ccf8b04b] waiting for daemon to start
02:37:57     	daemon.go:285: [d8d26ccf8b04b] waiting for daemon to start
02:37:57     	daemon.go:203: Error starting daemon with arguments: [--live-restore]
02:37:57     	daemon.go:275: [d8d26ccf8b04b] exiting daemon
02:37:57 FAIL

@thaJeztah
Copy link
Member

ping @cpuguy83 ^^ that test was added in #35805

Copy link
Member

Choose a reason for hiding this comment

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

but \x00 is NULL, not 0?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

'\x00' is a rune which results in a byte having all bits set to zero. 0 is an integer constant having all bits set to zero. These two have the same effect when assigned to a byte (see https://play.golang.org/p/IUboS5HCYkI). Now sure what do you mean by NULL, but if there would be NULL in Go as it is in C, it will also be equal to 0 and \x00 (although, if I remember it correctly, this is not guaranteed by the C language specification, but is true in practice for every implementation known). Now, nil might be equivalent, too, but due to strict[er than C] type checking it can not be assigned to a byte.

The code before and after this patch is equivalent in what it's doing, but somehow Go fails to optimize the binary code for the old version, and optimizes the new one pretty good, as described in patch commit message.

Copy link
Member

Choose a reason for hiding this comment

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

Oh, thanks! I for some reason thought NULL would be different than 0; shows what I know 😊

@cpuguy83
Copy link
Member

Error is because the daemon failed to start:

Error starting daemon: Error initializing network controller: Error creating default "bridge" network: Failed to Setup IP tables: Unable to enable SKIP DNAT rule: (iptables failed: iptables --wait -t nat -I DOCKER -i docker0 -j RETURN: iptables: No chain/target/match by that name.
(exit status 1))

1. The functionality of this test is superceded by
   `TestAPIIpcModeShareableAndContainer` (see
   integration-cli/docker_api_ipcmode_test.go).

2. This test won't work with --default-ipc-mode private.

Signed-off-by: Kir Kolyshkin <[email protected]>
`TestCleanupMountsAfterDaemonAndContainerKill` was supposedly written
when the container mounts were visible from the host. Currently they
all live in their own mount namespace and the only visible mount is
the tmpfs one for shareable /dev/shm inside the container (i.e.
/var/lib/docker/containers/<ID>/shm), which will no longer be there
in case of `--default-ipc-mode private` is used, and so the test will
fail. Add a check if any container mounts are visible from the host,
and skip the test if there are none, as there's nothing to check.

`TestCleanupMountsAfterDaemonCrash`: fix in a similar way, keeping
all the other checks it does, and skipping the "mounts gone" check
if there were no mounts visible from the host.

While at it, also fix the tests to use `d.Kill()` in order to not
leave behind a stale `docker.pid` files.

Signed-off-by: Kir Kolyshkin <[email protected]>
I got the following test failure on power:

10:00:56
10:00:56
----------------------------------------------------------------------
10:00:56 FAIL: docker_cli_build_test.go:3521:
DockerSuite.TestBuildNotVerboseFailureRemote
10:00:56
10:00:56 docker_cli_build_test.go:3536:
10:00:56     c.Fatal(fmt.Errorf("Test[%s] expected that quiet stderr and
verbose stdout are equal; quiet [%v], verbose [%v]", name,
quietResult.Stderr(), result.Combined()))
10:00:56 ... Error: Test[quiet_build_wrong_remote] expected that quiet
stderr and verbose stdout are equal; quiet [
10:00:56 unable to prepare context: unable to download remote context
http://something.invalid: Get http://something.invalid: dial tcp: lookup
something.invalid on 172.29.128.11:53: no such host
10:00:56 ], verbose [unable to prepare context: unable to download
remote context http://something.invalid: Get http://something.invalid:
dial tcp: lookup something.invalid on 8.8.8.8:53: no such host
10:00:56 ]
10:00:56
10:00:56
10:00:56
----------------------------------------------------------------------

The reason is, either more than one name server is configured, or
nameserver was reconfigured in the middle of the test run. In any case,
different nameserver IP in an error messages should not be treated
as a failure, so let's strip those out.

Signed-off-by: Kir Kolyshkin <[email protected]>
According to golang/go#5373, go recognizes
(and optimizes for) the following syntax:

```go
for i := range b {
	b[i] = 0
}
```

so let's use it. Limited testing shows ~7.5x speed increase,
compared to the previously used syntax.

Signed-off-by: Kir Kolyshkin <[email protected]>
Copy link
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

Copy link
Member

@vdemeester vdemeester left a comment

Choose a reason for hiding this comment

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

LGTM 🦁

Copy link
Member

@cpuguy83 cpuguy83 left a comment

Choose a reason for hiding this comment

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

LGTM

@cpuguy83 cpuguy83 merged commit c345c53 into moby:master Jan 2, 2018
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.

6 participants