Move RollingUpdateOnReady to stable (#2271)

* Move RollingUpdateOnReady to stable

* Address review feedbacks

* Fix test failures

Co-authored-by: Robert Bailey <robertbailey@google.com>

cloudbuild.yaml

@@ -232,7 +232,7 @@ steps:
 #
 
 - name: 'e2e-runner'
-  args: ['PlayerTracking=true&SDKWatchSendOnExecute=false&RollingUpdateOnReady=false&NodeExternalDNS=true&StateAllocationFilter=true&PlayerAllocationFilter=true&CustomFasSyncInterval=true', 'e2e-test-cluster']
+  args: ['PlayerTracking=true&SDKWatchSendOnExecute=false&NodeExternalDNS=true&StateAllocationFilter=true&PlayerAllocationFilter=true&CustomFasSyncInterval=true', 'e2e-test-cluster']
   id: e2e-feature-gates
   waitFor:
     - push-images

pkg/fleets/controller.go

@@ -603,57 +603,7 @@ func (c *Controller) rollingUpdateRestFixedOnReady(ctx context.Context, fleet *a
 
 // rollingUpdateRest applies the rolling update to the inactive GameServerSets
 func (c *Controller) rollingUpdateRest(ctx context.Context, fleet *agonesv1.Fleet, active *agonesv1.GameServerSet, rest []*agonesv1.GameServerSet) error {
-	if runtime.FeatureEnabled(runtime.FeatureRollingUpdateOnReady) {
-		return c.rollingUpdateRestFixedOnReady(ctx, fleet, active, rest)
-	}
-	return c.rollingUpdateRestBeforeFixOnReady(ctx, fleet, rest)
-}
-
-func (c *Controller) rollingUpdateRestBeforeFixOnReady(ctx context.Context, fleet *agonesv1.Fleet, rest []*agonesv1.GameServerSet) error {
-	if len(rest) == 0 {
-		return nil
-	}
-
-	r, err := intstr.GetValueFromIntOrPercent(fleet.Spec.Strategy.RollingUpdate.MaxUnavailable, int(fleet.Spec.Replicas), true)
-
-	if err != nil {
-		return errors.Wrapf(err, "error parsing MaxUnavailable value: %s", fleet.ObjectMeta.Name)
-	}
-	unavailable := int32(r)
-
-	for _, gsSet := range rest {
-
-		// if the status.Replicas are less than or equal to 0, then that means we are done
-		// scaling this GameServerSet down, and can therefore exit/move to the next one.
-		if gsSet.Status.Replicas <= 0 {
-			continue
-		}
-
-		// If the Spec.Replicas does not equal the Status.Replicas for this GameServerSet, this means
-		// that the rolling down process is currently ongoing, and we should therefore exit so we can wait for it to finish
-		if gsSet.Spec.Replicas != gsSet.Status.Replicas {
-			break
-		}
-		gsSetCopy := gsSet.DeepCopy()
-		newReplicasCount := fleet.LowerBoundReplicas(gsSetCopy.Spec.Replicas - unavailable)
-		if gsSet.Status.ShutdownReplicas == 0 {
-			gsSetCopy.Spec.Replicas = newReplicasCount
-			c.loggerForFleet(fleet).Debug(fmt.Sprintf("Replicas in a Shutdown state - %d", gsSet.Status.ShutdownReplicas))
-			c.loggerForFleet(fleet).WithField("gameserverset", gsSet.ObjectMeta.Name).WithField("replicas", gsSetCopy.Spec.Replicas).
-				Debug("applying rolling update to inactive gameserverset")
-
-			if _, err := c.gameServerSetGetter.GameServerSets(gsSetCopy.ObjectMeta.Namespace).Update(ctx, gsSetCopy, metav1.UpdateOptions{}); err != nil {
-				return errors.Wrapf(err, "error updating gameserverset %s", gsSetCopy.ObjectMeta.Name)
-			}
-			c.recorder.Eventf(fleet, corev1.EventTypeNormal, "ScalingGameServerSet",
-				"Scaling inactive GameServerSet %s from %d to %d", gsSetCopy.ObjectMeta.Name, gsSet.Spec.Replicas, gsSetCopy.Spec.Replicas)
-
-			// let's update just one at a time, slightly slower, but a simpler solution that doesn't require us
-			// to make sure we don't overshoot the amount that is being shutdown at any given point and time
-			break
-		}
-	}
-	return nil
+	return c.rollingUpdateRestFixedOnReady(ctx, fleet, active, rest)
 }
 
 // updateFleetStatus gets the GameServerSets for this Fleet and then

pkg/fleets/controller_test.go

@@ -196,10 +196,6 @@ func TestControllerSyncFleet(t *testing.T) {
 	})
 
 	t.Run("gameserverset with different image details", func(t *testing.T) {
-		utilruntime.FeatureTestMutex.Lock()
-		defer utilruntime.FeatureTestMutex.Unlock()
-		assert.NoError(t, utilruntime.ParseFeatures(string(utilruntime.FeatureRollingUpdateOnReady)+"=true"))
-
 		f := defaultFixture()
 		f.Spec.Strategy.Type = appsv1.RollingUpdateDeploymentStrategyType
 		f.Spec.Template.Spec.Ports = []agonesv1.GameServerPort{{HostPort: 5555}}
@@ -1006,12 +1002,7 @@ func TestControllerRollingUpdateDeploymentGSSUpdateFailedErrExpected(t *testing.
 	assert.EqualError(t, err, "error updating gameserverset inactive: random-err")
 }
 
-func TestFeatureRollingUpdateOnReady(t *testing.T) {
-	utilruntime.FeatureTestMutex.Lock()
-	defer utilruntime.FeatureTestMutex.Unlock()
-
-	assert.NoError(t, utilruntime.ParseFeatures(string(utilruntime.FeatureRollingUpdateOnReady)+"=true"))
-
+func TestRollingUpdateOnReady(t *testing.T) {
 	type expected struct {
 		inactiveSpecReplicas int32
 		replicas             int32
@@ -1118,11 +1109,6 @@ func TestFeatureRollingUpdateOnReady(t *testing.T) {
 func TestControllerRollingUpdateDeployment(t *testing.T) {
 	t.Parallel()
 
-	utilruntime.FeatureTestMutex.Lock()
-	defer utilruntime.FeatureTestMutex.Unlock()
-
-	assert.NoError(t, utilruntime.ParseFeatures(string(utilruntime.FeatureRollingUpdateOnReady)+"=false"))
-
 	type expected struct {
 		inactiveSpecReplicas int32
 		replicas             int32
@@ -1197,23 +1183,11 @@ func TestControllerRollingUpdateDeployment(t *testing.T) {
 			inactiveSpecReplicas:   95,
 			inactiveStatusReplicas: 95,
 			expected: expected{
-				inactiveSpecReplicas: 65,
+				inactiveSpecReplicas: 45,
 				replicas:             30,
 				updated:              true,
 			},
 		},
-		"statuses don't match the spec. nothing should happen": {
-			fleetSpecReplicas:      100,
-			activeSpecReplicas:     75,
-			activeStatusReplicas:   70,
-			inactiveSpecReplicas:   15,
-			inactiveStatusReplicas: 10,
-			expected: expected{
-				inactiveSpecReplicas: 15,
-				replicas:             75,
-				updated:              false,
-			},
-		},
 		"test smalled numbers of active and allocated": {
 			fleetSpecReplicas:                5,
 			activeSpecReplicas:               0,
@@ -1223,7 +1197,7 @@ func TestControllerRollingUpdateDeployment(t *testing.T) {
 			inactiveStatusAllocationReplicas: 2,
 
 			expected: expected{
-				inactiveSpecReplicas: 3,
+				inactiveSpecReplicas: 4,
 				replicas:             2,
 				updated:              true,
 			},

pkg/util/runtime/features.go

@@ -37,10 +37,6 @@ const (
 	// FeatureSDKWatchSendOnExecute is a feature flag to enable/disable immediate game server return after SDK.WatchGameServer is called
 	FeatureSDKWatchSendOnExecute Feature = "SDKWatchSendOnExecute"
 
-	// FeatureRollingUpdateOnReady is a feature flag to enable/disable rolling update fix of scale down, when ReadyReplicas
-	// count is taken into account
-	FeatureRollingUpdateOnReady Feature = "RollingUpdateOnReady"
-
 	// NodeExternalDNS is a feature flag to enable/disable node ExternalDNS and InternalDNS use as GameServer address
 	NodeExternalDNS Feature = "NodeExternalDNS"
 
@@ -66,7 +62,6 @@ var (
 		FeatureExample:                true,
 		FeaturePlayerTracking:         false,
 		FeatureSDKWatchSendOnExecute:  true,
-		FeatureRollingUpdateOnReady:   true,
 		NodeExternalDNS:               false,
 		FeatureStateAllocationFilter:  false,
 		FeaturePlayerAllocationFilter: false,

site/content/en/docs/Guides/feature-stages.md

@@ -42,7 +42,6 @@ The current set of `alpha` and `beta` feature gates are:
 | Example Gate (not in use) | `Example` | Disabled | None | 0.13.0 |
 | [Player Tracking]({{< ref "/docs/Guides/player-tracking.md" >}}) | `PlayerTracking` | Disabled | `Alpha` | 1.6.0 |
 | [SDK Send GameServer on Watch execution]({{< ref "/docs/Guides/Client SDKs/_index.md#watchgameserverfunctiongameserver" >}}) | `SDKWatchSendOnExecute` | Enabled | `Beta` | 1.12.0 |
-| Fix for RollingUpdate [Scale down](https://github.com/googleforgames/agones/issues/1625) | `RollingUpdateOnReady` | Enabled | `Beta` | 1.14.0 |
 | [Utilize Node ExternalDNS](https://github.com/googleforgames/agones/issues/1921) and additional [details]({{< ref "/docs/FAQ/_index.md" >}}) | `NodeExternalDNS` | Disabled | `Alpha` | 1.12.0 |
 | [Custom resync period for FleetAutoscaler](https://github.com/googleforgames/agones/issues/1955) | `CustomFasSyncInterval` | Disabled | `Alpha` | 1.17.0 |
 | [GameServer state filtering on GameServerAllocations](https://github.com/googleforgames/agones/issues/1239) | `StateAllocationFilter` | Disabled | `Alpha` | 1.14.0 |

test/e2e/fleet_test.go

@@ -297,20 +297,14 @@ func TestFleetRollingUpdate(t *testing.T) {
 					maxSurge, err := intstr.GetValueFromIntOrPercent(flt.Spec.Strategy.RollingUpdate.MaxSurge, int(flt.Spec.Replicas), true)
 					assert.Nil(t, err)
 
-					roundUp := true
-					if runtime.FeatureEnabled(runtime.FeatureRollingUpdateOnReady) {
-						roundUp = false
-					}
+					roundUp := false
 					maxUnavailable, err := intstr.GetValueFromIntOrPercent(flt.Spec.Strategy.RollingUpdate.MaxUnavailable, int(flt.Spec.Replicas), roundUp)
 
-					shift := 0
-					if runtime.FeatureEnabled(runtime.FeatureRollingUpdateOnReady) {
-						if maxUnavailable == 0 {
-							maxUnavailable = 1
-						}
-						// This difference is inevitable, also could be seen with Deployments and ReplicaSets
-						shift = maxUnavailable
+					if maxUnavailable == 0 {
+						maxUnavailable = 1
 					}
+					// This difference is inevitable, also could be seen with Deployments and ReplicaSets
+					shift := maxUnavailable
 					assert.Nil(t, err)
 
 					expectedTotal := targetScale + maxSurge + maxUnavailable + shift
@@ -349,9 +343,6 @@ func TestFleetRollingUpdate(t *testing.T) {
 }
 
 func TestUpdateFleetReplicaAndSpec(t *testing.T) {
-	if !runtime.FeatureEnabled(runtime.FeatureRollingUpdateOnReady) {
-		t.SkipNow()
-	}
 	t.Parallel()
 
 	client := framework.AgonesClient.AgonesV1()