Unit tests fail in docker container under root user

realloc commented

2023-03-15 10:15:58 +00:00

Owner

Current Behavior

I run unit test inside docker container and fail, while same tests pass well in normal Linux host environment.

As a consequence, unit tests fail in CI.

Steps to Reproduce

realloc@ruka ~> docker run --rm -it --entrypoint bash golang:1.19
root@21d95221cc37:/go# cd /tmp
root@21d95221cc37:/tmp# git clone https://git.frostfs.info/TrueCloudLab/frostfs-node.git
root@21d95221cc37:/tmp# cd frostfs-node/
root@21d95221cc37:/tmp/frostfs-node# make test
⇒ Running go test
...
--- FAIL: TestExists (0.03s)
    --- FAIL: TestExists/corrupt_direcrory (0.00s)
        exists_test.go:78: 
            	Error Trace:	/tmp/frostfs-node/pkg/local_object_storage/blobstor/exists_test.go:78
            	Error:      	An error is expected but got nil.
            	Test:       	TestExists/corrupt_direcrory
FAIL
FAIL	git.frostfs.info/TrueCloudLab/frostfs-node/pkg/local_object_storage/blobstor	0.512s
...
FAIL
make: *** [Makefile:128: test] Error 1

## Current Behavior I run unit test inside docker container and fail, while same tests pass well in normal Linux host environment. As a consequence, unit tests fail in CI. ## Steps to Reproduce ``` realloc@ruka ~> docker run --rm -it --entrypoint bash golang:1.19 root@21d95221cc37:/go# cd /tmp root@21d95221cc37:/tmp# git clone https://git.frostfs.info/TrueCloudLab/frostfs-node.git root@21d95221cc37:/tmp# cd frostfs-node/ root@21d95221cc37:/tmp/frostfs-node# make test ⇒ Running go test ... --- FAIL: TestExists (0.03s) --- FAIL: TestExists/corrupt_direcrory (0.00s) exists_test.go:78: Error Trace: /tmp/frostfs-node/pkg/local_object_storage/blobstor/exists_test.go:78 Error: An error is expected but got nil. Test: TestExists/corrupt_direcrory FAIL FAIL git.frostfs.info/TrueCloudLab/frostfs-node/pkg/local_object_storage/blobstor 0.512s ... FAIL make: *** [Makefile:128: test] Error 1 ```

realloc added the

good first issue

triage

labels 2023-03-15 10:15:58 +00:00

realloc commented

2023-03-15 10:17:09 +00:00

Author

Owner

Here is a test run log.

unitfail.txt

24 KiB

fyrchik was assigned by realloc

2023-03-15 10:19:56 +00:00

realloc added reference master

2023-03-15 10:25:27 +00:00

fyrchik commented

2023-03-15 12:17:46 +00:00

Owner

There is a bunch of tests where we try to emulate some problems with the storage. This is done by setting bad permissions on some internal directories.

Tests from docker are run from root, so changing permissions doesn't work.
I have tried adding a user and running tests from it -- everything works.

I see 2 solutions here:

Skip some tests in this case. This is not desireable, because I currently see no better way of doing this and the tests cover important logic in code.
Run tests not from root.

There is a bunch of tests where we try to emulate some problems with the storage. This is done by setting bad permissions on some internal directories. Tests from docker are run from root, so changing permissions doesn't work. I have tried adding a user and running tests from it -- everything works. I see 2 solutions here: 1. Skip some tests in this case. This is not desireable, because I currently see no better way of doing this and the tests cover important logic in code. 2. Run tests not from root.

ale64bit commented

2023-03-16 11:57:17 +00:00

Member

Is it possible to use https://pkg.go.dev/io/fs#FS instead of os everywhere? This way we can mock these things without resorting to perm tricks and make at least the unit tests fully hermetic.

Is it possible to use https://pkg.go.dev/io/fs#FS instead of `os` everywhere? This way we can mock these things without resorting to perm tricks and make at least the unit tests fully hermetic.

🚀 1

ale64bit was assigned by fyrchik

2023-03-16 15:25:39 +00:00

fyrchik removed their assignment 2023-03-16 15:25:43 +00:00

ale64bit commented

2023-03-17 10:14:39 +00:00

Member

Using fs.FS is not trivial for the following reasons:

It's only read-only for the time being (https://github.com/golang/go/issues/45757).
Even if we embed it in an extended interface supporting mutation, we depend on libraries which might not be interoperable with it. For example, bbolt allows overriding the OpenFile function, but this is not quite enough since it still requires an os.File in the return type and internally it uses several OS-specific operations.

We are evaluating what can be done.

Using `fs.FS` is not trivial for the following reasons: 1. It's only read-only for the time being (https://github.com/golang/go/issues/45757). 2. Even if we embed it in an extended interface supporting mutation, we depend on libraries which might not be interoperable with it. For example, bbolt allows overriding the `OpenFile` function, but this is not quite enough since it still requires an `os.File` in the return type and internally it uses several OS-specific operations. We are evaluating what can be done.

realloc removed the

triage

label 2023-03-17 14:10:21 +00:00

realloc changed title from ~~Unit tests fail in docker container~~ to Unit tests fail in docker container under root user

2023-03-17 14:20:28 +00:00

ale64bit commented

2023-03-20 09:04:12 +00:00

Member

Let's add some artificial unit test to show a warning when tests are running under root, at least for debugging purposes for now, since our code is quite entangled with os usage and bbolt itself.

Let's add some artificial unit test to show a warning when tests are running under `root`, at least for debugging purposes for now, since our code is quite entangled with `os` usage and bbolt itself.

dstepanov-yadro commented

2023-03-20 09:27:14 +00:00

Member

My suggestion:

Mark tests that require OS-interaction with the integration tag.
Fix make test to run unit tests with integration tag (unit tests with tag and without any tag will be run)
Add a new target, make ci-test, which will run unit tests without integration tag and be used in CI/CD pipeline.

My suggestion: 1. Mark tests that require OS-interaction with the ```integration``` tag. 2. Fix ```make test``` to run unit tests with ```integration``` tag (unit tests with tag and without any tag will be run) 3. Add a new target, ```make ci-test```, which will run unit tests without ```integration``` tag and be used in CI/CD pipeline.

ale64bit commented

2023-03-20 09:35:00 +00:00

Member

IMHO, any unit test that requires OS interaction is not really "unit" anymore. The thing with those failing tests is that they use some unorthodox way of verifying their behavior, so we should probably fix that if possible.

But the tag could be useful for e2e tests, etc. as you mention. I'll let others chime in.

IMHO, any unit test that requires OS interaction is not really "unit" anymore. The thing with those failing tests is that they use some unorthodox way of verifying their behavior, so we should probably fix that if possible. But the tag could be useful for e2e tests, etc. as you mention. I'll let others chime in.

👍 1

fyrchik commented

2023-03-20 12:09:44 +00:00

Owner

There are 2 groups of tests here:

Test how we handle error on a lower level on an upper level.
Test that specific errors (not wrapped as logical) increase engine error counter.

For most of this cases we could actually use a test interface (memory store).
I suggest making a list of currently failing tests, and change them one-by-one.

There are 2 groups of tests here: 1. Test how we handle error on a lower level on an upper level. 2. Test that specific errors (not wrapped as `logical`) increase engine error counter. For most of this cases we could actually use a test interface (memory store). I suggest making a list of currently failing tests, and change them one-by-one.

ale64bit closed this issue

2023-03-20 15:07:14 +00:00

ale64bit reopened this issue

2023-03-20 15:07:21 +00:00

ale64bit commented

2023-03-20 15:18:05 +00:00

Member

What about we create a TestStorage implementation of common.Storage which can wrap other implementations and passthrough certain calls to the underlying storage while setting some test expectations or mocking calls for others? That way, we can express exactly the expected behavior of each substorage while minimizing the amount of OS interaction.

e.g.

func TestXXX(t *testing.T) {
    testStorage := teststorage.New(t, fsstree.New(fstree.WithPath(p)))
    storages := []Substorage{
      {
        Storage: testStorage,
      },
    }

    (...)

    testStorage.OnGet(common.GetPrm{...}).Return(common.GetResp{...}, nil)
    testStorage.OnPut(common.PutPrm{...}).Return(nil, errors.New("terrible things happened"))
    testStorage.FailOnUnexpected()

Not necessarily like above, but hopefully you get the idea.

What about we create a `TestStorage` implementation of `common.Storage` which can wrap other implementations and passthrough certain calls to the underlying storage while setting some test expectations or mocking calls for others? That way, we can express exactly the expected behavior of each substorage while minimizing the amount of OS interaction. e.g. ``` func TestXXX(t *testing.T) { testStorage := teststorage.New(t, fsstree.New(fstree.WithPath(p))) storages := []Substorage{ { Storage: testStorage, }, } (...) testStorage.OnGet(common.GetPrm{...}).Return(common.GetResp{...}, nil) testStorage.OnPut(common.PutPrm{...}).Return(nil, errors.New("terrible things happened")) testStorage.FailOnUnexpected() ``` Not necessarily like above, but hopefully you get the idea.

👍 1

realloc added a new dependency 2023-03-20 15:40:53 +00:00

#149 Add Woodpecker CI pipelines

realloc removed a dependency 2023-03-20 15:41:25 +00:00

#149 Add Woodpecker CI pipelines

realloc commented

2023-03-20 15:42:10 +00:00

Author

Owner

Related: #149

👍 1

realloc referenced this issue from a commit

2023-03-20 18:40:58 +00:00

[#149] Use custom image and kludges for node

ale64bit referenced this issue

2023-03-22 08:19:26 +00:00

[#139] test: Add test storage implementation #160

ale64bit commented

2023-03-22 08:25:35 +00:00

Member

Let me know if the changes in #160 look reasonable. I'm not a big fan of mocking in general, but in measure I guess it's better than chmodding stuff or depending on particular conditions (such as the user running the test) for results.

There are two tests which still fail under root:

https://git.frostfs.info/TrueCloudLab/frostfs-node/src/branch/master/pkg/local_object_storage/blobstor/blobovniczatree/exists_test.go#L52: this one is specific to blobovnicza implementation. Not sure what to do about it yet.
https://git.frostfs.info/TrueCloudLab/frostfs-node/src/branch/master/pkg/local_object_storage/shard/dump_test.go#L284: this one returns an unexpected number of errors, but at least on my system it seems phony. When running under normal user and commenting the three blocks that supposedly introduce errors, the test passes anyway.

Let me know if the changes in #160 look reasonable. I'm not a big fan of mocking in general, but in measure I guess it's better than chmodding stuff or depending on particular conditions (such as the user running the test) for results. There are two tests which still fail under root: - https://git.frostfs.info/TrueCloudLab/frostfs-node/src/branch/master/pkg/local_object_storage/blobstor/blobovniczatree/exists_test.go#L52: this one is specific to blobovnicza implementation. Not sure what to do about it yet. - https://git.frostfs.info/TrueCloudLab/frostfs-node/src/branch/master/pkg/local_object_storage/shard/dump_test.go#L284: this one returns an unexpected number of errors, but at least on my system it seems phony. When running under normal user and commenting the three blocks that supposedly introduce errors, the test passes anyway.

ale64bit referenced this issue

2023-03-24 09:27:13 +00:00

[#139] test: Add test storage implementation #173

fyrchik referenced this issue from a commit

2023-03-29 14:28:50 +00:00

[#139] test: Add test storage implementation

ale64bit referenced this issue from a pull request that will close it,

2023-04-14 09:20:01 +00:00

[#139] Refactor blobovnicza exists test to not use chmod #253

fyrchik closed this issue

2023-04-14 10:17:59 +00:00

fyrchik referenced this issue from a commit