From 963a33c3b0b57218d557885fb7ad2083f828ad56 Mon Sep 17 00:00:00 2001 From: Alexander Apalikov Date: Mon, 18 Nov 2019 16:18:43 +0300 Subject: [PATCH] Fix infinite creation of GSSets if 1000m cpu used Add proper comparison when some changes were applied by Kubernetes - 1000m -> 1 CPU limit. Thus more accurate comparision of GSSet vs Fleet should be used. --- pkg/fleets/controller.go | 38 ++++++++++++++++++++++++- pkg/fleets/controller_test.go | 53 +++++++++++++++++++++++++++++++++++ test/e2e/fleet_test.go | 28 ++++++++++++++++++ 3 files changed, 118 insertions(+), 1 deletion(-) diff --git a/pkg/fleets/controller.go b/pkg/fleets/controller.go index 3b04d6ddb1..766ee6b1eb 100644 --- a/pkg/fleets/controller.go +++ b/pkg/fleets/controller.go @@ -534,6 +534,42 @@ func (c *Controller) updateFleetStatus(fleet *agonesv1.Fleet) error { return errors.Wrapf(err, "error updating status of fleet %s", fCopy.ObjectMeta.Name) } +// equalGameServerTemplates compares template with different Requests and Limits scales +func equalGameServerTemplates(a, b agonesv1.GameServerTemplateSpec) bool { + if reflect.DeepEqual(a, b) { + return true + } + aContainers := a.Spec.Template.Spec.Containers + bContainers := b.Spec.Template.Spec.Containers + if len(aContainers) != len(bContainers) { + return false + } + + // Resource Types + resourceTypes := []corev1.ResourceName{corev1.ResourceCPU, corev1.ResourceMemory, corev1.ResourceEphemeralStorage, corev1.ResourceStorage} + for i, aContainer := range aContainers { + for _, res := range resourceTypes { + resourceLimit := aContainer.Resources.Limits[res] + if resourceLimit.Cmp(bContainers[i].Resources.Limits[res]) != 0 { + return false + } + resourceRequest := aContainer.Resources.Requests[res] + if resourceRequest.Cmp(bContainers[i].Resources.Requests[res]) != 0 { + return false + } + + // Change equal values with different scale to be equal for a future DeepEqual() call + if v, ok := aContainer.Resources.Limits[res]; ok { + bContainers[i].Resources.Limits[res] = v + } + if v, ok := aContainer.Resources.Requests[res]; ok { + bContainers[i].Resources.Requests[res] = v + } + } + } + return reflect.DeepEqual(a, b) +} + // filterGameServerSetByActive returns the active GameServerSet (or nil if it // doesn't exist) and then the rest of the GameServerSets that are controlled // by this Fleet @@ -542,7 +578,7 @@ func (c *Controller) filterGameServerSetByActive(fleet *agonesv1.Fleet, list []* var rest []*agonesv1.GameServerSet for _, gsSet := range list { - if reflect.DeepEqual(gsSet.Spec.Template, fleet.Spec.Template) { + if equalGameServerTemplates(gsSet.Spec.Template, fleet.Spec.Template) { active = gsSet } else { rest = append(rest, gsSet) diff --git a/pkg/fleets/controller_test.go b/pkg/fleets/controller_test.go index 93801f9e9f..2005190f2b 100644 --- a/pkg/fleets/controller_test.go +++ b/pkg/fleets/controller_test.go @@ -31,6 +31,8 @@ import ( "github.com/stretchr/testify/assert" admv1beta1 "k8s.io/api/admission/v1beta1" appsv1 "k8s.io/api/apps/v1" + corev1 "k8s.io/api/core/v1" + "k8s.io/apimachinery/pkg/api/resource" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/util/intstr" @@ -39,6 +41,24 @@ import ( "k8s.io/client-go/tools/cache" ) +func TestControllerCompareFunc(t *testing.T) { + t.Parallel() + + a := *defaultGSSpec() + a.Spec.Template.Spec.Containers[0].Resources.Limits[corev1.ResourceCPU] = resource.MustParse("1000m") + b := *defaultGSSpec() + b.Spec.Template.Spec.Containers[0].Resources.Limits[corev1.ResourceCPU] = resource.MustParse("1") + + assert.True(t, equalGameServerTemplates(a, b), "Equal with different scales, but same value") + + a.Spec.Template.Spec.Containers[0].Resources.Requests[corev1.ResourceCPU] = resource.MustParse("1000m") + b.Spec.Template.Spec.Containers[0].Resources.Requests[corev1.ResourceCPU] = resource.MustParse("1") + assert.True(t, equalGameServerTemplates(a, b), "Equal with different scales, but same value") + + b.Spec.Template.Spec.Containers[0].Resources.Limits[corev1.ResourceCPU] = resource.MustParse("1.1") + assert.False(t, equalGameServerTemplates(a, b), "Not equal with different scales and different value") +} + func TestControllerSyncFleet(t *testing.T) { t.Parallel() @@ -805,3 +825,36 @@ func defaultFixture() *agonesv1.Fleet { f.ApplyDefaults() return f } + +func defaultGSSpec() *agonesv1.GameServerTemplateSpec { + return &agonesv1.GameServerTemplateSpec{ + Spec: agonesv1.GameServerSpec{ + Container: "udp-server", + Ports: []agonesv1.GameServerPort{{ + ContainerPort: 7654, + Name: "gameport", + PortPolicy: agonesv1.Dynamic, + Protocol: corev1.ProtocolUDP, + }}, + Template: corev1.PodTemplateSpec{ + Spec: corev1.PodSpec{ + Containers: []corev1.Container{{ + Name: "udp-server", + Image: "gcr.io/images/new:0.2", + ImagePullPolicy: corev1.PullIfNotPresent, + Resources: corev1.ResourceRequirements{ + Requests: corev1.ResourceList{ + corev1.ResourceCPU: resource.MustParse("30m"), + corev1.ResourceMemory: resource.MustParse("32Mi"), + }, + Limits: corev1.ResourceList{ + corev1.ResourceCPU: resource.MustParse("30m"), + corev1.ResourceMemory: resource.MustParse("32Mi"), + }, + }, + }}, + }, + }, + }, + } +} diff --git a/test/e2e/fleet_test.go b/test/e2e/fleet_test.go index b39f077bdc..703fb86454 100644 --- a/test/e2e/fleet_test.go +++ b/test/e2e/fleet_test.go @@ -33,6 +33,7 @@ import ( corev1 "k8s.io/api/core/v1" v1betaext "k8s.io/api/extensions/v1beta1" k8serrors "k8s.io/apimachinery/pkg/api/errors" + "k8s.io/apimachinery/pkg/api/resource" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/labels" "k8s.io/apimachinery/pkg/types" @@ -49,6 +50,33 @@ const ( replicasCount = 3 ) +// TestFleetRequestsLimits reproduces an issue when 1000m and 1 CPU is not equal, but should be equal +// Every fleet should create no more than 2 GameServerSet at once on a simple fleet patch +func TestFleetRequestsLimits(t *testing.T) { + t.Parallel() + + flt := defaultFleet(defaultNs) + flt.Spec.Template.Spec.Template.Spec.Containers[0].Resources.Limits[corev1.ResourceCPU] = *resource.NewScaledQuantity(1000, -3) + + client := framework.AgonesClient.AgonesV1() + flt, err := client.Fleets(defaultNs).Create(flt) + if assert.Nil(t, err) { + defer client.Fleets(defaultNs).Delete(flt.ObjectMeta.Name, nil) // nolint:errcheck + } + framework.AssertFleetCondition(t, flt, e2e.FleetReadyCount(flt.Spec.Replicas)) + + newReplicas := int32(5) + patch := fmt.Sprintf(`[{ "op": "replace", "path": "/spec/template/spec/template/spec/containers/0/resources/requests/cpu", "value": "1000m"}, + { "op": "replace", "path": "/spec/replicas", "value": %d}]`, newReplicas) + + _, err = framework.AgonesClient.AgonesV1().Fleets(defaultNs).Patch(flt.ObjectMeta.Name, types.JSONPatchType, []byte(patch)) + assert.Nil(t, err) + + // In bug scenario fleet was infinitely creating new GSSets (5 at a time), because 1000m CPU was changed to 1 CPU + // internally - thought as new wrong GSSet in a Fleet Controller + framework.AssertFleetCondition(t, flt, e2e.FleetReadyCount(newReplicas)) +} + func TestFleetScaleUpEditAndScaleDown(t *testing.T) { t.Parallel()