From b5c3c6781528bbc218e8f34a2bb0b9518d7de83c Mon Sep 17 00:00:00 2001 From: Ralf Haferkamp Date: Wed, 25 Sep 2024 09:59:35 +0200 Subject: [PATCH] fix(decomposedfs): available space calculation Move the calculation of the available size to the blobstore interface. We were just returning the available size of the local disk, which is wrong when using the S3 blobstore. Also, as the S3 blobstore doesn't have a concept of available size, we don't return a 'remaining' for S3 backed spaces with unlimited quota. Fixes: https://github.com/owncloud/ocis/issues/9245 --- .../unreleased/fix-available-diskspace.md | 6 ++ 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 | 12 +++- .../utils/decomposedfs/decomposedfs_test.go | 1 + pkg/storage/utils/decomposedfs/spaces.go | 13 +++-- .../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 + 15 files changed, 114 insertions(+), 8 deletions(-) create mode 100644 changelog/unreleased/fix-available-diskspace.md diff --git a/changelog/unreleased/fix-available-diskspace.md b/changelog/unreleased/fix-available-diskspace.md new file mode 100644 index 0000000000..39ee37620f --- /dev/null +++ b/changelog/unreleased/fix-available-diskspace.md @@ -0,0 +1,6 @@ +Bugfix: Fix remaining space calculation for S3 blobstore + +The calculation of the remaining space in the S3 blobstore was incorrectly using +the remaining space of the local disk instead. + +https://github.com/cs3org/reva/pull/4867 diff --git a/pkg/storage/fs/ocis/blobstore/blobstore.go b/pkg/storage/fs/ocis/blobstore/blobstore.go index b4293fa645..83604b015a 100644 --- a/pkg/storage/fs/ocis/blobstore/blobstore.go +++ b/pkg/storage/fs/ocis/blobstore/blobstore.go @@ -116,6 +116,11 @@ 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 cf01a9e138..5e152ec1ca 100644 --- a/pkg/storage/fs/posix/blobstore/blobstore.go +++ b/pkg/storage/fs/posix/blobstore/blobstore.go @@ -88,3 +88,8 @@ 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 537689d459..ef0e264ab7 100644 --- a/pkg/storage/fs/posix/posix.go +++ b/pkg/storage/fs/posix/posix.go @@ -126,6 +126,7 @@ 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 1ed248ecc0..0571cf42fa 100644 --- a/pkg/storage/fs/posix/testhelpers/helpers.go +++ b/pkg/storage/fs/posix/testhelpers/helpers.go @@ -178,6 +178,7 @@ 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 @@ -189,6 +190,7 @@ 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 aaeecf65f8..4751168a5a 100644 --- a/pkg/storage/fs/s3ng/blobstore/blobstore.go +++ b/pkg/storage/fs/s3ng/blobstore/blobstore.go @@ -29,6 +29,7 @@ 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" @@ -128,6 +129,12 @@ 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 65a9e0d983..18eb88eed4 100644 --- a/pkg/storage/utils/decomposedfs/aspects/aspects.go +++ b/pkg/storage/utils/decomposedfs/aspects/aspects.go @@ -23,6 +23,7 @@ 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" ) @@ -30,6 +31,7 @@ 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 fdd9557c48..70b0e1f284 100644 --- a/pkg/storage/utils/decomposedfs/decomposedfs.go +++ b/pkg/storage/utils/decomposedfs/decomposedfs.go @@ -22,6 +22,7 @@ import ( "context" "fmt" "io" + "math" "net/url" "path" "path/filepath" @@ -112,6 +113,7 @@ type SessionStore interface { type Decomposedfs struct { lu node.PathLookup tp node.Tree + bs tree.Blobstore trashbin trashbin.Trashbin o *options.Options p permissions.Permissions @@ -162,6 +164,7 @@ 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, @@ -223,6 +226,7 @@ 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, @@ -598,9 +602,11 @@ func (fs *Decomposedfs) GetQuota(ctx context.Context, ref *provider.Reference) ( quotaStr = string(ri.Opaque.Map["quota"].Value) } - // FIXME this reads remaining disk size from the local disk, not the blobstore - remaining, err = node.GetAvailableSize(n.InternalPath()) - if err != nil { + remaining, err = fs.bs.GetAvailableSize(n) + switch { + case errors.Is(err, tree.ErrSizeUnlimited): + remaining = math.MaxUint64 + case err != nil: return 0, 0, 0, err } diff --git a/pkg/storage/utils/decomposedfs/decomposedfs_test.go b/pkg/storage/utils/decomposedfs/decomposedfs_test.go index ef7bc144fe..d648d58291 100644 --- a/pkg/storage/utils/decomposedfs/decomposedfs_test.go +++ b/pkg/storage/utils/decomposedfs/decomposedfs_test.go @@ -58,6 +58,7 @@ 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 31d556a69c..ea51869e2c 100644 --- a/pkg/storage/utils/decomposedfs/spaces.go +++ b/pkg/storage/utils/decomposedfs/spaces.go @@ -45,6 +45,7 @@ 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" @@ -1028,9 +1029,11 @@ func (fs *Decomposedfs) StorageSpaceFromNode(ctx context.Context, n *node.Node, quotaStr = quotaInOpaque } - // FIXME this reads remaining disk size from the local disk, not the blobstore - remaining, err := node.GetAvailableSize(n.InternalPath()) - if err != nil { + 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) @@ -1039,7 +1042,9 @@ 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)) - space.Opaque = utils.AppendPlainToOpaque(space.Opaque, "quota.remaining", strconv.FormatUint(remaining, 10)) + if remaining != math.MaxUint64 { + 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 b8b9d5d53b..1c20fafc5a 100644 --- a/pkg/storage/utils/decomposedfs/testhelpers/helpers.go +++ b/pkg/storage/utils/decomposedfs/testhelpers/helpers.go @@ -175,6 +175,7 @@ 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{}, } @@ -290,6 +291,7 @@ 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 3f23a832e1..b5e8720f38 100644 --- a/pkg/storage/utils/decomposedfs/tree/mocks/Blobstore.go +++ b/pkg/storage/utils/decomposedfs/tree/mocks/Blobstore.go @@ -144,6 +144,62 @@ 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 f10735b664..58dc0c4e1f 100644 --- a/pkg/storage/utils/decomposedfs/tree/tree.go +++ b/pkg/storage/utils/decomposedfs/tree/tree.go @@ -48,7 +48,10 @@ import ( "golang.org/x/sync/errgroup" ) -var tracer trace.Tracer +var ( + tracer trace.Tracer + ErrSizeUnlimited = errors.New("blobstore size unlimited") +) func init() { tracer = otel.Tracer("github.com/cs3org/reva/pkg/storage/utils/decomposedfs/tree") @@ -59,6 +62,7 @@ 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 f5c0907202..8a2c0ce6cc 100644 --- a/pkg/storage/utils/decomposedfs/upload_async_test.go +++ b/pkg/storage/utils/decomposedfs/upload_async_test.go @@ -153,6 +153,7 @@ 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{ @@ -176,6 +177,7 @@ 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 a8bb593924..c4fd970125 100644 --- a/pkg/storage/utils/decomposedfs/upload_test.go +++ b/pkg/storage/utils/decomposedfs/upload_test.go @@ -136,11 +136,13 @@ 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{}, }