Timeout and unit tests for agent healthcheck #2437

sparrc · 2020-04-23T21:14:34Z

Summary

The docker healthcheck default timeout is 30s. We don't currently set any http timeouts on our internal healthcheck so if the request hangs then we don't log anything in agent or explicitly return a failed healthcheck to docker.

Adding a timeout of 25s means we can timeout in the agent codebase, log the error, and then explicitly return the failed healthcheck error code.

Also did a little refactor to allow adding unit tests.

New tests cover the changes: yes

test output:

% go test ./agent/app/. -count=1 -tags unit -v -run=Healthcheck
=== RUN   TestHealthcheck_Sunny
--- PASS: TestHealthcheck_Sunny (0.00s)
=== RUN   TestHealthcheck_InvalidURL2
--- PASS: TestHealthcheck_InvalidURL2 (0.00s)
=== RUN   TestHealthcheck_Timeout
level=error time=2020-04-23T21:51:23Z msg="error creating healthcheck request: parse   http://foobar: first path segment in URL cannot contain colon" module=healthcheck.go
level=error time=2020-04-23T21:51:25Z msg="health check [GET http://127.0.0.1:57227] failed with error: Get http://127.0.0.1:57227: net/http: request canceled (Client.Timeout exceeded while awaiting headers)" module=healthcheck.go
--- PASS: TestHealthcheck_Timeout (2.00s)
=== RUN   TestHealthcheck_404
--- PASS: TestHealthcheck_404 (0.00s)
PASS
ok  	github.com/aws/amazon-ecs-agent/agent/app	2.147s

Description for the changelog

Log agent healthcheck timeouts.

Licensing

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

fierlion

Nice to have the more granular view here.

petderek · 2020-04-23T22:15:44Z

agent/app/healthcheck_test.go

+)
+
+func TestHealthcheck_Sunny(t *testing.T) {
+	ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {


petderek · 2020-04-23T22:16:39Z

agent/app/healthcheck_test.go

+}
+
+func TestHealthcheck_InvalidURL2(t *testing.T) {
+	// leading space in url is invalid


I know it’s supposed to be there but it still really bothers me.

sparrc · 2020-04-24T00:20:40Z

agent/tcs/handler/handler.go

@@ -87,6 +87,11 @@ func StartSession(params *TelemetrySessionParams, statsEngine stats.Engine) erro
 			seelog.Errorf("Error: lost websocket connection with ECS Telemetry service (TCS): %v", tcsError)
 			params.time().Sleep(backoff.Duration())
 		}
+		select {


Unrelated to this change, but this is a fix for when TestDoStartCgroupInitHappyPath has a failure after the test goroutine has already exited.

sparrc · 2020-04-24T23:22:57Z

agent/engine/engine_unix_integ_test.go

@@ -172,6 +172,7 @@ func createVolumeTask(scope, arn, volume string, autoprovision bool) (*apitask.T
 		DriverOpts: map[string]string{
 			"device": tmpDirectory,
 			"o":      "bind",
+			"type":   "tmpfs",


this was added for compatibility with newer docker versions, see docker-archive/engine@d5b271c

sparrc force-pushed the healthcheck-timeout branch from 2646fcd to 402c05e Compare April 23, 2020 21:19

sparrc added the bot/test label Apr 23, 2020

amazon-ecs-bot removed the bot/test label Apr 23, 2020

fierlion approved these changes Apr 23, 2020

View reviewed changes

petderek approved these changes Apr 23, 2020

View reviewed changes

sparrc force-pushed the healthcheck-timeout branch 2 times, most recently from 96b9674 to 435b1f1 Compare April 24, 2020 00:09

sparrc commented Apr 24, 2020

View reviewed changes