Applying comments

Fixing tests: E2E and unit test. Adding docs section.
googleforgames · Sep 16, 2020 · dae07dd · dae07dd
1 parent c8c67bf
commit dae07dd
Show file tree

Hide file tree

Showing 5 changed files with 28 additions and 15 deletions.
diff --git a/pkg/fleets/controller.go b/pkg/fleets/controller.go
@@ -531,12 +531,14 @@ func (c *Controller) rollingUpdateRestFixedOnReady(fleet *agonesv1.Fleet, active
 
 	if maxScaledDown <= 0 {
 		return nil
-
 	}
 	rest, _, err = c.cleanupUnhealthyReplicas(rest, fleet, maxScaledDown)
 	if err != nil {
 		c.loggerForFleet(fleet).WithField("fleet", fleet.ObjectMeta.Name).WithField("maxScaledDown", maxScaledDown).
 			Debug("Can not cleanup Unhealth Replicas")
+		// There could be the case when GameServerSet would be updated from another place, say Status or Spec would be updated
+		// We don't want to propagate such errors further
+		// And this set in sync with reconcileOldReplicaSets() Kubernetes code
 		return nil
 	}
 	// Resulting value is readyReplicasCount + unavailable - fleet.Spec.Replicas

diff --git a/pkg/fleets/controller_test.go b/pkg/fleets/controller_test.go
@@ -1118,13 +1118,7 @@ func TestControllerRollingUpdateDeployment(t *testing.T) {
 	utilruntime.FeatureTestMutex.Lock()
 	defer utilruntime.FeatureTestMutex.Unlock()
 
-	inactiveReplicas1 := int32(65)
-	inactiveReplicas2 := int32(3)
-
-	if utilruntime.FeatureEnabled(utilruntime.FeatureRollingUpdateOnReady) {
-		inactiveReplicas1 = 45
-		inactiveReplicas2 = 4
-	}
+	assert.NoError(t, utilruntime.ParseFeatures(string(utilruntime.FeatureRollingUpdateOnReady)+"=false"))
 
 	type expected struct {
 		inactiveSpecReplicas int32
@@ -1200,7 +1194,7 @@ func TestControllerRollingUpdateDeployment(t *testing.T) {
 			inactiveSpecReplicas:   95,
 			inactiveStatusReplicas: 95,
 			expected: expected{
-				inactiveSpecReplicas: inactiveReplicas1,
+				inactiveSpecReplicas: 65,
 				replicas:             30,
 				updated:              true,
 			},
@@ -1226,7 +1220,7 @@ func TestControllerRollingUpdateDeployment(t *testing.T) {
 			inactiveStatusAllocationReplicas: 2,
 
 			expected: expected{
-				inactiveSpecReplicas: inactiveReplicas2,
+				inactiveSpecReplicas: 3,
 				replicas:             2,
 				updated:              true,
 			},

diff --git a/site/content/en/docs/Guides/feature-stages.md b/site/content/en/docs/Guides/feature-stages.md
@@ -24,15 +24,25 @@ that can be found in the [Helm configuration]({{< ref "/docs/Installation/Instal
 
 The current set of `alpha` and `beta` feature gates are:
 
+{{% feature publishVersion="1.9.0" %}}
 | Feature Name | Gate    | Default | Stage | Since |
 |--------------|---------|---------|-------|-------|
 | Multicluster Allocation<sup>*</sup> | N/A | Enabled | `Beta` | 1.6.0 |
 | Example Gate (not in use) | `Example` | Disabled | None | 0.13.0 |
 | [Port Allocations to Multiple Containers]({{< ref "/docs/Reference/gameserver.md" >}}) | `ContainerPortAllocation` | Enabled | `Beta` | 1.7.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` | Disabled | `Alpha` | 1.7.0 |
-| [Fix RollingUpdate Scaling Down ](https://github.com/googleforgames/agones/issues/1625) | `RollingUpdateOnReady` | Disabled | `Alpha` | 1.9.0 |
-
+| Fix for RollingUpdate [Scale down](https://github.com/googleforgames/agones/issues/1625) and additional [details]({{< ref "/docs/Guides/fleet-updates.md#alpha-feature-rollingupdateonready" >}}) | `RollingUpdateOnReady` | Disabled | `Alpha` | 1.9.0 |
+{{% /feature %}}
+{{% feature expiryVersion="1.9.0" %}}
+| Feature Name | Gate    | Default | Stage | Since |
+|--------------|---------|---------|-------|-------|
+| Multicluster Allocation<sup>*</sup> | N/A | Enabled | `Beta` | 1.6.0 |
+| Example Gate (not in use) | `Example` | Disabled | None | 0.13.0 |
+| [Port Allocations to Multiple Containers]({{< ref "/docs/Reference/gameserver.md" >}}) | `ContainerPortAllocation` | Enabled | `Beta` | 1.7.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` | Disabled | `Alpha` | 1.7.0 |
+{{% /feature %}}
 <sup>*</sup>Multicluster Allocation was started before this process was in place, and therefore does not have a
  feature gate and cannot be disabled.
 

diff --git a/site/content/en/docs/Guides/fleet-updates.md b/site/content/en/docs/Guides/fleet-updates.md
@@ -105,4 +105,11 @@ Fleets through that mechanism. The `preferred` label matching selector tells the
 all `GameServers` with the `v2` `Fleet` label, and if not found, search through the rest of the set.
 
 The above `GameServerAllocation` can then be used while you scale up the `v2` Fleet and scale down the original Fleet at
-the rate that you deem fit for your specific rollout. 
+the rate that you deem fit for your specific rollout.
+
+## Alpha Feature RollingUpdateOnReady
+
+{{< alpha title="Rolling Update on Ready" gate="RollingUpdateOnReady" >}}
+
+If we are updating the Fleet configuration, the new GameServerSet would be created with 0 GameServers at the beginning, if RollingUpdate deployment strategy is used. After creating a first batch of `MaxSurge` GameServers, old GameServerSet should be waiting before some of them become Ready, before scaling down GameServers which belong to an old GameServerSet.
+With this feature disabled old GameServerSet could scale down all of its GameServers down to zero and total number of Ready GameServers could be 0 which is more than `MaxUnavailable`, which could not be more than 99%, so this should never happen.
diff --git a/test/e2e/fleet_test.go b/test/e2e/fleet_test.go
@@ -27,7 +27,6 @@ import (
 	typedagonesv1 "agones.dev/agones/pkg/client/clientset/versioned/typed/agones/v1"
 	"agones.dev/agones/pkg/util/runtime"
 	e2e "agones.dev/agones/test/e2e/framework"
-	"github.com/pkg/errors"
 	"github.com/sirupsen/logrus"
 	"github.com/stretchr/testify/assert"
 	appsv1 "k8s.io/api/apps/v1"
@@ -301,12 +300,13 @@ func TestFleetRollingUpdate(t *testing.T) {
 						if maxUnavailable == 0 {
 							maxUnavailable = 1
 						}
+						// This difference is inevitable, also could be seen with Deployments and ReplicaSeets
 						shift = maxUnavailable
 					}
 					assert.Nil(t, err)
 
 					if len(list.Items) > targetScale+maxSurge+maxUnavailable+shift {
-						err = errors.New(fmt.Sprintf("New replicas should be less than target + maxSurge + maxUnavailable %d %d", len(list.Items), targetScale+maxSurge+maxUnavailable+shift))
+						err = fmt.Errorf("New replicas should be less than target + maxSurge + maxUnavailable %d %d", len(list.Items), targetScale+maxSurge+maxUnavailable+shift)
 					}
 					if err != nil {
 						return false, err