From 552242387137416c9ff6d061992595111996eca2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?M=C3=A1rton=20Elek?= Date: Thu, 17 Aug 2023 11:53:05 +0200 Subject: [PATCH] satellite/nodeselection: remove AutoExcludeSubnet filter It's statefull, therefore it can hit naive users. (NodeFilters couldn't be reused for more than one iterations). But looks like we don't need it, as `SelectBySubnet` doest the same job. Change-Id: Ie85b7f9c2bd9a47293f4e3b359f8b619215c7649 --- satellite/nodeselection/filter.go | 53 ++++++++++------------- satellite/nodeselection/filter_test.go | 48 +++++++------------- satellite/nodeselection/selector_test.go | 3 -- satellite/nodeselection/state.go | 17 +++++++- satellite/overlay/uploadselection.go | 10 +---- satellite/overlay/uploadselection_test.go | 4 +- 6 files changed, 55 insertions(+), 80 deletions(-) diff --git a/satellite/nodeselection/filter.go b/satellite/nodeselection/filter.go index a625f0527..c6c092e7b 100644 --- a/satellite/nodeselection/filter.go +++ b/satellite/nodeselection/filter.go @@ -45,6 +45,13 @@ func GetAnnotation(filter NodeFilter, name string) string { if annotated, ok := filter.(AnnotatedNodeFilter); ok { return annotated.Annotations[name] } + if filters, ok := filter.(NodeFilters); ok { + for _, filter := range filters { + if annotated, ok := filter.(AnnotatedNodeFilter); ok { + return annotated.Annotations[name] + } + } + } return "" } @@ -82,11 +89,6 @@ func (n NodeFilters) WithCountryFilter(permit location.Set) NodeFilters { return append(n, NewCountryFilter(permit)) } -// WithAutoExcludeSubnets is a helper to create a new filter with additional AutoExcludeSubnets. -func (n NodeFilters) WithAutoExcludeSubnets() NodeFilters { - return append(n, NewAutoExcludeSubnets()) -} - // WithExcludedIDs is a helper to create a new filter with additional WithExcludedIDs. func (n NodeFilters) WithExcludedIDs(ds []storj.NodeID) NodeFilters { return append(n, ExcludedIDs(ds)) @@ -113,32 +115,6 @@ func (p *CountryFilter) MatchInclude(node *SelectedNode) bool { var _ NodeFilter = &CountryFilter{} -// AutoExcludeSubnets pick at most one node from network. -// -// Stateful!!! should be re-created for each new selection request. -// It should only be used as the last filter. -type AutoExcludeSubnets struct { - seenSubnets map[string]struct{} -} - -// NewAutoExcludeSubnets creates an initialized AutoExcludeSubnets. -func NewAutoExcludeSubnets() *AutoExcludeSubnets { - return &AutoExcludeSubnets{ - seenSubnets: map[string]struct{}{}, - } -} - -// MatchInclude implements NodeFilter interface. -func (a *AutoExcludeSubnets) MatchInclude(node *SelectedNode) bool { - if _, found := a.seenSubnets[node.LastNet]; found { - return false - } - a.seenSubnets[node.LastNet] = struct{}{} - return true -} - -var _ NodeFilter = &AutoExcludeSubnets{} - // ExcludedNetworks will exclude nodes with specified networks. type ExcludedNetworks []string @@ -154,6 +130,21 @@ func (e ExcludedNetworks) MatchInclude(node *SelectedNode) bool { var _ NodeFilter = ExcludedNetworks{} +// ExcludedNodeNetworks exclude nodes which has same net as the one of the specified. +type ExcludedNodeNetworks []*SelectedNode + +// MatchInclude implements NodeFilter interface. +func (e ExcludedNodeNetworks) MatchInclude(node *SelectedNode) bool { + for _, n := range e { + if node.LastNet == n.LastNet { + return false + } + } + return true +} + +var _ NodeFilter = ExcludedNodeNetworks{} + // ExcludedIDs can blacklist NodeIDs. type ExcludedIDs []storj.NodeID diff --git a/satellite/nodeselection/filter_test.go b/satellite/nodeselection/filter_test.go index 4cfc453e2..fa871e4aa 100644 --- a/satellite/nodeselection/filter_test.go +++ b/satellite/nodeselection/filter_test.go @@ -17,23 +17,6 @@ import ( "storj.io/common/testrand" ) -func TestNodeFilter_AutoExcludeSubnet(t *testing.T) { - - criteria := NodeFilters{}.WithAutoExcludeSubnets() - - assert.True(t, criteria.MatchInclude(&SelectedNode{ - LastNet: "192.168.0.1", - })) - - assert.False(t, criteria.MatchInclude(&SelectedNode{ - LastNet: "192.168.0.1", - })) - - assert.True(t, criteria.MatchInclude(&SelectedNode{ - LastNet: "192.168.1.1", - })) -} - func TestCriteria_ExcludeNodeID(t *testing.T) { included := testrand.NodeID() excluded := testrand.NodeID() @@ -47,29 +30,29 @@ func TestCriteria_ExcludeNodeID(t *testing.T) { assert.True(t, criteria.MatchInclude(&SelectedNode{ ID: included, })) - } -func TestCriteria_NodeIDAndSubnet(t *testing.T) { - excluded := testrand.NodeID() +func TestCriteria_ExcludedNodeNetworks(t *testing.T) { + criteria := NodeFilters{} + criteria = append(criteria, ExcludedNodeNetworks{ + &SelectedNode{ + LastNet: "192.168.1.0", + }, &SelectedNode{ + LastNet: "192.168.2.0", + }, + }) - criteria := NodeFilters{}. - WithExcludedIDs([]storj.NodeID{excluded}). - WithAutoExcludeSubnets() - - // due to node id criteria assert.False(t, criteria.MatchInclude(&SelectedNode{ - ID: excluded, - LastNet: "192.168.0.1", + LastNet: "192.168.1.0", + })) + + assert.False(t, criteria.MatchInclude(&SelectedNode{ + LastNet: "192.168.2.0", })) - // should be included as previous one excluded and - // not stored for subnet exclusion assert.True(t, criteria.MatchInclude(&SelectedNode{ - ID: testrand.NodeID(), - LastNet: "192.168.0.2", + LastNet: "192.168.3.0", })) - } func TestCriteria_Geofencing(t *testing.T) { @@ -135,7 +118,6 @@ func BenchmarkNodeFilterFullTable(b *testing.B) { filters = append(filters, NodeFilterFunc(func(node *SelectedNode) bool { return true })) - filters = filters.WithAutoExcludeSubnets() benchmarkFilter(b, filters) } diff --git a/satellite/nodeselection/selector_test.go b/satellite/nodeselection/selector_test.go index 6aecd9c57..ad4220700 100644 --- a/satellite/nodeselection/selector_test.go +++ b/satellite/nodeselection/selector_test.go @@ -248,12 +248,9 @@ func TestSelectFiltered(t *testing.T) { selector := nodeselection.SelectByID(nodes) assert.Len(t, selector.Select(3, nodeselection.NodeFilters{}), 3) - assert.Len(t, selector.Select(3, nodeselection.NodeFilters{}.WithAutoExcludeSubnets()), 2) assert.Len(t, selector.Select(3, nodeselection.NodeFilters{}), 3) assert.Len(t, selector.Select(3, nodeselection.NodeFilters{}.WithExcludedIDs([]storj.NodeID{firstID, secondID})), 1) - assert.Len(t, selector.Select(3, nodeselection.NodeFilters{}.WithAutoExcludeSubnets()), 2) - assert.Len(t, selector.Select(3, nodeselection.NodeFilters{}.WithExcludedIDs([]storj.NodeID{thirdID}).WithAutoExcludeSubnets()), 1) } func TestSelectFilteredMulti(t *testing.T) { diff --git a/satellite/nodeselection/state.go b/satellite/nodeselection/state.go index 6ff909150..48d0bde8d 100644 --- a/satellite/nodeselection/state.go +++ b/satellite/nodeselection/state.go @@ -12,6 +12,14 @@ import ( "storj.io/common/storj" ) +const ( + // AutoExcludeSubnet is placement annotation key to turn off subnet restrictions. + AutoExcludeSubnet = "autoExcludeSubnet" + + // AutoExcludeSubnetOFF is the value of AutoExcludeSubnet to disable subnet restrictions. + AutoExcludeSubnetOFF = "off" +) + // ErrNotEnoughNodes is when selecting nodes failed with the given parameters. var ErrNotEnoughNodes = errs.Class("not enough nodes") @@ -114,8 +122,13 @@ func (state *State) Select(ctx context.Context, request Request) (_ []*SelectedN // Get all the remaining reputable nodes. reputableCount := totalCount - len(selected) - selected = append(selected, - reputableNodes.Select(reputableCount, request.NodeFilters)...) + + filters := request.NodeFilters + if GetAnnotation(filters, AutoExcludeSubnet) != AutoExcludeSubnetOFF { + filters = append(append(NodeFilters{}, request.NodeFilters...), ExcludedNodeNetworks(selected)) + } + + selected = append(selected, reputableNodes.Select(reputableCount, filters)...) if len(selected) < totalCount { return selected, ErrNotEnoughNodes.New("requested from cache %d, found %d", totalCount, len(selected)) diff --git a/satellite/overlay/uploadselection.go b/satellite/overlay/uploadselection.go index 04dff4a5f..32225e591 100644 --- a/satellite/overlay/uploadselection.go +++ b/satellite/overlay/uploadselection.go @@ -13,14 +13,6 @@ import ( "storj.io/storj/satellite/nodeselection" ) -const ( - // AutoExcludeSubnet is placement annotation key to turn off subnet restrictions. - AutoExcludeSubnet = "autoExcludeSubnet" - - // AutoExcludeSubnetOFF is the value of AutoExcludeSubnet to disable subnet restrictions. - AutoExcludeSubnetOFF = "off" -) - // UploadSelectionDB implements the database for upload selection cache. // // architecture: Database @@ -105,7 +97,7 @@ func (cache *UploadSelectionCache) GetNodes(ctx context.Context, req FindStorage } placementRules := cache.placementRules(req.Placement) - useSubnetExclusion := nodeselection.GetAnnotation(placementRules, AutoExcludeSubnet) != AutoExcludeSubnetOFF + useSubnetExclusion := nodeselection.GetAnnotation(placementRules, nodeselection.AutoExcludeSubnet) != nodeselection.AutoExcludeSubnetOFF filters := nodeselection.NodeFilters{placementRules} if len(req.ExcludedIDs) > 0 { diff --git a/satellite/overlay/uploadselection_test.go b/satellite/overlay/uploadselection_test.go index 5527a979b..ccd46fc11 100644 --- a/satellite/overlay/uploadselection_test.go +++ b/satellite/overlay/uploadselection_test.go @@ -215,7 +215,7 @@ func TestGetNodes(t *testing.T) { } placementRules := overlay.NewPlacementRules() placementRules.AddPlacementRule(storj.PlacementConstraint(5), nodeselection.NodeFilters{}.WithCountryFilter(location.NewSet(location.Germany))) - placementRules.AddPlacementRule(storj.PlacementConstraint(6), nodeselection.WithAnnotation(nodeselection.NodeFilters{}.WithCountryFilter(location.NewSet(location.Germany)), overlay.AutoExcludeSubnet, overlay.AutoExcludeSubnetOFF)) + placementRules.AddPlacementRule(storj.PlacementConstraint(6), nodeselection.WithAnnotation(nodeselection.NodeFilters{}.WithCountryFilter(location.NewSet(location.Germany)), nodeselection.AutoExcludeSubnet, nodeselection.AutoExcludeSubnetOFF)) cache, err := overlay.NewUploadSelectionCache(zap.NewNop(), db.OverlayCache(), @@ -526,7 +526,7 @@ func TestGetNodesDistinct(t *testing.T) { &mockDB, highStaleness, config, - nodeselection.NodeFilters{}.WithAutoExcludeSubnets(), + nodeselection.NodeFilters{}, overlay.NewPlacementRules().CreateFilters, ) require.NoError(t, err)