From f3d8bf8c8b664f4581ce47e7dbfa81d939fe20bd Mon Sep 17 00:00:00 2001 From: Alexander Apalikov Date: Thu, 30 May 2019 18:44:38 +0300 Subject: [PATCH] Fix Down Scale on RollingUpdate Return valid target number of replicas with E2E test. --- pkg/fleets/controller.go | 24 ++++++---- test/e2e/fleet_test.go | 101 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 116 insertions(+), 9 deletions(-) diff --git a/pkg/fleets/controller.go b/pkg/fleets/controller.go index 953fbc9d73..5807c315c9 100644 --- a/pkg/fleets/controller.go +++ b/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) } diff --git a/test/e2e/fleet_test.go b/test/e2e/fleet_test.go index ed240c96b8..3806637178 100644 --- a/test/e2e/fleet_test.go +++ b/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()