Fix infinite creation of GameServerSets when 1000m CPU limit was used

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",

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)

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"),
+							},
+						},
+					}},
+				},
+			},
+		},
+	}
+}

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()