From b1723447bae1777871a8d4c26fb5b185091e7853 Mon Sep 17 00:00:00 2001 From: Mark Mandel Date: Tue, 13 Nov 2018 10:01:14 -0800 Subject: [PATCH] FleetAutoscaler can be targeted at Non Existent Fleets This removes some previous restrictions with the FleetAutoscaler, as well as cleaning up a couple of items. - FleetAutoscalers can now have a Fleet target that doesn't exit. - If the target doesn't exist, this will be recorded as a warning event of type `FailedGetFleet`, rather than `ScalingLimited` status item, as it allows a human readable message, as well as give us data on how often this occurs, and it is readable by stat/alerting packages. - FleetAutoscalers can now have their Fleet target edited. This means that FleetAutoscalers can be transitioned from one Fleet to another. This may be useful in scenarios like red-green deployments, or if you wish to temporarily disable an Autoscaler, but not delete it. - `FleetAutoscaler.Status.ScalingLimited` was also replaced by a `ScalingLimited` Event, as it allows a human readable message, as well as give us data on how often this occurs, and it is readable by stat/alerting packages. Closes #406 --- .../templates/admissionregistration.yaml | 1 - install/yaml/install.yaml | 1 - pkg/apis/stable/v1alpha1/fleetautoscaler.go | 26 +-- .../stable/v1alpha1/fleetautoscaler_test.go | 188 ++++-------------- pkg/fleetautoscalers/controller.go | 151 +++----------- pkg/fleetautoscalers/controller_test.go | 121 ++++++----- pkg/gameservers/localsdk.go | 1 - 7 files changed, 126 insertions(+), 363 deletions(-) diff --git a/install/helm/agones/templates/admissionregistration.yaml b/install/helm/agones/templates/admissionregistration.yaml index 5b2cfa12b7..e816862fdf 100644 --- a/install/helm/agones/templates/admissionregistration.yaml +++ b/install/helm/agones/templates/admissionregistration.yaml @@ -86,7 +86,6 @@ webhooks: - "gameservers" - "fleets" - "fleetallocations" - - "fleetautoscalers" apiVersions: - "v1alpha1" operations: diff --git a/install/yaml/install.yaml b/install/yaml/install.yaml index 22729c0f73..d0bbf4af60 100644 --- a/install/yaml/install.yaml +++ b/install/yaml/install.yaml @@ -925,7 +925,6 @@ webhooks: - "gameservers" - "fleets" - "fleetallocations" - - "fleetautoscalers" apiVersions: - "v1alpha1" operations: diff --git a/pkg/apis/stable/v1alpha1/fleetautoscaler.go b/pkg/apis/stable/v1alpha1/fleetautoscaler.go index 716db8c22b..b04d56f499 100644 --- a/pkg/apis/stable/v1alpha1/fleetautoscaler.go +++ b/pkg/apis/stable/v1alpha1/fleetautoscaler.go @@ -102,37 +102,17 @@ type FleetAutoscalerStatus struct { // lastScaleTime is the last time the FleetAutoscaler scaled the attached fleet, // +optional LastScaleTime *metav1.Time `json:"lastScaleTime"` - - // AbleToScale indicates that we can access the target fleet - AbleToScale bool `json:"ableToScale"` - - // ScalingLimited indicates that the calculated scale would be above or below the range - // defined by MinReplicas and MaxReplicas, and has thus been capped. - ScalingLimited bool `json:"scalingLimited"` -} - -// ValidateUpdate validates when an update occurs -func (fas *FleetAutoscaler) ValidateUpdate(new *FleetAutoscaler, causes []metav1.StatusCause) []metav1.StatusCause { - if fas.Spec.FleetName != new.Spec.FleetName { - causes = append(causes, metav1.StatusCause{ - Type: metav1.CauseTypeFieldValueInvalid, - Field: "fleetName", - Message: "fleetName cannot be updated", - }) - } - - return new.ValidateAutoScalingSettings(causes) } -//ValidateAutoScalingSettings validates the FleetAutoscaler scaling settings -func (fas *FleetAutoscaler) ValidateAutoScalingSettings(causes []metav1.StatusCause) []metav1.StatusCause { +// Validate validates the FleetAutoscaler scaling settings +func (fas *FleetAutoscaler) Validate(causes []metav1.StatusCause) []metav1.StatusCause { if fas.Spec.Policy.Type == BufferPolicyType { causes = fas.Spec.Policy.Buffer.ValidateAutoScalingBufferPolicy(causes) } return causes } -//ValidateAutoScalingSettings validates the FleetAutoscaler Buffer policy settings +// ValidateAutoScalingBufferPolicy validates the FleetAutoscaler Buffer policy settings func (b *BufferPolicy) ValidateAutoScalingBufferPolicy(causes []metav1.StatusCause) []metav1.StatusCause { if b == nil { return append(causes, metav1.StatusCause{ diff --git a/pkg/apis/stable/v1alpha1/fleetautoscaler_test.go b/pkg/apis/stable/v1alpha1/fleetautoscaler_test.go index 57d65bc68b..c6b4841bf1 100644 --- a/pkg/apis/stable/v1alpha1/fleetautoscaler_test.go +++ b/pkg/apis/stable/v1alpha1/fleetautoscaler_test.go @@ -25,70 +25,10 @@ import ( func TestFleetAutoscalerValidateUpdate(t *testing.T) { t.Parallel() - t.Run("same fleet name", func(t *testing.T) { - - fas := &FleetAutoscaler{ - ObjectMeta: metav1.ObjectMeta{Name: "test"}, - Spec: FleetAutoscalerSpec{ - FleetName: "testing", - Policy: FleetAutoscalerPolicy{ - Type: BufferPolicyType, - Buffer: &BufferPolicy{ - BufferSize: intstr.FromInt(1), - MaxReplicas: 10, - }, - }, - }, - } - - causes := fas.ValidateUpdate(fas.DeepCopy(), nil) - assert.Len(t, causes, 0) - }) - - t.Run("different fleet name", func(t *testing.T) { - - fas := &FleetAutoscaler{ - ObjectMeta: metav1.ObjectMeta{Name: "test"}, - Spec: FleetAutoscalerSpec{ - FleetName: "testing", - Policy: FleetAutoscalerPolicy{ - Type: BufferPolicyType, - Buffer: &BufferPolicy{ - BufferSize: intstr.FromInt(1), - MaxReplicas: 10, - }, - }, - }, - } - fasCopy := fas.DeepCopy() - fasCopy.Spec.FleetName = "notthesame" - - causes := fas.ValidateUpdate(fasCopy, nil) - - assert.Len(t, causes, 1) - assert.Equal(t, "fleetName", causes[0].Field) - }) - t.Run("bad buffer size", func(t *testing.T) { - - fas := &FleetAutoscaler{ - ObjectMeta: metav1.ObjectMeta{Name: "test"}, - Spec: FleetAutoscalerSpec{ - FleetName: "testing", - Policy: FleetAutoscalerPolicy{ - Type: BufferPolicyType, - Buffer: &BufferPolicy{ - BufferSize: intstr.FromInt(1), - MaxReplicas: 10, - }, - }, - }, - } - - fasCopy := fas.DeepCopy() - fasCopy.Spec.Policy.Buffer.BufferSize = intstr.FromInt(0) - - causes := fas.ValidateUpdate(fasCopy, nil) + fas := defaultFixture() + fas.Spec.Policy.Buffer.BufferSize = intstr.FromInt(0) + causes := fas.Validate(nil) assert.Len(t, causes, 1) assert.Equal(t, "bufferSize", causes[0].Field) @@ -96,138 +36,82 @@ func TestFleetAutoscalerValidateUpdate(t *testing.T) { t.Run("bad min replicas", func(t *testing.T) { - fas := &FleetAutoscaler{ - ObjectMeta: metav1.ObjectMeta{Name: "test"}, - Spec: FleetAutoscalerSpec{ - FleetName: "testing", - Policy: FleetAutoscalerPolicy{ - Type: BufferPolicyType, - Buffer: &BufferPolicy{ - BufferSize: intstr.FromInt(5), - MaxReplicas: 10, - }, - }, - }, - } - - fasCopy := fas.DeepCopy() - fasCopy.Spec.Policy.Buffer.MinReplicas = 2 + fas := defaultFixture() + fas.Spec.Policy.Buffer.MinReplicas = 2 - causes := fas.ValidateUpdate(fasCopy, nil) + causes := fas.Validate(nil) assert.Len(t, causes, 1) assert.Equal(t, "minReplicas", causes[0].Field) }) t.Run("bad max replicas", func(t *testing.T) { - - fas := &FleetAutoscaler{ - ObjectMeta: metav1.ObjectMeta{Name: "test"}, - Spec: FleetAutoscalerSpec{ - FleetName: "testing", - Policy: FleetAutoscalerPolicy{ - Type: BufferPolicyType, - Buffer: &BufferPolicy{ - BufferSize: intstr.FromInt(5), - MaxReplicas: 10, - }, - }, - }, - } - - fasCopy := fas.DeepCopy() - fasCopy.Spec.Policy.Buffer.MaxReplicas = 2 - causes := fas.ValidateUpdate(fasCopy, nil) + fas := defaultFixture() + fas.Spec.Policy.Buffer.MaxReplicas = 2 + causes := fas.Validate(nil) assert.Len(t, causes, 1) assert.Equal(t, "maxReplicas", causes[0].Field) }) t.Run("minReplicas > maxReplicas", func(t *testing.T) { - - fas := &FleetAutoscaler{ - ObjectMeta: metav1.ObjectMeta{Name: "test"}, - Spec: FleetAutoscalerSpec{ - FleetName: "testing", - Policy: FleetAutoscalerPolicy{ - Type: BufferPolicyType, - Buffer: &BufferPolicy{ - BufferSize: intstr.FromInt(5), - MaxReplicas: 10, - }, - }, - }, - } - - fasCopy := fas.DeepCopy() - fasCopy.Spec.Policy.Buffer.MinReplicas = 20 - causes := fas.ValidateUpdate(fasCopy, nil) + fas := defaultFixture() + fas.Spec.Policy.Buffer.MinReplicas = 20 + causes := fas.Validate(nil) assert.Len(t, causes, 1) assert.Equal(t, "minReplicas", causes[0].Field) }) t.Run("bufferSize good percent", func(t *testing.T) { - - fas := &FleetAutoscaler{ - ObjectMeta: metav1.ObjectMeta{Name: "test"}, - Spec: FleetAutoscalerSpec{ - FleetName: "testing", - Policy: FleetAutoscalerPolicy{ - Type: BufferPolicyType, - Buffer: &BufferPolicy{ - BufferSize: intstr.FromInt(5), - MaxReplicas: 10, - }, - }, - }, - } - - fasCopy := fas.DeepCopy() - fasCopy.Spec.Policy.Buffer.BufferSize = intstr.FromString("20%") - causes := fas.ValidateUpdate(fasCopy, nil) + fas := defaultFixture() + fas.Spec.Policy.Buffer.BufferSize = intstr.FromString("20%") + causes := fas.Validate(nil) assert.Len(t, causes, 0) }) t.Run("bufferSize bad percent", func(t *testing.T) { - - fas := &FleetAutoscaler{ - ObjectMeta: metav1.ObjectMeta{Name: "test"}, - Spec: FleetAutoscalerSpec{ - FleetName: "testing", - Policy: FleetAutoscalerPolicy{ - Type: BufferPolicyType, - Buffer: &BufferPolicy{ - BufferSize: intstr.FromInt(5), - MaxReplicas: 10, - }, - }, - }, - } + fas := defaultFixture() fasCopy := fas.DeepCopy() fasCopy.Spec.Policy.Buffer.BufferSize = intstr.FromString("120%") - causes := fas.ValidateUpdate(fasCopy, nil) + causes := fasCopy.Validate(nil) assert.Len(t, causes, 1) assert.Equal(t, "bufferSize", causes[0].Field) fasCopy = fas.DeepCopy() fasCopy.Spec.Policy.Buffer.BufferSize = intstr.FromString("0%") - causes = fas.ValidateUpdate(fasCopy, nil) + causes = fasCopy.Validate(nil) assert.Len(t, causes, 1) assert.Equal(t, "bufferSize", causes[0].Field) fasCopy = fas.DeepCopy() fasCopy.Spec.Policy.Buffer.BufferSize = intstr.FromString("-10%") - causes = fas.ValidateUpdate(fasCopy, nil) + causes = fasCopy.Validate(nil) assert.Len(t, causes, 1) assert.Equal(t, "bufferSize", causes[0].Field) fasCopy = fas.DeepCopy() fasCopy.Spec.Policy.Buffer.BufferSize = intstr.FromString("notgood") - causes = fas.ValidateUpdate(fasCopy, nil) + causes = fasCopy.Validate(nil) assert.Len(t, causes, 1) assert.Equal(t, "bufferSize", causes[0].Field) }) } + +func defaultFixture() *FleetAutoscaler { + return &FleetAutoscaler{ + ObjectMeta: metav1.ObjectMeta{Name: "test"}, + Spec: FleetAutoscalerSpec{ + FleetName: "testing", + Policy: FleetAutoscalerPolicy{ + Type: BufferPolicyType, + Buffer: &BufferPolicy{ + BufferSize: intstr.FromInt(5), + MaxReplicas: 10, + }, + }, + }, + } +} diff --git a/pkg/fleetautoscalers/controller.go b/pkg/fleetautoscalers/controller.go index 1f29523767..19c27f2d95 100644 --- a/pkg/fleetautoscalers/controller.go +++ b/pkg/fleetautoscalers/controller.go @@ -30,7 +30,6 @@ import ( "agones.dev/agones/pkg/util/webhooks" "agones.dev/agones/pkg/util/workerqueue" "github.com/heptiolabs/healthcheck" - "github.com/mattbaird/jsonpatch" "github.com/pkg/errors" "github.com/sirupsen/logrus" admv1beta1 "k8s.io/api/admission/v1beta1" @@ -90,8 +89,8 @@ func NewController( c.recorder = eventBroadcaster.NewRecorder(scheme.Scheme, corev1.EventSource{Component: "fleetautoscaler-controller"}) kind := stablev1alpha1.Kind("FleetAutoscaler") - wh.AddHandler("/mutate", kind, admv1beta1.Create, c.creationMutationHandler) - wh.AddHandler("/validate", kind, admv1beta1.Update, c.mutationValidationHandler) + wh.AddHandler("/validate", kind, admv1beta1.Create, c.validationHandler) + wh.AddHandler("/validate", kind, admv1beta1.Update, c.validationHandler) fasInformer.AddEventHandler(cache.ResourceEventHandlerFuncs{ AddFunc: c.workerqueue.Enqueue, @@ -120,10 +119,10 @@ func (c *Controller) Run(workers int, stop <-chan struct{}) error { return nil } -// creationMutationHandler will intercept when a FleetAutoscaler is created, and attach it to the requested Fleet -// assuming that it exists. If not, it will reject the AdmissionReview. -func (c *Controller) creationMutationHandler(review admv1beta1.AdmissionReview) (admv1beta1.AdmissionReview, error) { - c.logger.WithField("review", review).Info("creationMutationHandler") +// validationHandler will intercept when a FleetAutoscaler is created, and +// validate its settings. +func (c *Controller) validationHandler(review admv1beta1.AdmissionReview) (admv1beta1.AdmissionReview, error) { + c.logger.WithField("review", review).Info("validationHandler") obj := review.Request.Object fas := &stablev1alpha1.FleetAutoscaler{} err := json.Unmarshal(obj.Raw, fas) @@ -132,7 +131,7 @@ func (c *Controller) creationMutationHandler(review admv1beta1.AdmissionReview) } var causes []metav1.StatusCause - causes = fas.ValidateAutoScalingSettings(causes) + causes = fas.Validate(causes) if len(causes) != 0 { review.Response.Allowed = false details := metav1.StatusDetails{ @@ -143,99 +142,7 @@ func (c *Controller) creationMutationHandler(review admv1beta1.AdmissionReview) } review.Response.Result = &metav1.Status{ Status: metav1.StatusFailure, - Message: "FleetAutoscaler creation is invalid", - Reason: metav1.StatusReasonInvalid, - Details: &details, - } - return review, nil - } - - // When being called from the API the fas.ObjectMeta.Namespace isn't populated - // (whereas it is from kubectl). So make sure to pull the namespace from the review - fleet, err := c.fleetLister.Fleets(review.Request.Namespace).Get(fas.Spec.FleetName) - if err != nil { - if k8serrors.IsNotFound(err) { - review.Response.Allowed = false - review.Response.Result = &metav1.Status{ - Status: metav1.StatusFailure, - Reason: metav1.StatusReasonInvalid, - Message: "Invalid FleetAutoscaler", - Details: &metav1.StatusDetails{ - Name: review.Request.Name, - Group: review.Request.Kind.Group, - Kind: review.Request.Kind.Kind, - Causes: []metav1.StatusCause{ - {Type: metav1.CauseTypeFieldValueNotFound, - Message: fmt.Sprintf("Could not find fleet %s in namespace %s", fas.Spec.FleetName, review.Request.Namespace), - Field: "fleetName"}}, - }, - } - return review, nil - } - return review, errors.Wrapf(err, "error retrieving fleet %s", fas.Name) - } - - // Attach to the fleet - // When a Fleet is deleted, the FleetAutoscaler should go with it - ref := metav1.NewControllerRef(fleet, stablev1alpha1.SchemeGroupVersion.WithKind("Fleet")) - fas.ObjectMeta.OwnerReferences = append(fas.ObjectMeta.OwnerReferences, *ref) - - // This event is relevant for both fleet and autoscaler, so we log it to both queues - c.recorder.Eventf(fleet, corev1.EventTypeNormal, "CreatingFleetAutoscaler", "Attached FleetAutoscaler %s to fleet %s", fas.ObjectMeta.Name, fas.Spec.FleetName) - c.recorder.Eventf(fas, corev1.EventTypeNormal, "CreatingFleetAutoscaler", "Attached FleetAutoscaler %s to fleet %s", fas.ObjectMeta.Name, fas.Spec.FleetName) - - newFS, err := json.Marshal(fas) - if err != nil { - return review, errors.Wrapf(err, "error marshalling FleetAutoscaler %s to json", fas.ObjectMeta.Name) - } - - patch, err := jsonpatch.CreatePatch(obj.Raw, newFS) - if err != nil { - return review, errors.Wrapf(err, "error creating patch for FleetAutoscaler %s", fas.ObjectMeta.Name) - } - - json, err := json.Marshal(patch) - if err != nil { - return review, errors.Wrapf(err, "error creating json for patch for FleetAutoscaler %s", fas.ObjectMeta.Name) - } - - c.logger.WithField("fas", fas.ObjectMeta.Name).WithField("patch", string(json)).Infof("patch created!") - - pt := admv1beta1.PatchTypeJSONPatch - review.Response.PatchType = &pt - review.Response.Patch = json - - return review, nil -} - -// mutationValidationHandler stops edits from happening to a -// FleetAutoscaler fleetName value -func (c *Controller) mutationValidationHandler(review admv1beta1.AdmissionReview) (admv1beta1.AdmissionReview, error) { - c.logger.WithField("review", review).Info("mutationValidationHandler") - - newFS := &stablev1alpha1.FleetAutoscaler{} - oldFS := &stablev1alpha1.FleetAutoscaler{} - - if err := json.Unmarshal(review.Request.Object.Raw, newFS); err != nil { - return review, errors.Wrapf(err, "error unmarshalling new FleetAutoscaler json: %s", review.Request.Object.Raw) - } - - if err := json.Unmarshal(review.Request.OldObject.Raw, oldFS); err != nil { - return review, errors.Wrapf(err, "error unmarshalling old FleetAutoscaler json: %s", review.Request.Object.Raw) - } - - causes := oldFS.ValidateUpdate(newFS, nil) - if len(causes) != 0 { - review.Response.Allowed = false - details := metav1.StatusDetails{ - Name: review.Request.Name, - Group: review.Request.Kind.Group, - Kind: review.Request.Kind.Kind, - Causes: causes, - } - review.Response.Result = &metav1.Status{ - Status: metav1.StatusFailure, - Message: "FleetAutoscaler update is invalid", + Message: "FleetAutoscaler is invalid", Reason: metav1.StatusReasonInvalid, Details: &details, } @@ -270,36 +177,36 @@ func (c *Controller) syncFleetAutoscaler(key string) error { fleet, err := c.fleetLister.Fleets(namespace).Get(fas.Spec.FleetName) if err != nil { if k8serrors.IsNotFound(err) { - logrus.WithError(err).WithField("fleetAutoscalerName", fas.Name). - WithField("fleetName", fas.Spec.FleetName). + logrus.WithError(err).WithField("fleetAutoscaler", fas.Name). + WithField("fleet", fas.Spec.FleetName). WithField("namespace", namespace). Warn("Could not find fleet for autoscaler. Skipping.") - return errors.Wrapf(err, "fleet %s not found in namespace %s", fas.Spec.FleetName, namespace) + + c.recorder.Eventf(fas, corev1.EventTypeWarning, "FailedGetFleet", + "could not fetch fleet: %s", fas.Spec.FleetName) + + // don't retry. Pick it up next sync. + err = nil } - result := errors.Wrapf(err, "error retrieving fleet %s from namespace %s", fas.Spec.FleetName, namespace) - err := c.updateStatusUnableToScale(fas) - if err != nil { - c.logger.WithError(err).WithField("fleetAutoscalerName", fas.Name). - Error("Failed to update FleetAutoscaler status") + + if err := c.updateStatusUnableToScale(fas); err != nil { + return err } - return result + + return err } currentReplicas := fleet.Status.Replicas desiredReplicas, scalingLimited, err := computeDesiredFleetSize(fas, fleet) if err != nil { - result := errors.Wrapf(err, "error calculating autoscaling fleet: %s", fleet.ObjectMeta.Name) - err := c.updateStatusUnableToScale(fas) - if err != nil { - c.logger.WithError(err).WithField("fleetAutoscalerName", fas.Name). - Error("Failed to update FleetAutoscaler status") + if err := c.updateStatusUnableToScale(fas); err != nil { + return err } - return result + return errors.Wrapf(err, "error calculating autoscaling fleet: %s", fleet.ObjectMeta.Name) } // Scale the fleet to the new size - err = c.scaleFleet(fas, fleet, desiredReplicas) - if err != nil { + if err = c.scaleFleet(fas, fleet, desiredReplicas); err != nil { return errors.Wrapf(err, "error autoscaling fleet %s to %d replicas", fas.Spec.FleetName, desiredReplicas) } @@ -326,8 +233,6 @@ func (c *Controller) scaleFleet(fas *stablev1alpha1.FleetAutoscaler, f *stablev1 // updateStatus updates the status of the given FleetAutoscaler func (c *Controller) updateStatus(fas *stablev1alpha1.FleetAutoscaler, currentReplicas int32, desiredReplicas int32, scaled bool, scalingLimited bool) error { fasCopy := fas.DeepCopy() - fasCopy.Status.AbleToScale = true - fasCopy.Status.ScalingLimited = scalingLimited fasCopy.Status.CurrentReplicas = currentReplicas fasCopy.Status.DesiredReplicas = desiredReplicas if scaled { @@ -336,6 +241,10 @@ func (c *Controller) updateStatus(fas *stablev1alpha1.FleetAutoscaler, currentRe } if !apiequality.Semantic.DeepEqual(fas.Status, fasCopy.Status) { + if scalingLimited { + c.recorder.Eventf(fas, corev1.EventTypeWarning, "ScalingLimited", "Scaling fleet %s was limited to maximum size of %d", fas.Spec.FleetName, desiredReplicas) + } + _, err := c.fleetAutoscalerGetter.FleetAutoscalers(fas.ObjectMeta.Namespace).Update(fasCopy) if err != nil { return errors.Wrapf(err, "error updating status for fleetautoscaler %s", fas.ObjectMeta.Name) @@ -348,8 +257,6 @@ func (c *Controller) updateStatus(fas *stablev1alpha1.FleetAutoscaler, currentRe // updateStatus updates the status of the given FleetAutoscaler in the case we're not able to scale func (c *Controller) updateStatusUnableToScale(fas *stablev1alpha1.FleetAutoscaler) error { fasCopy := fas.DeepCopy() - fasCopy.Status.AbleToScale = false - fasCopy.Status.ScalingLimited = false fasCopy.Status.CurrentReplicas = 0 fasCopy.Status.DesiredReplicas = 0 diff --git a/pkg/fleetautoscalers/controller_test.go b/pkg/fleetautoscalers/controller_test.go index b6fc3e9a5c..6d64271735 100644 --- a/pkg/fleetautoscalers/controller_test.go +++ b/pkg/fleetautoscalers/controller_test.go @@ -35,76 +35,41 @@ var ( gvk = metav1.GroupVersionKind(v1alpha1.SchemeGroupVersion.WithKind("FleetAutoscaler")) ) -func TestControllerCreationMutationHandler(t *testing.T) { +func TestControllerCreationValidationHandler(t *testing.T) { t.Parallel() - t.Run("fleet scaler has a fleet", func(t *testing.T) { + t.Run("valid fleet autoscaler", func(t *testing.T) { c, m := newFakeController() - fas, f := defaultFixtures() - m.AgonesClient.AddReactor("list", "fleets", func(action k8stesting.Action) (bool, runtime.Object, error) { - return true, &v1alpha1.FleetList{Items: []v1alpha1.Fleet{*f}}, nil - }) - + fas, _ := defaultFixtures() _, cancel := agtesting.StartInformers(m) defer cancel() review, err := newAdmissionReview(*fas) assert.Nil(t, err) - result, err := c.creationMutationHandler(review) + result, err := c.validationHandler(review) assert.Nil(t, err) assert.True(t, result.Response.Allowed, fmt.Sprintf("%#v", result.Response)) - assert.Equal(t, admv1beta1.PatchTypeJSONPatch, *result.Response.PatchType) - assert.Contains(t, string(result.Response.Patch), "/metadata/ownerReferences") - }) - - t.Run("fleet does not exist", func(t *testing.T) { - c, _ := newFakeController() - fas, _ := defaultFixtures() - - review, err := newAdmissionReview(*fas) - assert.Nil(t, err) - - result, err := c.creationMutationHandler(review) - assert.Nil(t, err) - assert.False(t, result.Response.Allowed) - assert.Equal(t, "fleetName", result.Response.Result.Details.Causes[0].Field) }) -} - -func TestControllerMutationValidationHandler(t *testing.T) { - t.Parallel() - c, _ := newFakeController() - t.Run("same fleetName", func(t *testing.T) { + t.Run("invalid fleet autoscaler", func(t *testing.T) { + c, m := newFakeController() fas, _ := defaultFixtures() + // this make it invalid + fas.Spec.Policy.Buffer = nil - review, err := newAdmissionReview(*fas) - assert.Nil(t, err) - review.Request.OldObject = *review.Request.Object.DeepCopy() - - result, err := c.mutationValidationHandler(review) - assert.Nil(t, err) - assert.True(t, result.Response.Allowed, fmt.Sprintf("%#v", result.Response)) - }) - - t.Run("different fleetname", func(t *testing.T) { - fas, _ := defaultFixtures() + _, cancel := agtesting.StartInformers(m) + defer cancel() review, err := newAdmissionReview(*fas) assert.Nil(t, err) - oldObject := fas.DeepCopy() - oldObject.Spec.FleetName = "changed" - json, err := json.Marshal(oldObject) + result, err := c.validationHandler(review) assert.Nil(t, err) - review.Request.OldObject = runtime.RawExtension{Raw: json} - - result, err := c.mutationValidationHandler(review) - assert.Nil(t, err) - assert.False(t, result.Response.Allowed) + assert.False(t, result.Response.Allowed, fmt.Sprintf("%#v", result.Response)) + assert.Equal(t, metav1.StatusFailure, result.Response.Result.Status) assert.Equal(t, metav1.StatusReasonInvalid, result.Response.Result.Reason) - assert.NotNil(t, result.Response.Result.Details) + assert.NotEmpty(t, result.Response.Result.Details) }) } @@ -134,8 +99,6 @@ func TestControllerSyncFleetAutoscaler(t *testing.T) { fasUpdated = true ca := action.(k8stesting.UpdateAction) fas := ca.GetObject().(*v1alpha1.FleetAutoscaler) - assert.Equal(t, fas.Status.AbleToScale, true) - assert.Equal(t, fas.Status.ScalingLimited, false) assert.Equal(t, fas.Status.CurrentReplicas, int32(5)) assert.Equal(t, fas.Status.DesiredReplicas, int32(12)) assert.NotNil(t, fas.Status.LastScaleTime) @@ -162,6 +125,7 @@ func TestControllerSyncFleetAutoscaler(t *testing.T) { assert.True(t, fUpdated, "fleet should have been updated") assert.True(t, fasUpdated, "fleetautoscaler should have been updated") agtesting.AssertEventContains(t, m.FakeRecorder.Events, "AutoScalingFleet") + agtesting.AssertNoEvent(t, m.FakeRecorder.Events) }) t.Run("scaling down", func(t *testing.T) { @@ -186,8 +150,6 @@ func TestControllerSyncFleetAutoscaler(t *testing.T) { fasUpdated = true ca := action.(k8stesting.UpdateAction) fas := ca.GetObject().(*v1alpha1.FleetAutoscaler) - assert.Equal(t, fas.Status.AbleToScale, true) - assert.Equal(t, fas.Status.ScalingLimited, false) assert.Equal(t, fas.Status.CurrentReplicas, int32(20)) assert.Equal(t, fas.Status.DesiredReplicas, int32(13)) assert.NotNil(t, fas.Status.LastScaleTime) @@ -215,6 +177,7 @@ func TestControllerSyncFleetAutoscaler(t *testing.T) { assert.True(t, fUpdated, "fleet should have been updated") assert.True(t, fasUpdated, "fleetautoscaler should have been updated") agtesting.AssertEventContains(t, m.FakeRecorder.Events, "AutoScalingFleet") + agtesting.AssertNoEvent(t, m.FakeRecorder.Events) }) t.Run("no scaling no update", func(t *testing.T) { @@ -250,6 +213,37 @@ func TestControllerSyncFleetAutoscaler(t *testing.T) { assert.Nil(t, err) agtesting.AssertNoEvent(t, m.FakeRecorder.Events) }) + + t.Run("fleet not available", func(t *testing.T) { + t.Parallel() + c, m := newFakeController() + fas, _ := defaultFixtures() + fas.Status.DesiredReplicas = 10 + fas.Status.CurrentReplicas = 5 + updated := false + + m.AgonesClient.AddReactor("list", "fleetautoscalers", func(action k8stesting.Action) (bool, runtime.Object, error) { + return true, &v1alpha1.FleetAutoscalerList{Items: []v1alpha1.FleetAutoscaler{*fas}}, nil + }) + + m.AgonesClient.AddReactor("update", "fleetautoscalers", func(action k8stesting.Action) (bool, runtime.Object, error) { + updated = true + ca := action.(k8stesting.UpdateAction) + fas := ca.GetObject().(*v1alpha1.FleetAutoscaler) + assert.Equal(t, fas.Status.CurrentReplicas, int32(0)) + assert.Equal(t, fas.Status.DesiredReplicas, int32(0)) + return true, fas, nil + }) + + _, cancel := agtesting.StartInformers(m, c.fleetAutoscalerSynced) + defer cancel() + + err := c.syncFleetAutoscaler("default/fas-1") + assert.Nil(t, err) + assert.True(t, updated) + + agtesting.AssertEventContains(t, m.FakeRecorder.Events, "FailedGetFleet") + }) } func TestControllerScaleFleet(t *testing.T) { @@ -306,8 +300,6 @@ func TestControllerUpdateStatus(t *testing.T) { fasUpdated = true ca := action.(k8stesting.UpdateAction) fas := ca.GetObject().(*v1alpha1.FleetAutoscaler) - assert.Equal(t, fas.Status.AbleToScale, true) - assert.Equal(t, fas.Status.ScalingLimited, false) assert.Equal(t, fas.Status.CurrentReplicas, int32(10)) assert.Equal(t, fas.Status.DesiredReplicas, int32(20)) assert.NotNil(t, fas.Status.LastScaleTime) @@ -327,8 +319,6 @@ func TestControllerUpdateStatus(t *testing.T) { c, m := newFakeController() fas, _ := defaultFixtures() - fas.Status.AbleToScale = true - fas.Status.ScalingLimited = false fas.Status.CurrentReplicas = 10 fas.Status.DesiredReplicas = 20 fas.Status.LastScaleTime = nil @@ -341,10 +331,19 @@ func TestControllerUpdateStatus(t *testing.T) { _, cancel := agtesting.StartInformers(m, c.fleetAutoscalerSynced) defer cancel() - err := c.updateStatus(fas, fas.Status.CurrentReplicas, fas.Status.DesiredReplicas, false, fas.Status.ScalingLimited) + err := c.updateStatus(fas, fas.Status.CurrentReplicas, fas.Status.DesiredReplicas, false, false) assert.Nil(t, err) agtesting.AssertNoEvent(t, m.FakeRecorder.Events) }) + + t.Run("update with a scaling limit", func(t *testing.T) { + c, m := newFakeController() + fas, _ := defaultFixtures() + + err := c.updateStatus(fas, 10, 20, true, true) + assert.Nil(t, err) + agtesting.AssertEventContains(t, m.FakeRecorder.Events, "ScalingLimited") + }) } func TestControllerUpdateStatusUnableToScale(t *testing.T) { @@ -353,7 +352,7 @@ func TestControllerUpdateStatusUnableToScale(t *testing.T) { t.Run("must update", func(t *testing.T) { c, m := newFakeController() fas, _ := defaultFixtures() - fas.Status.AbleToScale = true + fas.Status.DesiredReplicas = 10 fasUpdated := false @@ -361,8 +360,6 @@ func TestControllerUpdateStatusUnableToScale(t *testing.T) { fasUpdated = true ca := action.(k8stesting.UpdateAction) fas := ca.GetObject().(*v1alpha1.FleetAutoscaler) - assert.Equal(t, fas.Status.AbleToScale, false) - assert.Equal(t, fas.Status.ScalingLimited, false) assert.Equal(t, fas.Status.CurrentReplicas, int32(0)) assert.Equal(t, fas.Status.DesiredReplicas, int32(0)) assert.Nil(t, fas.Status.LastScaleTime) @@ -381,8 +378,6 @@ func TestControllerUpdateStatusUnableToScale(t *testing.T) { t.Run("must not update", func(t *testing.T) { c, m := newFakeController() fas, _ := defaultFixtures() - fas.Status.AbleToScale = false - fas.Status.ScalingLimited = false fas.Status.CurrentReplicas = 0 fas.Status.DesiredReplicas = 0 diff --git a/pkg/gameservers/localsdk.go b/pkg/gameservers/localsdk.go index ec2b04a9e4..e1596188cf 100644 --- a/pkg/gameservers/localsdk.go +++ b/pkg/gameservers/localsdk.go @@ -62,7 +62,6 @@ type LocalSDKServer struct { } // NewLocalSDKServer returns the default LocalSDKServer -// TODO: update all the tests at a later date func NewLocalSDKServer(filePath string) (*LocalSDKServer, error) { l := &LocalSDKServer{ gsMutex: sync.RWMutex{},