From f5558c861fa6484cf8fa61ea29522aeb5246db8b Mon Sep 17 00:00:00 2001 From: Jiaxin Shan Date: Sun, 19 Sep 2021 16:43:56 -0700 Subject: [PATCH] Move RollingUpdateOnReady to stable --- cloudbuild.yaml | 2 +- pkg/fleets/controller.go | 52 +------------------ pkg/fleets/controller_test.go | 16 +----- pkg/util/runtime/features.go | 5 -- site/content/en/docs/Guides/feature-stages.md | 1 - site/content/en/docs/Guides/fleet-updates.md | 1 + test/e2e/fleet_test.go | 18 ++----- 7 files changed, 9 insertions(+), 86 deletions(-) diff --git a/cloudbuild.yaml b/cloudbuild.yaml index a1f1e07528..709b2e99e3 100644 --- a/cloudbuild.yaml +++ b/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 diff --git a/pkg/fleets/controller.go b/pkg/fleets/controller.go index 12cecce3fd..3b72c87a54 100644 --- a/pkg/fleets/controller.go +++ b/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 diff --git a/pkg/fleets/controller_test.go b/pkg/fleets/controller_test.go index 9450729bbb..72a2eeeb47 100644 --- a/pkg/fleets/controller_test.go +++ b/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 diff --git a/pkg/util/runtime/features.go b/pkg/util/runtime/features.go index ef8ada812a..3f7d6542b3 100644 --- a/pkg/util/runtime/features.go +++ b/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" @@ -63,7 +59,6 @@ var ( FeatureExample: true, FeaturePlayerTracking: false, FeatureSDKWatchSendOnExecute: true, - FeatureRollingUpdateOnReady: true, NodeExternalDNS: false, FeatureStateAllocationFilter: false, FeaturePlayerAllocationFilter: false, diff --git a/site/content/en/docs/Guides/feature-stages.md b/site/content/en/docs/Guides/feature-stages.md index a2820407c5..26d5e5aaad 100644 --- a/site/content/en/docs/Guides/feature-stages.md +++ b/site/content/en/docs/Guides/feature-stages.md @@ -29,7 +29,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 | diff --git a/site/content/en/docs/Guides/fleet-updates.md b/site/content/en/docs/Guides/fleet-updates.md index e593dfe095..5b42c45cfc 100644 --- a/site/content/en/docs/Guides/fleet-updates.md +++ b/site/content/en/docs/Guides/fleet-updates.md @@ -35,6 +35,7 @@ So when a Fleet is edited (any field other than `replicas`, see note below), eit By default, a Fleet will wait for new `GameSevers` to become `Ready` during a Rolling Update before continuing to shutdown additional `GameServers`, only counting `GameServers` that are `Ready` as being available when calculating the current `maxUnavailable` value which controls the rate at which `GameServers` are updated. This ensures that a Fleet cannot accidentally have zero `GameServers` in the `Ready` state if something goes wrong during a Rolling Update or if `GameServers` have a long delay before moving to the `Ready` state. +update? {{< beta title="Rolling Update on Ready" gate="RollingUpdateOnReady" >}} {{< alert title="Note" color="info">}} diff --git a/test/e2e/fleet_test.go b/test/e2e/fleet_test.go index bb70a5c489..28296d46b0 100644 --- a/test/e2e/fleet_test.go +++ b/test/e2e/fleet_test.go @@ -297,20 +297,15 @@ 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 +344,6 @@ func TestFleetRollingUpdate(t *testing.T) { } func TestUpdateFleetReplicaAndSpec(t *testing.T) { - if !runtime.FeatureEnabled(runtime.FeatureRollingUpdateOnReady) { - t.SkipNow() - } t.Parallel() client := framework.AgonesClient.AgonesV1()