From ec051f7135ee6709aff8717fd1d24f74315a2f22 Mon Sep 17 00:00:00 2001 From: jkoberg Date: Tue, 22 Oct 2024 16:14:00 +0200 Subject: [PATCH 1/2] fix(decomposedfs): calculate remaining quota correctly Signed-off-by: jkoberg --- .../unreleased/fix-activitylog-issues.md | 5 ++++ .../utils/decomposedfs/decomposedfs.go | 25 +++++-------------- pkg/storage/utils/decomposedfs/spaces.go | 10 +------- 3 files changed, 12 insertions(+), 28 deletions(-) create mode 100644 changelog/unreleased/fix-activitylog-issues.md diff --git a/changelog/unreleased/fix-activitylog-issues.md b/changelog/unreleased/fix-activitylog-issues.md new file mode 100644 index 0000000000..05bab33199 --- /dev/null +++ b/changelog/unreleased/fix-activitylog-issues.md @@ -0,0 +1,5 @@ +Enhancement: Fix remaining quota calculation + +Remaining quota should only be total - used and not take disk space into account. + +https://github.com/cs3org/reva/pull/4897 diff --git a/pkg/storage/utils/decomposedfs/decomposedfs.go b/pkg/storage/utils/decomposedfs/decomposedfs.go index f7b8a84d63..824183a3e9 100644 --- a/pkg/storage/utils/decomposedfs/decomposedfs.go +++ b/pkg/storage/utils/decomposedfs/decomposedfs.go @@ -22,7 +22,6 @@ import ( "context" "fmt" "io" - "math" "net/url" "path" "path/filepath" @@ -602,20 +601,12 @@ func (fs *Decomposedfs) GetQuota(ctx context.Context, ref *provider.Reference) ( quotaStr = string(ri.Opaque.Map["quota"].Value) } - remaining, err = fs.bs.GetAvailableSize(n) - switch { - case errors.Is(err, tree.ErrSizeUnlimited): - remaining = math.MaxUint64 - case err != nil: - return 0, 0, 0, err - } - - return fs.calculateTotalUsedRemaining(quotaStr, ri.Size, remaining) + return fs.calculateTotalUsedRemaining(quotaStr, ri.Size) } -func (fs *Decomposedfs) calculateTotalUsedRemaining(quotaStr string, inUse, remaining uint64) (uint64, uint64, uint64, error) { +func (fs *Decomposedfs) calculateTotalUsedRemaining(quotaStr string, inUse uint64) (uint64, uint64, uint64, error) { var err error - var total uint64 + var total, remaining uint64 switch quotaStr { case node.QuotaUncalculated, node.QuotaUnknown: // best we can do is return current total @@ -628,14 +619,10 @@ func (fs *Decomposedfs) calculateTotalUsedRemaining(quotaStr string, inUse, rema return 0, 0, 0, err } - if total <= remaining { - // Prevent overflowing - if inUse >= total { - remaining = 0 - } else { - remaining = total - inUse - } + if total > inUse { + remaining = total - inUse } + } return total, inUse, remaining, nil } diff --git a/pkg/storage/utils/decomposedfs/spaces.go b/pkg/storage/utils/decomposedfs/spaces.go index c8f4268a33..12e708df53 100644 --- a/pkg/storage/utils/decomposedfs/spaces.go +++ b/pkg/storage/utils/decomposedfs/spaces.go @@ -46,7 +46,6 @@ import ( "github.com/cs3org/reva/v2/pkg/storage/utils/decomposedfs/metadata/prefixes" "github.com/cs3org/reva/v2/pkg/storage/utils/decomposedfs/node" "github.com/cs3org/reva/v2/pkg/storage/utils/decomposedfs/permissions" - "github.com/cs3org/reva/v2/pkg/storage/utils/decomposedfs/tree" "github.com/cs3org/reva/v2/pkg/storage/utils/templates" "github.com/cs3org/reva/v2/pkg/storagespace" "github.com/cs3org/reva/v2/pkg/utils" @@ -1030,14 +1029,7 @@ func (fs *Decomposedfs) StorageSpaceFromNode(ctx context.Context, n *node.Node, quotaStr = quotaInOpaque } - remaining, err := fs.bs.GetAvailableSize(n) - switch { - case errors.Is(err, tree.ErrSizeUnlimited): - remaining = math.MaxUint64 - case err != nil: - return nil, err - } - total, used, remaining, err := fs.calculateTotalUsedRemaining(quotaStr, space.GetRootInfo().GetSize(), remaining) + total, used, remaining, err := fs.calculateTotalUsedRemaining(quotaStr, space.GetRootInfo().GetSize()) if err != nil { return nil, err } From 48f4fb46f6466f7ba3e2181e757021d83e2cb43f Mon Sep 17 00:00:00 2001 From: jkoberg Date: Wed, 23 Oct 2024 11:43:39 +0200 Subject: [PATCH 2/2] Revert "fix(decomposedfs): available space calculation" This reverts commit b5c3c6781528bbc218e8f34a2bb0b9518d7de83c. --- pkg/storage/fs/ocis/blobstore/blobstore.go | 5 -- pkg/storage/fs/posix/blobstore/blobstore.go | 5 -- pkg/storage/fs/posix/posix.go | 1 - pkg/storage/fs/posix/testhelpers/helpers.go | 2 - pkg/storage/fs/s3ng/blobstore/blobstore.go | 7 --- .../utils/decomposedfs/aspects/aspects.go | 2 - .../utils/decomposedfs/decomposedfs.go | 3 - .../utils/decomposedfs/decomposedfs_test.go | 1 - pkg/storage/utils/decomposedfs/spaces.go | 4 +- .../utils/decomposedfs/testhelpers/helpers.go | 2 - .../decomposedfs/tree/mocks/Blobstore.go | 56 ------------------- pkg/storage/utils/decomposedfs/tree/tree.go | 6 +- .../utils/decomposedfs/upload_async_test.go | 2 - pkg/storage/utils/decomposedfs/upload_test.go | 2 - 14 files changed, 2 insertions(+), 96 deletions(-) diff --git a/pkg/storage/fs/ocis/blobstore/blobstore.go b/pkg/storage/fs/ocis/blobstore/blobstore.go index 83604b015a..b4293fa645 100644 --- a/pkg/storage/fs/ocis/blobstore/blobstore.go +++ b/pkg/storage/fs/ocis/blobstore/blobstore.go @@ -116,11 +116,6 @@ func (bs *Blobstore) Delete(node *node.Node) error { return nil } -// GetAvailableSize returns the available size in the blobstore in bytes -func (bs *Blobstore) GetAvailableSize(n *node.Node) (uint64, error) { - return node.GetAvailableSize(n.InternalPath()) -} - // List lists all blobs in the Blobstore func (bs *Blobstore) List() ([]*node.Node, error) { dirs, err := filepath.Glob(filepath.Join(bs.root, "spaces", "*", "*", "blobs", "*", "*", "*", "*", "*")) diff --git a/pkg/storage/fs/posix/blobstore/blobstore.go b/pkg/storage/fs/posix/blobstore/blobstore.go index 5e152ec1ca..cf01a9e138 100644 --- a/pkg/storage/fs/posix/blobstore/blobstore.go +++ b/pkg/storage/fs/posix/blobstore/blobstore.go @@ -88,8 +88,3 @@ func (bs *Blobstore) Download(node *node.Node) (io.ReadCloser, error) { func (bs *Blobstore) Delete(node *node.Node) error { return nil } - -// GetAvailableSize returns the available size in the blobstore in bytes -func (bs *Blobstore) GetAvailableSize(n *node.Node) (uint64, error) { - return node.GetAvailableSize(n.InternalPath()) -} diff --git a/pkg/storage/fs/posix/posix.go b/pkg/storage/fs/posix/posix.go index ef0e264ab7..537689d459 100644 --- a/pkg/storage/fs/posix/posix.go +++ b/pkg/storage/fs/posix/posix.go @@ -126,7 +126,6 @@ func New(m map[string]interface{}, stream events.Stream) (storage.FS, error) { aspects := aspects.Aspects{ Lookup: lu, Tree: tp, - Blobstore: bs, Permissions: p, EventStream: stream, UserMapper: um, diff --git a/pkg/storage/fs/posix/testhelpers/helpers.go b/pkg/storage/fs/posix/testhelpers/helpers.go index 0571cf42fa..1ed248ecc0 100644 --- a/pkg/storage/fs/posix/testhelpers/helpers.go +++ b/pkg/storage/fs/posix/testhelpers/helpers.go @@ -178,7 +178,6 @@ func NewTestEnv(config map[string]interface{}) (*TestEnv, error) { ) bs := &treemocks.Blobstore{} - bs.On("GetAvailableSize", mock.Anything).Return(uint64(1000000000), nil) tree, err := tree.New(lu, bs, um, &trashbin.Trashbin{}, o, nil, store.Create()) if err != nil { return nil, err @@ -190,7 +189,6 @@ func NewTestEnv(config map[string]interface{}) (*TestEnv, error) { aspects := aspects.Aspects{ Lookup: lu, Tree: tree, - Blobstore: bs, Permissions: permissions.NewPermissions(pmock, permissionsSelector), Trashbin: tb, } diff --git a/pkg/storage/fs/s3ng/blobstore/blobstore.go b/pkg/storage/fs/s3ng/blobstore/blobstore.go index 4751168a5a..aaeecf65f8 100644 --- a/pkg/storage/fs/s3ng/blobstore/blobstore.go +++ b/pkg/storage/fs/s3ng/blobstore/blobstore.go @@ -29,7 +29,6 @@ import ( "github.com/cs3org/reva/v2/pkg/storage/utils/decomposedfs/lookup" "github.com/cs3org/reva/v2/pkg/storage/utils/decomposedfs/node" - "github.com/cs3org/reva/v2/pkg/storage/utils/decomposedfs/tree" "github.com/minio/minio-go/v7" "github.com/minio/minio-go/v7/pkg/credentials" "github.com/pkg/errors" @@ -129,12 +128,6 @@ func (bs *Blobstore) Delete(node *node.Node) error { return nil } -// GetAvailableSize returns the available size in the blobstore in bytes -func (bs *Blobstore) GetAvailableSize(n *node.Node) (uint64, error) { - // S3 doen't have a concept of available size - return 0, tree.ErrSizeUnlimited -} - // List lists all blobs in the Blobstore func (bs *Blobstore) List() ([]*node.Node, error) { ch := bs.client.ListObjects(context.Background(), bs.bucket, minio.ListObjectsOptions{Recursive: true}) diff --git a/pkg/storage/utils/decomposedfs/aspects/aspects.go b/pkg/storage/utils/decomposedfs/aspects/aspects.go index 18eb88eed4..65a9e0d983 100644 --- a/pkg/storage/utils/decomposedfs/aspects/aspects.go +++ b/pkg/storage/utils/decomposedfs/aspects/aspects.go @@ -23,7 +23,6 @@ import ( "github.com/cs3org/reva/v2/pkg/storage/utils/decomposedfs/node" "github.com/cs3org/reva/v2/pkg/storage/utils/decomposedfs/permissions" "github.com/cs3org/reva/v2/pkg/storage/utils/decomposedfs/trashbin" - "github.com/cs3org/reva/v2/pkg/storage/utils/decomposedfs/tree" "github.com/cs3org/reva/v2/pkg/storage/utils/decomposedfs/usermapper" ) @@ -31,7 +30,6 @@ import ( type Aspects struct { Lookup node.PathLookup Tree node.Tree - Blobstore tree.Blobstore Trashbin trashbin.Trashbin Permissions permissions.Permissions EventStream events.Stream diff --git a/pkg/storage/utils/decomposedfs/decomposedfs.go b/pkg/storage/utils/decomposedfs/decomposedfs.go index 824183a3e9..7411b9ed95 100644 --- a/pkg/storage/utils/decomposedfs/decomposedfs.go +++ b/pkg/storage/utils/decomposedfs/decomposedfs.go @@ -112,7 +112,6 @@ type SessionStore interface { type Decomposedfs struct { lu node.PathLookup tp node.Tree - bs tree.Blobstore trashbin trashbin.Trashbin o *options.Options p permissions.Permissions @@ -163,7 +162,6 @@ func NewDefault(m map[string]interface{}, bs tree.Blobstore, es events.Stream) ( aspects := aspects.Aspects{ Lookup: lu, Tree: tp, - Blobstore: bs, Permissions: permissions.NewPermissions(node.NewPermissions(lu), permissionsSelector), EventStream: es, DisableVersioning: o.DisableVersioning, @@ -225,7 +223,6 @@ func New(o *options.Options, aspects aspects.Aspects) (storage.FS, error) { fs := &Decomposedfs{ tp: aspects.Tree, - bs: aspects.Blobstore, lu: aspects.Lookup, trashbin: aspects.Trashbin, o: o, diff --git a/pkg/storage/utils/decomposedfs/decomposedfs_test.go b/pkg/storage/utils/decomposedfs/decomposedfs_test.go index d648d58291..ef7bc144fe 100644 --- a/pkg/storage/utils/decomposedfs/decomposedfs_test.go +++ b/pkg/storage/utils/decomposedfs/decomposedfs_test.go @@ -58,7 +58,6 @@ var _ = Describe("Decomposed", func() { Describe("NewDefault", func() { It("works", func() { bs := &treemocks.Blobstore{} - bs.On("GetAvailableSize", mock.Anything).Return(uint64(1000000000), nil) _, err := decomposedfs.NewDefault(map[string]interface{}{ "root": env.Root, "permissionssvc": "any", diff --git a/pkg/storage/utils/decomposedfs/spaces.go b/pkg/storage/utils/decomposedfs/spaces.go index 12e708df53..760ad96552 100644 --- a/pkg/storage/utils/decomposedfs/spaces.go +++ b/pkg/storage/utils/decomposedfs/spaces.go @@ -1035,9 +1035,7 @@ func (fs *Decomposedfs) StorageSpaceFromNode(ctx context.Context, n *node.Node, } space.Opaque = utils.AppendPlainToOpaque(space.Opaque, "quota.total", strconv.FormatUint(total, 10)) space.Opaque = utils.AppendPlainToOpaque(space.Opaque, "quota.used", strconv.FormatUint(used, 10)) - if remaining != math.MaxUint64 { - space.Opaque = utils.AppendPlainToOpaque(space.Opaque, "quota.remaining", strconv.FormatUint(remaining, 10)) - } + space.Opaque = utils.AppendPlainToOpaque(space.Opaque, "quota.remaining", strconv.FormatUint(remaining, 10)) return space, nil } diff --git a/pkg/storage/utils/decomposedfs/testhelpers/helpers.go b/pkg/storage/utils/decomposedfs/testhelpers/helpers.go index 1c20fafc5a..b8b9d5d53b 100644 --- a/pkg/storage/utils/decomposedfs/testhelpers/helpers.go +++ b/pkg/storage/utils/decomposedfs/testhelpers/helpers.go @@ -175,7 +175,6 @@ func NewTestEnv(config map[string]interface{}) (*TestEnv, error) { aspects := aspects.Aspects{ Lookup: lu, Tree: tree, - Blobstore: bs, Permissions: permissions.NewPermissions(pmock, permissionsSelector), Trashbin: &decomposedfs.DecomposedfsTrashbin{}, } @@ -291,7 +290,6 @@ func (t *TestEnv) CreateTestStorageSpace(typ string, quota *providerv1beta1.Quot if typ == "personal" { owner = t.Owner } - t.Blobstore.On("GetAvailableSize", mock.Anything).Return(uint64(1000000000), nil) space, err := t.Fs.CreateStorageSpace(t.Ctx, &providerv1beta1.CreateStorageSpaceRequest{ Owner: owner, Type: typ, diff --git a/pkg/storage/utils/decomposedfs/tree/mocks/Blobstore.go b/pkg/storage/utils/decomposedfs/tree/mocks/Blobstore.go index b5e8720f38..3f23a832e1 100644 --- a/pkg/storage/utils/decomposedfs/tree/mocks/Blobstore.go +++ b/pkg/storage/utils/decomposedfs/tree/mocks/Blobstore.go @@ -144,62 +144,6 @@ func (_c *Blobstore_Download_Call) RunAndReturn(run func(*node.Node) (io.ReadClo return _c } -// GetAvailableSize provides a mock function with given fields: _a0 -func (_m *Blobstore) GetAvailableSize(_a0 *node.Node) (uint64, error) { - ret := _m.Called(_a0) - - if len(ret) == 0 { - panic("no return value specified for GetAvailableSize") - } - - var r0 uint64 - var r1 error - if rf, ok := ret.Get(0).(func(*node.Node) (uint64, error)); ok { - return rf(_a0) - } - if rf, ok := ret.Get(0).(func(*node.Node) uint64); ok { - r0 = rf(_a0) - } else { - r0 = ret.Get(0).(uint64) - } - - if rf, ok := ret.Get(1).(func(*node.Node) error); ok { - r1 = rf(_a0) - } else { - r1 = ret.Error(1) - } - - return r0, r1 -} - -// Blobstore_GetAvailableSize_Call is a *mock.Call that shadows Run/Return methods with type explicit version for method 'GetAvailableSize' -type Blobstore_GetAvailableSize_Call struct { - *mock.Call -} - -// GetAvailableSize is a helper method to define mock.On call -// - _a0 *node.Node -func (_e *Blobstore_Expecter) GetAvailableSize(_a0 interface{}) *Blobstore_GetAvailableSize_Call { - return &Blobstore_GetAvailableSize_Call{Call: _e.mock.On("GetAvailableSize", _a0)} -} - -func (_c *Blobstore_GetAvailableSize_Call) Run(run func(_a0 *node.Node)) *Blobstore_GetAvailableSize_Call { - _c.Call.Run(func(args mock.Arguments) { - run(args[0].(*node.Node)) - }) - return _c -} - -func (_c *Blobstore_GetAvailableSize_Call) Return(_a0 uint64, _a1 error) *Blobstore_GetAvailableSize_Call { - _c.Call.Return(_a0, _a1) - return _c -} - -func (_c *Blobstore_GetAvailableSize_Call) RunAndReturn(run func(*node.Node) (uint64, error)) *Blobstore_GetAvailableSize_Call { - _c.Call.Return(run) - return _c -} - // Upload provides a mock function with given fields: _a0, source func (_m *Blobstore) Upload(_a0 *node.Node, source string) error { ret := _m.Called(_a0, source) diff --git a/pkg/storage/utils/decomposedfs/tree/tree.go b/pkg/storage/utils/decomposedfs/tree/tree.go index 31ade9af4f..e253053047 100644 --- a/pkg/storage/utils/decomposedfs/tree/tree.go +++ b/pkg/storage/utils/decomposedfs/tree/tree.go @@ -48,10 +48,7 @@ import ( "golang.org/x/sync/errgroup" ) -var ( - tracer trace.Tracer - ErrSizeUnlimited = errors.New("blobstore size unlimited") -) +var tracer trace.Tracer func init() { tracer = otel.Tracer("github.com/cs3org/reva/pkg/storage/utils/decomposedfs/tree") @@ -62,7 +59,6 @@ type Blobstore interface { Upload(node *node.Node, source string) error Download(node *node.Node) (io.ReadCloser, error) Delete(node *node.Node) error - GetAvailableSize(node *node.Node) (uint64, error) } // Tree manages a hierarchical tree diff --git a/pkg/storage/utils/decomposedfs/upload_async_test.go b/pkg/storage/utils/decomposedfs/upload_async_test.go index 8a2c0ce6cc..f5c0907202 100644 --- a/pkg/storage/utils/decomposedfs/upload_async_test.go +++ b/pkg/storage/utils/decomposedfs/upload_async_test.go @@ -153,7 +153,6 @@ var _ = Describe("Async file uploads", Ordered, func() { }, ) bs = &treemocks.Blobstore{} - bs.On("GetAvailableSize", mock.Anything).Return(uint64(1000000000), nil) // create space uses CheckPermission endpoint cs3permissionsclient.On("CheckPermission", mock.Anything, mock.Anything, mock.Anything).Return(&cs3permissions.CheckPermissionResponse{ @@ -177,7 +176,6 @@ var _ = Describe("Async file uploads", Ordered, func() { aspects := aspects.Aspects{ Lookup: lu, Tree: tree, - Blobstore: bs, Permissions: permissions.NewPermissions(pmock, permissionsSelector), EventStream: stream.Chan{pub, con}, Trashbin: &DecomposedfsTrashbin{}, diff --git a/pkg/storage/utils/decomposedfs/upload_test.go b/pkg/storage/utils/decomposedfs/upload_test.go index c4fd970125..a8bb593924 100644 --- a/pkg/storage/utils/decomposedfs/upload_test.go +++ b/pkg/storage/utils/decomposedfs/upload_test.go @@ -136,13 +136,11 @@ var _ = Describe("File uploads", func() { AddGrant: true, }, nil).Times(1) var err error - bs.On("GetAvailableSize", mock.Anything).Return(uint64(1000000000), nil).Times(1) tree := tree.New(lu, bs, o, store.Create()) aspects := aspects.Aspects{ Lookup: lu, Tree: tree, - Blobstore: bs, Permissions: permissions.NewPermissions(pmock, permissionsSelector), Trashbin: &decomposedfs.DecomposedfsTrashbin{}, }