GameServer container restart before Ready, move to Unhealthy state After (v2) #1099

markmandel · 2019-10-04T01:17:34Z

This brings our implementation inline with what our health checking documentation states that we do.

This is done by implementing extra checks in the HealthController to determine if it's appropriate to move to Unhealthy rather than allow a restart to occur.

Replaced PR #1069

Closes #956

agones-bot · 2019-10-04T01:38:21Z

Build Failed 😱

Build Id: 5b35e2d0-dd16-4745-8bec-ff6a360671c5

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

markmandel · 2019-10-04T17:34:02Z

/hold

e2e test found a bug in my implementation. Need to fix!

roberthbailey · 2019-10-07T16:30:35Z

I'm waiting for @markmandel to remove the hold before I review.

markmandel · 2019-10-24T23:25:02Z

/hold cancel

While I think this should stay in feature freeze, as I would like to go through a development cycle for extra confidence, it is ready for review again now.

agones-bot · 2019-10-24T23:41:27Z

Build Succeeded 👏

Build Id: 2b82f742-c6c7-431b-9123-024ed8b01fb6

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

image: gcr.io/agones-images/agones-controller:1.1.0-2aab79d
image: gcr.io/agones-images/agones-sdk:1.1.0-2aab79d
image: gcr.io/agones-images/agones-ping:1.1.0-2aab79d
Linux C++ SDK (build): agonessdk-1.1.0-2aab79d-linux-arch_64.tar.gz
SDK Server: agonessdk-server-1.1.0-2aab79d.zip

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

https://2aab79d-dot-preview-dot-agones-images.appspot.com/

To install this version:

git fetch https://github.com/GoogleCloudPlatform/agones.git pull/1099/head:pr_1099 && git checkout pr_1099
helm install install/helm/agones --namespace agones-system --name agones --set agones.image.tag=1.1.0-2aab79d

markmandel · 2019-10-29T21:18:32Z

OMG! We can finally get this reviewed an in a release! 💃 I feel like I've been working on this bug forever 😄

roberthbailey · 2019-10-30T02:00:43Z

Now that the release is out this is the next review up on my list.

roberthbailey · 2019-10-30T04:34:27Z

pkg/apis/agones/v1/gameserver.go

@@ -91,6 +91,10 @@ const (
 	// DevAddressAnnotation is an annotation to indicate that a GameServer hosted outside of Agones.
 	// A locally hosted GameServer is not managed by Agones it is just simply registered.
 	DevAddressAnnotation = "agones.dev/dev-address"


(aside) Is there a reason that this one is prefixed with "agones.dev" instead of being agones.GroupName + "/dev-address"?

Good catch, we should fix this.

roberthbailey · 2019-10-30T07:04:18Z

test/e2e/gameserver_test.go

+		logger.WithError(err).WithField("gs", newGs.ObjectMeta.Name).WithField("state", newGs.Status.State).Info("no ready, trying again")
+		return false, nil
+	})
+	if !assert.NoError(t, err) {


elsewhere I just see assert.NoError() but this one wraps that and calls FailNow. Is that to short circuit running the remainder of the test? If so, would it be simpler to just write

if err != nil { assert.FailNow(t, ...) }

I find the statements that check the boolean result of the assert to be hard to read, and they are basically just checking if err is or isn't nil and calling another assert, which can be done more simply.

roberthbailey · 2019-10-30T07:07:37Z

test/e2e/gameserver_test.go

+		// also gives the process a chance to come back up
+		_, err = conn.Write([]byte("CRASH"))
+		logger.WithError(err).Info("no crash, trying again")
+		return false, nil


should this return false, err instead of nil? Or will returning an error short circuit the polling function? It seems weird that the error writing the udp packet is logged but otherwise ignored.

You are indeed correct - returning the error will short circuit the poll. It may return an error as the receiver may not be up and running yet, so it's useful for debugging purposes, but otherwise it is expected.

roberthbailey · 2019-10-30T07:09:23Z

test/e2e/gameserver_test.go

+			logger.WithError(err).Warn("could not send CRASH udp packet")
+			return false, nil
+		}
+		return true, nil


Why is successfully sending the packet sufficient to break the loop? Shouldn't we verify that the pod actually exited (e.g. the game server received the packet)?

Fixed, now we have a helper method.

roberthbailey · 2019-10-30T07:10:02Z

test/e2e/gameserver_test.go

+	// now crash, should be unhealthy, since it's after being Ready
+	logger.Info("crashing again, should be unhealthy")
+	// retry on crash, as with the restarts, sometimes Go takes a moment to send this through.
+	err = wait.Poll(time.Second, 3*time.Minute, func() (bool, error) {


All three of these udp sending loops are very similar in structure (write a packet, check for state, try again). Have you considered moving them into a helper function?

Nice. Done!

roberthbailey · 2019-10-30T07:18:25Z

pkg/gameservers/health.go

 	}

-	hc.recorder.Event(gs, corev1.EventTypeWarning, string(gsCopy.Status.State), "Issue with Gameserver pod")
+	if !skip {


This should be inverted.

if skip { // The game server hasn't been marked ready, so it doesn't transition to unhealthy. return nil }

To leave the other code with one less level of indentation.

Ah excellent suggestion!

roberthbailey · 2019-10-30T07:20:29Z

pkg/gameservers/health.go


 	return nil
 }
+
+// skipUnhealthy checks the GameServer is Ready or after Ready, and what the container crash state


This comment is really hard to read. I tried taking a stab at rewriting it but didn't have any particularly great ways to phrase it. If you can think of a way to say it more clearly that'd be awesome.

Took a stab. PTAL when I push my changes up. I think it's better though.

roberthbailey · 2019-10-30T07:25:48Z

pkg/gameservers/health_test.go

+			v.setup(gs, pod)
+
+			m.KubeClient.AddReactor("list", "pods", func(action k8stesting.Action) (bool, runtime.Object, error) {
+				return true, &corev1.PodList{Items: []corev1.Pod{*pod}}, nil


It looks like this doesn't cover the branches where pod doesn't exist or k8s returns an error to put the request back into the queue.

roberthbailey · 2019-10-30T07:26:32Z

pkg/gameservers/health_test.go

+					Name:  gs.Spec.Container,
+					State: corev1.ContainerState{Terminated: &corev1.ContainerStateTerminated{}},
+				}}
+			}, expected: true},


please put expected on a new line

roberthbailey · 2019-10-30T07:28:52Z

pkg/gameservers/health_test.go

+		},
+		"before ready, with no terminated container": {
+			setup: func(gs *agonesv1.GameServer, pod *corev1.Pod) {
+				gs.Status.State = agonesv1.GameServerStateScheduled


the before ready states are all the same, given the other test coverage it may not matter but you could vary which state is set to one of the other 4 states that are before ready.

This brings our implementation inline with what our health checking documentation states that we do. This is done by implementing extra checks in the HealthController to determine if it's appropriate to move to Unhealthy rather than allow a restart to occur. Replaced PR googleforgames#1069 Closes googleforgames#956

markmandel · 2019-11-02T01:08:53Z

@roberthbailey this should be good to review again 👍

agones-bot · 2019-11-02T01:22:54Z

Build Succeeded 👏

Build Id: 8344dedc-9848-4200-8c4e-c35d71ca9555

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

image: gcr.io/agones-images/agones-controller:1.2.0-ee1c3dd
image: gcr.io/agones-images/agones-sdk:1.2.0-ee1c3dd
image: gcr.io/agones-images/agones-ping:1.2.0-ee1c3dd
Linux C++ SDK (build): agonessdk-1.2.0-ee1c3dd-linux-arch_64.tar.gz
SDK Server: agonessdk-server-1.2.0-ee1c3dd.zip

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

https://ee1c3dd-dot-preview-dot-agones-images.appspot.com/

To install this version:

git fetch https://github.com/GoogleCloudPlatform/agones.git pull/1099/head:pr_1099 && git checkout pr_1099
helm install install/helm/agones --namespace agones-system --name agones --set agones.image.tag=1.2.0-ee1c3dd

google-oss-robot · 2019-11-02T06:44:16Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: 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 · 2019-11-02T17:47:05Z

markmandel added the kind/bug These are bugs. label Oct 4, 2019

google-oss-robot added the size/L label Oct 4, 2019

markmandel requested a review from roberthbailey October 4, 2019 01:17

markmandel changed the title ~~GameServer container restart before Ready, move to Unhealthy state After~~ GameServer container restart before Ready, move to Unhealthy state After (v2) Oct 4, 2019

markmandel mentioned this pull request Oct 4, 2019

GameServer container restart before Ready, move to Unhealthy state After #1069

Closed

markmandel added the area/user-experience Pertaining to developers trying to use Agones, e.g. SDK, installation, etc label Oct 4, 2019

markmandel force-pushed the bug/restart-before-ready-v2 branch from 08d6f54 to 47975e6 Compare October 4, 2019 03:46

google-oss-robot added the do-not-merge/hold label Oct 4, 2019

markmandel added the feature-freeze-do-not-merge Only eligible to be merged once we are out of feature freeze (next full release) label Oct 22, 2019

markmandel force-pushed the bug/restart-before-ready-v2 branch from 47975e6 to 60a955e Compare October 24, 2019 23:23

google-oss-robot added the approved label Oct 24, 2019

google-oss-robot removed the do-not-merge/hold label Oct 24, 2019

markmandel force-pushed the bug/restart-before-ready-v2 branch from 60a955e to 2aab79d Compare October 24, 2019 23:28

markmandel removed the feature-freeze-do-not-merge Only eligible to be merged once we are out of feature freeze (next full release) label Oct 29, 2019

roberthbailey reviewed Oct 30, 2019

View reviewed changes

markmandel force-pushed the bug/restart-before-ready-v2 branch from 2aab79d to ee1c3dd Compare November 2, 2019 01:08

roberthbailey approved these changes Nov 2, 2019

View reviewed changes

google-oss-robot assigned roberthbailey Nov 2, 2019

google-oss-robot added the lgtm label Nov 2, 2019

roberthbailey merged commit 3549367 into googleforgames:master Nov 2, 2019

markmandel deleted the bug/restart-before-ready-v2 branch November 2, 2019 17:45

markmandel added this to the 1.2.0 milestone Nov 2, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GameServer container restart before Ready, move to Unhealthy state After (v2) #1099

GameServer container restart before Ready, move to Unhealthy state After (v2) #1099

markmandel commented Oct 4, 2019

agones-bot commented Oct 4, 2019

markmandel commented Oct 4, 2019

roberthbailey commented Oct 7, 2019

markmandel commented Oct 24, 2019

agones-bot commented Oct 24, 2019

markmandel commented Oct 29, 2019

roberthbailey commented Oct 30, 2019

roberthbailey Oct 30, 2019

markmandel Oct 31, 2019

roberthbailey Oct 30, 2019

roberthbailey Oct 30, 2019

markmandel Oct 31, 2019

roberthbailey Oct 30, 2019

markmandel Oct 31, 2019

roberthbailey Oct 30, 2019

markmandel Oct 31, 2019

roberthbailey Oct 30, 2019

markmandel Oct 31, 2019

roberthbailey Oct 30, 2019

markmandel Oct 31, 2019

roberthbailey Oct 30, 2019

roberthbailey Oct 30, 2019

roberthbailey Oct 30, 2019

markmandel commented Nov 2, 2019

agones-bot commented Nov 2, 2019

google-oss-robot commented Nov 2, 2019

markmandel commented Nov 2, 2019

GameServer container restart before Ready, move to Unhealthy state After (v2) #1099

GameServer container restart before Ready, move to Unhealthy state After (v2) #1099

Conversation

markmandel commented Oct 4, 2019

agones-bot commented Oct 4, 2019

markmandel commented Oct 4, 2019

roberthbailey commented Oct 7, 2019

markmandel commented Oct 24, 2019

agones-bot commented Oct 24, 2019

markmandel commented Oct 29, 2019

roberthbailey commented Oct 30, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

markmandel commented Nov 2, 2019

agones-bot commented Nov 2, 2019

google-oss-robot commented Nov 2, 2019

markmandel commented Nov 2, 2019