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

engine: docker healthcheck logLength index fix #1239

Merged
merged 2 commits into from
Feb 19, 2018
Merged

Conversation

adnxn
Copy link
Contributor

@adnxn adnxn commented Feb 16, 2018

Summary

This change addresses #1237 by adding a zero check before accessing the Health.Log slice. Also adds corresponding test.

The issue was surfaced when the agent recieved a Docker health check message indicating the container was unhealthy but sent an empty []HealthCheck in Health.Log.

We have not reproduced the issue, but are handling this case by doing a length check on Health.Log.

Implementation details

Testing

  • Builds on Linux (make release)
  • Builds on Windows (go build -out amazon-ecs-agent.exe ./agent)
  • Unit tests on Linux (make test) pass
  • Unit tests on Windows (go test -timeout=25s ./agent/...) pass
  • Integration tests on Linux (make run-integ-tests) pass
  • Integration tests on Windows (.\scripts\run-integ-tests.ps1) pass
  • Functional tests on Linux (make run-functional-tests) pass
  • Functional tests on Windows (.\scripts\run-functional-tests.ps1) pass

New tests cover the changes:

Description for the changelog

Licensing

This contribution is under the terms of the Apache 2.0 License:

@adnxn adnxn self-assigned this Feb 16, 2018
@jahkeup
Copy link
Member

jahkeup commented Feb 16, 2018

I was able to reproduce this state of the health check using this Dockerfile:

FROM debian:latest
HEALTHCHECK --interval=3s --timeout=1s --start-period=56s CMD /bin/true
CMD ["/bin/sleep", "60"]

And running it using the following commands:

docker build -t healthchecked .
docker run -d --name healthchecked healthchecked
# wait for container to start
sleep 2 
# check the container's state for health checks observed:
docker inspect -f '{{.State.Health}}' healthchecked

Which shows the health state with no checks performed and still in the starting state:

{starting 0 []}

@aaithal
Copy link
Contributor

aaithal commented Feb 16, 2018

@jahkeup in your example, the container's health status is starting and not actually unhealthy right?

@jahkeup
Copy link
Member

jahkeup commented Feb 16, 2018

Yep, I see where we're checking the logs now. Let me try this out with the Agent running to see if there's some other way that the logs could be empty.

@jahkeup
Copy link
Member

jahkeup commented Feb 16, 2018

Doesn't appear that we could easily get into this state, the Docker daemon always appends to its Log field when handling a check result and this is also where it's setting the unhealthy/healthy status.

@adnxn adnxn added this to the 1.17.1 milestone Feb 16, 2018
@aaithal
Copy link
Contributor

aaithal commented Feb 19, 2018

@adnxn don't forget to add an entry in the chanelog before merging this.

@adnxn adnxn merged commit 109af5f into aws:dev Feb 19, 2018
adnxn added a commit that referenced this pull request Feb 19, 2018
@adnxn adnxn deleted the loglen-index-fix branch June 4, 2018 16:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants