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

Add the "<filter></filter>" only when logfile environment Variable is set #2598

Merged
merged 1 commit into from
Sep 3, 2020

Conversation

ubhattacharjya
Copy link
Contributor

@ubhattacharjya ubhattacharjya commented Aug 28, 2020

Summary

This PR fixes the empty (filter)(/filter) when ECS_LOGFILE is not set by ecs-init. This would cause certain environment variables related to agent logging to not have their expected behaviour when the user is not using ECS optimized AMI.
This issue is mentioned in #2591

Problem:

For example,

$ docker run -e ECS_LOG_OUTPUT_FORMAT=json amazon/amazon-ecs-agent:v1.44.0
1598513604929243900 [Error] node must have children

This error is caused because ecs agent is receiving empty (filter)(/filter) when ECS_LOGFILE is not set by ecs-init.

However, when the below is run, everything is fine.

docker run -e ECS_LOG_OUTPUT_FORMAT=json --env=ECS_LOGFILE=/log/ecs-agent.log amazon/amazon-ecs-agent:v1.44.0
{"level": "info", "time": "2020-08-28T21:08:40Z", "msg": "Loading configuration", "module": "agent.go"}

Implementation details

This PR adds the filter block only when ECS_LOGFILE is present as env var. Otherwise it is not added

Testing

Unit tests cover the changes
Manual test:

[ec2-user@ip-172-31-3-170 ~]$ docker run -e ECS_LOG_OUTPUT_FORMAT=json amazon/amazon-ecs-agent:latest
{"level": "info", "time": "2020-08-28T22:24:32Z", "msg": "Loading configuration", "module": "agent.go"}

Also tested that the /var/log/ecs/ecs-agent.log file also got populated in json format when ECS_LOG_OUTPUT_FORMAT=json is mentioned ecs.config file

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.

@ubhattacharjya ubhattacharjya changed the title Add the <filter></filter> only when logfile environment Variable is set Add the "<filter></filter>" only when logfile environment Variable is set Aug 28, 2020
@ubhattacharjya ubhattacharjya requested a review from a team August 28, 2020 22:27
@mssrivas
Copy link
Contributor

mssrivas commented Sep 2, 2020

Can we add a test/unit test to make sure this works as we think it should?

@ubhattacharjya
Copy link
Contributor Author

Can we add a test/unit test to make sure this works as we think it should?

added unit tests for testing a scenario when logfile is not mentioned

@ubhattacharjya ubhattacharjya modified the milestone: 1.45.0 Sep 3, 2020
@ubhattacharjya ubhattacharjya merged commit 8b39b68 into aws:dev Sep 3, 2020
@mssrivas mssrivas added this to the 1.44.4 milestone Sep 9, 2020
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