From ba0ddbb14fc7de848368ba25abd6cc1e4a047fba Mon Sep 17 00:00:00 2001 From: Mark Mandel Date: Thu, 23 Feb 2023 10:43:57 -0800 Subject: [PATCH] Make Alpha & Beta CRD declaration consistent. (#2982) We realised that with the CRD schema application being hidden behind a feature flag in the helm CRD yaml, then if the feature flag was turned off, an end user wouldn't get a validation error since the values passed in would be pruned by the K8s control plane. To make thins consistent across Alpha CRD values: * Alpha & Beta schemas are always available within the CRD definitions. * Validation checks are in place for the feature flag (no change needed) * Alpha & Beta CRD values are pointers, so they can be nullable (this required a change in Eviction handling and ApiHooks interface) * Some general cleanup of comments and descriptions in the schema. --- .../templates/crds/_gameserverspecschema.yaml | 14 +- .../templates/crds/_gameserverstatus.yaml | 13 +- .../agones/templates/crds/gameserver.yaml | 2 +- install/yaml/install.yaml | 147 ++++++++++++++++++ pkg/apis/agones/v1/apihooks.go | 2 +- pkg/apis/agones/v1/apihooksfake.go | 6 +- pkg/apis/agones/v1/gameserver.go | 17 +- pkg/apis/agones/v1/gameserver_test.go | 21 ++- pkg/apis/agones/v1/zz_generated.deepcopy.go | 12 +- pkg/cloudproduct/generic/generic.go | 13 +- pkg/cloudproduct/generic/generic_test.go | 18 +-- pkg/cloudproduct/gke/gke.go | 9 +- pkg/cloudproduct/gke/gke_test.go | 18 +-- pkg/testing/apihooks.go | 2 +- .../Reference/agones_crd_api_reference.html | 1 + 15 files changed, 236 insertions(+), 59 deletions(-) diff --git a/install/helm/agones/templates/crds/_gameserverspecschema.yaml b/install/helm/agones/templates/crds/_gameserverspecschema.yaml index e2de536654..06794e0c0f 100644 --- a/install/helm/agones/templates/crds/_gameserverspecschema.yaml +++ b/install/helm/agones/templates/crds/_gameserverspecschema.yaml @@ -153,7 +153,6 @@ properties: type: integer title: The initial player capacity of this Game Server minimum: 0 -{{- if .featureCountsAndLists }} counters: type: object title: Map of player, room, session, etc. counters @@ -162,11 +161,13 @@ properties: additionalProperties: type: object properties: - count: # initial count + count: + title: Initial count value type: integer default: 0 minimum: 0 - capacity: # max capacity of the counter + capacity: + title: Max capacity of the counter type: integer minimum: 0 lists: @@ -177,18 +178,18 @@ properties: additionalProperties: type: object properties: - capacity: # max capacity of the array (can be less than or equal to value of maxItems) + capacity: type: integer + title: Max capacity of the array (can be less than or equal to value of maxItems) minimum: 0 maximum: 1000 # must be equal to values.maxItems values: + title: set of all the items in the list type: array x-kubernetes-list-type: set # Requires items in the array to be unique maxItems: 1000 # max possible size of the value array (cannot be updated) items: # name of the item (player1, session1, room1, etc.) type: string -{{- end }} -{{- if .featureSafeToEvict }} eviction: type: object title: Eviction tolerance of the game server @@ -211,4 +212,3 @@ properties: minimum: 1 maximum: 1 {{- end }} -{{- end }} diff --git a/install/helm/agones/templates/crds/_gameserverstatus.yaml b/install/helm/agones/templates/crds/_gameserverstatus.yaml index caa653897a..17877134f9 100644 --- a/install/helm/agones/templates/crds/_gameserverstatus.yaml +++ b/install/helm/agones/templates/crds/_gameserverstatus.yaml @@ -65,7 +65,6 @@ status: nullable: true items: type: string -{{- if .featureCountsAndLists }} counters: type: object title: Map of player, room, session, etc. counters @@ -74,11 +73,12 @@ status: additionalProperties: type: object properties: - count: # initial count + count: + title: The current count type: integer default: 0 minimum: 0 - capacity: # max capacity of the counter + capacity: type: integer minimum: 0 lists: @@ -89,18 +89,18 @@ status: additionalProperties: type: object properties: - capacity: # max capacity of the array (can be less than or equal to value of values.maxItems) + capacity: + title: Max capacity of the array (can be less than or equal to value of values.maxItems) type: integer minimum: 0 maximum: 1000 # must be equal to values.maxItems values: + title: Set of all the items in the list type: array x-kubernetes-list-type: set # Requires items in the array to be unique maxItems: 1000 # max possible size of the value array (cannot be updated) items: # name of the item (player1, session1, room1, etc.) type: string -{{- end }} -{{- if .featureSafeToEvict }} eviction: type: object properties: @@ -117,4 +117,3 @@ status: minimum: 1 maximum: 1 {{- end}} -{{- end}} diff --git a/install/helm/agones/templates/crds/gameserver.yaml b/install/helm/agones/templates/crds/gameserver.yaml index 1f78a2df81..e1a125b22b 100644 --- a/install/helm/agones/templates/crds/gameserver.yaml +++ b/install/helm/agones/templates/crds/gameserver.yaml @@ -56,7 +56,7 @@ spec: type: date schema: openAPIV3Schema: - {{- $data := dict "podPreserveUnknownFields" .Values.gameservers.podPreserveUnknownFields "featureSafeToEvict" $featureGates.SafeToEvict "featureCountsAndLists" $featureGates.CountsAndLists }} + {{- $data := dict "podPreserveUnknownFields" .Values.gameservers.podPreserveUnknownFields }} {{- include "gameserver.schema" $data | indent 9 }}{{- /* include the schema then status, as it's easier to align */ -}} {{- include "gameserver.status" $data | indent 11 }} {{- if $featureGates.SafeToEvict }} diff --git a/install/yaml/install.yaml b/install/yaml/install.yaml index b240bd874c..e769b9a93c 100644 --- a/install/yaml/install.yaml +++ b/install/yaml/install.yaml @@ -5099,6 +5099,43 @@ spec: type: integer title: The initial player capacity of this Game Server minimum: 0 + counters: + type: object + title: Map of player, room, session, etc. counters + nullable: true + maxProperties: 1000 + additionalProperties: + type: object + properties: + count: + title: Initial count value + type: integer + default: 0 + minimum: 0 + capacity: + title: Max capacity of the counter + type: integer + minimum: 0 + lists: + type: object + title: Map of player, room, session, etc. lists + nullable: true + maxProperties: 1000 + additionalProperties: + type: object + properties: + capacity: + type: integer + title: Max capacity of the array (can be less than or equal to value of maxItems) + minimum: 0 + maximum: 1000 # must be equal to values.maxItems + values: + title: set of all the items in the list + type: array + x-kubernetes-list-type: set # Requires items in the array to be unique + maxItems: 1000 # max possible size of the value array (cannot be updated) + items: # name of the item (player1, session1, room1, etc.) + type: string eviction: type: object title: Eviction tolerance of the game server @@ -10083,6 +10120,43 @@ spec: type: integer title: The initial player capacity of this Game Server minimum: 0 + counters: + type: object + title: Map of player, room, session, etc. counters + nullable: true + maxProperties: 1000 + additionalProperties: + type: object + properties: + count: + title: Initial count value + type: integer + default: 0 + minimum: 0 + capacity: + title: Max capacity of the counter + type: integer + minimum: 0 + lists: + type: object + title: Map of player, room, session, etc. lists + nullable: true + maxProperties: 1000 + additionalProperties: + type: object + properties: + capacity: + type: integer + title: Max capacity of the array (can be less than or equal to value of maxItems) + minimum: 0 + maximum: 1000 # must be equal to values.maxItems + values: + title: set of all the items in the list + type: array + x-kubernetes-list-type: set # Requires items in the array to be unique + maxItems: 1000 # max possible size of the value array (cannot be updated) + items: # name of the item (player1, session1, room1, etc.) + type: string eviction: type: object title: Eviction tolerance of the game server @@ -10155,6 +10229,42 @@ spec: nullable: true items: type: string + counters: + type: object + title: Map of player, room, session, etc. counters + nullable: true + maxProperties: 1000 + additionalProperties: + type: object + properties: + count: + title: The current count + type: integer + default: 0 + minimum: 0 + capacity: + type: integer + minimum: 0 + lists: + type: object + title: Map of player, room, session, etc. lists + nullable: true + maxProperties: 1000 + additionalProperties: + type: object + properties: + capacity: + title: Max capacity of the array (can be less than or equal to value of values.maxItems) + type: integer + minimum: 0 + maximum: 1000 # must be equal to values.maxItems + values: + title: Set of all the items in the list + type: array + x-kubernetes-list-type: set # Requires items in the array to be unique + maxItems: 1000 # max possible size of the value array (cannot be updated) + items: # name of the item (player1, session1, room1, etc.) + type: string eviction: type: object properties: @@ -15197,6 +15307,43 @@ spec: type: integer title: The initial player capacity of this Game Server minimum: 0 + counters: + type: object + title: Map of player, room, session, etc. counters + nullable: true + maxProperties: 1000 + additionalProperties: + type: object + properties: + count: + title: Initial count value + type: integer + default: 0 + minimum: 0 + capacity: + title: Max capacity of the counter + type: integer + minimum: 0 + lists: + type: object + title: Map of player, room, session, etc. lists + nullable: true + maxProperties: 1000 + additionalProperties: + type: object + properties: + capacity: + type: integer + title: Max capacity of the array (can be less than or equal to value of maxItems) + minimum: 0 + maximum: 1000 # must be equal to values.maxItems + values: + title: set of all the items in the list + type: array + x-kubernetes-list-type: set # Requires items in the array to be unique + maxItems: 1000 # max possible size of the value array (cannot be updated) + items: # name of the item (player1, session1, room1, etc.) + type: string eviction: type: object title: Eviction tolerance of the game server diff --git a/pkg/apis/agones/v1/apihooks.go b/pkg/apis/agones/v1/apihooks.go index 5b2b6df458..7f5da25b66 100644 --- a/pkg/apis/agones/v1/apihooks.go +++ b/pkg/apis/agones/v1/apihooks.go @@ -32,5 +32,5 @@ type APIHooks interface { MutateGameServerPodSpec(*GameServerSpec, *corev1.PodSpec) error // SetEviction is called by gs.Pod to enforce GameServer.Status.Eviction. - SetEviction(EvictionSafe, *corev1.Pod) error + SetEviction(*Eviction, *corev1.Pod) error } diff --git a/pkg/apis/agones/v1/apihooksfake.go b/pkg/apis/agones/v1/apihooksfake.go index f24bdbed90..8666c2880c 100644 --- a/pkg/apis/agones/v1/apihooksfake.go +++ b/pkg/apis/agones/v1/apihooksfake.go @@ -26,7 +26,7 @@ type fakeAPIHooks struct { StubValidateGameServerSpec func(*GameServerSpec) []metav1.StatusCause StubValidateScheduling func(apis.SchedulingStrategy) []metav1.StatusCause StubMutateGameServerPodSpec func(*GameServerSpec, *corev1.PodSpec) error - StubSetEviction func(EvictionSafe, *corev1.Pod) error + StubSetEviction func(*Eviction, *corev1.Pod) error } var _ APIHooks = fakeAPIHooks{} @@ -56,9 +56,9 @@ func (f fakeAPIHooks) MutateGameServerPodSpec(gss *GameServerSpec, podSpec *core } // SetEviction is called by gs.Pod to enforce GameServer.Status.Eviction. -func (f fakeAPIHooks) SetEviction(safe EvictionSafe, pod *corev1.Pod) error { +func (f fakeAPIHooks) SetEviction(eviction *Eviction, pod *corev1.Pod) error { if f.StubSetEviction != nil { - return f.StubSetEviction(safe, pod) + return f.StubSetEviction(eviction, pod) } return nil } diff --git a/pkg/apis/agones/v1/gameserver.go b/pkg/apis/agones/v1/gameserver.go index 11ff97686c..cc8cbc6d1d 100644 --- a/pkg/apis/agones/v1/gameserver.go +++ b/pkg/apis/agones/v1/gameserver.go @@ -193,7 +193,7 @@ type GameServerSpec struct { Lists map[string]ListSpec `json:"lists,omitempty"` // (Alpha, SafeToEvict feature flag) Eviction specifies the eviction tolerance of the GameServer. Defaults to "Never". // +optional - Eviction Eviction `json:"eviction,omitempty"` + Eviction *Eviction `json:"eviction,omitempty"` // immutableReplicas is present in gameservers.agones.dev but omitted here (it's always 1). } @@ -293,7 +293,8 @@ type GameServerStatus struct { // +optional Players *PlayerStatus `json:"players"` // (Alpha, SafeToEvict feature flag) Eviction specifies the eviction tolerance of the GameServer. - Eviction Eviction `json:"eviction,omitempty"` + // +optional + Eviction *Eviction `json:"eviction,omitempty"` // immutableReplicas is present in gameservers.agones.dev but omitted here (it's always 1). } @@ -422,6 +423,9 @@ func (gss *GameServerSpec) applyEvictionDefaults() { if !runtime.FeatureEnabled(runtime.FeatureSafeToEvict) { return } + if gss.Eviction == nil { + gss.Eviction = &Eviction{} + } if gss.Eviction.Safe == "" { gss.Eviction.Safe = EvictionSafeNever } @@ -431,8 +435,11 @@ func (gs *GameServer) applyEvictionStatus() { if !runtime.FeatureEnabled(runtime.FeatureSafeToEvict) { return } - gs.Status.Eviction = gs.Spec.Eviction + gs.Status.Eviction = gs.Spec.Eviction.DeepCopy() if gs.Spec.Template.ObjectMeta.Annotations[PodSafeToEvictAnnotation] == "true" { + if gs.Status.Eviction == nil { + gs.Status.Eviction = &Eviction{} + } gs.Status.Eviction.Safe = EvictionSafeAlways } } @@ -469,7 +476,7 @@ func (gss *GameServerSpec) validateFeatureGates() []metav1.StatusCause { } if !runtime.FeatureEnabled(runtime.FeatureSafeToEvict) { - if gss.Eviction.Safe != "" { + if gss.Eviction != nil { causes = append(causes, metav1.StatusCause{ Type: metav1.CauseTypeFieldValueNotSupported, Field: "eviction.safe", @@ -747,7 +754,7 @@ func (gs *GameServer) Pod(apiHooks APIHooks, sidecars ...corev1.Container) (*cor if err := apiHooks.MutateGameServerPodSpec(&gs.Spec, &pod.Spec); err != nil { return nil, err } - if err := apiHooks.SetEviction(gs.Status.Eviction.Safe, pod); err != nil { + if err := apiHooks.SetEviction(gs.Status.Eviction, pod); err != nil { return nil, err } diff --git a/pkg/apis/agones/v1/gameserver_test.go b/pkg/apis/agones/v1/gameserver_test.go index 20df4d9a22..67e813d194 100644 --- a/pkg/apis/agones/v1/gameserver_test.go +++ b/pkg/apis/agones/v1/gameserver_test.go @@ -320,7 +320,7 @@ func TestGameServerApplyDefaults(t *testing.T) { }, "eviction.safe: Always": { gameServer: defaultGameServerAnd(func(gss *GameServerSpec) { - gss.Eviction.Safe = EvictionSafeAlways + gss.Eviction = &Eviction{Safe: EvictionSafeAlways} }), expected: wantDefaultAnd(func(e *expected) { e.evictionSafeSpec = EvictionSafeAlways @@ -329,7 +329,7 @@ func TestGameServerApplyDefaults(t *testing.T) { }, "eviction.safe: OnUpgrade": { gameServer: defaultGameServerAnd(func(gss *GameServerSpec) { - gss.Eviction.Safe = EvictionSafeOnUpgrade + gss.Eviction = &Eviction{Safe: EvictionSafeOnUpgrade} }), expected: wantDefaultAnd(func(e *expected) { e.evictionSafeSpec = EvictionSafeOnUpgrade @@ -338,7 +338,7 @@ func TestGameServerApplyDefaults(t *testing.T) { }, "eviction.safe: Never": { gameServer: defaultGameServerAnd(func(gss *GameServerSpec) { - gss.Eviction.Safe = EvictionSafeNever + gss.Eviction = &Eviction{Safe: EvictionSafeNever} }), expected: wantDefaultAnd(func(e *expected) { e.evictionSafeSpec = EvictionSafeNever @@ -365,7 +365,7 @@ func TestGameServerApplyDefaults(t *testing.T) { }, "safe-to-evict=false AND eviction.safe: Always => eviction.safe: Always": { gameServer: defaultGameServerAnd(func(gss *GameServerSpec) { - gss.Eviction.Safe = EvictionSafeAlways + gss.Eviction = &Eviction{Safe: EvictionSafeAlways} gss.Template.ObjectMeta.Annotations = map[string]string{PodSafeToEvictAnnotation: "false"} }), expected: wantDefaultAnd(func(e *expected) { @@ -401,8 +401,17 @@ func TestGameServerApplyDefaults(t *testing.T) { assert.Nil(t, test.gameServer.Spec.Players) assert.Nil(t, test.gameServer.Status.Players) } - assert.Equal(t, test.expected.evictionSafeSpec, spec.Eviction.Safe) - assert.Equal(t, test.expected.evictionSafeStatus, test.gameServer.Status.Eviction.Safe) + if len(test.expected.evictionSafeSpec) > 0 { + assert.Equal(t, test.expected.evictionSafeSpec, spec.Eviction.Safe) + } else { + assert.Nil(t, spec.Eviction) + } + + if len(test.expected.evictionSafeStatus) > 0 { + assert.Equal(t, test.expected.evictionSafeStatus, test.gameServer.Status.Eviction.Safe) + } else { + assert.Nil(t, test.gameServer.Status.Eviction) + } }) } } diff --git a/pkg/apis/agones/v1/zz_generated.deepcopy.go b/pkg/apis/agones/v1/zz_generated.deepcopy.go index cba296e2c6..ca32e8d69b 100644 --- a/pkg/apis/agones/v1/zz_generated.deepcopy.go +++ b/pkg/apis/agones/v1/zz_generated.deepcopy.go @@ -386,7 +386,11 @@ func (in *GameServerSpec) DeepCopyInto(out *GameServerSpec) { (*out)[key] = *val.DeepCopy() } } - out.Eviction = in.Eviction + if in.Eviction != nil { + in, out := &in.Eviction, &out.Eviction + *out = new(Eviction) + **out = **in + } return } @@ -417,7 +421,11 @@ func (in *GameServerStatus) DeepCopyInto(out *GameServerStatus) { *out = new(PlayerStatus) (*in).DeepCopyInto(*out) } - out.Eviction = in.Eviction + if in.Eviction != nil { + in, out := &in.Eviction, &out.Eviction + *out = new(Eviction) + **out = **in + } return } diff --git a/pkg/cloudproduct/generic/generic.go b/pkg/cloudproduct/generic/generic.go index 492ef76eb8..9fde3eef9a 100644 --- a/pkg/cloudproduct/generic/generic.go +++ b/pkg/cloudproduct/generic/generic.go @@ -39,23 +39,26 @@ func (*generic) ValidateScheduling(apis.SchedulingStrategy) []metav1.StatusCause func (*generic) MutateGameServerPodSpec(*agonesv1.GameServerSpec, *corev1.PodSpec) error { return nil } // SetEviction sets disruptions controls based on GameServer.Status.Eviction. -func (*generic) SetEviction(safe agonesv1.EvictionSafe, pod *corev1.Pod) error { +func (*generic) SetEviction(eviction *agonesv1.Eviction, pod *corev1.Pod) error { if !runtime.FeatureEnabled(runtime.FeatureSafeToEvict) { return nil } + if eviction == nil { + return errors.New("No eviction value set. Should be the default value") + } if _, exists := pod.ObjectMeta.Annotations[agonesv1.PodSafeToEvictAnnotation]; !exists { - switch safe { + switch eviction.Safe { case agonesv1.EvictionSafeAlways: pod.ObjectMeta.Annotations[agonesv1.PodSafeToEvictAnnotation] = agonesv1.True case agonesv1.EvictionSafeOnUpgrade, agonesv1.EvictionSafeNever: // For EvictionSafeOnUpgrade and EvictionSafeNever, we block Cluster Autoscaler. pod.ObjectMeta.Annotations[agonesv1.PodSafeToEvictAnnotation] = agonesv1.False default: - return errors.Errorf("unknown eviction.safe value %q", string(safe)) + return errors.Errorf("unknown eviction.safe value %q", string(eviction.Safe)) } } if _, exists := pod.ObjectMeta.Labels[agonesv1.SafeToEvictLabel]; !exists { - switch safe { + switch eviction.Safe { case agonesv1.EvictionSafeAlways, agonesv1.EvictionSafeOnUpgrade: // For EvictionSafeAlways and EvictionSafeOnUpgrade, we use a label value // that does not match the agones-gameserver-safe-to-evict-false PDB. But @@ -66,7 +69,7 @@ func (*generic) SetEviction(safe agonesv1.EvictionSafe, pod *corev1.Pod) error { // For EvictionSafeNever, match gones-gameserver-safe-to-evict-false PDB. pod.ObjectMeta.Labels[agonesv1.SafeToEvictLabel] = agonesv1.False default: - return errors.Errorf("unknown eviction.safe value %q", string(safe)) + return errors.Errorf("unknown eviction.safe value %q", string(eviction.Safe)) } } return nil diff --git a/pkg/cloudproduct/generic/generic_test.go b/pkg/cloudproduct/generic/generic_test.go index cb10b71a4f..a8857de8b3 100644 --- a/pkg/cloudproduct/generic/generic_test.go +++ b/pkg/cloudproduct/generic/generic_test.go @@ -41,7 +41,7 @@ func TestSetEviction(t *testing.T) { } for desc, tc := range map[string]struct { featureFlags string - safeToEvict agonesv1.EvictionSafe + eviction *agonesv1.Eviction pod *corev1.Pod wantPod *corev1.Pod }{ @@ -51,7 +51,7 @@ func TestSetEviction(t *testing.T) { }, "SafeToEvict: Always, no incoming labels/annotations": { featureFlags: "SafeToEvict=true", - safeToEvict: agonesv1.EvictionSafeAlways, + eviction: &agonesv1.Eviction{Safe: agonesv1.EvictionSafeAlways}, pod: emptyPodAnd(func(*corev1.Pod) {}), wantPod: emptyPodAnd(func(pod *corev1.Pod) { pod.ObjectMeta.Annotations[agonesv1.PodSafeToEvictAnnotation] = agonesv1.True @@ -60,7 +60,7 @@ func TestSetEviction(t *testing.T) { }, "SafeToEvict: OnUpgrade, no incoming labels/annotations": { featureFlags: "SafeToEvict=true", - safeToEvict: agonesv1.EvictionSafeOnUpgrade, + eviction: &agonesv1.Eviction{Safe: agonesv1.EvictionSafeOnUpgrade}, pod: emptyPodAnd(func(*corev1.Pod) {}), wantPod: emptyPodAnd(func(pod *corev1.Pod) { pod.ObjectMeta.Annotations[agonesv1.PodSafeToEvictAnnotation] = agonesv1.False @@ -69,7 +69,7 @@ func TestSetEviction(t *testing.T) { }, "SafeToEvict: Never, no incoming labels/annotations": { featureFlags: "SafeToEvict=true", - safeToEvict: agonesv1.EvictionSafeNever, + eviction: &agonesv1.Eviction{Safe: agonesv1.EvictionSafeNever}, pod: emptyPodAnd(func(*corev1.Pod) {}), wantPod: emptyPodAnd(func(pod *corev1.Pod) { pod.ObjectMeta.Annotations[agonesv1.PodSafeToEvictAnnotation] = agonesv1.False @@ -78,7 +78,7 @@ func TestSetEviction(t *testing.T) { }, "SafeToEvict: Always, incoming labels/annotations": { featureFlags: "SafeToEvict=true", - safeToEvict: agonesv1.EvictionSafeAlways, + eviction: &agonesv1.Eviction{Safe: agonesv1.EvictionSafeAlways}, pod: emptyPodAnd(func(pod *corev1.Pod) { pod.ObjectMeta.Annotations[agonesv1.PodSafeToEvictAnnotation] = "just don't touch, ok?" pod.ObjectMeta.Labels[agonesv1.SafeToEvictLabel] = "seriously, leave it" @@ -90,7 +90,7 @@ func TestSetEviction(t *testing.T) { }, "SafeToEvict: OnUpgrade, incoming labels/annotations": { featureFlags: "SafeToEvict=true", - safeToEvict: agonesv1.EvictionSafeOnUpgrade, + eviction: &agonesv1.Eviction{Safe: agonesv1.EvictionSafeOnUpgrade}, pod: emptyPodAnd(func(pod *corev1.Pod) { pod.ObjectMeta.Annotations[agonesv1.PodSafeToEvictAnnotation] = "better not touch" pod.ObjectMeta.Labels[agonesv1.SafeToEvictLabel] = "not another one" @@ -102,7 +102,7 @@ func TestSetEviction(t *testing.T) { }, "SafeToEvict: Never, incoming labels/annotations": { featureFlags: "SafeToEvict=true", - safeToEvict: agonesv1.EvictionSafeNever, + eviction: &agonesv1.Eviction{Safe: agonesv1.EvictionSafeNever}, pod: emptyPodAnd(func(pod *corev1.Pod) { pod.ObjectMeta.Annotations[agonesv1.PodSafeToEvictAnnotation] = "a passthrough" pod.ObjectMeta.Labels[agonesv1.SafeToEvictLabel] = "or is it passthru?" @@ -117,7 +117,7 @@ func TestSetEviction(t *testing.T) { err := runtime.ParseFeatures(tc.featureFlags) assert.NoError(t, err) - err = (&generic{}).SetEviction(tc.safeToEvict, tc.pod) + err = (&generic{}).SetEviction(tc.eviction, tc.pod) assert.NoError(t, err) assert.Equal(t, tc.wantPod, tc.pod) }) @@ -192,7 +192,7 @@ func TestGameServerPodAutoscalerAnnotations(t *testing.T) { fixture := &agonesv1.GameServer{ ObjectMeta: metav1.ObjectMeta{Name: "logan"}, Spec: agonesv1.GameServerSpec{Container: "sheep"}, - Status: agonesv1.GameServerStatus{Eviction: agonesv1.Eviction{Safe: agonesv1.EvictionSafeNever}}, + Status: agonesv1.GameServerStatus{Eviction: &agonesv1.Eviction{Safe: agonesv1.EvictionSafeNever}}, } for _, tc := range testCases { t.Run(tc.description, func(t *testing.T) { diff --git a/pkg/cloudproduct/gke/gke.go b/pkg/cloudproduct/gke/gke.go index fff9b84019..7221e8c815 100644 --- a/pkg/cloudproduct/gke/gke.go +++ b/pkg/cloudproduct/gke/gke.go @@ -168,15 +168,18 @@ func podSpecSeccompUnconfined(podSpec *corev1.PodSpec) { // safe-to-evict=false but can support a restrictive PDB, we can support Never and Always, but // OnUpgrade doesn't make sense on Autopilot today. - an overly restrictive PDB prevents // any sort of graceful eviction. -func (*gkeAutopilot) SetEviction(safe agonesv1.EvictionSafe, pod *corev1.Pod) error { +func (*gkeAutopilot) SetEviction(eviction *agonesv1.Eviction, pod *corev1.Pod) error { if !runtime.FeatureEnabled(runtime.FeatureSafeToEvict) { return nil } if safeAnnotation := pod.ObjectMeta.Annotations[agonesv1.PodSafeToEvictAnnotation]; safeAnnotation == agonesv1.False { delete(pod.ObjectMeta.Annotations, agonesv1.PodSafeToEvictAnnotation) } + if eviction == nil { + return errors.New("No eviction value set. Should be the default value") + } if _, exists := pod.ObjectMeta.Labels[agonesv1.SafeToEvictLabel]; !exists { - switch safe { + switch eviction.Safe { case agonesv1.EvictionSafeAlways: // For EvictionSafeAlways, we use a label value that does not match the // agones-gameserver-safe-to-evict-false PDB. But we go ahead and label @@ -186,7 +189,7 @@ func (*gkeAutopilot) SetEviction(safe agonesv1.EvictionSafe, pod *corev1.Pod) er case agonesv1.EvictionSafeNever: pod.ObjectMeta.Labels[agonesv1.SafeToEvictLabel] = agonesv1.False default: - return errors.Errorf("eviction.safe == %s, which webhook should have rejected on Autopilot", safe) + return errors.Errorf("eviction.safe == %s, which webhook should have rejected on Autopilot", eviction.Safe) } } return nil diff --git a/pkg/cloudproduct/gke/gke_test.go b/pkg/cloudproduct/gke/gke_test.go index f4e6665668..d9f78f7542 100644 --- a/pkg/cloudproduct/gke/gke_test.go +++ b/pkg/cloudproduct/gke/gke_test.go @@ -158,7 +158,7 @@ func TestValidateGameServer(t *testing.T) { causes := (&gkeAutopilot{}).ValidateGameServerSpec(&agonesv1.GameServerSpec{ Ports: tc.ports, Scheduling: tc.scheduling, - Eviction: agonesv1.Eviction{Safe: tc.safeToEvict}, + Eviction: &agonesv1.Eviction{Safe: tc.safeToEvict}, }) require.Equal(t, tc.want, causes) }) @@ -223,7 +223,7 @@ func TestSetSafeToEvict(t *testing.T) { } for desc, tc := range map[string]struct { featureFlags string - safeToEvict agonesv1.EvictionSafe + eviction *agonesv1.Eviction pod *corev1.Pod wantPod *corev1.Pod wantErr bool @@ -234,7 +234,7 @@ func TestSetSafeToEvict(t *testing.T) { }, "SafeToEvict: Always, no incoming labels/annotations": { featureFlags: "SafeToEvict=true", - safeToEvict: agonesv1.EvictionSafeAlways, + eviction: &agonesv1.Eviction{Safe: agonesv1.EvictionSafeAlways}, pod: emptyPodAnd(func(*corev1.Pod) {}), wantPod: emptyPodAnd(func(pod *corev1.Pod) { pod.ObjectMeta.Labels[agonesv1.SafeToEvictLabel] = agonesv1.True @@ -242,7 +242,7 @@ func TestSetSafeToEvict(t *testing.T) { }, "SafeToEvict: Never, no incoming labels/annotations": { featureFlags: "SafeToEvict=true", - safeToEvict: agonesv1.EvictionSafeNever, + eviction: &agonesv1.Eviction{Safe: agonesv1.EvictionSafeNever}, pod: emptyPodAnd(func(*corev1.Pod) {}), wantPod: emptyPodAnd(func(pod *corev1.Pod) { pod.ObjectMeta.Labels[agonesv1.SafeToEvictLabel] = agonesv1.False @@ -250,13 +250,13 @@ func TestSetSafeToEvict(t *testing.T) { }, "SafeToEvict: OnUpgrade => error": { featureFlags: "SafeToEvict=true", - safeToEvict: agonesv1.EvictionSafeOnUpgrade, + eviction: &agonesv1.Eviction{Safe: agonesv1.EvictionSafeOnUpgrade}, pod: emptyPodAnd(func(*corev1.Pod) {}), wantErr: true, }, "SafeToEvict: Always, incoming labels/annotations": { featureFlags: "SafeToEvict=true", - safeToEvict: agonesv1.EvictionSafeAlways, + eviction: &agonesv1.Eviction{Safe: agonesv1.EvictionSafeAlways}, pod: emptyPodAnd(func(pod *corev1.Pod) { pod.ObjectMeta.Annotations[agonesv1.PodSafeToEvictAnnotation] = "just don't touch, ok?" pod.ObjectMeta.Labels[agonesv1.SafeToEvictLabel] = "seriously, leave it" @@ -268,7 +268,7 @@ func TestSetSafeToEvict(t *testing.T) { }, "SafeToEvict: Never, incoming labels/annotations": { featureFlags: "SafeToEvict=true", - safeToEvict: agonesv1.EvictionSafeNever, + eviction: &agonesv1.Eviction{Safe: agonesv1.EvictionSafeNever}, pod: emptyPodAnd(func(pod *corev1.Pod) { pod.ObjectMeta.Annotations[agonesv1.PodSafeToEvictAnnotation] = "a passthrough" pod.ObjectMeta.Labels[agonesv1.SafeToEvictLabel] = "or is it passthru?" @@ -280,7 +280,7 @@ func TestSetSafeToEvict(t *testing.T) { }, "SafeToEvict: Never, but safe-to-evict pod annotation set to false": { featureFlags: "SafeToEvict=true", - safeToEvict: agonesv1.EvictionSafeNever, + eviction: &agonesv1.Eviction{Safe: agonesv1.EvictionSafeNever}, pod: emptyPodAnd(func(pod *corev1.Pod) { pod.ObjectMeta.Annotations[agonesv1.PodSafeToEvictAnnotation] = agonesv1.False }), @@ -293,7 +293,7 @@ func TestSetSafeToEvict(t *testing.T) { err := runtime.ParseFeatures(tc.featureFlags) assert.NoError(t, err) - err = (&gkeAutopilot{}).SetEviction(tc.safeToEvict, tc.pod) + err = (&gkeAutopilot{}).SetEviction(tc.eviction, tc.pod) if tc.wantErr { assert.Error(t, err) return diff --git a/pkg/testing/apihooks.go b/pkg/testing/apihooks.go index d591021abc..fb0dab039c 100644 --- a/pkg/testing/apihooks.go +++ b/pkg/testing/apihooks.go @@ -43,6 +43,6 @@ func (f FakeAPIHooks) MutateGameServerPodSpec(_ *agonesv1.GameServerSpec, podSpe } // SetEviction is called by gs.Pod to enforce GameServer.Status.Eviction. -func (f FakeAPIHooks) SetEviction(_ agonesv1.EvictionSafe, pod *corev1.Pod) error { +func (f FakeAPIHooks) SetEviction(_ *agonesv1.Eviction, pod *corev1.Pod) error { return nil } diff --git a/site/content/en/docs/Reference/agones_crd_api_reference.html b/site/content/en/docs/Reference/agones_crd_api_reference.html index f204899db4..74a3ec271a 100644 --- a/site/content/en/docs/Reference/agones_crd_api_reference.html +++ b/site/content/en/docs/Reference/agones_crd_api_reference.html @@ -4379,6 +4379,7 @@

GameServerStatus +(Optional)

(Alpha, SafeToEvict feature flag) Eviction specifies the eviction tolerance of the GameServer.