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

making the ECS init log level configurable #4097

Merged
merged 1 commit into from
Feb 26, 2024
Merged

Conversation

Yiyuanzzz
Copy link
Contributor

@Yiyuanzzz Yiyuanzzz commented Feb 20, 2024

Summary

Currently, the ECS agent logging level can be configured using the ECS_LOGLEVEL option. The ECS init does not honor this option and always logs at info level only. This pr is to address this gap and make the ECS init log level configurable as well.

Implementation details

  • Removed scripts/bundle_log_config.sh and create logger file based on our existing logger file under ecs-init/volumes/logger

  • ECS init logging level can also be configured using the ECS_LOGLEVEL env variable

  • Example logs

level=info time=2024-02-23T03:28:11Z msg="pre-start: enabling loopback routing"
level=debug time=2024-02-23T03:28:11Z msg="testing"
level=info time=2024-02-23T03:28:11Z msg="pre-start: disabling ipv6 router advertisements"
level=debug time=2024-02-23T03:28:11Z msg="testing"
level=info time=2024-02-23T03:28:11Z msg="pre-start: creating credentials proxy route"
level=debug time=2024-02-23T03:28:11Z msg="testing"
level=info time=2024-02-23T03:28:11Z msg="pre-start: checking ecs agent container image loaded presence"
level=info time=2024-02-23T03:28:11Z msg="pre-start: ecs agent co

Testing

Launched an instance with ECS_INIT_LOGLEVEL=debug and checked ecs-init.log is expected

New tests cover the changes: yes

Description for the changelog

Enhancement - make the ECS init log level configurable

Licensing

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

@Yiyuanzzz Yiyuanzzz requested a review from a team as a code owner February 20, 2024 23:43
@Yiyuanzzz Yiyuanzzz force-pushed the ecs-init-log branch 2 times, most recently from f88f044 to b75d09a Compare February 22, 2024 22:42
@Yiyuanzzz Yiyuanzzz changed the title [WIP]making the ECS init log level configurable making the ECS init log level configurable Feb 22, 2024
@Yiyuanzzz Yiyuanzzz force-pushed the ecs-init-log branch 2 times, most recently from 5842a03 to f9d4ee0 Compare February 23, 2024 03:59
README.md Outdated
@@ -259,6 +259,7 @@ additional details on each available environment variable.
| `ECS_DYNAMIC_HOST_PORT_RANGE` | `100-200` | This specifies the dynamic host port range that the agent uses to assign host ports from, for container ports mapping. If there are no available ports in the range for containers, including customer containers and Service Connect Agent containers (if Service Connect is enabled), service deployments would fail. | Defined by `/proc/sys/net/ipv4/ip_local_port_range` | `49152-65535` |
| `ECS_TASK_PIDS_LIMIT` | `100` | Specifies the per-task pids limit cgroup setting for each task launched on the container instance. This setting maps to the pids.max cgroup setting at the ECS task level. See https://www.kernel.org/doc/html/latest/admin-guide/cgroup-v2.html#pid. If unset, pids will be unlimited. Min value is 1 and max value is 4194304 (4*1024*1024) | `unset` | Not Supported on Windows |
| `ECS_EBSTA_SUPPORTED` | `true` | Whether to use the container instance with EBS Task Attach support. ecs-init sets this variable for the ECS Agent if the instance can support mounting EBS volumes or not. ECS only schedules EBSTA tasks if this feature is supported by the platform type | `true` | `true` |
| `ECS_INIT_LOGLEVEL` | <crit> | <error> | <warn> | <info> | <debug> | The level of detail to be logged. | info | info |
Copy link
Contributor

Choose a reason for hiding this comment

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

I understand this is an ecs-init config? This should probably be added to the ecs-init variables table below

Copy link
Contributor Author

@Yiyuanzzz Yiyuanzzz Feb 23, 2024

Choose a reason for hiding this comment

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

I see, will move it to table below

Copy link
Contributor

Choose a reason for hiding this comment

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

why introduce a new var and not re-use ECS_LOGLEVEL?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good call out, re-use ECS_LOGLEVEL would be a better idea, will do

@@ -0,0 +1,118 @@
// Copyright 2019 Amazon.com, Inc. or its affiliates. All Rights Reserved.
Copy link
Contributor

Choose a reason for hiding this comment

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

The year seems to vary with files, and many files do not have it. Can the year be removed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sure! will remove it

package logger

import (
"fmt"
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: maybe standard library imports can be imported together as a block first followed by other imports, following go styleguide

Copy link
Contributor Author

Choose a reason for hiding this comment

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

got it thanks!

@sparrc
Copy link
Contributor

sparrc commented Feb 23, 2024

I am wondering why don't we just use the existing ECS_LOGLEVEL and apply it to the init process too? Doesn't seem particularly high-risk to add slightly more scope to that env var

@Yiyuanzzz Yiyuanzzz merged commit a5d4167 into aws:dev Feb 26, 2024
36 checks passed
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.

5 participants