Locate object-storing shards #1635

Open
elebedeva wants to merge 3 commits from elebedeva/frostfs-node:feat/shard-info-for-object into master
Member

Added method to ControlService to find info about shards storing an object by object ID and added locate sub-command to frostfs-cli control .

frostfs-node on  feat/shard-info-for-object [$?] via 🐹 v1.23.2 
❯ ./bin/frostfs-cli control locate-object -c ./dev/storage/cfg2.yml --cid FYW7qAB1EKQgcFAQg3e2KQ5Nz8USVL9LFaxSMvLyUPf8 --oid Dg6kH7ZMKdUA2A9kY9k5Ymhqw4B77P6RKhYfovGfcgGG
Shard WXYH4qKZcuVDctYpjc28Mc:
Mode: read-write
Metabase: /home/w0lframm/tmp/frostfs-node/.cache/storage/s2/meta0
Blobstor:
        Path 0: /home/w0lframm/tmp/frostfs-node/.cache/storage/s2/blobovnicza0
        Type 0: blobovnicza
        Path 1: /home/w0lframm/tmp/frostfs-node/.cache/storage/s2/fstree0
        Type 1: fstree
Write-cache: /home/w0lframm/tmp/frostfs-node/.cache/storage/s2/wc0
Pilorama: /home/w0lframm/tmp/frostfs-node/.cache/storage/s2/pilorama0
Error count: 0
Evacuation in progress: false

Signed-off-by: Ekaterina Lebedeva ekaterina.lebedeva@yadro.com

Added method to ControlService to find info about shards storing an object by object ID and added `locate` sub-command to `frostfs-cli control `. ``` frostfs-node on  feat/shard-info-for-object [$?] via 🐹 v1.23.2 ❯ ./bin/frostfs-cli control locate-object -c ./dev/storage/cfg2.yml --cid FYW7qAB1EKQgcFAQg3e2KQ5Nz8USVL9LFaxSMvLyUPf8 --oid Dg6kH7ZMKdUA2A9kY9k5Ymhqw4B77P6RKhYfovGfcgGG Shard WXYH4qKZcuVDctYpjc28Mc: Mode: read-write Metabase: /home/w0lframm/tmp/frostfs-node/.cache/storage/s2/meta0 Blobstor: Path 0: /home/w0lframm/tmp/frostfs-node/.cache/storage/s2/blobovnicza0 Type 0: blobovnicza Path 1: /home/w0lframm/tmp/frostfs-node/.cache/storage/s2/fstree0 Type 1: fstree Write-cache: /home/w0lframm/tmp/frostfs-node/.cache/storage/s2/wc0 Pilorama: /home/w0lframm/tmp/frostfs-node/.cache/storage/s2/pilorama0 Error count: 0 Evacuation in progress: false ``` Signed-off-by: Ekaterina Lebedeva <ekaterina.lebedeva@yadro.com>
elebedeva added 2 commits 2025-02-04 18:36:57 +00:00
Signed-off-by: Ekaterina Lebedeva <ekaterina.lebedeva@yadro.com>
[#XXX] cli: Add flags to get object's shard info
Some checks failed
DCO action / DCO (pull_request) Failing after 34s
Tests and linters / Run gofumpt (pull_request) Successful in 44s
Vulncheck / Vulncheck (pull_request) Successful in 51s
Build / Build Components (pull_request) Successful in 1m45s
Pre-commit hooks / Pre-commit (pull_request) Successful in 2m3s
Tests and linters / Tests (pull_request) Successful in 2m14s
Tests and linters / Staticcheck (pull_request) Successful in 2m13s
Tests and linters / gopls check (pull_request) Successful in 2m52s
Tests and linters / Lint (pull_request) Successful in 3m4s
Tests and linters / Tests with -race (pull_request) Successful in 3m18s
36744bbe9c
Signed-off-by: Ekaterina Lebedeva <ekaterina.lebedeva@yadro.com>
elebedeva force-pushed feat/shard-info-for-object from 36744bbe9c to a7ce8dc3ec 2025-02-06 13:54:04 +00:00 Compare
elebedeva changed title from WIP: Locate object-storing shard to Locate object-storing shard 2025-02-06 14:04:40 +00:00
requested reviews from storage-core-committers, storage-core-developers 2025-02-06 14:04:40 +00:00
acid-ant requested changes 2025-02-06 14:20:16 +00:00
@ -443,2 +443,4 @@
return s.hash
}
func (e *StorageEngine) GetShardByObjectID(ctx context.Context, obj oid.Address) ([]shard.Info, error) {
Member

Need to take mtx here to iterate over shards.

Need to take mtx here to iterate over shards.
Author
Member

Fixed.

Fixed.
elebedeva force-pushed feat/shard-info-for-object from a7ce8dc3ec to 70083f6e1c 2025-02-06 14:55:30 +00:00 Compare
elebedeva force-pushed feat/shard-info-for-object from 70083f6e1c to 04a50e8c59 2025-02-06 15:08:55 +00:00 Compare
elebedeva force-pushed feat/shard-info-for-object from 04a50e8c59 to 02d25794fe 2025-02-06 15:11:06 +00:00 Compare
elebedeva force-pushed feat/shard-info-for-object from 02d25794fe to 6829489621 2025-02-06 15:13:32 +00:00 Compare
elebedeva force-pushed feat/shard-info-for-object from 6829489621 to ecfe06bd7c 2025-02-06 15:16:53 +00:00 Compare
aarifullin reviewed 2025-02-07 08:12:01 +00:00
@ -0,0 +25,4 @@
var obj oid.ID
err = obj.DecodeString(req.GetBody().GetOid())
if err != nil {
return nil, status.Error(codes.Internal, err.Error())
Member

If request sets oid with an invalid format, then we can set codes.InvalidArgument instead of codes.Internal. Thus, a client won't retry GetShardByObjectID on invalid data :)

The same for cid below

If request sets oid with an invalid format, then we can set `codes.InvalidArgument` instead of `codes.Internal`. Thus, a client won't retry `GetShardByObjectID` on invalid data :) The same for cid below
Author
Member

Thank you! Fixed.

Thank you! Fixed.
aarifullin marked this conversation as resolved
aarifullin reviewed 2025-02-07 08:14:33 +00:00
@ -0,0 +67,4 @@
shardInfo := new(control.ShardInfo)
shardInfo.SetShard_ID(*info.ID)
shardInfo.SetMetabasePath(info.MetaBaseInfo.Path)
shardInfo.Blobstor = blobstorInfoToProto(info.BlobStorInfo)
Member

Why don't we use SetBlobstor and use explicit assignment?

Why don't we use `SetBlobstor` and use explicit assignment?
Author
Member

Changed to shardInfo.SetBlobstor().

Changed to `shardInfo.SetBlobstor()`.
aarifullin marked this conversation as resolved
fyrchik requested changes 2025-02-07 10:00:38 +00:00
@ -0,0 +24,4 @@
"github.com/spf13/cobra"
)
var objectLocateCmd = &cobra.Command{
Owner

control service commands should be called with frostfs-cli control ...
Otherwise, it creates confusion

`control` service commands should be called with `frostfs-cli control ...` Otherwise, it creates confusion
Author
Member

Moved this command to frostfs-cli control. I thought since this command finds object's location - it belongs to frostfs-cli object.

Moved this command to `frostfs-cli control`. I thought since this command finds object's location - it belongs to `frostfs-cli object`.
@ -0,0 +114,4 @@
}
}
func prettyPrintShardsJSON(cmd *cobra.Command, ii []control.ShardInfo) {
Owner

All this info is unrelated to the command.
What is the purpose?

All this info is unrelated to the command. What is the purpose?
Author
Member

I looked at shards list and it has --json flag, so I thought json-formatted output here might also be useful.

I looked at `shards list` and it has `--json` flag, so I thought json-formatted output here might also be useful.
@ -445,0 +445,4 @@
func (e *StorageEngine) GetShardByObjectID(ctx context.Context, obj oid.Address) ([]shard.Info, error) {
var err error
info := make([]shard.Info, 0, len(e.shards))
Owner

We do not actually know slice capacity, and most likely there is 0 or 1 shard.
Preallocation is not needed here, just var info []shard.Info.

We do not actually know slice capacity, and _most likely_ there is 0 or 1 shard. Preallocation is not needed here, just `var info []shard.Info`.
Author
Member

Fixed

Fixed
@ -0,0 +16,4 @@
)
func (s *Server) GetShardByObjectID(ctx context.Context, req *control.GetShardByObjectIDRequest) (*control.GetShardByObjectIDResponse, error) {
// verify request
Owner

Please, remove this comment, I find it useless.

Please, remove this comment, I find it useless.
Author
Member

Fixed

Fixed
@ -90,2 +90,4 @@
// StartShardRebuild starts shard rebuild process.
rpc StartShardRebuild(StartShardRebuildRequest) returns (StartShardRebuildResponse);
// GetShardByObjectID returns shard info where object is stored.
Owner

I think GetShardByObjectID is a bad naming -- ObjectID is not some persistent identifier of a shard, it is something that a shard can store.
May be sth like ListShardsForObject?

I think `GetShardByObjectID` is a bad naming -- `ObjectID` is not some persistent identifier of a shard, it is something that a shard can store. May be sth like `ListShardsForObject`?
Author
Member

Thanks, renamed GetShardByObjectID to ListShardsForObject.

Thanks, renamed `GetShardByObjectID` to `ListShardsForObject`.
@ -732,0 +735,4 @@
message GetShardByObjectIDRequest {
message Body {
string oid = 1;
Owner

object_id and container_id would be better (in accordance with other fields like chain_id or shard_id)

`object_id` and `container_id` would be better (in accordance with other fields like `chain_id` or `shard_id`)
Author
Member

Fixed

Fixed
elebedeva force-pushed feat/shard-info-for-object from ecfe06bd7c to 51ffe64227 2025-02-10 11:42:21 +00:00 Compare
dstepanov-yadro requested changes 2025-02-10 12:16:29 +00:00
@ -0,0 +70,4 @@
}
}
func readObjectAddress(cmd *cobra.Command, cnr *cid.ID, obj *oid.ID) oid.Address {

duplicate

func readObjectAddress(cmd *cobra.Command, cnr *cid.ID, obj *oid.ID) oid.Address {

duplicate https://git.frostfs.info/TrueCloudLab/frostfs-node/src/commit/5d79abe523b0254fe0484220d6d2c57a172090e1/cmd/frostfs-cli/modules/object/util.go#L77
Author
Member

Made object.readObjectAddress() public and reused it in locateObject().

Made `object.readObjectAddress()` public and reused it in `locateObject()`.
dstepanov-yadro marked this conversation as resolved
@ -445,0 +450,4 @@
Address: obj,
}
e.iterateOverUnsortedShards(func(hs hashedShard) (stop bool) {

Engine.Exists has some error handling:

func (e *StorageEngine) exists(ctx context.Context, shPrm shard.ExistsPrm) (bool, bool, error) {


Is this error checking not performed here intentionally?

`Engine.Exists` has some error handling: https://git.frostfs.info/TrueCloudLab/frostfs-node/src/commit/5d79abe523b0254fe0484220d6d2c57a172090e1/pkg/local_object_storage/engine/exists.go#L16 Is this error checking not performed here intentionally?
Author
Member

It was intentional as I assumed we should stop on every error. I didn't consider that ObjectNotFound could be an error.
Added error checks.

It was intentional as I assumed we should stop on every error. I didn't consider that `ObjectNotFound` could be an error. Added error checks.
dstepanov-yadro marked this conversation as resolved
a-savchuk reviewed 2025-02-10 15:22:04 +00:00
@ -0,0 +21,4 @@
}
func initControlLocateObjectCmd() {
commonflags.Init(locateObjectCmd)
Member

Control commands have different common flags, see the command for shard listing

initControlFlags(listShardsCmd)

Please double-check and fix if needed

Control commands have different common flags, see the command for shard listing https://git.frostfs.info/TrueCloudLab/frostfs-node/src/commit/5d79abe523b0254fe0484220d6d2c57a172090e1/cmd/frostfs-cli/modules/control/shards_list.go#L27 Please double-check and fix if needed
Author
Member

Fixed

Fixed
a-savchuk marked this conversation as resolved
@ -0,0 +49,4 @@
req.SetBody(body)
signRequest(cmd, pk, req)
cli := internalclient.GetSDKClientByFlag(cmd, pk, commonflags.RPC)
Member

You should use control endpoint, please see the command for shard listing

cli := getClient(cmd, pk)

You should use control endpoint, please see the command for shard listing https://git.frostfs.info/TrueCloudLab/frostfs-node/src/commit/5d79abe523b0254fe0484220d6d2c57a172090e1/cmd/frostfs-cli/modules/control/shards_list.go#L41
Author
Member

Fixed

Fixed
a-savchuk marked this conversation as resolved
a-savchuk reviewed 2025-02-10 15:32:29 +00:00
@ -732,0 +735,4 @@
message ListShardsForObjectRequest {
message Body {
string object_id = 1;
Member

How about using bytes instead of string? An ID as bytes takes 32 bytes, the same ID as a string takes about 44 bytes. Also I see we already use bytes in control IR service.

What do you think?

How about using bytes instead of string? An ID as bytes takes 32 bytes, the same ID as a string takes about 44 bytes. Also I see we already use bytes in control IR service. What do you think?

I think it is an an error that we use bytes for shardID. String is more human readable and control API is not so performance critical to save 12 bytes:)

I think it is an an error that we use `bytes` for shardID. String is more human readable and control API is not so performance critical to save 12 bytes:)
a-savchuk marked this conversation as resolved
elebedeva force-pushed feat/shard-info-for-object from 51ffe64227 to a6bf8437d1 2025-02-13 15:27:23 +00:00 Compare
elebedeva changed title from Locate object-storing shard to WIP: Locate object-storing shard 2025-02-17 07:25:13 +00:00
elebedeva force-pushed feat/shard-info-for-object from a6bf8437d1 to eaf7688ee9 2025-02-18 10:10:52 +00:00 Compare
elebedeva force-pushed feat/shard-info-for-object from eaf7688ee9 to 4a54367233 2025-02-18 13:52:41 +00:00 Compare
elebedeva changed title from WIP: Locate object-storing shard to Locate object-storing shard 2025-02-18 14:03:31 +00:00
requested reviews from storage-core-committers, storage-core-developers 2025-02-18 14:03:31 +00:00
elebedeva changed title from Locate object-storing shard to Locate object-storing shards 2025-02-18 14:05:24 +00:00
a-savchuk reviewed 2025-02-18 14:27:02 +00:00
@ -445,0 +461,4 @@
return true
}
var siErr *objectSDK.SplitInfoError
Member

ECInfoError too

`ECInfoError` too
Author
Member

Added check for ECInfoError

Added check for `ECInfoError`
a-savchuk marked this conversation as resolved
elebedeva force-pushed feat/shard-info-for-object from 4a54367233 to 466779e627 2025-02-18 14:50:29 +00:00 Compare
aarifullin approved these changes 2025-02-18 16:20:22 +00:00
aarifullin left a comment
Member

I find this very useful 👍

I find this very useful 👍
a-savchuk reviewed 2025-02-19 09:50:49 +00:00
@ -445,0 +453,4 @@
Address: obj,
}
e.iterateOverUnsortedShards(func(hs hashedShard) (stop bool) {
Member

Let's iterate over sorted shards

Let's iterate over _sorted_ shards

Why? It is required to find all shards store object, but not the first shard stores object.

Why? It is required to find all shards store object, but not the first shard stores object.
Member

Ok, nevermind

Ok, nevermind
a-savchuk marked this conversation as resolved
@ -445,0 +461,4 @@
return true
}
var siErr *objectSDK.SplitInfoError
Member

Move these variables definitions outside shard iterating

Move these variables definitions outside shard iterating
@ -445,0 +462,4 @@
}
var siErr *objectSDK.SplitInfoError
if errors.As(exErr, &siErr) {
Member

Let's merge similar check together and make less ifs

Let's merge similar check together and make less `if`s
@ -445,0 +482,4 @@
e.reportShardError(ctx, hs, "could not check existence of object in shard", exErr, zap.Stringer("address", prm.Address))
}
}
if res.Exists() {
Member

I think Exists should not be called if an error was returned. Now this happens if you get an unexpected error you then report. You can either add return false after error reporting or use else or do something else

I think `Exists` should not be called if an error was returned. Now this happens if you get an unexpected error you then report. You can either add return `false` after error reporting or use `else` or do something else
All checks were successful
DCO action / DCO (pull_request) Successful in 30s
Required
Details
Vulncheck / Vulncheck (pull_request) Successful in 54s
Required
Details
Pre-commit hooks / Pre-commit (pull_request) Successful in 1m29s
Required
Details
Build / Build Components (pull_request) Successful in 1m33s
Required
Details
Tests and linters / Run gofumpt (pull_request) Successful in 2m27s
Required
Details
Tests and linters / Tests (pull_request) Successful in 2m48s
Required
Details
Tests and linters / Lint (pull_request) Successful in 2m55s
Required
Details
Tests and linters / Staticcheck (pull_request) Successful in 2m54s
Required
Details
Tests and linters / gopls check (pull_request) Successful in 3m2s
Required
Details
Tests and linters / Tests with -race (pull_request) Successful in 4m32s
Required
Details
This pull request doesn't have enough approvals yet. 1 of 2 approvals granted.
You are not authorized to merge this pull request.
View command line instructions

Checkout

From your project repository, check out a new branch and test the changes.
git fetch -u feat/shard-info-for-object:elebedeva-feat/shard-info-for-object
git checkout elebedeva-feat/shard-info-for-object
Sign in to join this conversation.
No reviewers
TrueCloudLab/storage-core-committers
No milestone
No project
No assignees
6 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference: TrueCloudLab/frostfs-node#1635
No description provided.