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

@@ -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
 

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) {
+func TestCriteria_ExcludedNodeNetworks(t *testing.T) {
-	excluded := testrand.NodeID()
+	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.1.0",
-		LastNet: "192.168.0.1",
+	}))
+
+	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.3.0",
-		LastNet: "192.168.0.2",
 	}))
-
 }
 
 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)
 }
 

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) {

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))

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 {

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)