traverser: Check for placement vector out of range #1516

Merged
fyrchik merged 1 commit from acid-ant/frostfs-node:bugfix/traverser-out-range into master 2024-11-21 11:27:53 +00:00
2 changed files with 49 additions and 2 deletions

View file

@ -114,8 +114,9 @@ func NewTraverser(opts ...Option) (*Traverser, error) {
var unsortedVector []netmap.NodeInfo var unsortedVector []netmap.NodeInfo

Unrelated non-functional change, please, remove.

Unrelated non-functional change, please, remove.

Other than this LGTM

Other than this LGTM

Reverted.

Reverted.
var regularVector []netmap.NodeInfo var regularVector []netmap.NodeInfo
for i := range rem { for i := range rem {
unsortedVector = append(unsortedVector, ns[i][:rem[i]]...) pivot := min(len(ns[i]), rem[i])

Looks like you have forgotten regularVector = append(regularVector, ns[i][rem[i]:]...) here

Looks like you have forgotten `regularVector = append(regularVector, ns[i][rem[i]:]...)` here

But the like you wrote will panic!

But the like you wrote will panic!

I meant that the second slice is not updated.

I meant that the second slice is not updated.

It should not because we do not have enough nodes for that in the current vector.

It should not because we do not have enough nodes for that in the current vector.

Linter complains about the size of this function.
How about

pivot := min(len(ns[i]), rem[i])
unsortedVector = append(unsortedVector, ns[i][:pivot]...)
regularVector = append(regularVector, ns[i][pivot:]...)
Linter complains about the size of this function. How about ``` pivot := min(len(ns[i]), rem[i]) unsortedVector = append(unsortedVector, ns[i][:pivot]...) regularVector = append(regularVector, ns[i][pivot:]...) ```

It is also somewhat more readable and concise IMO

It is also somewhat more readable and concise IMO

Nice! Updated.

Nice! Updated.
regularVector = append(regularVector, ns[i][rem[i]:]...) unsortedVector = append(unsortedVector, ns[i][:pivot]...)
regularVector = append(regularVector, ns[i][pivot:]...)
} }
rem = []int{-1, -1} rem = []int{-1, -1}

View file

@ -356,6 +356,52 @@ func TestTraverserPriorityMetrics(t *testing.T) {
require.Nil(t, next) require.Nil(t, next)
}) })
t.Run("one rep one metric fewer nodes", func(t *testing.T) {
selectors := []int{2}
replicas := []int{3}
nodes, cnr := testPlacement(selectors, replicas)
// Node_0, PK - ip4/0.0.0.0/tcp/0
nodes[0][0].SetAttribute("ClusterName", "A")
// Node_1, PK - ip4/0.0.0.0/tcp/1
nodes[0][1].SetAttribute("ClusterName", "B")
sdkNode := testNode(5)
sdkNode.SetAttribute("ClusterName", "B")
nodesCopy := copyVectors(nodes)
m := []Metric{NewAttributeMetric("ClusterName")}
tr, err := NewTraverser(
ForContainer(cnr),
UseBuilder(&testBuilder{
vectors: nodesCopy,
}),
WithoutSuccessTracking(),
WithPriorityMetrics(m),
WithNodeState(&nodeState{
node: &sdkNode,
}),
)
require.NoError(t, err)
// Without priority metric `ClusterName` the order will be:
// [ {Node_0 A}, {Node_1 A} ]
// With priority metric `ClusterName` and current node in cluster B
// the order should be:
// [ {Node_1 B}, {Node_0 A} ]
next := tr.Next()
require.NotNil(t, next)
require.Equal(t, 2, len(next))
require.Equal(t, "/ip4/0.0.0.0/tcp/1", string(next[0].PublicKey()))
require.Equal(t, "/ip4/0.0.0.0/tcp/0", string(next[1].PublicKey()))
next = tr.Next()
require.Nil(t, next)
})
t.Run("two reps two metrics", func(t *testing.T) { t.Run("two reps two metrics", func(t *testing.T) {
selectors := []int{3, 3} selectors := []int{3, 3}
replicas := []int{2, 2} replicas := []int{2, 2}