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

manager/state/raft/storage: fix TestCreateOpenInvalidDirFails #3087

Merged
merged 1 commit into from
Nov 19, 2022

Conversation

thaJeztah
Copy link
Member

This test failed on macOS, but passed in CI:

--- FAIL: TestCreateOpenInvalidDirFails (0.00s)
    walwrap_test.go:205:
            Error Trace:	walwrap_test.go:205
            Error:      	An error is expected but got nil.
            Test:       	TestCreateOpenInvalidDirFails
time="2022-11-19T14:36:00Z" level=info msg="repaired WAL error" error="unexpected EOF"
FAIL

It looks like this test was always broken; this test was added in 3e2cebe and running the test from that commit and with the Go version used at the time showed the test was failing;

git checkout 3e2cebe76b8c7e1ae67b89de3b5fa063234edc85

docker run -it --rm -v $(pwd):/go/src/github.com/docker/swarmkit -w /go/src/github.com/docker/swarmkit golang:1.7 sh -c 'go test -v -run TestCreateOpenInvalidDirFails ./manager/state/raft/storage/'

=== RUN   TestCreateOpenInvalidDirFails
--- FAIL: TestCreateOpenInvalidDirFails (0.01s)
        Error Trace:    walwrap_test.go:193
    Error:		An error is expected but got nil.

FAIL
exit status 1
FAIL	github.com/docker/swarmkit/manager/state/raft/storage	0.031s

It took some digging to understand why, but when debugging the error in CI (this part of the test expected an error), it became apparent:

--- FAIL: TestCreateOpenInvalidDirFails (0.00s)
    walwrap_test.go:207:
            Error Trace:	walwrap_test.go:207
            Error:      	Received unexpected error:
                            mkdir /not: permission denied
            Test:       	TestCreateOpenInvalidDirFails
time="2022-11-19T16:55:42Z" level=info msg="repaired WAL error" error="unexpected EOF"

So what happened was that;

  • walCryptor.Create() is called, which calls wal.Create() (from go.etcd.io/etcd/server/v3/wal/wal.go).
  • wal.Create() creates a new .tmp directory at the same location as the specified path (see https://github.com/etcd-io/etcd/blob/server/v3.5.1/server/wal/wal.go#L110-L127)
  • in our case, the directory passed was /not/existing/directory, so it tries to create /not/existing/directory.tmp.
  • which fails, because CI doesn't run as root and doesn't have permissions to create the path.

On macOS, running the test inside a container failed, as the tests are run as root inside the container. Similarly, when updating the test to use t.TempDir(), and using /not/existing/directory within that directory, the test failed as well (as there's no permissions issue when creating the .tmp directory inside the temp-dir).

So, in short, the test was broken from the start; calling was.Create() using a non-existing directory is allowed, and not an error condition. (If we do want it to be an error-condition, we should implement code to disallow this).

This patch removes the broken part from the test, and updates the remaining part to use t.TempDir() and a directory inside that's certain to be empty.

Also renaming the test to TestOpenInvalidDirFails, as the test now only covers the Open case.

@thaJeztah
Copy link
Member Author

@corhere @neersighted @dperny PTAL

This test failed on macOS, but passed in CI:

    --- FAIL: TestCreateOpenInvalidDirFails (0.00s)
        walwrap_test.go:205:
                Error Trace:	walwrap_test.go:205
                Error:      	An error is expected but got nil.
                Test:       	TestCreateOpenInvalidDirFails
    time="2022-11-19T14:36:00Z" level=info msg="repaired WAL error" error="unexpected EOF"
    FAIL

It looks like this test was always broken; this test was added in
3e2cebe and running the test from
that commit and with the Go version used at the time showed the test
was failing;

    git checkout 3e2cebe

    docker run -it --rm -v $(pwd):/go/src/github.com/docker/swarmkit -w /go/src/github.com/docker/swarmkit golang:1.7 sh -c 'go test -v -run TestCreateOpenInvalidDirFails ./manager/state/raft/storage/'

    === RUN   TestCreateOpenInvalidDirFails
    --- FAIL: TestCreateOpenInvalidDirFails (0.01s)
            Error Trace:    walwrap_test.go:193
        Error:		An error is expected but got nil.

    FAIL
    exit status 1
    FAIL	github.com/docker/swarmkit/manager/state/raft/storage	0.031s

It took some digging to understand why, but when debugging the error
in CI (this part of the test _expected_ an error), it became apparent:

    --- FAIL: TestCreateOpenInvalidDirFails (0.00s)
        walwrap_test.go:207:
                Error Trace:	walwrap_test.go:207
                Error:      	Received unexpected error:
                                mkdir /not: permission denied
                Test:       	TestCreateOpenInvalidDirFails
    time="2022-11-19T16:55:42Z" level=info msg="repaired WAL error" error="unexpected EOF"

So what happened was that;

- `walCryptor.Create()` is called, which calls `wal.Create()` (from
  go.etcd.io/etcd/server/v3/wal/wal.go).
- `wal.Create()` creates a new `.tmp` directory at the same location as
  the specified path (see https://github.com/etcd-io/etcd/blob/server/v3.5.1/server/wal/wal.go#L110-L127)
- in our case, the directory passed was `/not/existing/directory`, so
  it tries to create  `/not/existing/directory.tmp`.
- which fails, because CI doesn't run as `root` and doesn't have permissions
  to create the path.

On macOS, running the test inside a container failed, as the tests are
run as `root` inside the container. Similarly, when updating the test
to use `t.TempDir()`, and using `/not/existing/directory` within that
directory, the test failed as well (as there's no permissions issue when
creating the `.tmp` directory inside the temp-dir).

So, in short, the test was broken from the start; calling `was.Create()`
using a non-existing directory is allowed, and not an error condition.
(If we _do_ want it to be an error-condition, we should implement code
to disallow this).

This patch removes the broken part from the test, and updates the
remaining part to use `t.TempDir()` and a directory inside that's
certain to be empty.

Also renaming the test to `TestOpenInvalidDirFails`, as the test now
only covers the `Open` case.

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
@thaJeztah thaJeztah force-pushed the fix_TestCreateOpenInvalidDirFails branch from 35ab499 to a78d56a Compare November 19, 2022 17:50
@codecov-commenter
Copy link

Codecov Report

❗ No coverage uploaded for pull request base (master@f34bfc0). Click here to learn what that means.
The diff coverage is n/a.

@@            Coverage Diff            @@
##             master    #3087   +/-   ##
=========================================
  Coverage          ?   62.00%           
=========================================
  Files             ?      156           
  Lines             ?    24618           
  Branches          ?        0           
=========================================
  Hits              ?    15264           
  Misses            ?     7763           
  Partials          ?     1591           

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@thaJeztah
Copy link
Member Author

Thx! Let me bring this one in

@thaJeztah thaJeztah merged commit e516381 into moby:master Nov 19, 2022
@thaJeztah thaJeztah deleted the fix_TestCreateOpenInvalidDirFails branch November 19, 2022 22:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants