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

Flakiness: TestGameServerAllocationDeletionOnUnAllocate #1328

Conversation

markmandel
Copy link
Member

@markmandel markmandel commented Feb 8, 2020

Realised that Go test will compile and run tests in parallel per file,
which means we could shutdown the controller while an allocation is in
process - which means, it will fail completely, as the Kubernetes API
cannot reach the controller's http endpoint for allocation.

Split the controller tests into a separate package which could be run
with -parallel=1 so that there can be no race condition.

Also did some grouping of e2e test functionality to reduce code
repetition.

Closes #1326

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

Build Succeeded 👏

Build Id: 38d4ccc6-d19f-4f1f-89ba-1120c2416aab

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/1328/head:pr_1328 && git checkout pr_1328
  • helm install ./install/helm/agones --namespace agones-system --name agones --set agones.image.tag=1.4.0-90c0f81

@agones-bot
Copy link
Collaborator

Build Succeeded 👏

Build Id: c7752cf5-faf8-449f-afae-eeb9056c45f1

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/1328/head:pr_1328 && git checkout pr_1328
  • helm install ./install/helm/agones --namespace agones-system --name agones --set agones.image.tag=1.4.0-90c0f81

@agones-bot
Copy link
Collaborator

Build Succeeded 👏

Build Id: 56c9a35f-385e-411e-8eb6-cb78fff75d40

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/1328/head:pr_1328 && git checkout pr_1328
  • helm install ./install/helm/agones --namespace agones-system --name agones --set agones.image.tag=1.4.0-90c0f81

@agones-bot
Copy link
Collaborator

Build Failed 😱

Build Id: 7e56763e-881f-42b6-9716-60df13350e64

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

@markmandel
Copy link
Member Author

Got bit by different flaky bug #1276 - running again.

@markmandel markmandel linked an issue Feb 8, 2020 that may be closed by this pull request
@agones-bot
Copy link
Collaborator

Build Succeeded 👏

Build Id: 91da735d-9b25-46c4-8b66-538aa59682ff

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/1328/head:pr_1328 && git checkout pr_1328
  • helm install ./install/helm/agones --namespace agones-system --name agones --set agones.image.tag=1.4.0-c613c8c

@markmandel
Copy link
Member Author

Running a bunch of tests to make sure it's working, and if that passes, will cleanup and submit.

@agones-bot
Copy link
Collaborator

Build Succeeded 👏

Build Id: d4871666-6079-4b34-b11e-0aeb86433870

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/1328/head:pr_1328 && git checkout pr_1328
  • helm install ./install/helm/agones --namespace agones-system --name agones --set agones.image.tag=1.4.0-c613c8c

@agones-bot
Copy link
Collaborator

Build Succeeded 👏

Build Id: 6b119ad8-3c68-4f60-b748-95aef181f5b6

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/1328/head:pr_1328 && git checkout pr_1328
  • helm install ./install/helm/agones --namespace agones-system --name agones --set agones.image.tag=1.4.0-c613c8c

@markmandel markmandel force-pushed the flaky/TestGameServerAllocationDeletionOnUnAllocate branch from c613c8c to 6574c55 Compare February 8, 2020 22:09
@markmandel markmandel marked this pull request as ready for review February 8, 2020 22:09
@markmandel markmandel force-pushed the flaky/TestGameServerAllocationDeletionOnUnAllocate branch from 6574c55 to 569daf4 Compare February 8, 2020 22:23
@agones-bot
Copy link
Collaborator

Build Failed 😱

Build Id: 3affb86f-ed11-49d7-93cf-01d915a01f02

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

@agones-bot
Copy link
Collaborator

Build Succeeded 👏

Build Id: 0226c81d-675e-4b13-bbea-65a8787eb1a9

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/1328/head:pr_1328 && git checkout pr_1328
  • helm install ./install/helm/agones --namespace agones-system --name agones --set agones.image.tag=1.4.0-569daf4

@markmandel markmandel removed the request for review from dzlier-gcp February 10, 2020 21:44
@pooneh-m pooneh-m requested review from pooneh-m and removed request for cyriltovena February 10, 2020 22:30
@@ -12,7 +12,7 @@
// See the License for the specific language governing permissions and
// limitations under the License.

package e2e
package controller
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we rename the file name to something else? missing_test.go sounds like it includes the tests that were missing from test coverage :D

Copy link
Member Author

@markmandel markmandel Feb 10, 2020

Choose a reason for hiding this comment

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

crash_test.go ? That better?

Tests where the controller crashes for whatever reason? WDYT?

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks, much better!

Copy link
Member Author

Choose a reason for hiding this comment

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

SGTM! Will make the change.

// getAgonesControllerPods returns all the Agones controller pods
func getAgonesControllerPods() (*corev1.PodList, error) {
opts := metav1.ListOptions{LabelSelector: labels.Set{"agones.dev/role": "controller"}.String()}
return framework.KubeClient.CoreV1().Pods("agones-system").List(opts)
}

func defaultGameServer(namespace string) *agonesv1.GameServer {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we refactor this instead?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah I thought about centralising that too - moving it into framework maybe. Wasn't sure if 2 duplication was enough to warrant it. If you think so, I'm happy to do it.

I assume that is what you mean here?

Copy link
Contributor

Choose a reason for hiding this comment

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

Correct, thanks for the clarification. framework.go should be a good place or you can add a new file such as testhelper and centralize the common helper methods there?

Copy link
Member Author

Choose a reason for hiding this comment

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

Sounds good - I like the idea of testhelper.go or similar (I think we have a previous pattern for this somewhere, maybe, I can copy).

@google-oss-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: markmandel, pooneh-m

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,pooneh-m]

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

Realised that Go test will compile and run tests in parallel per file,
which means we could shutdown the controller while an allocation is in
process - which means, it will fail completely, as the Kubernetes API
cannot reach the controller's http endpoint for allocation.

Split the controller tests into a separate package which could be run
with -parallel=1 so that there can be no race condition.

Also did some grouping of e2e test functionality to reduce code
repetition.

Closes googleforgames#1326
@markmandel markmandel force-pushed the flaky/TestGameServerAllocationDeletionOnUnAllocate branch from 569daf4 to b94b0f0 Compare February 11, 2020 01:56
@google-oss-robot
Copy link

New changes are detected. LGTM label has been removed.

@agones-bot
Copy link
Collaborator

Build Failed 😱

Build Id: ae943e9a-ac4b-4da6-965f-cff6d3f585c3

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

@agones-bot
Copy link
Collaborator

Build Succeeded 👏

Build Id: dd3a7cd5-cf28-47e3-9a06-64dcbb4a4a61

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/1328/head:pr_1328 && git checkout pr_1328
  • helm install ./install/helm/agones --namespace agones-system --name agones --set agones.image.tag=1.4.0-b94b0f0

@markmandel markmandel merged commit f998c8e into googleforgames:master Feb 11, 2020
@markmandel markmandel added this to the 1.4.0 milestone Feb 11, 2020
@markmandel markmandel deleted the flaky/TestGameServerAllocationDeletionOnUnAllocate branch February 13, 2020 18:11
ilkercelikyilmaz pushed a commit to ilkercelikyilmaz/agones that referenced this pull request Oct 23, 2020
Realised that Go test will compile and run tests in parallel per file,
which means we could shutdown the controller while an allocation is in
process - which means, it will fail completely, as the Kubernetes API
cannot reach the controller's http endpoint for allocation.

Split the controller tests into a separate package which could be run
with -parallel=1 so that there can be no race condition.

Also did some grouping of e2e test functionality to reduce code
repetition.

Closes googleforgames#1326
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. size/L
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Flaky: TestGameServerAllocationDeletionOnUnAllocate
4 participants