From 27ebe14cbb8120ed69a3469dddfb10ef9e017d92 Mon Sep 17 00:00:00 2001 From: "A.Unger" Date: Tue, 2 Feb 2021 12:40:02 +0100 Subject: [PATCH 1/5] add test file to capture a lack of atomicity on the oCIS storage driver upload --- pkg/storage/fs/ocis/ocis_test.go | 155 +++++++++++++++++++++++++++++++ 1 file changed, 155 insertions(+) create mode 100644 pkg/storage/fs/ocis/ocis_test.go diff --git a/pkg/storage/fs/ocis/ocis_test.go b/pkg/storage/fs/ocis/ocis_test.go new file mode 100644 index 0000000000..3de7882518 --- /dev/null +++ b/pkg/storage/fs/ocis/ocis_test.go @@ -0,0 +1,155 @@ +// +build storageRace + +package ocis + +import ( + "context" + "fmt" + userpb "github.com/cs3org/go-cs3apis/cs3/identity/user/v1beta1" + provider "github.com/cs3org/go-cs3apis/cs3/storage/provider/v1beta1" + "github.com/cs3org/reva/pkg/user" + "github.com/stretchr/testify/assert" + "io/ioutil" + "os" + "path" + "sync" + "testing" +) + +// TestLackAdvisoryLocks demonstrates that access to a file +// is not mutually exclusive on the oCIS storage. +var ( + config = make(map[string]interface{}) + ctx context.Context + f, f1 *os.File + tmpDir string +) + +func TestMain(m *testing.M) { + var err error + + // prepare storage + { + tmpDir, _ = ioutil.TempDir("", "ocis_fs_unittests") + { + config["root"] = tmpDir + config["enable_home"] = false + config["user_layout"] = "{{.Id.OpaqueId}}" + config["owner"] = "f7fbf8c8-139b-4376-b307-cf0a8c2d0d9c" + } + } + + // prepare context + { + u := &userpb.User{ + Id: &userpb.UserId{ + OpaqueId: "f7fbf8c8-139b-4376-b307-cf0a8c2d0d9c", + }, + Username: "test", + Mail: "marie@example.org", + DisplayName: "Marie Curie", + Groups: []string{ + "radium-lovers", + "polonium-lovers", + "physics-lovers", + }, + } + ctx = user.ContextSetUser(context.Background(), u) + } + + // do not do this. Prepare f0 + if err = ioutil.WriteFile(fmt.Sprintf("%s/%s", tmpDir,"f.lol"), []byte("test"), 0644); err != nil { + panic(err) + } + f, err = os.Open(fmt.Sprintf("%s/%s", tmpDir,"f.lol")) + if err != nil { + panic(err) + } + + // do not do this. Prepare f1 + if err = ioutil.WriteFile(fmt.Sprintf("%s/%s", tmpDir,"f1.lol"), []byte("another run"), 0644); err != nil { + panic(err) + } + f1, err = os.Open(fmt.Sprintf("%s/%s", tmpDir,"f1.lol")) + if err != nil { + panic(err) + } + + fmt.Printf("%s\n", tmpDir) + m.Run() + + cts, err := ioutil.ReadFile(path.Join(tmpDir, "nodes", "root", "uploaded.txt")) + if err != nil { + panic(err) + } + fmt.Println(string(cts)) +} + +// Scenario: start 2 uploads, pause the first one, let the second one finish first, +// resume the first one at some point in time. Both uploads should finish. +// Needs to result in 2 versions, last finished is the most recent version. +func TestTwoUploadsVersioning(t *testing.T) { + //runtime.GOMAXPROCS(1) // uncomment to remove concurrency and see revisions working. + ofs, err := New(config) + if err != nil { + t.Error(err) + } + + wg := &sync.WaitGroup{} + wg.Add(2) + + // upload file with contents: "test" + go func(wg *sync.WaitGroup) { + ofs.Upload(ctx, &provider.Reference{ + Spec: &provider.Reference_Path{Path: "uploaded.txt"}, + }, f) + wg.Done() + }(wg) + + // upload file with contents: "another run" + go func(wg *sync.WaitGroup) { + ofs.Upload(ctx, &provider.Reference{ + Spec: &provider.Reference_Path{Path: "uploaded.txt"}, + }, f1) + wg.Done() + }(wg) + + // this test, by the way the oCIS storage is implemented, is non-deterministic, and the contents + // of uploaded.txt will change on each run depending on which of the 2 routines above makes it + // first into the scheduler. In order to make it deterministic, we have to consider the Upload impl- + // ementation and we can leverage concurrency and add locks only when the destination path are the + // same for 2 uploads. + + wg.Wait() + revisions, err := ofs.ListRevisions(ctx, &provider.Reference{ + Spec: &provider.Reference_Path{Path: "uploaded.txt"}, + }) + assert.NoError(t, err) + assert.Equal(t, 1, len(revisions)) +} + +// TestParallelMkcol ensures that, on an unit level, if multiple requests fight for creating a directory (race condition) +// only the first one will create it. Note that there is little to synchronize here because if the folder is already +// created, the underlying filesystem (not the storage driver layer) will fail when attempting to create the directory. +func TestParallelMkcol(t *testing.T) { + ofs, err := New(config) + if err != nil { + t.Error(err) + } + + for i:=0; i< 10; i++ { + t.Run("", func (t *testing.T) { + t.Parallel() + if err := ofs.CreateDir(ctx, "fightforit"); err != nil { + rinfo, err := ofs.GetMD(ctx, &provider.Reference{ + Spec: &provider.Reference_Path{Path: "fightforit"}, + }, nil) + if err != nil { + t.Error(err) + } + + assert.NotNil(t, rinfo) + } + }) + } +} From 8d2c1162ecf34f162f12147a8d6b1e3a59eb868f Mon Sep 17 00:00:00 2001 From: "A.Unger" Date: Tue, 2 Feb 2021 12:42:32 +0100 Subject: [PATCH 2/5] add changelog --- .../unreleased/non-deterministic-ocis-storage-test.md | 7 +++++++ 1 file changed, 7 insertions(+) create mode 100644 changelog/unreleased/non-deterministic-ocis-storage-test.md diff --git a/changelog/unreleased/non-deterministic-ocis-storage-test.md b/changelog/unreleased/non-deterministic-ocis-storage-test.md new file mode 100644 index 0000000000..a8f2fa157b --- /dev/null +++ b/changelog/unreleased/non-deterministic-ocis-storage-test.md @@ -0,0 +1,7 @@ +Enhancement: Capture non-deterministic behavior on upload on the oCIS storage driver. + +As a developer creating/maintaining a storage driver I want to be able to validate the atomicity of all my storage driver operations. +* Test for: Start 2 uploads, pause the first one, let the second one finish first, resume the first one at some point in time. Both uploads should finish. Needs to result in 2 versions, last finished is the most recent version. +* Test for: Start 2 MKCOL requests with the same path, one needs to fail. + +https://github.com/cs3org/reva/pull/1430 From f3f2798d66618a867d4b202bd5681bda7c24517d Mon Sep 17 00:00:00 2001 From: "A.Unger" Date: Tue, 2 Feb 2021 14:29:38 +0100 Subject: [PATCH 3/5] add license header --- pkg/storage/fs/ocis/ocis_test.go | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/pkg/storage/fs/ocis/ocis_test.go b/pkg/storage/fs/ocis/ocis_test.go index 3de7882518..f1704ada00 100644 --- a/pkg/storage/fs/ocis/ocis_test.go +++ b/pkg/storage/fs/ocis/ocis_test.go @@ -1,3 +1,21 @@ +// Copyright 2018-2021 CERN +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. +// +// In applying this license, CERN does not waive the privileges and immunities +// granted to it by virtue of its status as an Intergovernmental Organization +// or submit itself to any jurisdiction. + // +build storageRace package ocis From b4525a8e77a45a6645d0439c2095b7d7787f4c32 Mon Sep 17 00:00:00 2001 From: "A.Unger" Date: Tue, 2 Feb 2021 14:54:22 +0100 Subject: [PATCH 4/5] fix changelog... --- changelog/unreleased/non-deterministic-ocis-storage-test.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/changelog/unreleased/non-deterministic-ocis-storage-test.md b/changelog/unreleased/non-deterministic-ocis-storage-test.md index a8f2fa157b..9a34da848f 100644 --- a/changelog/unreleased/non-deterministic-ocis-storage-test.md +++ b/changelog/unreleased/non-deterministic-ocis-storage-test.md @@ -1,4 +1,4 @@ -Enhancement: Capture non-deterministic behavior on upload on the oCIS storage driver. +Enhancement: Capture non-deterministic behavior on upload on the oCIS storage driver As a developer creating/maintaining a storage driver I want to be able to validate the atomicity of all my storage driver operations. * Test for: Start 2 uploads, pause the first one, let the second one finish first, resume the first one at some point in time. Both uploads should finish. Needs to result in 2 versions, last finished is the most recent version. From 4b31bc6a0c9692af02c086b2a33e80a590256ea4 Mon Sep 17 00:00:00 2001 From: "A.Unger" Date: Tue, 2 Feb 2021 15:14:17 +0100 Subject: [PATCH 5/5] fix changelog...AGAIN --- changelog/unreleased/non-deterministic-ocis-storage-test.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/changelog/unreleased/non-deterministic-ocis-storage-test.md b/changelog/unreleased/non-deterministic-ocis-storage-test.md index 9a34da848f..a175e31469 100644 --- a/changelog/unreleased/non-deterministic-ocis-storage-test.md +++ b/changelog/unreleased/non-deterministic-ocis-storage-test.md @@ -1,4 +1,4 @@ -Enhancement: Capture non-deterministic behavior on upload on the oCIS storage driver +Enhancement: Capture non-deterministic behavior on storages As a developer creating/maintaining a storage driver I want to be able to validate the atomicity of all my storage driver operations. * Test for: Start 2 uploads, pause the first one, let the second one finish first, resume the first one at some point in time. Both uploads should finish. Needs to result in 2 versions, last finished is the most recent version.