SPopN doesn't need to be inside a transaction

Gopkg.toml

@@ -85,7 +85,7 @@
 
 [[dependencies]]
   name = "github.com/topfreegames/extensions"
-  version = "v2.5.2"
+  version = "v2.5.3"
 
 [[dependencies]]
   name = "github.com/go-redis/redis"

controller/controller.go

@@ -274,17 +274,11 @@ func ScaleDown(logger logrus.FieldLogger, mr *models.MixedMetricsReporter, db pg
 	}()
 
 	l.Debug("accessing redis")
-	pipe := redisClient.TxPipeline()
-	sReady := pipe.SPopN(models.GetRoomStatusSetRedisKey(scheduler.Name, models.StatusReady), int64(amount))
-	_, err := pipe.Exec()
-	if err != nil {
-		l.WithError(err).Error("scale down error")
-		return err
-	}
+	sReady := redisClient.SPopN(models.GetRoomStatusSetRedisKey(scheduler.Name, models.StatusReady), int64(amount))
 
 	idleRooms, err := sReady.Result()
 	if err != nil {
-		l.WithError(err).Error("scale down error")
+		l.WithError(err).Error("scale down error, failed to retrieve ready rooms from redis")
 		return err
 	}
 
@@ -349,14 +343,6 @@ func deleteSchedulerHelper(logger logrus.FieldLogger, mr *models.MixedMetricsRep
 		}
 	}
 
-	err = mr.WithSegment(models.SegmentDelete, func() error {
-		return scheduler.Delete(db)
-	})
-	if err != nil {
-		logger.WithError(err).Error("failed to delete scheduler from database while deleting scheduler")
-		return err
-	}
-
 	configYAML, _ := models.NewConfigYAML(scheduler.YAML)
 	// Delete pods and wait for graceful termination before deleting the namespace
 	err = mr.WithSegment(models.SegmentNamespace, func() error {
@@ -416,6 +402,18 @@ func deleteSchedulerHelper(logger logrus.FieldLogger, mr *models.MixedMetricsRep
 			time.Sleep(time.Duration(1) * time.Second)
 		}
 	}
+
+	// Delete from DB must be the last operation because
+	// if kubernetes failed to delete service and pods, watcher will recreate
+	// and keep the last state
+	err = mr.WithSegment(models.SegmentDelete, func() error {
+		return scheduler.Delete(db)
+	})
+	if err != nil {
+		logger.WithError(err).Error("failed to delete scheduler from database while deleting scheduler")
+		return err
+	}
+
 	return nil
 }
 

controller/controller_test.go

@@ -739,11 +739,9 @@ var _ = Describe("Controller", func() {
 			names, err := controller.GetServiceNames(scaleDownAmount, scheduler.Name, clientset)
 			Expect(err).NotTo(HaveOccurred())
 
-			mockRedisClient.EXPECT().TxPipeline().Return(mockPipeline)
-			mockPipeline.EXPECT().
+			mockRedisClient.EXPECT().
 				SPopN(models.GetRoomStatusSetRedisKey(configYaml1.Name, models.StatusReady), int64(scaleDownAmount)).
 				Return(redis.NewStringSliceResult(names, nil))
-			mockPipeline.EXPECT().Exec()
 
 			for _, name := range names {
 				mockRedisClient.EXPECT().TxPipeline().Return(mockPipeline)
@@ -792,11 +790,9 @@ var _ = Describe("Controller", func() {
 			names, err := controller.GetServiceNames(scaleDownAmount, scheduler.Name, clientset)
 			Expect(err).NotTo(HaveOccurred())
 
-			mockRedisClient.EXPECT().TxPipeline().Return(mockPipeline)
-			mockPipeline.EXPECT().
+			mockRedisClient.EXPECT().
 				SPopN(models.GetRoomStatusSetRedisKey(configYaml1.Name, models.StatusReady), int64(scaleDownAmount)).
 				Return(redis.NewStringSliceResult(names, nil))
-			mockPipeline.EXPECT().Exec()
 
 			mockRedisClient.EXPECT().TxPipeline().Return(mockPipeline)
 			room := models.NewRoom(names[0], scheduler.Name)
@@ -843,11 +839,9 @@ var _ = Describe("Controller", func() {
 			names, err := controller.GetServiceNames(scaleDownAmount, scheduler.Name, clientset)
 			Expect(err).NotTo(HaveOccurred())
 
-			mockRedisClient.EXPECT().TxPipeline().Return(mockPipeline)
-			mockPipeline.EXPECT().
+			mockRedisClient.EXPECT().
 				SPopN(models.GetRoomStatusSetRedisKey(configYaml1.Name, models.StatusReady), int64(scaleDownAmount)).
-				Return(redis.NewStringSliceResult(names, nil))
-			mockPipeline.EXPECT().Exec().Return([]redis.Cmder{}, errors.New("some error in redis"))
+				Return(redis.NewStringSliceResult(names, errors.New("some error in redis")))
 
 			timeoutSec = 0
 			err = controller.ScaleDown(logger, mr, mockDb, mockRedisClient, clientset, scheduler, scaleDownAmount, timeoutSec)
@@ -878,11 +872,9 @@ var _ = Describe("Controller", func() {
 			// ScaleDown
 			scaleDownAmount := 2
 
-			mockRedisClient.EXPECT().TxPipeline().Return(mockPipeline)
-			mockPipeline.EXPECT().
+			mockRedisClient.EXPECT().
 				SPopN(models.GetRoomStatusSetRedisKey(configYaml1.Name, models.StatusReady), int64(scaleDownAmount)).
 				Return(redis.NewStringSliceResult(nil, errors.New("some error in redis")))
-			mockPipeline.EXPECT().Exec()
 
 			timeoutSec = 0
 			err = controller.ScaleDown(logger, mr, mockDb, mockRedisClient, clientset, scheduler, scaleDownAmount, timeoutSec)
@@ -901,11 +893,9 @@ var _ = Describe("Controller", func() {
 			names := []string{"non-existing-service"}
 			Expect(err).NotTo(HaveOccurred())
 
-			mockRedisClient.EXPECT().TxPipeline().Return(mockPipeline)
-			mockPipeline.EXPECT().
+			mockRedisClient.EXPECT().
 				SPopN(models.GetRoomStatusSetRedisKey(configYaml1.Name, models.StatusReady), int64(scaleDownAmount)).
 				Return(redis.NewStringSliceResult(names, nil))
-			mockPipeline.EXPECT().Exec()
 
 			timeoutSec = 300
 			err = controller.ScaleDown(logger, mr, mockDb, mockRedisClient, clientset, scheduler, scaleDownAmount, timeoutSec)
@@ -938,11 +928,9 @@ var _ = Describe("Controller", func() {
 			names, err := controller.GetServiceNames(scaleDownAmount, scheduler.Name, clientset)
 			Expect(err).NotTo(HaveOccurred())
 
-			mockRedisClient.EXPECT().TxPipeline().Return(mockPipeline)
-			mockPipeline.EXPECT().
+			mockRedisClient.EXPECT().
 				SPopN(models.GetRoomStatusSetRedisKey(configYaml1.Name, models.StatusReady), int64(scaleDownAmount)).
 				Return(redis.NewStringSliceResult(names, nil))
-			mockPipeline.EXPECT().Exec()
 
 			for _, name := range names {
 				mockRedisClient.EXPECT().TxPipeline().Return(mockPipeline)

watcher/watcher_test.go

@@ -515,11 +515,9 @@ var _ = Describe("Watcher", func() {
 			scaleDownAmount := 2
 			names, err := controller.GetServiceNames(scaleDownAmount, scheduler.Name, clientset)
 			Expect(err).NotTo(HaveOccurred())
-			mockRedisClient.EXPECT().TxPipeline().Return(mockPipeline)
-			mockPipeline.EXPECT().
+			mockRedisClient.EXPECT().
 				SPopN(models.GetRoomStatusSetRedisKey(configYaml1.Name, models.StatusReady), int64(scaleDownAmount)).
 				Return(redis.NewStringSliceResult(names, nil))
-			mockPipeline.EXPECT().Exec()
 			for _, name := range names {
 				mockRedisClient.EXPECT().TxPipeline().Return(mockPipeline)
 				room := models.NewRoom(name, scheduler.Name)