Looks like currently, d.log is always the default noplogger, so nothing gets logged (I didn't find any occurrences of WithTestLogger() used anywhere)

I wonder if we should/could have these logs printed only in the case that the test fails. I seem to recall that was the default behaviour of Go tests (see golang/go#21461), but it looks like having -v (verbose) prints each test, including logs and things printed on stdout/stderr (and for the junit.xml files to be created, I think we need -v)

Dropped this commit, because this would also unmount the daemon root if live-restore is enabled, probably causing this failure:

 === FAIL: amd64.integration-cli TestDockerDaemonSuite/TestExecWithUserAfterLiveRestore (2.90s)
     --- FAIL: TestDockerDaemonSuite/TestExecWithUserAfterLiveRestore (2.90s)
         daemon.go:26: Creating a new daemon at: "/go/src/github.com/docker/docker/bundles/test-integration/TestDockerDaemonSuite/TestExecWithUserAfterLiveRestore"
         docker_cli_daemon_test.go:2723: assertion failed: expression is false: err == nil: Output: unable to find user test: no matching entries in passwd file

While at it, we can switch to using mount.Unmount() which does all the same things.

Yet better, use RecursiveUnmount() to further simplify things here.

I now recall I had a PR that made changes here; #36511, but for some reason it kept failing.

Let me try using the RecursiveUnmount() that sounds like a clean approach (if it works)

I take that back. Neither filepath.Walk() nor RecursiveUnmount() should be used here, as per #36511 (comment)

I.e. the approach in #36511 is the correct one; I would take that one here )

Let me rebase the other one, to keep this one simpler (and in case it's still problematic). I can rebase it on top of this one

Would be good to note in the commit message that this fixes bogus error from removing docker.pid that was never created.

If this is just to fix the error when the pid file doesn't exist, maybe check `os.IsNotExist so we don't ignore anything else? Something like:

if d.pidFile != "" {
  if err := os.Remove(d.pidFile); !os.IsNotExist(err) {
    return err
  }
}
return nil

@SamWhited I would

• not care about error returned from os.Remove() at all
• not return any error here as this will fail the test

I.e. current approach looks fine for me.

I know this was already in there, but it seems rather fragile. Maybe we could file an issue to fix this later, or if it's already returning a specific error type we could check if it's the appropriate type?

Perhaps we could have it check the daemon logs to check if it's the actual error we're expecting (although that one also changed between versions);

19.03

docker run -it --rm --privileged -v /var/lib/docker docker:19.03-dind dockerd --bridge=nosuchbridge --bip=1.1.1.1
# ....
# failed to start daemon: You specified -b & --bip, mutually exclusive options. Please specify only one

17.06

docker run -it --rm --privileged -v /var/lib/docker docker:17.06-dind dockerd --bridge=nosuchbridge --bip=1.1.1.1
# ....
# Error starting daemon: You specified -b & --bip, mutually exclusive options. Please specify only one

oh, actually, that's the error we're looking for below 🤦‍♂

If this is just to fix the error when the pid file doesn't exist, maybe check `os.IsNotExist so we don't ignore anything else? Something like:

if d.pidFile != "" {
  if err := os.Remove(d.pidFile); !os.IsNotExist(err) {
    return err
  }
}
return nil

Should we also log the error here in case a pid file still exists that needs to be cleaned up?

@SamWhited I initially had that (see #40062 (comment)); question is though; is it important to fail (or log) the failure to remove the pidfile (if the test otherwise completed successfully)?

It could potentially let us know that something is wrong with our tests. I'd say that it's certainly not worth failing tests over, but is worth logging personally, but I defer to your judgement.