SDK WatchGameServer logs error on shutdown #3271

Kalaiselvi84 · 2023-07-18T03:39:18Z

What type of PR is this?

Uncomment only one /kind <> line, press enter to put that in a new line, and remove leading whitespace from that line:

/kind breaking

/kind bug

/kind cleanup
/kind documentation
/kind feature
/kind hotfix
/kind release

What this PR does / Why we need it:
Replaced fmt statements with log statements and added a condition to prevent logging when gs.ObjectMeta.DeletionTimestamp is not equal to zero

Which issue(s) this PR fixes:

Closes #3245

Special notes for your reviewer:

agones-bot · 2023-07-18T04:44:49Z

Build Succeeded 👏

Build Id: e6fbbb4f-7855-4029-84b8-8908dffc15ec

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://3a80553-dot-preview-dot-agones-images.appspot.com/

To install this version:

git fetch https://github.com/googleforgames/agones.git pull/3271/head:pr_3271 && git checkout pr_3271
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.34.0-dev-3a80553-amd64

aimuz · 2023-07-18T05:50:05Z

@clairesc for review

markmandel · 2023-07-18T20:23:04Z

sdks/go/sdk.go

 				// This is to wait for the reconnection, and not peg the CPU at 100%.
 				time.Sleep(time.Second)
 				continue
 			}
+
+			if gs.ObjectMeta.DeletionTimestamp != 0 {


🤔

So I would handle this a bit differently. There are two problems we are trying to resolve:

Not output errors here when shutting down

Have a pluggable log system.

So, I would actually make the second part, part of the first - because we still want to handle io.EOF as above, but not output the log.

So I would have a pluggable log as per:
#3245 (comment)

But have a function that is called here where the log functions are, that checks if gs.ObjectMeta.DeletionTimestamp.isZero() (I think that's the right function)` and if not, then runs the pluggable log function.

Does that make sense?

I've included the Logger function to log the error message when gs.ObjectMeta.DeletionTimestamp is not zero and removed log when "err == io.EOF" and any other error occurs while watching the GameServer.

DeletionTimestamp is of type int64, so I added the following condition:
if reflect.ValueOf(gs.ObjectMeta.DeletionTimestamp).IsZero() to check if it is zero or not.
reference

sdks/go/sdk.go

agones-bot · 2023-07-19T00:48:27Z

Build Succeeded 👏

Build Id: 4569518d-cef7-4b6d-bee7-e5d769f3f415

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://8320f71-dot-preview-dot-agones-images.appspot.com/

To install this version:

git fetch https://github.com/googleforgames/agones.git pull/3271/head:pr_3271 && git checkout pr_3271
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.34.0-dev-8320f71-amd64

agones-bot · 2023-07-19T04:57:54Z

Build Succeeded 👏

Build Id: 03f175d6-130d-4014-abc9-91a637d5495a

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://c956122-dot-preview-dot-agones-images.appspot.com/

To install this version:

git fetch https://github.com/googleforgames/agones.git pull/3271/head:pr_3271 && git checkout pr_3271
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.34.0-dev-c956122-amd64

markmandel · 2023-07-19T16:11:49Z

sdks/go/sdk.go

@@ -132,17 +140,22 @@ func (s *SDK) WatchGameServer(f GameServerCallback) error {
 	if err != nil {
 		return errors.Wrap(err, "could not watch gameserver")
 	}
-
+	log := func(gs *sdk.GameServer, msg string, err error) {
+		if time.Unix(gs.ObjectMeta.DeletionTimestamp, 0).IsZero() {


Suggested change

if time.Unix(gs.ObjectMeta.DeletionTimestamp, 0).IsZero() {

if gs.ObjectMeta.DeletionTimestamp.IsZero() {

This should work 🤔 , here is an example - we do this a bunch of places.

agones/pkg/gameservers/controller.go

Line 428 in 80f74c4

if gs.ObjectMeta.DeletionTimestamp.IsZero() {

More examples: https://github.com/search?q=repo%3Agoogleforgames%2Fagones%20IsZero&type=code

Okay, big slice for humble 🥧 for me. I didn't realise this was the protobuf version, not the K8s API version 🤦🏻

So, we can simplify this to:

gs.ObjectMeta.DeletionTimestamp == 0

And that will make everything happy.

Sorry about that!

No worries. You are the best, always!👍🏻

agones-bot · 2023-07-19T18:30:43Z

Build Succeeded 👏

Build Id: 8ba80b50-b2e2-46d1-8cde-b1040636ed2b

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://647b22b-dot-preview-dot-agones-images.appspot.com/

To install this version:

git fetch https://github.com/googleforgames/agones.git pull/3271/head:pr_3271 && git checkout pr_3271
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.34.0-dev-647b22b-amd64

markmandel

LGTM!

@clairesc can you take a quick look, make sure we didn't miss anything?

clairesc · 2023-07-20T21:25:25Z

I wanted to say LGTM but I gave it a quick go and got this exception when the GameServer is shutdown (via Fleet autoscaling)

goroutine 23 [running]:
agones.dev/agones/sdks/go.(*SDK).WatchGameServer.func1(0xc00036f5b0?, {0x146da6b?, 0xc00008ff20?}, {0x0?, 0x0?})
        /go/src/agones/sdks/go/sdk.go:144 +0x23
agones.dev/agones/sdks/go.(*SDK).WatchGameServer.func2()
        /go/src/agones/sdks/go/sdk.go:155 +0x119
created by agones.dev/agones/sdks/go.(*SDK).WatchGameServer
        /go/src/agones/sdks/go/sdk.go:149 +0x14a
panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x28 pc=0x11e0003]

I guess gs is nil when stream.Recv returns an error? (which would makes sense)

markmandel · 2023-07-20T21:39:07Z

sdks/go/sdk.go

+
+// Logger is a pluggable function that outputs the error message to standard error.
+var Logger ErrorLog = func(msg string, err error) {
+	fmt.Fprintf(os.Stderr, "%s: %s\n", msg, err)


Suggested change

fmt.Fprintf(os.Stderr, "%s: %s\n", msg, err)

if gs == nil {

return

}

fmt.Fprintf(os.Stderr, "%s: %s\n", msg, err)

Since @clairesc 's comment is very good point!

can I add the gs == nil check here? WDYT?

log := func(gs *sdk.GameServer, msg string, err error) { if gs == nil || gs.ObjectMeta.DeletionTimestamp == 0 { return } Logger(msg, err) }

Yeah that's way better.

agones-bot · 2023-07-20T22:14:36Z

Build Failed 😱

Build Id: 694a9184-fbd4-496b-ab13-9e0d1f4a84ef

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

markmandel · 2023-07-20T22:55:02Z

I think I fixed this flake. Updating.

agones-bot · 2023-07-21T02:00:14Z

Build Succeeded 👏

Build Id: 89a7745a-8c4a-47b8-aa7d-2c9e2985d319

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://9886604-dot-preview-dot-agones-images.appspot.com/

To install this version:

git fetch https://github.com/googleforgames/agones.git pull/3271/head:pr_3271 && git checkout pr_3271
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.34.0-dev-9886604-amd64

markmandel · 2023-07-21T16:20:28Z

@clairesc look good now?

clairesc · 2023-07-21T19:51:35Z

@clairesc look good now?

Yes LGTM

aimuz · 2023-07-22T03:53:34Z

/lgtm

agones-bot · 2023-07-22T04:25:32Z

Build Succeeded 👏

Build Id: 63e69115-e00c-4daf-9c02-1f99e580f86c

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://f2b6ea9-dot-preview-dot-agones-images.appspot.com/

To install this version:

git fetch https://github.com/googleforgames/agones.git pull/3271/head:pr_3271 && git checkout pr_3271
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.34.0-dev-f2b6ea9-amd64

Kalaiselvi84 · 2023-07-24T16:52:08Z

@markmandel, It looks like the PR didn't get merged automatically after the successful build. Could you kindly perform the merge for this PR?🙂

markmandel

Looks like I forgot to approve. Done!

google-oss-prow · 2023-07-24T21:48:58Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Kalaiselvi84, markmandel

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [markmandel]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Kalaiselvi84 added 2 commits July 18, 2023 03:22

SDK WatchGameServer logs error on shutdown

2d519a0

update-comment

3a80553

Kalaiselvi84 requested a review from markmandel July 18, 2023 03:39

google-oss-prow bot added the size/XS label Jul 18, 2023

github-actions bot added the kind/bug These are bugs. label Jul 18, 2023

google-oss-prow bot requested review from cyriltovena and roberthbailey July 18, 2023 03:39

markmandel reviewed Jul 18, 2023

View reviewed changes

suggested changes in sdk.go

8e2fe04

google-oss-prow bot added size/S and removed size/XS labels Jul 18, 2023

added !

8320f71

markmandel reviewed Jul 18, 2023

View reviewed changes

sdks/go/sdk.go Outdated Show resolved Hide resolved

sdks/go/sdk.go Outdated Show resolved Hide resolved

sdks/go/sdk.go Outdated Show resolved Hide resolved

Recommended modification

c956122

markmandel reviewed Jul 19, 2023

View reviewed changes

Kalaiselvi84 added 2 commits July 19, 2023 17:13

change in condition

cb2e542

Merge branch 'main' into issues/291643455

647b22b

markmandel reviewed Jul 19, 2023

View reviewed changes

Kalaiselvi84 mentioned this pull request Jul 20, 2023

Golang SDK WatchGameServer logs error on shutdown #3245

Closed

markmandel reviewed Jul 20, 2023

View reviewed changes

Kalaiselvi84 added 2 commits July 20, 2023 22:00

check nil pointer for gs

f650b52

Merge branch 'main' into issues/291643455

cf244a3

Merge branch 'main' into issues/291643455

9886604

markmandel enabled auto-merge (squash) July 21, 2023 20:06

Merge branch 'main' into issues/291643455

f2b6ea9

google-oss-prow bot assigned aimuz Jul 22, 2023

google-oss-prow bot added the lgtm label Jul 22, 2023

markmandel approved these changes Jul 24, 2023

View reviewed changes

markmandel merged commit 2f8a583 into googleforgames:main Jul 24, 2023

google-oss-prow bot assigned markmandel Jul 24, 2023

google-oss-prow bot added the approved label Jul 24, 2023

Kalaiselvi84 deleted the issues/291643455 branch March 15, 2024 01:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SDK WatchGameServer logs error on shutdown #3271

SDK WatchGameServer logs error on shutdown #3271

Kalaiselvi84 commented Jul 18, 2023

agones-bot commented Jul 18, 2023

aimuz commented Jul 18, 2023

markmandel Jul 18, 2023

Kalaiselvi84 Jul 18, 2023

agones-bot commented Jul 19, 2023

agones-bot commented Jul 19, 2023

markmandel Jul 19, 2023

markmandel Jul 19, 2023

Kalaiselvi84 Jul 19, 2023

agones-bot commented Jul 19, 2023

markmandel left a comment

clairesc commented Jul 20, 2023

markmandel Jul 20, 2023

Kalaiselvi84 Jul 20, 2023 •

edited

Loading

markmandel Jul 20, 2023

agones-bot commented Jul 20, 2023

markmandel commented Jul 20, 2023

agones-bot commented Jul 21, 2023

markmandel commented Jul 21, 2023

clairesc commented Jul 21, 2023

aimuz commented Jul 22, 2023

agones-bot commented Jul 22, 2023

Kalaiselvi84 commented Jul 24, 2023

markmandel left a comment

google-oss-prow bot commented Jul 24, 2023

	if time.Unix(gs.ObjectMeta.DeletionTimestamp, 0).IsZero() {
	if gs.ObjectMeta.DeletionTimestamp.IsZero() {

SDK WatchGameServer logs error on shutdown #3271

SDK WatchGameServer logs error on shutdown #3271

Conversation

Kalaiselvi84 commented Jul 18, 2023

agones-bot commented Jul 18, 2023

aimuz commented Jul 18, 2023

markmandel Jul 18, 2023

Choose a reason for hiding this comment

Kalaiselvi84 Jul 18, 2023

Choose a reason for hiding this comment

agones-bot commented Jul 19, 2023

agones-bot commented Jul 19, 2023

markmandel Jul 19, 2023

Choose a reason for hiding this comment

markmandel Jul 19, 2023

Choose a reason for hiding this comment

Kalaiselvi84 Jul 19, 2023

Choose a reason for hiding this comment

agones-bot commented Jul 19, 2023

markmandel left a comment

Choose a reason for hiding this comment

clairesc commented Jul 20, 2023

markmandel Jul 20, 2023

Choose a reason for hiding this comment

Kalaiselvi84 Jul 20, 2023 • edited Loading

Choose a reason for hiding this comment

markmandel Jul 20, 2023

Choose a reason for hiding this comment

agones-bot commented Jul 20, 2023

markmandel commented Jul 20, 2023

agones-bot commented Jul 21, 2023

markmandel commented Jul 21, 2023

clairesc commented Jul 21, 2023

aimuz commented Jul 22, 2023

agones-bot commented Jul 22, 2023

Kalaiselvi84 commented Jul 24, 2023

markmandel left a comment

Choose a reason for hiding this comment

google-oss-prow bot commented Jul 24, 2023

Kalaiselvi84 Jul 20, 2023 •

edited

Loading