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

Flaky: TestGameServerReserve #1565

Merged

Conversation

markmandel
Copy link
Member

What type of PR is this?

Uncomment only one /kind <> line, hit 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

What this PR does / Why we need it:
Trying to test transitive state while it is actively changing is fraught with peril. Tests flake, hair is pulled, and screams echo throughout the lands.

This PR allows one to send a duration to the RESERVE command to udp-simple, as well as update TestGameServerReserve to only confirm concrete states, which therefore can't flake.

Which issue(s) this PR fixes:

Closes #1543

Special notes for your reviewer:

Tests will fail, as image does not yet exist. Once initial reviews are complete, I'll push up the new udp-simple image.

@markmandel markmandel added kind/bug These are bugs. area/tests Unit tests, e2e tests, anything to make sure things don't break labels May 17, 2020
@agones-bot
Copy link
Collaborator

Build Failed 😱

Build Id: 82f1d240-d5ec-4b72-b25e-3d5787daf7aa

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

@markmandel markmandel force-pushed the flaky/TestGameServerReserve-5 branch from 3aed700 to de6b01e Compare May 18, 2020 00:09
@agones-bot
Copy link
Collaborator

Build Failed 😱

Build Id: 2d8d2e20-6781-48c3-be24-1a488cffe7ec

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

Copy link
Member

@roberthbailey roberthbailey left a comment

Choose a reason for hiding this comment

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

The good news is that the test failure wasn't in the flake you were trying to fix:

FAIL: TestAllocator (304.57s)

examples/simple-udp/main.go Outdated Show resolved Hide resolved
@markmandel markmandel force-pushed the flaky/TestGameServerReserve-5 branch 2 times, most recently from 1926502 to 2dd1475 Compare May 18, 2020 17:34
@markmandel
Copy link
Member Author

Image has been pushed up. This was making me climb up the walls with frustration. Feels like it's failing on just about every run.

@@ -144,7 +145,17 @@ func readWriteLoop(conn net.PacketConn, stop chan struct{}, s *sdk.SDK) {
allocate(s)

case "RESERVE":
reserve(s)
if len(parts) == 2 {
Copy link
Member

Choose a reason for hiding this comment

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

Sorry I didn't catch this last time, but it would be a bit more idiomatic to do:

if len(parts) != 2 {
  respond()
  continue
}
if dur, err := ... ; err != nil {
  respond()
  continue
}
reserve(s, dur)

so that the non-error flow isn't indented quite so much.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah that is nicer. I'll make that change.

@roberthbailey roberthbailey self-assigned this May 18, 2020
@markmandel markmandel force-pushed the flaky/TestGameServerReserve-5 branch from 2dd1475 to 870bf21 Compare May 18, 2020 17:59
@agones-bot
Copy link
Collaborator

Build Failed 😱

Build Id: 47517cbe-d137-4c2b-bbea-da1605ac7667

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

@agones-bot
Copy link
Collaborator

Build Failed 😱

Build Id: bb256476-006c-47de-b0b2-7d79788682d3

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

@markmandel
Copy link
Member Author

Would help if I actually pushed the right image.

@agones-bot
Copy link
Collaborator

Build Failed 😱

Build Id: ebf7437c-4978-4c96-9efb-0d69d2176a14

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

Trying to test transitive state while it is actively changing is
fraught with peril. Tests flake, hair is pulled, and screams echo
throughout the lands.

This PR allows one to send a duration to the RESERVE command to
udp-simple, as well as update TestGameServerReserve to only confirm
concrete states, which therefore can't flake.

Closes googleforgames#1543
Included instructions on how to update.
@@ -456,72 +456,81 @@ func TestAutoscalerWebhook(t *testing.T) {
assert.True(t, found, "Expected error was not received")
}

// Instructions: https://agones.dev/site/docs/getting-started/create-webhook-fleetautoscaler/#chapter-2-configuring-https-fleetautoscaler-webhook-with-ca-bundle
Copy link
Member Author

Choose a reason for hiding this comment

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

fyi @aLekSer , I updated the certs here, and added a comment on how to update and when expiry should happen.

@agones-bot
Copy link
Collaborator

Build Succeeded 👏

Build Id: d5d5d152-9e8b-4c10-96c5-ce139bc7fc82

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/GoogleCloudPlatform/agones.git pull/1565/head:pr_1565 && git checkout pr_1565
  • helm install ./install/helm/agones --namespace agones-system --name agones --set agones.image.tag=1.6.0-cf3d23f

Copy link
Collaborator

@aLekSer aLekSer left a comment

Choose a reason for hiding this comment

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

Thanks for the fix of TestTlsWebhook.

@google-oss-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: aLekSer, markmandel, roberthbailey

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,roberthbailey]

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

@markmandel markmandel merged commit 96a5ff5 into googleforgames:master May 18, 2020
@markmandel markmandel deleted the flaky/TestGameServerReserve-5 branch May 18, 2020 21:05
@markmandel markmandel added this to the 1.6.0 milestone May 18, 2020
ilkercelikyilmaz pushed a commit to ilkercelikyilmaz/agones that referenced this pull request Oct 23, 2020
* Flaky: TestGameServerReserve

Trying to test transitive state while it is actively changing is
fraught with peril. Tests flake, hair is pulled, and screams echo
throughout the lands.

This PR allows one to send a duration to the RESERVE command to
udp-simple, as well as update TestGameServerReserve to only confirm
concrete states, which therefore can't flake.

Closes googleforgames#1543

* Updated certs for TestTlsWebhook

Included instructions on how to update.
ilkercelikyilmaz pushed a commit to ilkercelikyilmaz/agones that referenced this pull request Oct 23, 2020
* Flaky: TestGameServerReserve

Trying to test transitive state while it is actively changing is
fraught with peril. Tests flake, hair is pulled, and screams echo
throughout the lands.

This PR allows one to send a duration to the RESERVE command to
udp-simple, as well as update TestGameServerReserve to only confirm
concrete states, which therefore can't flake.

Closes googleforgames#1543

* Updated certs for TestTlsWebhook

Included instructions on how to update.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved area/tests Unit tests, e2e tests, anything to make sure things don't break kind/bug These are bugs. lgtm size/L
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Flaky: TestGameServerReserve
5 participants