Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: discard disconnected game server streams #3328

Merged
merged 2 commits into from
Aug 17, 2023

Conversation

antiphp
Copy link
Contributor

@antiphp antiphp commented Aug 16, 2023

What type of PR is this?

/kind feature

What this PR does / Why we need it:

This PR adds the ability for the SDK server to remove closed streams and only log once when a closed stream is found.
GRPC streams take a context, which is forwarded by the SDK, and when that context is cancelled, currently the SDK server starts logging repeatedly about the connection being closed.
With the changes from this pull request, what happens instead is that when a closed stream is found, an error is logged and it is then removed from the connectedStreams slice.
In order to know whether the stream is closed, we check the status of the stream.Context().

Unit tests are updated to cover all relevant cases due to the changes in this PR.

Which issue(s) this PR fixes:

Fixes #3327

Special notes for your reviewer:

The unit test is a copy&paste from the above test, just with an adjustment to test the removal of the stream.

This MR was also tested with a new build image for the sidecar, applied via agones-controller in Kubernetes.
The game server called the sidecar's WatchGameServer, and later cancelled the connection.

The fix reported the context cancellation once and removed the stream from the connected streams.

No more error reports.

@google-oss-prow google-oss-prow bot requested a review from aLekSer August 16, 2023 14:09
@github-actions github-actions bot added the kind/feature New features for Agones label Aug 16, 2023
@agones-bot
Copy link
Collaborator

Build Failed 😱

Build Id: 10bc5578-edd6-4631-b69e-030ab01fa90a

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

@agones-bot
Copy link
Collaborator

Build Failed 😱

Build Id: 27be38fd-286a-4d48-ace8-c5e48cf34159

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

@agones-bot
Copy link
Collaborator

Build Succeeded 👏

Build Id: a0fb9429-48a4-4d16-90ac-f79549f852c9

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):

To install this version:

  • git fetch https://github.com/googleforgames/agones.git pull/3328/head:pr_3328 && git checkout pr_3328
  • 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.35.0-dev-1cdb6c4-amd64

Copy link
Member

@markmandel markmandel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice update! Thanks!

@google-oss-prow
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: antiphp, 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:

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

@google-oss-prow
Copy link

New changes are detected. LGTM label has been removed.

@agones-bot
Copy link
Collaborator

Build Succeeded 👏

Build Id: 7a09118d-4123-4898-85ad-ba2be58335d6

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):

To install this version:

  • git fetch https://github.com/googleforgames/agones.git pull/3328/head:pr_3328 && git checkout pr_3328
  • 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.35.0-dev-c20ddbf-amd64

@markmandel markmandel merged commit a2b9806 into googleforgames:main Aug 17, 2023
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved kind/feature New features for Agones size/M
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add the ability for the SDK server to remove closed streams
3 participants