From 0abc1bec122a5f35123fab6065fae15b534130c7 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 Semantic.DeepEqual() to compare equal values autoupdated after creation, for instance 1000m -> 1 CPU limit. More accurate comparision of GSSet vs Fleet is used. --- pkg/apis/agones/v1/gameserverset.go | 5 +-- pkg/fleets/controller.go | 4 +- pkg/fleets/controller_test.go | 67 ++++++++++++++++++++++++++++- test/e2e/fleet_test.go | 28 ++++++++++++ 4 files changed, 98 insertions(+), 6 deletions(-) diff --git a/pkg/apis/agones/v1/gameserverset.go b/pkg/apis/agones/v1/gameserverset.go index 26e2536b94..2325aeb7c8 100644 --- a/pkg/apis/agones/v1/gameserverset.go +++ b/pkg/apis/agones/v1/gameserverset.go @@ -15,10 +15,9 @@ package v1 import ( - "reflect" - "agones.dev/agones/pkg/apis" "agones.dev/agones/pkg/apis/agones" + apiequality "k8s.io/apimachinery/pkg/api/equality" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" ) @@ -82,7 +81,7 @@ type GameServerSetStatus struct { // is the new GameServerSet, being passed into the old GameServerSet func (gsSet *GameServerSet) ValidateUpdate(new *GameServerSet) ([]metav1.StatusCause, bool) { causes := validateName(new) - if !reflect.DeepEqual(gsSet.Spec.Template, new.Spec.Template) { + if !apiequality.Semantic.DeepEqual(gsSet.Spec.Template, new.Spec.Template) { causes = append(causes, metav1.StatusCause{ Type: metav1.CauseTypeFieldValueInvalid, Field: "template", diff --git a/pkg/fleets/controller.go b/pkg/fleets/controller.go index 3b04d6ddb1..813d3cf45c 100644 --- a/pkg/fleets/controller.go +++ b/pkg/fleets/controller.go @@ -17,7 +17,6 @@ package fleets import ( "encoding/json" "fmt" - "reflect" "time" "agones.dev/agones/pkg/apis/agones" @@ -40,6 +39,7 @@ import ( corev1 "k8s.io/api/core/v1" extclientset "k8s.io/apiextensions-apiserver/pkg/client/clientset/clientset" "k8s.io/apiextensions-apiserver/pkg/client/clientset/clientset/typed/apiextensions/v1beta1" + apiequality "k8s.io/apimachinery/pkg/api/equality" k8serrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/util/intstr" @@ -542,7 +542,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 apiequality.Semantic.DeepEqual(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..627ee0aebe 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" @@ -71,7 +73,7 @@ func TestControllerSyncFleet(t *testing.T) { agtesting.AssertEventContains(t, m.FakeRecorder.Events, "CreatingGameServerSet") }) - t.Run("gamserverset with the same number of replicas", func(t *testing.T) { + t.Run("gameserverset with the same number of replicas", func(t *testing.T) { t.Parallel() f := defaultFixture() f.Spec.Strategy.Type = appsv1.RecreateDeploymentStrategyType @@ -617,6 +619,36 @@ func TestControllerUpsertGameServerSet(t *testing.T) { }) } +func TestResourcesRequestsAndLimits(t *testing.T) { + t.Parallel() + + gsSpec := *defaultGSSpec() + c, _ := newFakeController() + f := defaultFixture() + f.Spec.Template = gsSpec + f.Spec.Template.Spec.Template.Spec.Containers[0].Resources.Limits[corev1.ResourceCPU] = resource.MustParse("1000m") + + // Semantically equal definition, 1 == 1000m CPU + gsSet1 := f.GameServerSet() + gsSet1.Spec.Template = gsSpec + gsSet1.Spec.Template.Spec.Template.Spec.Containers[0].Resources.Limits[corev1.ResourceCPU] = resource.MustParse("1") + + // Absolutely different GameServer Spec, 1.1 CPU + gsSet3 := f.GameServerSet() + gsSet3.Spec.Template = *gsSpec.DeepCopy() + gsSet3.Spec.Template.Spec.Template.Spec.Containers[0].Resources.Limits[corev1.ResourceCPU] = resource.MustParse("1.1") + active, rest := c.filterGameServerSetByActive(f, []*agonesv1.GameServerSet{gsSet1, gsSet3}) + assert.Equal(t, gsSet1, active) + assert.Equal(t, []*agonesv1.GameServerSet{gsSet3}, rest) + + gsSet2 := f.GameServerSet() + gsSet2.Spec.Template = *gsSpec.DeepCopy() + gsSet2.Spec.Template.Spec.Template.Spec.Containers[0].Resources.Limits[corev1.ResourceCPU] = resource.MustParse("1000m") + active, rest = c.filterGameServerSetByActive(f, []*agonesv1.GameServerSet{gsSet2, gsSet3}) + assert.Equal(t, gsSet2, active) + assert.Equal(t, []*agonesv1.GameServerSet{gsSet3}, rest) +} + func TestControllerDeleteEmptyGameServerSets(t *testing.T) { t.Parallel() @@ -805,3 +837,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 ce9c2ba611..8895d0aa7b 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()