Changing update list to update list values along with the capacity.

[chrisfoster121]

/kind bug

What this PR does / Why we need it:
This PR updates the UpdateList functionality to update the list values along side the given capacity. This is very important because without it managing game server keys gets very complicated and often causes unwanted race conditions when sending multiple additions or removals to lists in quick succession.

Which issue(s) this PR fixes:
Closes #3870

[google-cla]

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

[agones-bot]

Build Failed 😱

Build Id: 5b639d5e-252c-447e-ba3d-24a6e928ac47

• Cloud Build view
• Cloud Build log download

To get permission to view the Cloud Build view, join the agones-discuss Google Group.

[chrisfoster121]

@igooch This built locally but it looks like the cloud build failed. I am unable to view build:

Troubleshooting info:
  Principal: chrisfosterxvi@gmail.com
  Resource: 258182270954
  Troubleshooting URL: console.cloud.google.com/iam-admin/troubleshooter;permissions=cloudbuild.builds.get;principal=chrisfosterxvi@gmail.com;resources=%2F%2Fcloudresourcemanager.googleapis.com%2Fprojects%2F258182270954/result

Missing permissions:
  cloudbuild.builds.get

[igooch]

Looks like it's failing on make test

            	Test:       	TestSDKServerUpdateList/set_capacity_to_max
            	            	  }
            	            	+ Values: ([]string) {
            	            	-  (string) (len=4) "four"
            	            	-  (string) (len=5) "three",
            	            	-  (string) (len=3) "two",
            	            	-  (string) (len=3) "one",
            	            	- Values: ([]string) (len=4) {
            	            	  Capacity: (int64) 1000,
            	            	@@ -2,7 +2,3 @@
            	            	+++ Actual
            	            	--- Expected
            	            	Diff:
            	            	
            	            	actual  : v1.ListStatus{Capacity:1000, Values:[]string{}}
            	            	expected: v1.ListStatus{Capacity:1000, Values:[]string{"one", "two", "three", "four"}}
            	Error:      	Not equal: 
            	Error Trace:	/go/src/agones.dev/agones/pkg/sdkserver/sdkserver_test.go:1807
        sdkserver_test.go:1807: 
            	Test:       	TestSDKServerUpdateList/set_capacity_to_max
            	            	 }
            	            	+([]string) {
            	            	- (string) (len=4) "four"
            	            	- (string) (len=5) "three",
            	            	- (string) (len=3) "two",
            	            	- (string) (len=3) "one",
            	            	-([]string) (len=4) {
            	            	@@ -1,6 +1,2 @@
            	            	+++ Actual
            	            	--- Expected
            	            	Diff:
            	            	
            	            	actual  : []string{}
            	            	expected: []string{"one", "two", "three", "four"}
            	Error:      	Not equal: 
            	Error Trace:	/go/src/agones.dev/agones/pkg/sdkserver/sdkserver_test.go:1799
        sdkserver_test.go:1799: 
    --- FAIL: TestSDKServerUpdateList/set_capacity_to_max (1.11s)
--- FAIL: TestSDKServerUpdateList (43.96s)

[igooch]

You need to be in https://groups.google.com/a/google.com/g/agones-discuss to get access to the build logs.

Also looks like you're missing the CLA.

[chrisfoster121]

Looks like it's failing on make test

            	Test:       	TestSDKServerUpdateList/set_capacity_to_max
            	            	  }
            	            	+ Values: ([]string) {
            	            	-  (string) (len=4) "four"
            	            	-  (string) (len=5) "three",
            	            	-  (string) (len=3) "two",
            	            	-  (string) (len=3) "one",
            	            	- Values: ([]string) (len=4) {
            	            	  Capacity: (int64) 1000,
            	            	@@ -2,7 +2,3 @@
            	            	+++ Actual
            	            	--- Expected
            	            	Diff:
            	            	
            	            	actual  : v1.ListStatus{Capacity:1000, Values:[]string{}}
            	            	expected: v1.ListStatus{Capacity:1000, Values:[]string{"one", "two", "three", "four"}}
            	Error:      	Not equal: 
            	Error Trace:	/go/src/agones.dev/agones/pkg/sdkserver/sdkserver_test.go:1807
        sdkserver_test.go:1807: 
            	Test:       	TestSDKServerUpdateList/set_capacity_to_max
            	            	 }
            	            	+([]string) {
            	            	- (string) (len=4) "four"
            	            	- (string) (len=5) "three",
            	            	- (string) (len=3) "two",
            	            	- (string) (len=3) "one",
            	            	-([]string) (len=4) {
            	            	@@ -1,6 +1,2 @@
            	            	+++ Actual
            	            	--- Expected
            	            	Diff:
            	            	
            	            	actual  : []string{}
            	            	expected: []string{"one", "two", "three", "four"}
            	Error:      	Not equal: 
            	Error Trace:	/go/src/agones.dev/agones/pkg/sdkserver/sdkserver_test.go:1799
        sdkserver_test.go:1799: 
    --- FAIL: TestSDKServerUpdateList/set_capacity_to_max (1.11s)
--- FAIL: TestSDKServerUpdateList (43.96s)

I am looking at that now and I see the following:

"set capacity to max": {
			listName: "foo",
			request: &beta.UpdateListRequest{
				List: &beta.List{
					Name:     "foo",
					Capacity: int64(1000),
				},
				UpdateMask: &fieldmaskpb.FieldMask{Paths: []string{"capacity"}},
			},
			want:                    agonesv1.ListStatus{Values: []string{"one", "two", "three", "four"}, Capacity: int64(1000)},
			updateErr:               false,
			updated:                 true,
			expectedUpdatesQueueLen: 0,
		},

If I am not mistaken it looks like the test is expecting the return to have values associated with it. In all my testing of the agones SDK I have not been able to get any update list call to return the correct data. I think this is due to the fact that this gets put into a queue to get processed asynchronously, for example RemoveListValue also returns an empty object.

Would adjusting the test to match this pattern be a reasonable approach?

[agones-bot]

Build Failed 😱

Build Id: e76e1f1a-1713-4f6a-b56f-b9f54d05d847

• Cloud Build view
• Cloud Build log download

To get permission to view the Cloud Build view, join the agones-discuss Google Group.

[igooch]

The test is failing on lines Error Trace: /go/src/agones.dev/agones/pkg/sdkserver/sdkserver_test.go:1807 and Error Trace: /go/src/agones.dev/agones/pkg/sdkserver/sdkserver_test.go:1799.

These are checking that the values from GetList and the K8s API are as expected after the update. An important line in the test is UpdateMask: &fieldmaskpb.FieldMask{Paths: []string{"capacity"}}, which means that the request is only updating the capacity, and not the list values, so the list values should not change.

agones/pkg/sdkserver/sdkserver_test.go

Lines 1797 to 1800 in 31ed93e

	got, err := sc.GetList(context.Background(), &beta.GetListRequest{Name: testCase.listName})
	assert.NoError(t, err)
	assert.Equal(t, testCase.want.Values, got.Values)
	assert.Equal(t, testCase.want.Capacity, got.Capacity)

agones/pkg/sdkserver/sdkserver_test.go

Lines 1805 to 1809 in 31ed93e

	case value := <-updated:
	assert.NotNil(t, value[testCase.listName])
	assert.Equal(t,
	agonesv1.ListStatus{Values: testCase.want.Values, Capacity: testCase.want.Capacity},
	value[testCase.listName])

From taking a quick look at your code it doesn't use the fieldMask values. You can take a look at the localsdkserver code which uses the field masks.