ROX-16108: Replace Skip Scheduling with Schedulable; fix Placement lo…

…gic (stackrox#904) * Remove skip scheduling. * Add migration to delete the SkipScheduling column. * Remove final references to skip scheduling. Replace with use of schedulable. * Remove SkipScheduling, add Schedulable to cluster API and table columns * Tests * Fixed tests for placement. Removed supported instance type from cluster search API. * Fix migration * Fix migration ID * Remove unnecessary columns in migrations, per @machi1990's review comments. * Fix typo * Update moq file * Fix unit test * Fixes * Remove gorm default value from Schedulable. No columns use default values and, while 'false' may be surprising, the default value should never be used (aside from when this change is initially rolled out until the first reconciliation of the data plane config in saas.yaml is merged). * Update Schedulable field in database if the field's value changes in the dataplane config * Use values map to convince Gorm to update * Update moq --------- Co-authored-by: Vlad Bologa <[email protected]> Co-authored-by: Manyanda Chitimbo <[email protected]>
dsp-testing · Mar 30, 2023 · 4cdd011 · 4cdd011
1 parent ec52fac
commit 4cdd011
Show file tree

Hide file tree

Showing 14 changed files with 261 additions and 154 deletions.
diff --git a/.secrets.baseline b/.secrets.baseline
@@ -257,63 +257,63 @@
         "filename": "dev/env/manifests/shared/03-configmap-config.yaml",
         "hashed_secret": "13032f402fed753c2248419ea4f69f99931f6dbc",
         "is_verified": false,
-        "line_number": 48
+        "line_number": 49
       },
       {
         "type": "Base64 High Entropy String",
         "filename": "dev/env/manifests/shared/03-configmap-config.yaml",
         "hashed_secret": "30025f80f6e22cdafb85db387d50f90ea884576a",
         "is_verified": false,
-        "line_number": 48
+        "line_number": 49
       },
       {
         "type": "Base64 High Entropy String",
         "filename": "dev/env/manifests/shared/03-configmap-config.yaml",
         "hashed_secret": "355f24fd038bcaf85617abdcaa64af51ed19bbcf",
         "is_verified": false,
-        "line_number": 48
+        "line_number": 49
       },
       {
         "type": "Base64 High Entropy String",
         "filename": "dev/env/manifests/shared/03-configmap-config.yaml",
         "hashed_secret": "3d8a1dcd2c3c765ce35c9a9552d23273cc4ddace",
         "is_verified": false,
-        "line_number": 48
+        "line_number": 49
       },
       {
         "type": "Base64 High Entropy String",
         "filename": "dev/env/manifests/shared/03-configmap-config.yaml",
         "hashed_secret": "4ac7b0522761eba972467942cd5cd7499dd2c361",
         "is_verified": false,
-        "line_number": 48
+        "line_number": 49
       },
       {
         "type": "Base64 High Entropy String",
         "filename": "dev/env/manifests/shared/03-configmap-config.yaml",
         "hashed_secret": "7639ab2a6bcf2ea30a055a99468c9cd844d4c22a",
         "is_verified": false,
-        "line_number": 48
+        "line_number": 49
       },
       {
         "type": "Base64 High Entropy String",
         "filename": "dev/env/manifests/shared/03-configmap-config.yaml",
         "hashed_secret": "b56360daf4793d2a74991a972b34d95bc00fb2da",
         "is_verified": false,
-        "line_number": 48
+        "line_number": 49
       },
       {
         "type": "Base64 High Entropy String",
         "filename": "dev/env/manifests/shared/03-configmap-config.yaml",
         "hashed_secret": "c9a73ef9ee8ce9f38437227801c70bcc6740d1a1",
         "is_verified": false,
-        "line_number": 48
+        "line_number": 49
       },
       {
         "type": "Base64 High Entropy String",
         "filename": "dev/env/manifests/shared/03-configmap-config.yaml",
         "hashed_secret": "14736999d9940728c5294277831a702f7882dece",
         "is_verified": false,
-        "line_number": 85
+        "line_number": 86
       }
     ],
     "fleetshard/pkg/central/cloudprovider/dbclient_moq.go": [
@@ -546,5 +546,5 @@
       }
     ]
   },
-  "generated_at": "2023-02-06T09:40:26Z"
+  "generated_at": "2023-03-28T21:54:47Z"
 }
diff --git a/dev/env/manifests/shared/03-configmap-config.yaml b/dev/env/manifests/shared/03-configmap-config.yaml
@@ -7,6 +7,7 @@ data:
         cluster_id: "$CLUSTER_ID"
         cloud_provider: standalone
         region: standalone
+        multi_az: true
         schedulable: true
         status: ready
         provider_type: standalone

diff --git a/internal/dinosaur/pkg/converters/cluster.go b/internal/dinosaur/pkg/converters/cluster.go
@@ -33,6 +33,7 @@ func ConvertCluster(cluster *api.Cluster) []map[string]interface{} {
 			"provider_type":  cluster.ProviderType.String(),
 			"provider_spec":  p,
 			"cluster_spec":   c,
+			"schedulable":    cluster.Schedulable,
 		},
 	}
 }

diff --git a/internal/dinosaur/pkg/migrations/202303220000_drop_skip_scheduling_from_clusters.go b/internal/dinosaur/pkg/migrations/202303220000_drop_skip_scheduling_from_clusters.go
@@ -0,0 +1,46 @@
+package migrations
+
+// Migrations should NEVER use types from other packages. Types can change
+// and then migrations run on a _new_ database will fail or behave unexpectedly.
+// Instead of importing types, always re-create the type in the migration, as
+// is done here, even though the same type is defined in pkg/api
+
+import (
+	"github.com/golang/glog"
+
+	"github.com/go-gormigrate/gormigrate/v2"
+	"github.com/pkg/errors"
+	"github.com/stackrox/acs-fleet-manager/pkg/db"
+	"gorm.io/gorm"
+)
+
+func dropSkipSchedulingFromClusters() *gormigrate.Migration {
+	type Cluster struct {
+		db.Model
+		SkipScheduling bool `json:"skip_scheduling" gorm:"default:false"` // To be dropped
+	}
+
+	id := "202303221200"
+	colName := "SkipScheduling"
+	return &gormigrate.Migration{
+		ID: id,
+		Migrate: func(tx *gorm.DB) error {
+			if tx.Migrator().HasColumn(&Cluster{}, colName) {
+				if err := tx.Migrator().DropColumn(&Cluster{}, colName); err != nil {
+					return errors.Wrapf(err, "rolling back from column %s in migration %s", colName, id)
+				}
+				glog.Infof("Successfully removed the %s column", colName)
+			}
+			return nil
+		},
+		Rollback: func(tx *gorm.DB) error {
+			if !tx.Migrator().HasColumn(&Cluster{}, colName) {
+				if err := tx.Migrator().AddColumn(&Cluster{}, colName); err != nil {
+					return errors.Wrapf(err, "adding column %s in migration %s", colName, id)
+				}
+				glog.Infof("Successfully added the %s column", colName)
+			}
+			return nil
+		},
+	}
+}
diff --git a/internal/dinosaur/pkg/migrations/202303230000_add_schedulable_to_clusters.go b/internal/dinosaur/pkg/migrations/202303230000_add_schedulable_to_clusters.go
@@ -0,0 +1,46 @@
+package migrations
+
+// Migrations should NEVER use types from other packages. Types can change
+// and then migrations run on a _new_ database will fail or behave unexpectedly.
+// Instead of importing types, always re-create the type in the migration, as
+// is done here, even though the same type is defined in pkg/api
+
+import (
+	"github.com/golang/glog"
+
+	"github.com/go-gormigrate/gormigrate/v2"
+	"github.com/pkg/errors"
+	"github.com/stackrox/acs-fleet-manager/pkg/db"
+	"gorm.io/gorm"
+)
+
+func addSchedulableToClusters() *gormigrate.Migration {
+	type Cluster struct {
+		db.Model
+		Schedulable bool `json:"schedulable"` // To be added
+	}
+
+	id := "202303231200"
+	colName := "Schedulable"
+	return &gormigrate.Migration{
+		ID: id,
+		Migrate: func(tx *gorm.DB) error {
+			if !tx.Migrator().HasColumn(&Cluster{}, colName) {
+				if err := tx.Migrator().AddColumn(&Cluster{}, colName); err != nil {
+					return errors.Wrapf(err, "adding column %s in migration %s", colName, id)
+				}
+				glog.Infof("Successfully added the %s column", colName)
+			}
+			return nil
+		},
+		Rollback: func(tx *gorm.DB) error {
+			if tx.Migrator().HasColumn(&Cluster{}, colName) {
+				if err := tx.Migrator().DropColumn(&Cluster{}, colName); err != nil {
+					return errors.Wrapf(err, "rolling back from column %s in migration %s", colName, id)
+				}
+				glog.Infof("Successfully removed the %s column", colName)
+			}
+			return nil
+		},
+	}
+}
diff --git a/internal/dinosaur/pkg/migrations/migrations.go b/internal/dinosaur/pkg/migrations/migrations.go
@@ -41,6 +41,8 @@ func getMigrations() []*gormigrate.Migration {
 		addOrganisationNameToCentralRequest(),
 		addInternalToCentralRequest(),
 		addCentralDefaultVersion(),
+		dropSkipSchedulingFromClusters(),
+		addSchedulableToClusters(),
 	}
 }
 

diff --git a/internal/dinosaur/pkg/services/cluster_placement_strategy.go b/internal/dinosaur/pkg/services/cluster_placement_strategy.go
@@ -1,11 +1,9 @@
 package services
 
 import (
-	"errors"
 	"strings"
 
 	"github.com/stackrox/acs-fleet-manager/internal/dinosaur/pkg/api/dbapi"
-	"github.com/stackrox/acs-fleet-manager/internal/dinosaur/pkg/config"
 	"github.com/stackrox/acs-fleet-manager/pkg/api"
 )
 
@@ -20,7 +18,7 @@ type ClusterPlacementStrategy interface {
 // NewClusterPlacementStrategy return a concrete strategy impl. depends on the
 // placement configuration. An appropriate ClusterPlacementStrategy implementation
 // is returned based on the received parameters content
-func NewClusterPlacementStrategy(clusterService ClusterService, dataplaneClusterConfig *config.DataplaneClusterConfig) ClusterPlacementStrategy {
+func NewClusterPlacementStrategy(clusterService ClusterService) ClusterPlacementStrategy {
 	return &FirstReadyPlacementStrategy{clusterService: clusterService}
 }
 
@@ -33,18 +31,23 @@ type FirstReadyPlacementStrategy struct {
 
 // FindCluster ...
 func (d FirstReadyPlacementStrategy) FindCluster(central *dbapi.CentralRequest) (*api.Cluster, error) {
-	clusters, err := d.clusterService.FindAllClusters(FindClusterCriteria{Status: api.ClusterReady})
+	clusters, err := d.clusterService.FindAllClusters(FindClusterCriteria{
+		Provider: central.CloudProvider,
+		Region:   central.Region,
+		MultiAZ:  central.MultiAZ,
+		Status:   api.ClusterReady,
+	})
 	if err != nil {
 		return nil, err
 	}
 
 	for _, c := range clusters {
-		if !c.SkipScheduling && supportsInstanceType(c, central.InstanceType) {
+		if c.Schedulable && supportsInstanceType(c, central.InstanceType) {
 			return c, nil
 		}
 	}
 
-	return nil, errors.New("no schedulable cluster found")
+	return nil, nil
 }
 
 func supportsInstanceType(c *api.Cluster, instanceType string) bool {

diff --git a/internal/dinosaur/pkg/services/cluster_placement_strategy_test.go b/internal/dinosaur/pkg/services/cluster_placement_strategy_test.go
@@ -1,7 +1,6 @@
 package services
 
 import (
-	"errors"
 	"testing"
 
 	"github.com/stackrox/acs-fleet-manager/internal/dinosaur/pkg/api/dbapi"
@@ -33,14 +32,39 @@ func TestPlacementStrategyType(t *testing.T) {
 
 	for _, tc := range tt {
 		t.Run(tc.description, func(t *testing.T) {
-			strategy := NewClusterPlacementStrategy(tc.createClusterService(), tc.dataPlaneConfig)
+			strategy := NewClusterPlacementStrategy(tc.createClusterService())
 
 			require.IsType(t, tc.expectedType, strategy)
 		})
 	}
 }
 
 func TestFirstClusterPlacementStrategy(t *testing.T) {
+	centralRequest := buildCentralRequest(func(centralRequest *dbapi.CentralRequest) {
+		centralRequest.InstanceType = "standard"
+	})
+
+	notSchedulable := buildCluster(func(cluster *api.Cluster) {
+		cluster.ClusterID = "notSchedulable"
+		cluster.SupportedInstanceType = "standard,eval"
+		cluster.Schedulable = false
+	})
+	notSupported := buildCluster(func(cluster *api.Cluster) {
+		cluster.ClusterID = "notSupported"
+		cluster.SupportedInstanceType = "eval"
+		cluster.Schedulable = true
+	})
+	goodCluster1 := buildCluster(func(cluster *api.Cluster) {
+		cluster.ClusterID = "good1"
+		cluster.SupportedInstanceType = "standard,eval"
+		cluster.Schedulable = true
+	})
+	goodCluster2 := buildCluster(func(cluster *api.Cluster) {
+		cluster.ClusterID = "good2"
+		cluster.SupportedInstanceType = "standard,eval"
+		cluster.Schedulable = true
+	})
+
 	tt := []struct {
 		description           string
 		newClusterServiceMock func() ClusterService
@@ -57,82 +81,61 @@ func TestFirstClusterPlacementStrategy(t *testing.T) {
 					},
 				}
 			},
-			central:         buildCentralRequest(func(centralRequest *dbapi.CentralRequest) {}),
+			central:         centralRequest,
 			expectedError:   serviceErrors.New(apiErrors.ErrorGeneral, "error in FindAllClusters"),
 			expectedCluster: nil,
 		},
 		{
-			description: "should return error if clusters is empty",
+			description: "should return nil if clusters is empty",
 			newClusterServiceMock: func() ClusterService {
 				return &ClusterServiceMock{
 					FindAllClustersFunc: func(criteria FindClusterCriteria) ([]*api.Cluster, *serviceErrors.ServiceError) {
 						return []*api.Cluster{}, nil
 					},
 				}
 			},
-			central:         buildCentralRequest(func(centralRequest *dbapi.CentralRequest) {}),
-			expectedError:   errors.New("no schedulable cluster found"),
+			central:         centralRequest,
+			expectedError:   nil,
 			expectedCluster: nil,
 		},
 		{
-			description: "should return error if no clusters with SkipScheduling false was found",
+			description: "should return nil if no cluster supporting central instancetype was found",
 			newClusterServiceMock: func() ClusterService {
 				return &ClusterServiceMock{
 					FindAllClustersFunc: func(criteria FindClusterCriteria) ([]*api.Cluster, *serviceErrors.ServiceError) {
-						return []*api.Cluster{
-							buildCluster(func(cluster *api.Cluster) {
-								cluster.SkipScheduling = true
-							}),
-							buildCluster(func(cluster *api.Cluster) {
-								cluster.SkipScheduling = true
-							}),
-						}, nil
+						return []*api.Cluster{notSupported}, nil
 					},
 				}
 			},
-			central:         buildCentralRequest(func(centralRequest *dbapi.CentralRequest) {}),
-			expectedError:   errors.New("no schedulable cluster found"),
+			central:         centralRequest,
+			expectedError:   nil,
 			expectedCluster: nil,
 		},
 		{
-			description: "should return error if no cluster supporting central instancetype was found",
+			description: "should return nil if no cluster is schedulable",
 			newClusterServiceMock: func() ClusterService {
 				return &ClusterServiceMock{
 					FindAllClustersFunc: func(criteria FindClusterCriteria) ([]*api.Cluster, *serviceErrors.ServiceError) {
-						return []*api.Cluster{
-							buildCluster(func(cluster *api.Cluster) {}),
-							buildCluster(func(cluster *api.Cluster) {}),
-						}, nil
+						return []*api.Cluster{notSchedulable}, nil
 					},
 				}
 			},
-			central: buildCentralRequest(func(centralRequest *dbapi.CentralRequest) {
-				centralRequest.InstanceType = "standard"
-			}),
-			expectedError:   errors.New("no schedulable cluster found"),
+			central:         centralRequest,
+			expectedError:   nil,
 			expectedCluster: nil,
 		},
 		{
-			description: "should return first schedulable cluster",
+			description: "should return first good cluster",
 			newClusterServiceMock: func() ClusterService {
 				return &ClusterServiceMock{
 					FindAllClustersFunc: func(criteria FindClusterCriteria) ([]*api.Cluster, *serviceErrors.ServiceError) {
-						return []*api.Cluster{
-							buildCluster(func(cluster *api.Cluster) {}),
-							buildCluster(func(cluster *api.Cluster) {
-								cluster.SupportedInstanceType = "standard,eval"
-							}),
-						}, nil
+						return []*api.Cluster{notSupported, goodCluster1, notSchedulable, goodCluster2}, nil
 					},
 				}
 			},
-			central: buildCentralRequest(func(centralRequest *dbapi.CentralRequest) {
-				centralRequest.InstanceType = "standard"
-			}),
-			expectedError: nil,
-			expectedCluster: buildCluster(func(cluster *api.Cluster) {
-				cluster.SupportedInstanceType = "standard,eval"
-			}),
+			central:         centralRequest,
+			expectedError:   nil,
+			expectedCluster: goodCluster1,
 		},
 	}