TrueCloudLab/frostfs-node
20
1
Fork 28
Code Issues 113 Pull requests 3 Projects Releases 2 Activity Actions

Do not change shard mode to DEGRADED_READ_ONLY in case of no space left from blobovnicza #1166

Merged
fyrchik merged 2 commits from dstepanov-yadro/frostfs-node:fix/out_of_space_dro into master 2024-09-04 19:51:09 +00:00
Conversation 0 Commits 2 Files changed 5 +33 -3
5 changed files with 33 additions and 3 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

6
pkg/local_object_storage/blobovnicza/delete.go
View file

@ -2,6 +2,8 @@ package blobovnicza
import (
"context"
"errors"
"syscall"
"git.frostfs.info/TrueCloudLab/frostfs-node/internal/logs"
tracingPkg "git.frostfs.info/TrueCloudLab/frostfs-node/pkg/tracing"
@ -97,5 +99,9 @@ func (b *Blobovnicza) Delete(ctx context.Context, prm DeletePrm) (DeleteRes, err
b.itemDeleted(recordSize)
}
if errors.Is(err, syscall.ENOSPC) {
err = ErrNoSpace
}
return DeleteRes{}, err
}

6
pkg/local_object_storage/blobovnicza/errors.go Normal file
View file

@ -0,0 +1,6 @@
package blobovnicza
import "git.frostfs.info/TrueCloudLab/frostfs-node/pkg/local_object_storage/util/logicerr"
// ErrNoSpace returned if blobovnicza failed to perform an operation because of syscall.ENOSPC.
var ErrNoSpace = logicerr.New("no space left on device with blobovnicza")
dstepanov-yadro commented 2024-06-06 16:04:09 +00:00
Outdated
Review
Copy link

To not to use blobstor's ErrNoSpace: blobstor should depend on blobovnicza, not vice versa.

To not to use blobstor's ErrNoSpace: blobstor should depend on blobovnicza, not vice versa.

15
pkg/local_object_storage/blobovnicza/move.go
View file

@ -2,7 +2,9 @@ package blobovnicza
import (
"context"
"errors"
"fmt"
"syscall"
"git.frostfs.info/TrueCloudLab/frostfs-observability/tracing"
oid "git.frostfs.info/TrueCloudLab/frostfs-sdk-go/object/id"
@ -29,7 +31,7 @@ func (b *Blobovnicza) PutMoveInfo(ctx context.Context, prm MoveInfo) error {
key := addressKey(prm.Address)
return b.boltDB.Update(func(tx *bbolt.Tx) error {
err := b.boltDB.Update(func(tx *bbolt.Tx) error {
bucket, err := tx.CreateBucketIfNotExists(incompletedMoveBucketName)
if err != nil {
return err
@ -41,6 +43,11 @@ func (b *Blobovnicza) PutMoveInfo(ctx context.Context, prm MoveInfo) error {
return nil
})
if errors.Is(err, syscall.ENOSPC) {
err = ErrNoSpace
}
return err
}
func (b *Blobovnicza) DropMoveInfo(ctx context.Context, address oid.Address) error {
@ -53,7 +60,7 @@ func (b *Blobovnicza) DropMoveInfo(ctx context.Context, address oid.Address) err
key := addressKey(address)
return b.boltDB.Update(func(tx *bbolt.Tx) error {
err := b.boltDB.Update(func(tx *bbolt.Tx) error {
bucket := tx.Bucket(incompletedMoveBucketName)
if bucket == nil {
return nil
@ -72,6 +79,10 @@ func (b *Blobovnicza) DropMoveInfo(ctx context.Context, address oid.Address) err
return nil
})
if errors.Is(err, syscall.ENOSPC) {
err = ErrNoSpace
}
return err
}
func (b *Blobovnicza) ListMoveInfo(ctx context.Context) ([]MoveInfo, error) {

4
pkg/local_object_storage/blobovnicza/put.go
View file

@ -2,7 +2,9 @@ package blobovnicza
import (
"context"
"errors"
"fmt"
"syscall"
"git.frostfs.info/TrueCloudLab/frostfs-node/pkg/local_object_storage/util/logicerr"
"git.frostfs.info/TrueCloudLab/frostfs-observability/tracing"
@ -95,6 +97,8 @@ func (b *Blobovnicza) Put(ctx context.Context, prm PutPrm) (PutRes, error) {
})
if err == nil {
b.itemAdded(recordSize)
} else if errors.Is(err, syscall.ENOSPC) {
fyrchik marked this conversation as resolved Outdated
fyrchik commented 2024-06-07 12:10:16 +00:00
Outdated
Review
Copy link

Any modifying method can allocate new pages, even delete.

Any modifying method can allocate new pages, even delete.
dstepanov-yadro commented 2024-06-07 13:41:05 +00:00
Outdated
Review
Copy link

I thought about it, but have found this comment of contributor: https://github.com/etcd-io/bbolt/issues/288#issuecomment-919971605

Anyway, ok, I will fix it.

I thought about it, but have found this comment of contributor: https://github.com/etcd-io/bbolt/issues/288#issuecomment-919971605 Anyway, ok, I will fix it.
dstepanov-yadro commented 2024-06-07 14:16:28 +00:00
Outdated
Review
Copy link

Done

Done
fyrchik commented 2024-06-10 07:54:26 +00:00
Outdated
Review
Copy link

The comment has different context (shrinking the DB), we already have experienced situations where deletions lead to db remap leading to a deadlock (with the write-cache)

The comment has different context (shrinking the DB), we already have experienced situations where deletions lead to db remap leading to a deadlock (with the write-cache)
err = ErrNoSpace
}
return PutRes{}, err

5
pkg/local_object_storage/blobstor/blobovniczatree/put.go
View file

@ -2,6 +2,7 @@ package blobovniczatree
import (
"context"
"errors"
"path/filepath"
"time"
@ -108,7 +109,9 @@ func (i *putIterator) iterate(ctx context.Context, lvlPath string) (bool, error)
zap.String("error", err.Error()),
zap.String("trace_id", tracingPkg.GetTraceID(ctx)))
}
if errors.Is(err, blobovnicza.ErrNoSpace) {
i.AllFull = true
fyrchik commented 2024-06-10 07:57:08 +00:00
Review
Copy link

Again, do we exit if we received this error from at least 1 blobovnicza? Until we have vacuum I think it is not worth having this optimization, as others blobovniczas may still have free pages.

Again, do we exit if we received this error from at least 1 blobovnicza? Until we have vacuum I think it is not worth having this optimization, as others blobovniczas may still have free pages.
dstepanov-yadro commented 2024-06-10 08:23:45 +00:00
Review
Copy link

No, blobstor will try all databases:

	i.AllFull = false <------- here AllFull resets

	_, err = active.Blobovnicza().Put(ctx, i.PutPrm)
	if err != nil {
		if !isLogical(err) {
			i.B.reportError(logs.BlobovniczatreeCouldNotPutObjectToActiveBlobovnicza, err)
		} else {
			i.B.log.Debug(logs.BlobovniczatreeCouldNotPutObjectToActiveBlobovnicza,
				zap.String("path", active.SystemPath()),
				zap.String("error", err.Error()),
				zap.String("trace_id", tracingPkg.GetTraceID(ctx)))
		}

		if errors.Is(err, blobovnicza.ErrNoSpace) {
			i.AllFull = true
No, blobstor will try all databases: ``` i.AllFull = false <------- here AllFull resets _, err = active.Blobovnicza().Put(ctx, i.PutPrm) if err != nil { if !isLogical(err) { i.B.reportError(logs.BlobovniczatreeCouldNotPutObjectToActiveBlobovnicza, err) } else { i.B.log.Debug(logs.BlobovniczatreeCouldNotPutObjectToActiveBlobovnicza, zap.String("path", active.SystemPath()), zap.String("error", err.Error()), zap.String("trace_id", tracingPkg.GetTraceID(ctx))) } if errors.Is(err, blobovnicza.ErrNoSpace) { i.AllFull = true ```
fyrchik commented 2024-06-10 10:07:26 +00:00
Review
Copy link

I don't understand, why do we need this change in this PR? Is something wrong without it?

I don't understand, why do we need this change in this PR? Is something wrong without it?
dstepanov-yadro commented 2024-06-10 10:38:04 +00:00
Review
Copy link

Without this change blobovnicza tree will return non logical error:

Line 64 in a0c5882
return common.PutRes{}, errPutFailed


So shard will increase error counter.

Without this change blobovnicza tree will return non logical error: https://git.frostfs.info/TrueCloudLab/frostfs-node/src/commit/a0c588263bd550bb131a8ed2f1a5ad318811018c/pkg/local_object_storage/blobstor/blobovniczatree/put.go#L64 So shard will increase error counter.
fyrchik commented 2024-06-10 11:28:25 +00:00
Review
Copy link

Hm, but why iterateDeepest return non-nil error?

Hm, but why `iterateDeepest` return non-nil error?
dstepanov-yadro commented 2024-06-10 11:43:09 +00:00
Review
Copy link

By design.

By design.
}
return false, nil
}