Skip to content

Commit e4060cc

Browse files
Wouter Verlaekmarkmandel
andauthored
Fix Rolling Update with Allocated GameServers (agones-dev#2420)
* e2e test to reproduce issue#2397 Fix for bug! Co-authored-by: Mark Mandel <markmandel@google.com>
1 parent ed17358 commit e4060cc

3 files changed

Lines changed: 188 additions & 97 deletions

File tree

‎pkg/fleets/controller.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -529,7 +529,7 @@ func (c *Controller) rollingUpdateRestFixedOnReady(ctx context.Context, fleet *a
529529

530530
// Check if we are ready to scale down
531531
allPodsCount := agonesv1.SumSpecReplicas(allGSS)
532-
newGSSUnavailablePodCount := active.Spec.Replicas - active.Status.ReadyReplicas
532+
newGSSUnavailablePodCount := active.Spec.Replicas - active.Status.ReadyReplicas - active.Status.AllocatedReplicas
533533
maxScaledDown := allPodsCount - minAvailable - newGSSUnavailablePodCount
534534

535535
if maxScaledDown <= 0 {

‎test/e2e/fleet_test.go‎

Lines changed: 160 additions & 96 deletions
Original file line numberDiff line numberDiff line change
@@ -222,123 +222,187 @@ func TestFleetScaleUpEditAndScaleDown(t *testing.T) {
222222
func TestFleetRollingUpdate(t *testing.T) {
223223
t.Parallel()
224224
ctx := context.Background()
225-
// Use scaleFleetPatch (true) or scaleFleetSubresource (false)
226-
fixtures := []bool{true, false}
227-
maxSurge := []string{"25%", "10%"}
225+
fixtures := []struct {
226+
// Use scaleFleetPatch (true) or scaleFleetSubresource (false)
227+
usePatch bool
228+
maxSurge string
229+
// If true, create and cycle GS Allocations when triggering a rolling update:
230+
// - Repeatedly allocate and shutdown GameServers (keeping ~50% of the Fleet in an Allocated state).
231+
// - Once 50% of the Fleet is Allocated, trigger the rolling update.
232+
// - Keep allocating/shutting down GameServers, to allocate in both the old and new GSSets.
233+
// - Verify the rolling update completes.
234+
// This simulates updating a Fleet that is live/in-use, and reproduces an issue where allocated GameServers
235+
// causes a rolling update to get stuck and keep the old GameServerSet up.
236+
cycleAllocations bool
237+
}{
238+
{
239+
usePatch: true,
240+
maxSurge: "25%",
241+
cycleAllocations: false,
242+
},
243+
{
244+
usePatch: true,
245+
maxSurge: "10%",
246+
cycleAllocations: false,
247+
},
248+
{
249+
usePatch: false,
250+
maxSurge: "25%",
251+
cycleAllocations: false,
252+
},
253+
{
254+
usePatch: false,
255+
maxSurge: "10%",
256+
cycleAllocations: false,
257+
},
258+
{
259+
usePatch: true,
260+
maxSurge: "25%",
261+
cycleAllocations: true,
262+
},
263+
}
264+
for i := range fixtures {
265+
fixture := fixtures[i]
266+
t.Run(fmt.Sprintf("Use fleet Patch %t %s cycle %t", fixture.usePatch, fixture.maxSurge, fixture.cycleAllocations), func(t *testing.T) {
267+
t.Parallel()
228268

229-
for _, usePatch := range fixtures {
230-
for _, maxSurgeParam := range maxSurge {
231-
usePatch := usePatch
232-
maxSurgeParam := maxSurgeParam
233-
t.Run(fmt.Sprintf("Use fleet Patch %t %s", usePatch, maxSurgeParam), func(t *testing.T) {
234-
t.Parallel()
235-
236-
client := framework.AgonesClient.AgonesV1()
237-
238-
flt := defaultFleet(framework.Namespace)
239-
flt.ApplyDefaults()
240-
flt.Spec.Replicas = 1
241-
rollingUpdatePercent := intstr.FromString(maxSurgeParam)
242-
flt.Spec.Strategy.RollingUpdate.MaxSurge = &rollingUpdatePercent
243-
flt.Spec.Strategy.RollingUpdate.MaxUnavailable = &rollingUpdatePercent
244-
245-
flt, err := client.Fleets(framework.Namespace).Create(ctx, flt, metav1.CreateOptions{})
246-
if assert.Nil(t, err) {
247-
defer client.Fleets(framework.Namespace).Delete(ctx, flt.ObjectMeta.Name, metav1.DeleteOptions{}) // nolint:errcheck
248-
}
269+
client := framework.AgonesClient.AgonesV1()
249270

250-
assert.Equal(t, int32(1), flt.Spec.Replicas)
251-
assert.Equal(t, maxSurgeParam, flt.Spec.Strategy.RollingUpdate.MaxSurge.StrVal)
252-
assert.Equal(t, maxSurgeParam, flt.Spec.Strategy.RollingUpdate.MaxUnavailable.StrVal)
271+
flt := defaultFleet(framework.Namespace)
272+
flt.ApplyDefaults()
273+
flt.Spec.Replicas = 1
274+
rollingUpdatePercent := intstr.FromString(fixture.maxSurge)
275+
flt.Spec.Strategy.RollingUpdate.MaxSurge = &rollingUpdatePercent
276+
flt.Spec.Strategy.RollingUpdate.MaxUnavailable = &rollingUpdatePercent
253277

254-
framework.AssertFleetCondition(t, flt, e2e.FleetReadyCount(flt.Spec.Replicas))
278+
flt, err := client.Fleets(framework.Namespace).Create(ctx, flt, metav1.CreateOptions{})
279+
require.NoError(t, err)
280+
defer client.Fleets(framework.Namespace).Delete(ctx, flt.ObjectMeta.Name, metav1.DeleteOptions{}) // nolint:errcheck
255281

256-
// scale up
257-
const targetScale = 8
258-
if usePatch {
259-
flt = scaleFleetPatch(ctx, t, flt, targetScale)
260-
assert.Equal(t, int32(targetScale), flt.Spec.Replicas)
261-
} else {
262-
flt = scaleFleetSubresource(ctx, t, flt, targetScale)
263-
}
282+
assert.Equal(t, int32(1), flt.Spec.Replicas)
283+
assert.Equal(t, fixture.maxSurge, flt.Spec.Strategy.RollingUpdate.MaxSurge.StrVal)
284+
assert.Equal(t, fixture.maxSurge, flt.Spec.Strategy.RollingUpdate.MaxUnavailable.StrVal)
264285

265-
framework.AssertFleetCondition(t, flt, e2e.FleetReadyCount(targetScale))
286+
framework.AssertFleetCondition(t, flt, e2e.FleetReadyCount(flt.Spec.Replicas))
266287

267-
flt, err = client.Fleets(framework.Namespace).Get(ctx, flt.ObjectMeta.GetName(), metav1.GetOptions{})
268-
assert.NoError(t, err)
288+
// scale up
289+
const targetScale = 8
290+
if fixture.usePatch {
291+
flt = scaleFleetPatch(ctx, t, flt, targetScale)
292+
assert.Equal(t, int32(targetScale), flt.Spec.Replicas)
293+
} else {
294+
flt = scaleFleetSubresource(ctx, t, flt, targetScale)
295+
}
296+
297+
framework.AssertFleetCondition(t, flt, e2e.FleetReadyCount(targetScale))
298+
299+
flt, err = client.Fleets(framework.Namespace).Get(ctx, flt.ObjectMeta.GetName(), metav1.GetOptions{})
300+
require.NoError(t, err)
301+
302+
cycleCtx, cancelCycle := context.WithCancel(ctx)
303+
defer cancelCycle()
304+
if fixture.cycleAllocations {
305+
// Repeatedly cycle allocations to keep ~half of the GameServers Allocated. Repeatedly Allocate and
306+
// delete such that both the old and new GSSet contain allocated GameServers.
307+
const halfScale = targetScale / 2
308+
const period = 3 * time.Second
309+
go framework.CycleAllocations(cycleCtx, t, flt, period, period*halfScale)
310+
311+
// Wait for at least half of the fleet to have be cycled (either Allocated or shutting down)
312+
// before updating the fleet.
313+
err = framework.WaitForFleetCondition(t, flt, func(entry *logrus.Entry, fleet *agonesv1.Fleet) bool {
314+
return fleet.Status.ReadyReplicas < halfScale
315+
})
316+
}
269317

270-
// Change ContainerPort to trigger creating a new GSSet
318+
// Change ContainerPort to trigger creating a new GSSet. Retry in case of a conflict.
319+
fltName := flt.GetName()
320+
require.Eventually(t, func() bool {
321+
flt, err = client.Fleets(framework.Namespace).Get(ctx, fltName, metav1.GetOptions{})
322+
require.NoError(t, err)
271323
fltCopy := flt.DeepCopy()
272324
fltCopy.Spec.Template.Spec.Ports[0].ContainerPort++
273325
flt, err = client.Fleets(framework.Namespace).Update(ctx, fltCopy, metav1.UpdateOptions{})
274-
assert.NoError(t, err)
326+
return err == nil
327+
}, time.Minute, time.Second)
275328

276-
selector := labels.SelectorFromSet(labels.Set{agonesv1.FleetNameLabel: flt.ObjectMeta.Name})
277-
// New GSS was created
278-
err = wait.PollImmediate(1*time.Second, 30*time.Second, func() (bool, error) {
279-
gssList, err := framework.AgonesClient.AgonesV1().GameServerSets(framework.Namespace).List(ctx,
280-
metav1.ListOptions{LabelSelector: selector.String()})
281-
if err != nil {
282-
return false, err
283-
}
284-
return len(gssList.Items) == 2, nil
285-
})
286-
assert.NoError(t, err)
287-
// Check that total number of gameservers in the system does not exceed the RollingUpdate
288-
// parameters (creating no more than maxSurge, deleting maxUnavailable servers at a time)
289-
// Wait for old GSSet to be deleted
290-
err = wait.PollImmediate(1*time.Second, 5*time.Minute, func() (bool, error) {
291-
list, err := framework.AgonesClient.AgonesV1().GameServers(framework.Namespace).List(ctx,
292-
metav1.ListOptions{LabelSelector: selector.String()})
293-
if err != nil {
294-
return false, err
295-
}
329+
selector := labels.SelectorFromSet(labels.Set{agonesv1.FleetNameLabel: flt.ObjectMeta.Name})
330+
// New GSS was created
331+
err = wait.PollImmediate(1*time.Second, 30*time.Second, func() (bool, error) {
332+
gssList, err := framework.AgonesClient.AgonesV1().GameServerSets(framework.Namespace).List(ctx,
333+
metav1.ListOptions{LabelSelector: selector.String()})
334+
if err != nil {
335+
return false, err
336+
}
337+
return len(gssList.Items) == 2, nil
338+
})
339+
assert.NoError(t, err)
340+
// Check that total number of gameservers in the system does not exceed the RollingUpdate
341+
// parameters (creating no more than maxSurge, deleting maxUnavailable servers at a time)
342+
// Wait for old GSSet to be deleted
343+
err = wait.PollImmediate(1*time.Second, 5*time.Minute, func() (bool, error) {
344+
list, err := framework.AgonesClient.AgonesV1().GameServers(framework.Namespace).List(ctx,
345+
metav1.ListOptions{LabelSelector: selector.String()})
346+
if err != nil {
347+
return false, err
348+
}
296349

297-
maxSurge, err := intstr.GetValueFromIntOrPercent(flt.Spec.Strategy.RollingUpdate.MaxSurge, int(flt.Spec.Replicas), true)
298-
assert.Nil(t, err)
350+
maxSurge, err := intstr.GetValueFromIntOrPercent(flt.Spec.Strategy.RollingUpdate.MaxSurge, int(flt.Spec.Replicas), true)
351+
assert.Nil(t, err)
299352

300-
roundUp := false
301-
maxUnavailable, err := intstr.GetValueFromIntOrPercent(flt.Spec.Strategy.RollingUpdate.MaxUnavailable, int(flt.Spec.Replicas), roundUp)
353+
roundUp := false
354+
maxUnavailable, err := intstr.GetValueFromIntOrPercent(flt.Spec.Strategy.RollingUpdate.MaxUnavailable, int(flt.Spec.Replicas), roundUp)
302355

303-
if maxUnavailable == 0 {
304-
maxUnavailable = 1
305-
}
306-
// This difference is inevitable, also could be seen with Deployments and ReplicaSets
307-
shift := maxUnavailable
308-
assert.Nil(t, err)
356+
if maxUnavailable == 0 {
357+
maxUnavailable = 1
358+
}
359+
// This difference is inevitable, also could be seen with Deployments and ReplicaSets
360+
shift := maxUnavailable
361+
assert.Nil(t, err)
309362

310-
expectedTotal := targetScale + maxSurge + maxUnavailable + shift
311-
if len(list.Items) > expectedTotal {
312-
err = fmt.Errorf("new replicas should be less than target + maxSurge + maxUnavailable + shift. Replicas: %d, Expected: %d", len(list.Items), expectedTotal)
313-
}
314-
if err != nil {
315-
return false, err
316-
}
317-
gssList, err := framework.AgonesClient.AgonesV1().GameServerSets(framework.Namespace).List(ctx,
318-
metav1.ListOptions{LabelSelector: selector.String()})
319-
if err != nil {
320-
return false, err
363+
// Ignore any GameServers that are shutting down (resulting from Allocation cycling).
364+
shuttingDown := 0
365+
for _, gs := range list.Items {
366+
if gs.IsBeingDeleted() {
367+
shuttingDown++
321368
}
322-
return len(gssList.Items) == 1, nil
323-
})
369+
}
370+
expectedTotal := targetScale + maxSurge + maxUnavailable + shift + shuttingDown
371+
if len(list.Items) > expectedTotal {
372+
err = fmt.Errorf("new replicas should be less than target + maxSurge + maxUnavailable + shift + shuttingDown. Replicas: %d, Expected: %d", len(list.Items), expectedTotal)
373+
}
374+
if err != nil {
375+
return false, err
376+
}
377+
gssList, err := framework.AgonesClient.AgonesV1().GameServerSets(framework.Namespace).List(ctx,
378+
metav1.ListOptions{LabelSelector: selector.String()})
379+
if err != nil {
380+
return false, err
381+
}
382+
return len(gssList.Items) == 1, nil
383+
})
324384

325-
assert.NoError(t, err)
385+
assert.NoError(t, err)
326386

327-
// scale down, with allocation
328-
const scaleDownTarget = 1
329-
if usePatch {
330-
flt = scaleFleetPatch(ctx, t, flt, scaleDownTarget)
331-
} else {
332-
flt = scaleFleetSubresource(ctx, t, flt, scaleDownTarget)
333-
}
387+
// Stop cycling Allocations.
388+
// The AssertFleetConditions below will wait until the Allocation cycling has
389+
// fully stopped (when all Allocated GameServers are shut down).
390+
cancelCycle()
391+
392+
// scale down, with allocation
393+
const scaleDownTarget = 1
394+
if fixture.usePatch {
395+
flt = scaleFleetPatch(ctx, t, flt, scaleDownTarget)
396+
} else {
397+
flt = scaleFleetSubresource(ctx, t, flt, scaleDownTarget)
398+
}
334399

335-
framework.AssertFleetCondition(t, flt, e2e.FleetReadyCount(1))
400+
framework.AssertFleetCondition(t, flt, e2e.FleetReadyCount(1))
336401

337-
framework.AssertFleetCondition(t, flt, func(log *logrus.Entry, fleet *agonesv1.Fleet) bool {
338-
return fleet.Status.AllocatedReplicas == 0
339-
})
402+
framework.AssertFleetCondition(t, flt, func(log *logrus.Entry, fleet *agonesv1.Fleet) bool {
403+
return fleet.Status.AllocatedReplicas == 0
340404
})
341-
}
405+
})
342406
}
343407
}
344408

‎test/e2e/framework/framework.go‎

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -278,6 +278,33 @@ func (f *Framework) WaitForGameServerState(t *testing.T, gs *agonesv1.GameServer
278278
state, gs.Namespace, gs.Name)
279279
}
280280

281+
// CycleAllocations repeatedly Allocates a GameServer in the Fleet (if one is available), once every specified period.
282+
// Each Allocated GameServer gets deleted allocDuration after it was Allocated.
283+
// GameServers will continue to be Allocated until a message is passed to the done channel.
284+
func (f *Framework) CycleAllocations(ctx context.Context, t *testing.T, flt *agonesv1.Fleet, period time.Duration, allocDuration time.Duration) {
285+
err := wait.PollImmediateUntil(period, func() (bool, error) {
286+
gsa := GetAllocation(flt)
287+
gsa, err := f.AgonesClient.AllocationV1().GameServerAllocations(flt.Namespace).Create(context.Background(), gsa, metav1.CreateOptions{})
288+
if err != nil || gsa.Status.State != allocationv1.GameServerAllocationAllocated {
289+
// Ignore error. Could be that the buffer was empty, will try again next cycle.
290+
return false, nil
291+
}
292+
293+
// Deallocate after allocDuration.
294+
go func(gsa *allocationv1.GameServerAllocation) {
295+
time.Sleep(allocDuration)
296+
err := f.AgonesClient.AgonesV1().GameServers(gsa.Namespace).Delete(context.Background(), gsa.Status.GameServerName, metav1.DeleteOptions{})
297+
require.NoError(t, err)
298+
}(gsa)
299+
300+
return false, nil
301+
}, ctx.Done())
302+
// Ignore wait timeout error, will always be returned when the context is cancelled at the end of the test.
303+
if err != wait.ErrWaitTimeout {
304+
require.NoError(t, err)
305+
}
306+
}
307+
281308
// AssertFleetCondition waits for the Fleet to be in a specific condition or fails the test if the condition can't be met in 5 minutes.
282309
func (f *Framework) AssertFleetCondition(t *testing.T, flt *agonesv1.Fleet, condition func(*logrus.Entry, *agonesv1.Fleet) bool) {
283310
err := f.WaitForFleetCondition(t, flt, condition)

0 commit comments

Comments
 (0)