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

Expose PlatformFields field in order for it to be persisted in state #1480

Merged
merged 2 commits into from
Aug 22, 2018

Conversation

julienduchesne
Copy link

Summary

We run windows clusters with the ECS_ENABLE_CPU_UNBOUNDED_WINDOWS_WORKAROUND environment variable. We also reboot our instances in order to install some software when we start them. When rebooting, the tasks that have already been received by the instance are persisted to disk but the variable where ECS_ENABLE_CPU_UNBOUNDED_WINDOWS_WORKAROUND is not exposed in the JSON. These tasks are therefore restarted with 2 CPU units instead of 0 (unbounded). PlatformFields must be persisted in order to maintain consistent behavior.

Thanks

Implementation details

Converted PlatformFields to a public attribute and added json field tags.

Testing

Runs on our ECS clusters

  • 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

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

@julienduchesne
Copy link
Author

cc @jocgir @mouellet-coveo

@adnxn
Copy link
Contributor

adnxn commented Aug 3, 2018

/ping @sharanyad: did you end up running the tests on this?

@sharanyad
Copy link
Contributor

all tests pass.

Copy link
Contributor

@sharanyad sharanyad left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution

@sharanyad
Copy link
Contributor

@aws/aws-ecs-agent ptal

Copy link

@richardpen richardpen left a comment

Choose a reason for hiding this comment

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

This may need a changelog update.

@julienduchesne
Copy link
Author

julienduchesne commented Aug 22, 2018

Hi, could you guys merge this please?

Also, could you take a look at my comment here: #1144 (comment)?

We are currently using a fork of amazon-ecs-agent and if I could get this issue and my other one sorted out, we could get back on the official version and that would be awesome.

Thanks a lot

@sharanyad sharanyad merged commit 6ab4e3b into aws:dev Aug 22, 2018
@julienduchesne julienduchesne deleted the expose-platform-fields branch August 23, 2018 10:43
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