container/ape: Ignore an error when getting a role #1454

a-savchuk · 2024-10-29T13:11:58Z

a-savchuk commented

2024-10-29 13:11:58 +00:00

When getting a role in the APE checker for the container services, an error may be returned if network maps of the previous two epochs don't have enough nodes to fulfil a container placement policy. It's a logical error, so we should ignore it.

The affected function isContainerNode is used for determining if an actor is a container role when checking an APE rule. It's guaranteed that the policy of a container is valid because that container has been with that policy before. So, the only error could be returned if the map doesn't have enough nodes. It's a logical error, we should ignore it because it doesn't affect an actor role determining.

Before:

~/repos/frostfs/dev-env$ ./frostfs-cli -r s01.frostfs.devenv:8080 container get \
    -g --cid G1B2X97U5w6iLbjazJUNgkaA4WQ9LDvDnw1vyS3QyfKv
rpc error: status: code = 1024 message = not enough nodes to SELECT from

After:

~/repos/frostfs/dev-env$ ./frostfs-cli -r s01.frostfs.devenv:8080 container get \
    -g --cid 9HnHVeeNDMz8Zbphio4APTKQnVf7Xbremw1x8BaCE6NB
CID: 9HnHVeeNDMz8Zbphio4APTKQnVf7Xbremw1x8BaCE6NB
owner ID: NbUgTSFvPmsRxmGeWpuuGeJUoRoi6PErcM
basic ACL: 1c8c8ccc (private)
       RangeHASH    Range      Search     Delete     Put        Head       Get
0 1    1 1 0 0      1 0 0 0    1 1 0 0    1 0 0 0    1 1 0 0    1 1 0 0    1 1 0 0
X F    U S O B      U S O B    U S O B    U S O B    U S O B    U S O B    U S O B
  X-Sticky F-Final U-User S-System O-Others B-Bearer
created: 2024-10-29 17:35:23 +0300 MSK
attributes:
        Timestamp=1730212523
placement policy:
REP 1
CBF 1

Close #1448 When getting a role in the APE checker for the container services, an error may be returned if network maps of the previous two epochs don't have enough nodes to fulfil a container placement policy. It's a logical error, so we should ignore it. The affected function `isContainerNode` is used for determining if an actor is a container role when checking an APE rule. It's guaranteed that the policy of a container is valid because that container has been with that policy before. So, the only error could be returned if the map doesn't have enough nodes. It's a logical error, we should ignore it because it doesn't affect an actor role determining. Before: ``` ~/repos/frostfs/dev-env$ ./frostfs-cli -r s01.frostfs.devenv:8080 container get \ -g --cid G1B2X97U5w6iLbjazJUNgkaA4WQ9LDvDnw1vyS3QyfKv rpc error: status: code = 1024 message = not enough nodes to SELECT from ``` After: ``` ~/repos/frostfs/dev-env$ ./frostfs-cli -r s01.frostfs.devenv:8080 container get \ -g --cid 9HnHVeeNDMz8Zbphio4APTKQnVf7Xbremw1x8BaCE6NB CID: 9HnHVeeNDMz8Zbphio4APTKQnVf7Xbremw1x8BaCE6NB owner ID: NbUgTSFvPmsRxmGeWpuuGeJUoRoi6PErcM basic ACL: 1c8c8ccc (private) RangeHASH Range Search Delete Put Head Get 0 1 1 1 0 0 1 0 0 0 1 1 0 0 1 0 0 0 1 1 0 0 1 1 0 0 1 1 0 0 X F U S O B U S O B U S O B U S O B U S O B U S O B U S O B X-Sticky F-Final U-User S-System O-Others B-Bearer created: 2024-10-29 17:35:23 +0300 MSK attributes: Timestamp=1730212523 placement policy: REP 1 CBF 1 ```

a-savchuk added 1 commit 2024-10-29 13:11:59 +00:00

[#1448 ] services/container/ape: Ignore an error when getting a role

DCO action / DCO (pull_request) Successful in 44s

Details

Tests and linters / Run gofumpt (pull_request) Successful in 2m14s

Details

Tests and linters / Staticcheck (pull_request) Successful in 2m42s

Details

Pre-commit hooks / Pre-commit (pull_request) Successful in 2m50s

Details

Build / Build Components (pull_request) Successful in 3m0s

Details

Vulncheck / Vulncheck (pull_request) Successful in 2m52s

Details

Tests and linters / gopls check (pull_request) Successful in 3m45s

Details

Tests and linters / Tests with -race (pull_request) Successful in 3m50s

Details

Tests and linters / Lint (pull_request) Successful in 5m47s

Details

Tests and linters / Tests (pull_request) Successful in 8m24s

Details

96b5b6a28b

When getting a role in the APE checker for the container services,
an error may be returned if network maps of the previous two epochs
don't have enough nodes to fulfil a container placement policy.
It's a logical error, so we should ignore it.

Signed-off-by: Aleksey Savchuk <a.savchuk@yadro.com>

a-savchuk changed title from ~~WIP: services/container/ape: Ignore an error when getting a role~~ to WIP: container/ape: Ignore an error when getting a role

2024-10-29 13:13:44 +00:00

a-savchuk force-pushed container-get-on-first-epoch from 96b5b6a28b to 2fa82acd1c

2024-10-29 13:13:48 +00:00

Compare

a-savchuk force-pushed container-get-on-first-epoch from 2fa82acd1c to e70c2cedbe

2024-10-29 14:36:02 +00:00

Compare

a-savchuk changed title from ~~WIP: container/ape: Ignore an error when getting a role~~ to container/ape: Ignore an error when getting a role

2024-10-29 14:41:45 +00:00

a-savchuk requested review from storage-core-developers 2024-10-29 14:41:56 +00:00

a-savchuk requested review from storage-core-committers 2024-10-29 14:41:59 +00:00

fyrchik approved these changes 2024-10-29 14:56:39 +00:00

Dismissed

dstepanov-yadro reviewed 2024-10-29 15:18:32 +00:00

pkg/services/container/ape.go Outdated

					
				@ -562,0 +554,4 @@

					// don't have enough nodes to fulfil a container placement policy.

					// It's a logical error, so we should ignore it.

					// See https://git.frostfs.info/TrueCloudLab/frostfs-node/issues/1448.

					cnrVectors, _ := nm.ContainerNodes(cont.Value.PlacementPolicy(), binCnrID)

dstepanov-yadro commented

2024-10-29 15:18:32 +00:00

Why is any error ignored, but not only specific one?

a-savchuk commented

2024-10-30 09:43:34 +00:00

Made the PR description more detailed:

The affected function isContainerNode is used for determining if an actor is a container role when checking an APE rule. It's guaranteed that the policy of a container is valid because that container has been with that policy before. So, the only error could be returned if the map doesn't have enough nodes. It's a logical error, we should ignore it because it doesn't affect an actor role determining.

Moreover, that error is not a part of the SDK interface, so if I ignore only that error, it'll look like this

if err != nil && !strings.Contains(err.Error(), "not enough nodes to SELECT from") {
    return false, err
}

Made the PR description more detailed: > The affected function `isContainerNode` is used for determining if an actor is a container role when checking an APE rule. It's guaranteed that the policy of a container is valid because that container has been with that policy before. So, the only error could be returned if the map doesn't have enough nodes. It's a logical error, we should ignore it because it doesn't affect an actor role determining. Moreover, that error is not a part of the SDK interface, so if I ignore only that error, it'll look like this ```go if err != nil && !strings.Contains(err.Error(), "not enough nodes to SELECT from") { return false, err } ```

dstepanov-yadro marked this conversation as resolved

acid-ant reviewed 2024-10-29 15:31:14 +00:00

pkg/services/container/ape.go Outdated

					
				@ -562,0 +553,4 @@

					// An error may be returned if network maps of the previous two epochs

					// don't have enough nodes to fulfil a container placement policy.

					// It's a logical error, so we should ignore it.

					// See https://git.frostfs.info/TrueCloudLab/frostfs-node/issues/1448.

acid-ant commented

2024-10-29 15:31:14 +00:00

Link to the issue looks redundant here, it will be in the commit message.

a-savchuk commented

2024-10-30 09:49:40 +00:00

Yeah, removed it✅

a-savchuk changed title from ~~container/ape: Ignore an error when getting a role~~ to WIP: container/ape: Ignore an error when getting a role

2024-10-30 09:43:58 +00:00

a-savchuk added 1 commit 2024-10-30 09:48:47 +00:00

[#1448 ] container/ape: Ignore an error when getting a role

Tests and linters / Run gofumpt (pull_request) Successful in 1m26s

Details

DCO action / DCO (pull_request) Successful in 1m39s

Details

Vulncheck / Vulncheck (pull_request) Successful in 2m9s

Details

Pre-commit hooks / Pre-commit (pull_request) Successful in 2m22s

Details

Build / Build Components (pull_request) Successful in 2m28s

Details

Tests and linters / gopls check (pull_request) Successful in 2m54s

Details

Tests and linters / Staticcheck (pull_request) Successful in 2m56s

Details

Tests and linters / Lint (pull_request) Successful in 4m12s

Details

Tests and linters / Tests (pull_request) Successful in 4m44s

Details

Tests and linters / Tests with -race (pull_request) Successful in 6m5s

Details

6daa1fda7f

When getting a role in the APE checker for the container services,
an error may be returned if network maps of the previous two epochs
don't have enough nodes to fulfil a container placement policy.
It's a logical error, so we should ignore it.

Signed-off-by: Aleksey Savchuk <a.savchuk@yadro.com>

a-savchuk dismissed fyrchik's review 2024-10-30 09:48:47 +00:00

Reason:

New commits pushed, approval review dismissed automatically according to repository settings

a-savchuk force-pushed container-get-on-first-epoch from 6daa1fda7f to d28a5d2d7a

2024-10-30 09:52:13 +00:00

Compare

dstepanov-yadro approved these changes 2024-10-30 09:53:12 +00:00

a-savchuk changed title from ~~WIP: container/ape: Ignore an error when getting a role~~ to container/ape: Ignore an error when getting a role

2024-10-30 09:54:12 +00:00

fyrchik merged commit d28a5d2d7a into master

2024-10-30 10:41:29 +00:00

a-savchuk deleted branch container-get-on-first-epoch

2024-10-30 11:10:57 +00:00

Sign in to join this conversation.

No reviewers

TrueCloudLab/storage-core-developers