fix(SdkList): fix list delete values panic #3615

GStones · 2024-01-27T03:45:47Z

What type of PR is this?
/kind bug

What this PR does / Why we need it:
It will make Sdk sidecar panic
Which issue(s) this PR fixes:

google-cla · 2024-01-27T03:45:52Z

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 · 2024-01-27T04:51:20Z

Build Succeeded 👏

Build Id: d5214018-41c1-43ef-be5b-1f278643961c

The following development artifacts have been built, and will exist for the next 30 days:

A preview of the website (the last 30 builds are retained):

https://23913d2-dot-preview-dot-agones-images.appspot.com/

To install this version:

git fetch https://github.com/googleforgames/agones.git pull/3615/head:pr_3615 && git checkout pr_3615
helm install agones ./install/helm/agones --namespace agones-system --set agones.image.registry=us-docker.pkg.dev/agones-images/ci --set agones.image.tag=1.38.0-dev-23913d2-amd64

agones-bot · 2024-01-27T08:15:04Z

Build Failed 😱

Build Id: 406745f3-3460-459c-a5e6-5b348ebfd0bd

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

jlory · 2024-01-27T13:17:12Z

I saw that issue as well, as you said it's fixed by the other mr, I would say that it's still dangerous to have possible negative capacity so it should be fixed however I'm not sure there is a real performance improvement by trying to prealocate the map.

GStones · 2024-01-27T13:55:06Z

I saw that issue as well, as you said it's fixed by the other mr, I would say that it's still dangerous to have possible negative capacity so it should be fixed however I'm not sure there is a real performance improvement by trying to prealocate the map.

Here is preallocate a slice, I think it can be removed,because the list max available is 1000, slice grow has limit(1024).

markmandel · 2024-01-30T05:04:14Z

pkg/sdkserver/sdkserver.go

-	for i, val := range valuesList {
-		if _, ok := toDeleteValues[val]; !ok {
-			newList = append(newList, valuesList[i])
+	newValuesList := make([]string, 0, len(valuesList))


Possibly easier suggestion for defensive programming.

Suggested change

newValuesList := make([]string, 0, len(valuesList))

newLen = len(valuesList)-len(toDeleteValues)

if newLen < 0 {

newLen = 0

}

newList := make([]string, 0, newLen)

And then we continue on as before.

Or I think as you said, make newValueList := []string{} and let it expand naturally - which is a bit more readable IMHO, but I don't have strong opinions about either approach.

Thanks for your suggestions! Already fixed, please review

…gones into fix-list-remove-values

agones-bot · 2024-01-31T03:48:39Z

Build Failed 😱

Build Id: 6fca4e56-da57-434a-a57f-5d90dea4f9bd

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

markmandel · 2024-01-31T03:58:38Z

I see this failure in the unit tests:

--- FAIL: TestSDKServerRemoveListValue (28.53s)
    --- FAIL: TestSDKServerRemoveListValue/Remove_all_values (1.11s)
        sdkserver_test.go:1618: 
            	Error Trace:	/go/src/agones.dev/agones/pkg/sdkserver/sdkserver_test.go:1618
            	Error:      	Not equal: 
            	            	expected: []string{}
            	            	actual  : []string(nil)
            	            	
            	            	Diff:
            	            	--- Expected
            	            	+++ Actual
            	            	@@ -1,3 +1,2 @@
            	            	-([]string) {
            	            	-}
            	            	+([]string) <nil>
            	            	 
            	Test:       	TestSDKServerRemoveListValue/Remove_all_values
        sdkserver_test.go:1626: 
            	Error Trace:	/go/src/agones.dev/agones/pkg/sdkserver/sdkserver_test.go:1626
            	Error:      	Not equal: 
            	            	expected: v1.ListStatus{Capacity:100, Values:[]string{}}
            	            	actual  : v1.ListStatus{Capacity:100, Values:[]string(nil)}
            	            	
            	            	Diff:
            	            	--- Expected
            	            	+++ Actual
            	            	@@ -2,4 +2,3 @@
            	            	  Capacity: (int64) 100,
            	            	- Values: ([]string) {
            	            	- }
            	            	+ Values: ([]string) <nil>
            	            	 }
            	Test:       	TestSDKServerRemoveListValue/Remove_all_values

Do you see the same thing locally, or is this a flake?

…gones into fix-list-remove-values

agones-bot · 2024-01-31T07:24:53Z

Build Failed 😱

Build Id: fd39d453-c4c3-42b3-9f3a-f0a1c3d1c562

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

markmandel · 2024-01-31T07:27:52Z

--- FAIL: TestSDKServerAddListValue (31.96s)
    --- FAIL: TestSDKServerAddListValue/Add_multiple_values_past_capacity (1.12s)
        sdkserver_test.go:1477: 
            	Error Trace:	/go/src/agones.dev/agones/pkg/sdkserver/sdkserver_test.go:1477
            	Error:      	Not equal: 
            	            	expected: v1.ListStatus{Capacity:10, Values:[]string{"one", "two", "three", "four", "five", "six", "seven", "eight", "nine", "ten"}}
            	            	actual  : v1.ListStatus{Capacity:10, Values:[]string{"one", "two", "three", "four", "five", "six", "seven", "eight", "nine"}}
            	            	
            	            	Diff:
            	            	--- Expected
            	            	+++ Actual
            	            	@@ -2,3 +2,3 @@
            	            	  Capacity: (int64) 10,
            	            	- Values: ([]string) (len=10) {
            	            	+ Values: ([]string) (len=9) {
            	            	   (string) (len=3) "one",
            	            	@@ -11,4 +11,3 @@
            	            	   (string) (len=5) "eight",
            	            	-  (string) (len=4) "nine",
            	            	-  (string) (len=3) "ten"
            	            	+  (string) (len=4) "nine"
            	            	  }
            	Test:       	TestSDKServerAddListValue/Add_multiple_values_past_capacity
        sdkserver_test.go:1487: 
            	Error Trace:	/go/src/agones.dev/agones/pkg/sdkserver/sdkserver_test.go:1487
            	Error:      	Not equal: 
            	            	expected: 0
            	            	actual  : 1
            	Test:       	TestSDKServerAddListValue/Add_multiple_values_past_capacity

Looks like this flake is hitting us hard. We should dig into this one @igooch

markmandel · 2024-02-01T04:52:34Z

Okay I can at least replicate this! It's going to only happen under load.

So to replicate, we drop in a time.Sleep(time.Second) to replicate load in the for loop.

			for i, req := range testCase.requests {
				_, err = sc.AddListValue(context.Background(), req)
				if testCase.updateErrs[i] {
					assert.Error(t, err)
				} else {
					assert.NoError(t, err)
				}
				time.Sleep(time.Second)
			}

/cc @igooch in case you see something I'm missing.

markmandel · 2024-02-01T05:11:08Z

Got it! Fixed. PR incoming shortly.

markmandel · 2024-02-01T05:34:12Z

Whoops, was writing comments on the wrong issue! 🤦🏻

Either way #3627 will fix the flake.

agones-bot · 2024-02-01T09:48:29Z

Build Succeeded 👏

Build Id: 4fded0b4-d6e3-455d-816e-ace22f645b32

The following development artifacts have been built, and will exist for the next 30 days:

A preview of the website (the last 30 builds are retained):

https://0fbde76-dot-preview-dot-agones-images.appspot.com/

To install this version:

git fetch https://github.com/googleforgames/agones.git pull/3615/head:pr_3615 && git checkout pr_3615
helm install agones ./install/helm/agones --namespace agones-system --set agones.image.registry=us-docker.pkg.dev/agones-images/ci --set agones.image.tag=1.39.0-dev-0fbde76-amd64

fix(SdkList): fix list delete values panic

23913d2

github-actions bot added kind/other size/S and removed kind/other labels Jan 27, 2024

Merge branch 'main' into fix-list-remove-values

6f23c34

github-actions bot added the kind/other label Jan 27, 2024

markmandel reviewed Jan 30, 2024

View reviewed changes

GStones added 2 commits January 31, 2024 11:28

fix(SdkList): fix delete values

2491c7a

Merge branch 'fix-list-remove-values' of https://github.com/GStones/a…

f3e815d

…gones into fix-list-remove-values

github-actions bot added the kind/bug These are bugs. label Jan 31, 2024

Merge branch 'main' into fix-list-remove-values

b81be63

GStones added 2 commits January 31, 2024 15:09

fix(SdkList): fix delete values

f486231

Merge branch 'fix-list-remove-values' of https://github.com/GStones/a…

65ee4fb

…gones into fix-list-remove-values

markmandel mentioned this pull request Feb 1, 2024

Flaky: unit test TestSDKServerAddListValue #3622

Closed

Merge branch 'main' into fix-list-remove-values

0fbde76

markmandel approved these changes Feb 1, 2024

View reviewed changes

markmandel merged commit 5f58fce into googleforgames:main Feb 1, 2024
4 checks passed

markmandel added kind/cleanup Refactoring code, fixing up documentation, etc and removed kind/other labels Feb 1, 2024

GStones deleted the fix-list-remove-values branch February 1, 2024 11:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(SdkList): fix list delete values panic #3615

fix(SdkList): fix list delete values panic #3615

GStones commented Jan 27, 2024 •

edited

Loading

google-cla bot commented Jan 27, 2024

agones-bot commented Jan 27, 2024

agones-bot commented Jan 27, 2024

jlory commented Jan 27, 2024

GStones commented Jan 27, 2024

markmandel Jan 30, 2024

GStones Jan 31, 2024

agones-bot commented Jan 31, 2024

markmandel commented Jan 31, 2024

agones-bot commented Jan 31, 2024

markmandel commented Jan 31, 2024

markmandel commented Feb 1, 2024

markmandel commented Feb 1, 2024

markmandel commented Feb 1, 2024

agones-bot commented Feb 1, 2024

-	newValuesList := make([]string, 0, len(valuesList))
+  newLen = len(valuesList)-len(toDeleteValues)
+  if newLen < 0 {
+    newLen = 0
+  }
+	newList := make([]string, 0, newLen)

fix(SdkList): fix list delete values panic #3615

fix(SdkList): fix list delete values panic #3615

Conversation

GStones commented Jan 27, 2024 • edited Loading

google-cla bot commented Jan 27, 2024

agones-bot commented Jan 27, 2024

agones-bot commented Jan 27, 2024

jlory commented Jan 27, 2024

GStones commented Jan 27, 2024

markmandel Jan 30, 2024

Choose a reason for hiding this comment

GStones Jan 31, 2024

Choose a reason for hiding this comment

agones-bot commented Jan 31, 2024

markmandel commented Jan 31, 2024

agones-bot commented Jan 31, 2024

markmandel commented Jan 31, 2024

markmandel commented Feb 1, 2024

markmandel commented Feb 1, 2024

markmandel commented Feb 1, 2024

agones-bot commented Feb 1, 2024

GStones commented Jan 27, 2024 •

edited

Loading