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

ECS agent to acknowledge server heartbeat messages #2837

Merged
merged 1 commit into from
Apr 22, 2021

Conversation

LiangChiAmzn
Copy link
Contributor

Summary

This PR adds the support for ECS agent to acknowledge server heartbeat messages. This would help both ECS agent and ECS backend server to detect stale socket connections, then take proactive actions to handle it.

Implementation details

ECS server will soon start to actively clean up stale connections if ECS agent is not acknowledging heartbeat messages. Since such change is backward-incompatible, a new protocolVersion is introduced for ECS agent to control the behavior from client side when establishing the WebSocket connection.

If the version is set as 2, then the ECS agent must acknowledge all server heartbeats. Null protocol version is treated as 1.

Testing

make test
make release

Manually tested the change by building the agent, connect to ECS, then inspect ECS backend logs:

  • Positive case: connection is healthy when protocol version = 2 and heartbeats are being acked
  • Negative case: ECS will cut the connection if protocol version = 2 but heartbeat acks are missing

New tests cover the changes: yes

Description for the changelog

Enhancement - Add support for ECS agent to acknowledge server heartbeat messages

Licensing

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

Copy link
Contributor

@fenxiong fenxiong left a comment

Choose a reason for hiding this comment

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

lgtm other than cam's comment

sparrc
sparrc previously approved these changes Apr 22, 2021
Copy link
Contributor

@sparrc sparrc left a comment

Choose a reason for hiding this comment

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

lgtm assuming tests pass

@fenxiong fenxiong merged commit 2bd05e5 into aws:dev Apr 22, 2021
@fenxiong fenxiong added this to the 1.52.0 milestone Apr 27, 2021
@fenxiong fenxiong mentioned this pull request Apr 28, 2021
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