Delete List Value(s) on Game Server Allocation (#4054)

* Adds DeleteValues as a ListAction during allocation

* Generated Files

* Adds tests

* Adds DeleteValues documentation

* Updates tabs to spaces per protobuf style guide

* Generate Rust file

* Adds unit test for DeleteListValues

examples/gameserverallocation.yaml

@@ -116,3 +116,6 @@ spec:
         - x7un
         - 8inz
       capacity: 40  # Updates the maximum capacity of the Counter to this number. Min 0, Max 1000.
+      deleteValues:  # removes values from a List's Valules array. Any nonexistant values are ignored.
+        - alice
+        - bob

pkg/allocation/converters/converter.go

@@ -615,6 +615,12 @@ func convertAllocationListsToGSAListActions(in map[string]*pb.ListAction) map[st
 			copy(copyValues, addValues)
 			la.AddValues = copyValues
 		}
+		if v.DeleteValues != nil {
+			deleteValues := v.GetDeleteValues()
+			copyValues := make([]string, len(deleteValues))
+			copy(copyValues, deleteValues)
+			la.DeleteValues = copyValues
+		}
 		if v.Capacity != nil {
 			capacity := v.Capacity.GetValue()
 			la.Capacity = &capacity
@@ -639,6 +645,11 @@ func convertGSAListActionsToAllocationLists(in map[string]allocationv1.ListActio
 			copy(copyValues, v.AddValues)
 			la.AddValues = copyValues
 		}
+		if v.DeleteValues != nil {
+			copyValues := make([]string, len(v.DeleteValues))
+			copy(copyValues, v.DeleteValues)
+			la.DeleteValues = copyValues
+		}
 		if v.Capacity != nil {
 			la.Capacity = wrapperspb.Int64(*v.Capacity)
 		}

pkg/allocation/converters/converter_test.go

@@ -135,8 +135,9 @@ func TestConvertAllocationRequestToGameServerAllocation(t *testing.T) {
 				},
 				Lists: map[string]*pb.ListAction{
 					"p": {
-						AddValues: []string{"foo", "bar", "baz"},
-						Capacity:  wrapperspb.Int64(10),
+						AddValues:    []string{"foo", "bar", "baz"},
+						Capacity:     wrapperspb.Int64(10),
+						DeleteValues: []string{"alice", "bob", "cat"},
 					},
 				},
 				Scheduling: pb.AllocationRequest_Packed,
@@ -217,8 +218,9 @@ func TestConvertAllocationRequestToGameServerAllocation(t *testing.T) {
 					},
 					Lists: map[string]allocationv1.ListAction{
 						"p": {
-							AddValues: []string{"foo", "bar", "baz"},
-							Capacity:  &ten,
+							AddValues:    []string{"foo", "bar", "baz"},
+							Capacity:     &ten,
+							DeleteValues: []string{"alice", "bob", "cat"},
 						},
 					},
 					Selectors: []allocationv1.GameServerSelector{
@@ -742,6 +744,9 @@ func TestConvertGSAToAllocationRequest(t *testing.T) {
 						"d": {
 							Capacity: &two,
 						},
+						"c": {
+							DeleteValues: []string{"good", "bye"},
+						},
 					},
 					Scheduling: apis.Distributed,
 				},
@@ -821,6 +826,9 @@ func TestConvertGSAToAllocationRequest(t *testing.T) {
 					"d": {
 						Capacity: wrapperspb.Int64(two),
 					},
+					"c": {
+						DeleteValues: []string{"good", "bye"},
+					},
 				},
 			},
 		},

pkg/allocation/go/allocation.swagger.json

@@ -351,9 +351,15 @@
         "capacity": {
           "type": "string",
           "format": "int64"
+        },
+        "deleteValues": {
+          "type": "array",
+          "items": {
+            "type": "string"
+          }
         }
       },
-      "description": "ListAction is an optional action that can be performed on a List at allocation.\nAddValues: Append values to a List's Values array (optional). Any duplicate values will be ignored.\nCapacity: Update the maximum capacity of the Counter to this number (optional). Min 0, Max 1000."
+      "description": "ListAction is an optional action that can be performed on a List at allocation.\nAddValues: Append values to a List's Values array (optional). Any duplicate values will be ignored.\nCapacity: Update the maximum capacity of the Counter to this number (optional). Min 0, Max 1000.\nDeleteValues: Remove values from a List's Values array (optional). Any nonexistant values will be ignored."
     },
     "allocationListSelector": {
       "type": "object",

pkg/apis/agones/v1/gameserver.go

@@ -1026,6 +1026,37 @@ func (gs *GameServer) AppendListValues(name string, values []string) error {
 	return errors.Errorf("unable to AppendListValues: Name %s, Values %s. List not found in GameServer %s", name, values, gs.ObjectMeta.GetName())
 }
 
+// DeleteListValues removes values from the ListStatus Values list. Values in the DeleteListValues
+// list that are not in the ListStatus Values list are ignored.
+func (gs *GameServer) DeleteListValues(name string, values []string) error {
+	if values == nil {
+		return errors.Errorf("unable to DeleteListValues: Name %s, Values %s. Values must not be nil", name, values)
+	}
+	if list, ok := gs.Status.Lists[name]; ok {
+		deleteValuesMap := make(map[string]bool)
+		for _, value := range values {
+			deleteValuesMap[value] = true
+		}
+		newList := deleteValues(list.Values, deleteValuesMap)
+		list.Values = newList
+		gs.Status.Lists[name] = list
+		return nil
+	}
+	return errors.Errorf("unable to DeleteListValues: Name %s, Values %s. List not found in GameServer %s", name, values, gs.ObjectMeta.GetName())
+}
+
+// deleteValues returns a new list with all the values in valuesList that are not keys in deleteValuesMap.
+func deleteValues(valuesList []string, deleteValuesMap map[string]bool) []string {
+	newValuesList := []string{}
+	for _, value := range valuesList {
+		if _, ok := deleteValuesMap[value]; ok {
+			continue
+		}
+		newValuesList = append(newValuesList, value)
+	}
+	return newValuesList
+}
+
 // truncateList truncates the list to the given capacity
 func truncateList(capacity int64, list []string) []string {
 	if list == nil || len(list) <= int(capacity) {

pkg/apis/agones/v1/gameserver_test.go

@@ -2223,6 +2223,63 @@ func TestGameServerAppendListValues(t *testing.T) {
 	}
 }
 
+func TestGameServerDeleteListValues(t *testing.T) {
+	t.Parallel()
+
+	testCases := map[string]struct {
+		gs      GameServer
+		name    string
+		want    ListStatus
+		values  []string
+		wantErr bool
+	}{
+		"list not in game server no-op and error": {
+			gs: GameServer{Status: GameServerStatus{
+				Lists: map[string]ListStatus{
+					"foos": {
+						Values:   []string{"foo", "bar", "bax"},
+						Capacity: 100,
+					},
+				},
+			}},
+			name:    "foo",
+			values:  []string{"bar", "baz"},
+			wantErr: true,
+		},
+		"delete list value - one value not present": {
+			gs: GameServer{Status: GameServerStatus{
+				Lists: map[string]ListStatus{
+					"foo": {
+						Values:   []string{"foo", "bar", "bax"},
+						Capacity: 100,
+					},
+				},
+			}},
+			name:    "foo",
+			values:  []string{"bar", "baz"},
+			wantErr: false,
+			want: ListStatus{
+				Values:   []string{"foo", "bax"},
+				Capacity: 100,
+			},
+		},
+	}
+
+	for test, testCase := range testCases {
+		t.Run(test, func(t *testing.T) {
+			err := testCase.gs.DeleteListValues(testCase.name, testCase.values)
+			if err != nil {
+				assert.True(t, testCase.wantErr)
+			} else {
+				assert.False(t, testCase.wantErr)
+			}
+			if list, ok := testCase.gs.Status.Lists[testCase.name]; ok {
+				assert.Equal(t, testCase.want, list)
+			}
+		})
+	}
+}
+
 func TestMergeRemoveDuplicates(t *testing.T) {
 	t.Parallel()
 

pkg/apis/allocation/v1/gameserverallocation.go

@@ -209,6 +209,9 @@ type ListAction struct {
 	// AddValues appends values to a List's Values array. Any duplicate values will be ignored.
 	// +optional
 	AddValues []string `json:"addValues,omitempty"`
+	// DeleteValues removes values from a List's Values array. Any nonexistant values will be ignored.
+	// +optional
+	DeleteValues []string `json:"deleteValues,omitempty"`
 	// Capacity updates the maximum capacity of the Counter to this number. Min 0, Max 1000.
 	// +optional
 	Capacity *int64 `json:"capacity,omitempty"`
@@ -349,6 +352,12 @@ func (la *ListAction) ListActions(list string, gs *agonesv1.GameServer) error {
 			errs = errors.Join(errs, cntErr)
 		}
 	}
+	if len(la.DeleteValues) > 0 {
+		cntErr := gs.DeleteListValues(list, la.DeleteValues)
+		if cntErr != nil {
+			errs = errors.Join(errs, cntErr)
+		}
+	}
 	return errs
 }
 

pkg/apis/allocation/v1/gameserverallocation_test.go

@@ -1019,20 +1019,21 @@ func TestGameServerListActions(t *testing.T) {
 		},
 		"update list values and capacity": {
 			la: ListAction{
-				AddValues: []string{"magician1", "magician3"},
-				Capacity:  int64Pointer(42),
+				AddValues:    []string{"magician1", "magician3"},
+				Capacity:     int64Pointer(42),
+				DeleteValues: []string{"magician2", "magician5", "magician6"},
 			},
 			list: "magicians",
 			gs: &agonesv1.GameServer{Status: agonesv1.GameServerStatus{
 				Lists: map[string]agonesv1.ListStatus{
 					"magicians": {
-						Values:   []string{"magician1", "magician2"},
+						Values:   []string{"magician1", "magician2", "magician4", "magician5"},
 						Capacity: 100,
 					}}}},
 			want: &agonesv1.GameServer{Status: agonesv1.GameServerStatus{
 				Lists: map[string]agonesv1.ListStatus{
 					"magicians": {
-						Values:   []string{"magician1", "magician2", "magician3"},
+						Values:   []string{"magician1", "magician4", "magician3"},
 						Capacity: 42,
 					}}}},
 			wantErr: false,
@@ -1263,7 +1264,8 @@ func TestValidateListActions(t *testing.T) {
 					Capacity: int64Pointer(0),
 				},
 				"baz": {
-					AddValues: []string{},
+					AddValues:    []string{},
+					DeleteValues: []string{"good", "bye"},
 				},
 			},
 			wantErr: false,

pkg/gameserverallocations/allocator_test.go

@@ -309,7 +309,7 @@ func TestAllocatorApplyAllocationToGameServerCountsListsActions(t *testing.T) {
 		Status: agonesv1.GameServerStatus{NodeName: "node1", State: agonesv1.GameServerStateReady,
 			Lists: map[string]agonesv1.ListStatus{
 				"players": {
-					Values:   []string{},
+					Values:   []string{"alice", "bob", "cat"},
 					Capacity: 100,
 				},
 			},
@@ -339,7 +339,7 @@ func TestAllocatorApplyAllocationToGameServerCountsListsActions(t *testing.T) {
 		wantCounters map[string]agonesv1.CounterStatus
 		wantLists    map[string]agonesv1.ListStatus
 	}{
-		"CounterActions increment and ListActions append and update capacity": {
+		"CounterActions increment and ListActions add, delete, and update capacity": {
 			features: fmt.Sprintf("%s=true", runtime.FeatureCountsAndLists),
 			gs:       &gs1,
 			gsa: &allocationv1.GameServerAllocation{
@@ -356,8 +356,9 @@ func TestAllocatorApplyAllocationToGameServerCountsListsActions(t *testing.T) {
 						}},
 					Lists: map[string]allocationv1.ListAction{
 						"players": {
-							AddValues: []string{"x7un", "8inz"},
-							Capacity:  &FORTY,
+							AddValues:    []string{"x7un", "8inz"},
+							Capacity:     &FORTY,
+							DeleteValues: []string{"bob"},
 						}}}},
 			wantCounters: map[string]agonesv1.CounterStatus{
 				"rooms": {
@@ -366,7 +367,7 @@ func TestAllocatorApplyAllocationToGameServerCountsListsActions(t *testing.T) {
 				}},
 			wantLists: map[string]agonesv1.ListStatus{
 				"players": {
-					Values:   []string{"x7un", "8inz"},
+					Values:   []string{"alice", "cat", "x7un", "8inz"},
 					Capacity: 40,
 				}},
 		},