Fix Down Scale on RollingUpdate

pkg/fleets/controller.go

@@ -223,7 +223,7 @@ func (c *Controller) loggerForFleetKey(key string) *logrus.Entry {
 func (c *Controller) loggerForFleet(f *v1alpha1.Fleet) *logrus.Entry {
 	fleetName := "NilFleet"
 	if f != nil {
-		fleetName = f.Namespace + "/" + f.Name
+		fleetName = f.ObjectMeta.Namespace + "/" + f.ObjectMeta.Name
 	}
 	return c.loggerForFleetKey(fleetName).WithField("fleet", f)
 }
@@ -278,6 +278,7 @@ func (c *Controller) syncFleet(key string) error {
 	}
 
 	active, rest := c.filterGameServerSetByActive(fleet, list)
+
 	// if there isn't an active gameServerSet, create one (but don't persist yet)
 	if active == nil {
 		c.loggerForFleet(fleet).Info("could not find active GameServerSet, creating")
@@ -416,14 +417,20 @@ func (c *Controller) rollingUpdateDeployment(fleet *stablev1alpha1.Fleet, active
 // and returns what its replica value should be set to
 func (c *Controller) rollingUpdateActive(fleet *stablev1alpha1.Fleet, active *stablev1alpha1.GameServerSet, rest []*stablev1alpha1.GameServerSet) (int32, error) {
 	replicas := active.Spec.Replicas
+	// always leave room for Allocated GameServers
+	sumAllocated := stablev1alpha1.SumStatusAllocatedReplicas(rest)
 
 	// if the active spec replicas are greater than or equal the fleet spec replicas, then we don't
 	// need to another rolling update upwards.
 	// Likewise if the active spec replicas don't equal the active status replicas, this means we are
 	// in the middle of a rolling update, and should wait for it to complete.
-	if active.Spec.Replicas >= fleet.Spec.Replicas || active.Spec.Replicas != active.Status.Replicas {
+
+	if active.Spec.Replicas != active.Status.Replicas {
 		return replicas, nil
 	}
+	if active.Spec.Replicas >= (fleet.Spec.Replicas - sumAllocated) {
+		return fleet.Spec.Replicas - sumAllocated, nil
+	}
 
 	r, err := intstr.GetValueFromIntOrPercent(fleet.Spec.Strategy.RollingUpdate.MaxSurge, int(fleet.Spec.Replicas), true)
 	if err != nil {
@@ -439,12 +446,9 @@ func (c *Controller) rollingUpdateActive(fleet *stablev1alpha1.Fleet, active *st
 		replicas = fleet.LowerBoundReplicas(replicas - (total - maxSurge))
 	}
 
-	// always leave room for Allocated GameServers
-	sumAllocated := stablev1alpha1.SumStatusAllocatedReplicas(rest)
-
 	// make room for allocated game servers, but not over the fleet replica count
 	if replicas+sumAllocated > fleet.Spec.Replicas {
-		replicas = fleet.LowerBoundReplicas(replicas - sumAllocated)
+		replicas = fleet.LowerBoundReplicas(fleet.Spec.Replicas - sumAllocated)
 	}
 
 	c.loggerForFleet(fleet).WithField("gameserverset", active.ObjectMeta.Name).WithField("replicas", replicas).
@@ -506,7 +510,10 @@ func (c *Controller) updateFleetStatus(fleet *stablev1alpha1.Fleet) error {
 		return err
 	}
 
-	fCopy := fleet.DeepCopy()
+	fCopy, err := c.fleetGetter.Fleets(fleet.ObjectMeta.Namespace).Get(fleet.ObjectMeta.GetName(), metav1.GetOptions{})
+	if err != nil {
+		return err
+	}
 	fCopy.Status.Replicas = 0
 	fCopy.Status.ReadyReplicas = 0
 	fCopy.Status.ReservedReplicas = 0
@@ -518,8 +525,7 @@ func (c *Controller) updateFleetStatus(fleet *stablev1alpha1.Fleet) error {
 		fCopy.Status.ReservedReplicas += gsSet.Status.ReservedReplicas
 		fCopy.Status.AllocatedReplicas += gsSet.Status.AllocatedReplicas
 	}
-
-	_, err = c.fleetGetter.Fleets(fCopy.Namespace).UpdateStatus(fCopy)
+	_, err = c.fleetGetter.Fleets(fCopy.ObjectMeta.Namespace).UpdateStatus(fCopy)
 	return errors.Wrapf(err, "error updating status of fleet %s", fCopy.ObjectMeta.Name)
 }
 

test/e2e/fleet_test.go

@@ -122,6 +122,107 @@ func TestCreateFullFleetAndCantFleetAllocate(t *testing.T) {
 	}
 }
 
+func TestFleetScaleUpEditAndScaleDown(t *testing.T) {
+	t.Parallel()
+
+	//Use scaleFleetPatch (true) or scaleFleetSubresource (false)
+	fixtures := []bool{true, false}
+
+	for _, usePatch := range fixtures {
+		t.Run("Use fleet Patch "+fmt.Sprint(usePatch), func(t *testing.T) {
+			alpha1 := framework.AgonesClient.StableV1alpha1()
+
+			flt := defaultFleet()
+			flt.Spec.Replicas = 1
+			flt, err := alpha1.Fleets(defaultNs).Create(flt)
+			if assert.Nil(t, err) {
+				defer alpha1.Fleets(defaultNs).Delete(flt.ObjectMeta.Name, nil) // nolint:errcheck
+			}
+
+			assert.Equal(t, int32(1), flt.Spec.Replicas)
+
+			framework.WaitForFleetCondition(t, flt, e2e.FleetReadyCount(flt.Spec.Replicas))
+
+			// scale up
+			const targetScale = 3
+			if usePatch {
+				flt = scaleFleetPatch(t, flt, targetScale)
+				assert.Equal(t, int32(targetScale), flt.Spec.Replicas)
+			} else {
+				flt = scaleFleetSubresource(t, flt, targetScale)
+			}
+
+			framework.WaitForFleetCondition(t, flt, e2e.FleetReadyCount(targetScale))
+
+			// get an allocation
+
+			fa := &v1alpha1.FleetAllocation{
+				ObjectMeta: metav1.ObjectMeta{GenerateName: "allocation-", Namespace: defaultNs},
+				Spec: v1alpha1.FleetAllocationSpec{
+					FleetName: flt.ObjectMeta.Name,
+				},
+			}
+
+			fa, err = alpha1.FleetAllocations(defaultNs).Create(fa)
+			assert.Nil(t, err)
+			assert.Equal(t, v1alpha1.GameServerStateAllocated, fa.Status.GameServer.Status.State)
+			framework.WaitForFleetCondition(t, flt, func(fleet *v1alpha1.Fleet) bool {
+				return fleet.Status.AllocatedReplicas == 1
+			})
+
+			flt, err = alpha1.Fleets(defaultNs).Get(flt.ObjectMeta.GetName(), metav1.GetOptions{})
+			assert.Nil(t, err)
+
+			// Change ContainerPort to trigger creating a new GSSet
+			fltCopy := flt.DeepCopy()
+			fltCopy.Spec.Template.Spec.Ports[0].ContainerPort++
+			flt, err = alpha1.Fleets(defaultNs).Update(fltCopy)
+			assert.Nil(t, err)
+
+			// Wait for one more GSSet to be created and ReadyReplicas created in new GSS
+			err = wait.PollImmediate(1*time.Second, 15*time.Second, func() (bool, error) {
+				selector := labels.SelectorFromSet(labels.Set{v1alpha1.FleetNameLabel: flt.ObjectMeta.Name})
+				list, err := framework.AgonesClient.StableV1alpha1().GameServerSets(defaultNs).List(
+					metav1.ListOptions{LabelSelector: selector.String()})
+				if err != nil {
+					return false, err
+				}
+				ready := false
+				if len(list.Items) == 2 {
+					for _, v := range list.Items {
+						if v.Status.ReadyReplicas > 0 && v.Status.AllocatedReplicas == 0 {
+							ready = true
+						}
+					}
+				}
+				return ready, nil
+			})
+
+			assert.Nil(t, err)
+
+			// scale down, with allocation
+			const scaleDownTarget = 1
+			if usePatch {
+				flt = scaleFleetPatch(t, flt, scaleDownTarget)
+			} else {
+				flt = scaleFleetSubresource(t, flt, scaleDownTarget)
+			}
+			framework.WaitForFleetCondition(t, flt, e2e.FleetReadyCount(0))
+
+			// delete the allocated GameServer
+			gp := int64(1)
+			err = alpha1.GameServers(defaultNs).Delete(fa.Status.GameServer.ObjectMeta.Name, &metav1.DeleteOptions{GracePeriodSeconds: &gp})
+			assert.Nil(t, err)
+
+			framework.WaitForFleetCondition(t, flt, e2e.FleetReadyCount(1))
+
+			framework.WaitForFleetCondition(t, flt, func(fleet *v1alpha1.Fleet) bool {
+				return fleet.Status.AllocatedReplicas == 0
+			})
+		})
+	}
+}
+
 func TestScaleFleetUpAndDownWithFleetAllocation(t *testing.T) {
 	t.Parallel()