From e83b3e483962b2a5a2af7f3064c8e822d907fb98 Mon Sep 17 00:00:00 2001 From: Ekaterina Pavlova Date: Wed, 23 Oct 2024 13:03:40 +0300 Subject: [PATCH 1/5] cli: fix error handling in upload-bin command Fix shared global error reuse. Close #3634 Signed-off-by: Ekaterina Pavlova --- cli/util/uploader.go | 57 ++++++++++++++++++++++---------------------- 1 file changed, 29 insertions(+), 28 deletions(-) diff --git a/cli/util/uploader.go b/cli/util/uploader.go index 022c0ad8a..c18b1c3d5 100644 --- a/cli/util/uploader.go +++ b/cli/util/uploader.go @@ -155,16 +155,17 @@ func uploadBin(ctx *cli.Context) error { defer wg.Done() for blockIndex := batchStart + i; blockIndex < batchEnd; blockIndex += numWorkers { var blk *block.Block - err = retry(func() error { - blk, err = rpc.GetBlockByIndex(uint32(blockIndex)) - if err != nil { - return fmt.Errorf("failed to fetch block %d: %w", blockIndex, err) + errGet := retry(func() error { + var errGetBlock error + blk, errGetBlock = rpc.GetBlockByIndex(uint32(blockIndex)) + if errGetBlock != nil { + return fmt.Errorf("failed to fetch block %d: %w", blockIndex, errGetBlock) } return nil }) - if err != nil { + if errGet != nil { select { - case errorCh <- err: + case errorCh <- errGet: default: } return @@ -185,12 +186,12 @@ func uploadBin(ctx *cli.Context) error { } objBytes := bw.Bytes() - err = retry(func() error { + errRetr := retry(func() error { return uploadObj(ctx.Context, p, signer, acc.PrivateKey().GetScriptHash(), containerID, objBytes, attrs, homomorphicHashingDisabled) }) - if err != nil { + if errRetr != nil { select { - case errorCh <- err: + case errorCh <- errRetr: default: } return @@ -205,7 +206,7 @@ func uploadBin(ctx *cli.Context) error { }() select { - case err := <-errorCh: + case err = <-errorCh: return cli.Exit(fmt.Errorf("upload error: %w", err), 1) case <-doneCh: } @@ -315,16 +316,14 @@ func updateIndexFiles(ctx *cli.Context, p *pool.Pool, containerID cid.ID, accoun filters.AddFilter(attributeKey, fmt.Sprintf("%d", 0), object.MatchNumGE) filters.AddFilter("IndexSize", fmt.Sprintf("%d", indexFileSize), object.MatchStringEqual) prm.SetFilters(filters) - var ( - objectIDs []oid.ID - err error - ) - err = retry(func() error { - objectIDs, err = neofs.ObjectSearch(ctx.Context, p, account.PrivateKey(), containerID.String(), prm) - return err + var objectIDs []oid.ID + errSearch := retry(func() error { + var errSearchIndex error + objectIDs, errSearchIndex = neofs.ObjectSearch(ctx.Context, p, account.PrivateKey(), containerID.String(), prm) + return errSearchIndex }) - if err != nil { - return fmt.Errorf("search of index files failed: %w", err) + if errSearch != nil { + return fmt.Errorf("search of index files failed: %w", errSearch) } existingIndexCount := uint(len(objectIDs)) @@ -346,13 +345,14 @@ func updateIndexFiles(ctx *cli.Context, p *pool.Pool, containerID cid.ID, accoun go func() { for id := range oidCh { var obj *object.Object - err = retry(func() error { - obj, err = p.ObjectHead(context.Background(), containerID, id, signer, client.PrmObjectHead{}) - return err + errRetr := retry(func() error { + var errGetHead error + obj, errGetHead = p.ObjectHead(context.Background(), containerID, id, signer, client.PrmObjectHead{}) + return errGetHead }) - if err != nil { + if errRetr != nil { select { - case errCh <- fmt.Errorf("failed to fetch object %s: %w", id.String(), err): + case errCh <- fmt.Errorf("failed to fetch object %s: %w", id.String(), errRetr): default: } } @@ -384,9 +384,10 @@ func updateIndexFiles(ctx *cli.Context, p *pool.Pool, containerID cid.ID, accoun filters.AddFilter(blockAttributeKey, fmt.Sprintf("%d", end), object.MatchNumLT) prm.SetFilters(filters) var objIDs []oid.ID - err = retry(func() error { - objIDs, err = neofs.ObjectSearch(ctx.Context, p, account.PrivateKey(), containerID.String(), prm) - return err + err := retry(func() error { + var errSearchIndex error + objIDs, errSearchIndex = neofs.ObjectSearch(ctx.Context, p, account.PrivateKey(), containerID.String(), prm) + return errSearchIndex }) if err != nil { @@ -417,7 +418,7 @@ func updateIndexFiles(ctx *cli.Context, p *pool.Pool, containerID cid.ID, accoun *object.NewAttribute(attributeKey, strconv.Itoa(int(i))), *object.NewAttribute("IndexSize", strconv.Itoa(int(indexFileSize))), } - err = uploadObj(ctx.Context, p, signer, account.PrivateKey().GetScriptHash(), containerID, buffer, attrs, homomorphicHashingDisabled) + err := uploadObj(ctx.Context, p, signer, account.PrivateKey().GetScriptHash(), containerID, buffer, attrs, homomorphicHashingDisabled) if err != nil { return fmt.Errorf("failed to upload index file %d: %w", i, err) } From 5b793bcf1bb4dab60ff50071852d43f9164a7e92 Mon Sep 17 00:00:00 2001 From: Ekaterina Pavlova Date: Wed, 23 Oct 2024 13:12:21 +0300 Subject: [PATCH 2/5] cli: fix process termination in `upload-bin` command Add a return statement to properly handle errors, ensuring the process terminates as expected. Signed-off-by: Ekaterina Pavlova --- cli/util/uploader.go | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/cli/util/uploader.go b/cli/util/uploader.go index c18b1c3d5..28ae5a23a 100644 --- a/cli/util/uploader.go +++ b/cli/util/uploader.go @@ -355,6 +355,7 @@ func updateIndexFiles(ctx *cli.Context, p *pool.Pool, containerID cid.ID, accoun case errCh <- fmt.Errorf("failed to fetch object %s: %w", id.String(), errRetr): default: } + return } blockIndex, err := getBlockIndex(obj, blockAttributeKey) if err != nil { @@ -362,6 +363,7 @@ func updateIndexFiles(ctx *cli.Context, p *pool.Pool, containerID cid.ID, accoun case errCh <- fmt.Errorf("failed to get block index from object %s: %w", id.String(), err): default: } + return } offset := (uint(blockIndex) % indexFileSize) * oidSize id.Encode(buffer[offset:]) @@ -391,7 +393,10 @@ func updateIndexFiles(ctx *cli.Context, p *pool.Pool, containerID cid.ID, accoun }) if err != nil { - errCh <- fmt.Errorf("failed to search for objects from %d to %d for index file %d: %w", j, end, i, err) + select { + case errCh <- fmt.Errorf("failed to search for objects from %d to %d for index file %d: %w", j, end, i, err): + default: + } return } From 8b43c33e44a45dbb2f0ae00e755ad9e0b3a0cbab Mon Sep 17 00:00:00 2001 From: Ekaterina Pavlova Date: Wed, 23 Oct 2024 13:14:35 +0300 Subject: [PATCH 3/5] cli: extend object attribute parsing error in `upload-bin` Signed-off-by: Ekaterina Pavlova --- cli/util/uploader.go | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/cli/util/uploader.go b/cli/util/uploader.go index 28ae5a23a..f8e049ad4 100644 --- a/cli/util/uploader.go +++ b/cli/util/uploader.go @@ -483,7 +483,12 @@ func uploadObj(ctx context.Context, p *pool.Pool, signer user.Signer, owner util func getBlockIndex(header *object.Object, attribute string) (int, error) { for _, attr := range header.UserAttributes() { if attr.Key() == attribute { - return strconv.Atoi(attr.Value()) + value := attr.Value() + blockIndex, err := strconv.Atoi(value) + if err != nil { + return -1, fmt.Errorf("attribute %s has invalid value: %s, error: %w", attribute, value, err) + } + return blockIndex, nil } } return -1, fmt.Errorf("attribute %s not found", attribute) From 42c8e40eaaf2e880eca1836c5e26270bae77d8eb Mon Sep 17 00:00:00 2001 From: Ekaterina Pavlova Date: Wed, 23 Oct 2024 13:21:32 +0300 Subject: [PATCH 4/5] cli: add retry to all requests to NeoFS in `upload-bin` Add retry to `NetworkInfo` and `uploadObj`. Signed-off-by: Ekaterina Pavlova --- cli/util/uploader.go | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/cli/util/uploader.go b/cli/util/uploader.go index f8e049ad4..cba55c7b9 100644 --- a/cli/util/uploader.go +++ b/cli/util/uploader.go @@ -20,6 +20,7 @@ import ( "github.com/nspcc-dev/neofs-sdk-go/client" "github.com/nspcc-dev/neofs-sdk-go/container" cid "github.com/nspcc-dev/neofs-sdk-go/container/id" + "github.com/nspcc-dev/neofs-sdk-go/netmap" "github.com/nspcc-dev/neofs-sdk-go/object" oid "github.com/nspcc-dev/neofs-sdk-go/object/id" "github.com/nspcc-dev/neofs-sdk-go/pool" @@ -99,7 +100,13 @@ func uploadBin(ctx *cli.Context) error { return cli.Exit(fmt.Sprintf("failed to dial NeoFS pool: %v", err), 1) } defer p.Close() - net, err := p.NetworkInfo(ctx.Context, client.PrmNetworkInfo{}) + + var net netmap.NetworkInfo + err = retry(func() error { + var errNet error + net, errNet = p.NetworkInfo(ctx.Context, client.PrmNetworkInfo{}) + return errNet + }) if err != nil { return cli.Exit(fmt.Errorf("failed to get network info: %w", err), 1) } @@ -423,7 +430,9 @@ func updateIndexFiles(ctx *cli.Context, p *pool.Pool, containerID cid.ID, accoun *object.NewAttribute(attributeKey, strconv.Itoa(int(i))), *object.NewAttribute("IndexSize", strconv.Itoa(int(indexFileSize))), } - err := uploadObj(ctx.Context, p, signer, account.PrivateKey().GetScriptHash(), containerID, buffer, attrs, homomorphicHashingDisabled) + err := retry(func() error { + return uploadObj(ctx.Context, p, signer, account.PrivateKey().GetScriptHash(), containerID, buffer, attrs, homomorphicHashingDisabled) + }) if err != nil { return fmt.Errorf("failed to upload index file %d: %w", i, err) } From a5e9ab6979988d02e366240098dd6039fc09c3a3 Mon Sep 17 00:00:00 2001 From: Ekaterina Pavlova Date: Wed, 23 Oct 2024 15:27:45 +0300 Subject: [PATCH 5/5] cli: verify index file construction in `upload-bin` command Verify that there are no empty OIDs in the constructed index file, as empty payloads can result in improperly set attributes, leading to empty OIDs being added to the index file. Close #3628 Signed-off-by: Ekaterina Pavlova --- cli/util/uploader.go | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/cli/util/uploader.go b/cli/util/uploader.go index cba55c7b9..96df4a2d2 100644 --- a/cli/util/uploader.go +++ b/cli/util/uploader.go @@ -4,6 +4,7 @@ import ( "context" "crypto/sha256" "fmt" + "slices" "strconv" "sync" "time" @@ -346,6 +347,8 @@ func updateIndexFiles(ctx *cli.Context, p *pool.Pool, containerID cid.ID, accoun buffer = make([]byte, indexFileSize*oidSize) oidCh = make(chan oid.ID, indexFileSize) oidFetcherToProcessor = make(chan struct{}, indexFileSize) + + emptyOid = make([]byte, oidSize) ) defer close(oidCh) for range maxParallelSearches { @@ -426,6 +429,14 @@ func updateIndexFiles(ctx *cli.Context, p *pool.Pool, containerID cid.ID, accoun } } } + // Check if there are any empty oids in the created index file. + // This could happen if object payload is empty -> + // attribute is not set correctly -> empty oid is added to the index file. + for k := 0; k < len(buffer); k += oidSize { + if slices.Compare(buffer[k:k+oidSize], emptyOid) == 0 { + return fmt.Errorf("empty oid found in index file %d at position %d (block index %d)", i, k/oidSize, i+uint(k/oidSize)) + } + } attrs := []object.Attribute{ *object.NewAttribute(attributeKey, strconv.Itoa(int(i))), *object.NewAttribute("IndexSize", strconv.Itoa(int(indexFileSize))),