TrueCloudLab/frostfs-sdk-go
33
0
Fork 21
Code Issues 34 Pull requests 1 Releases Activity Actions

Allow to select insufficient number of nodes #167

Merged
fyrchik merged 3 commits from fyrchik/frostfs-sdk-go:fix-policy into master 2024-09-04 19:51:15 +00:00
Conversation 0 Commits 3 Files changed 6 +136 -36
6 changed files with 136 additions and 36 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

4
netmap/context.go
View file

@ -40,6 +40,10 @@ type context struct {
// policy uses the UNIQUE flag. Nodes marked as used are not used in subsequent // policy uses the UNIQUE flag. Nodes marked as used are not used in subsequent
// base selections. // base selections.
usedNodes map[uint64]bool usedNodes map[uint64]bool
// If true, returns an error when netmap does not contain enough nodes for selection.
// By default best effort is taken.
strict bool
} }
// Various validation errors. // Various validation errors.

6
netmap/json_test.go
View file

@ -2,6 +2,7 @@ package netmap
import ( import (
"encoding/json" "encoding/json"
"fmt"
"os" "os"
"path/filepath" "path/filepath"
"testing" "testing"
@ -47,7 +48,8 @@ func TestPlacementPolicy_Interopability(t *testing.T) {
require.NoError(t, err) require.NoError(t, err)
for i := range ds { for i := range ds {
bs, err := os.ReadFile(filepath.Join(testsDir, ds[i].Name())) filename := filepath.Join(testsDir, ds[i].Name())
bs, err := os.ReadFile(filename)
require.NoError(t, err) require.NoError(t, err)
var tc TestCase var tc TestCase
@ -56,7 +58,7 @@ func TestPlacementPolicy_Interopability(t *testing.T) {
srcNodes := make([]NodeInfo, len(tc.Nodes)) srcNodes := make([]NodeInfo, len(tc.Nodes))
copy(srcNodes, tc.Nodes) copy(srcNodes, tc.Nodes)
t.Run(tc.Name, func(t *testing.T) { t.Run(fmt.Sprintf("%s:%s", filename, tc.Name), func(t *testing.T) {
var nm NetMap var nm NetMap
nm.SetNodes(tc.Nodes) nm.SetNodes(tc.Nodes)

95
netmap/json_tests/non_strict.json Normal file
View file

@ -0,0 +1,95 @@
{
"name": "non-strict selections",
"comment": "These test specify loose selection behaviour, to allow fetching already PUT objects even when there is not enough nodes to select from.",
"nodes": [
{
"attributes": [
{
"key": "Country",
"value": "Russia"
}
]
},
{
"attributes": [
{
"key": "Country",
"value": "Germany"
}
]
},
{
"attributes": [ ]
}
],
"tests": {
"not enough nodes (backup factor)": {
"policy": {
"replicas": [
{
"count": 1,
"selector": "MyStore"
}
],
"containerBackupFactor": 2,
"selectors": [
{
"name": "MyStore",
"count": 2,
"clause": "DISTINCT",
"attribute": "Country",
"filter": "FromRU"
}
],
"filters": [
{
"name": "FromRU",
"key": "Country",
"op": "EQ",
"value": "Russia",
"filters": [ ]
}
]
},
"result": [
[
0
]
]
},
"not enough nodes (buckets)": {
"policy": {
"replicas": [
{
"count": 1,
"selector": "MyStore"
}
],
"containerBackupFactor": 1,
"selectors": [
{
"name": "MyStore",
"count": 2,
"clause": "DISTINCT",
"attribute": "Country",
"filter": "FromRU"
}
],
"filters": [
{
"name": "FromRU",
"key": "Country",
"op": "EQ",
"value": "Russia",
"filters": [ ]
}
]
},
"result": [
[
0
]
]
}
}
}

9
netmap/json_tests/rep_only.json
View file

@ -104,7 +104,14 @@
"selectors": [], "selectors": [],
"filters": [] "filters": []
}, },
"error": "not enough nodes" "result": [
[
0,
1,
2,
3
]
]
} }
} }
} }

47
netmap/json_tests/selector_invalid.json
View file

@ -24,7 +24,12 @@
"tests": { "tests": {
"missing filter": { "missing filter": {
"policy": { "policy": {
"replicas": [], "replicas": [
{
"count": 1,
"selector": "MyStore"
}
],
"containerBackupFactor": 1, "containerBackupFactor": 1,
"selectors": [ "selectors": [
{ {
@ -47,9 +52,14 @@
}, },
"error": "filter not found" "error": "filter not found"
}, },
"not enough nodes (backup factor)": { "not enough nodes (filter results in empty set)": {
"policy": { "policy": {
"replicas": [], "replicas": [
{
"count": 1,
"selector": "MyStore"
}
],
"containerBackupFactor": 2, "containerBackupFactor": 2,
"selectors": [ "selectors": [
{ {
@ -57,40 +67,15 @@
"count": 2, "count": 2,
"clause": "DISTINCT", "clause": "DISTINCT",
"attribute": "Country", "attribute": "Country",
"filter": "FromRU" "filter": "FromMoon"
} }
], ],
"filters": [ "filters": [
{ {
"name": "FromRU", "name": "FromMoon",
"key": "Country", "key": "Country",
"op": "EQ", "op": "EQ",
"value": "Russia", "value": "Moon",
"filters": []
}
]
},
"error": "not enough nodes"
},
"not enough nodes (buckets)": {
"policy": {
"replicas": [],
"containerBackupFactor": 1,
"selectors": [
{
"name": "MyStore",
"count": 2,
"clause": "DISTINCT",
"attribute": "Country",
"filter": "FromRU"
}
],
"filters": [
{
"name": "FromRU",
"key": "Country",
"op": "EQ",
"value": "Russia",
"filters": [] "filters": []
} }
] ]

11
netmap/selector.go
View file

@ -60,7 +60,7 @@ func (c *context) getSelection(s netmap.Selector) ([]nodes, error) {
bucketCount, nodesInBucket := calcNodesCount(s) bucketCount, nodesInBucket := calcNodesCount(s)
buckets := c.getSelectionBase(s) buckets := c.getSelectionBase(s)
if len(buckets) < bucketCount { if c.strict && len(buckets) < bucketCount {
aarifullin commented 2023-09-15 12:06:49 +00:00
Review
Copy link

The desctiption says

Currently this is done with always constant strict=false now. In future this flag should be set for container creation in the frostfs-cli

But it seems it always was implictly strict=true?

The desctiption says > Currently this is done with always constant strict=false now. In future this flag should be set for container creation in the frostfs-cli But it seems it always was implictly `strict=true`?
fyrchik commented 2023-09-15 12:10:03 +00:00
Review
Copy link

Yes, we change the behaviour here. This is intentional, to make everything else pick up this change automatically: one can argue it is the user expectation, to GET objects it has previously PUT.
I consider the change backwards compatible, because we only allow some outputs which have previously resulted in error.

Yes, we change the behaviour here. This is intentional, to make everything else pick up this change automatically: one can argue it is the user expectation, to GET objects it has previously PUT. I consider the change backwards compatible, because we only allow some outputs which have previously resulted in error.
👍 1
return nil, fmt.Errorf("%w: '%s'", errNotEnoughNodes, s.GetName()) return nil, fmt.Errorf("%w: '%s'", errNotEnoughNodes, s.GetName())
} }
@ -96,7 +96,7 @@ func (c *context) getSelection(s netmap.Selector) ([]nodes, error) {
if len(res) < bucketCount { if len(res) < bucketCount {
// Fallback to using minimum allowed backup factor (1). // Fallback to using minimum allowed backup factor (1).
res = append(res, fallback...) res = append(res, fallback...)
if len(res) < bucketCount { if c.strict && len(res) < bucketCount {
return nil, fmt.Errorf("%w: '%s'", errNotEnoughNodes, s.GetName()) return nil, fmt.Errorf("%w: '%s'", errNotEnoughNodes, s.GetName())
} }
} }
@ -110,6 +110,13 @@ func (c *context) getSelection(s netmap.Selector) ([]nodes, error) {
hrw.SortHasherSliceByWeightValue(res, weights, c.hrwSeedHash) hrw.SortHasherSliceByWeightValue(res, weights, c.hrwSeedHash)
} }
if len(res) < bucketCount {
if len(res) == 0 {
return nil, errNotEnoughNodes
}
bucketCount = len(res)
}
if s.GetAttribute() == "" { if s.GetAttribute() == "" {
res, fallback = res[:bucketCount], res[bucketCount:] res, fallback = res[:bucketCount], res[bucketCount:]
for i := range fallback { for i := range fallback {