Merging our systemd unit files. #1

JasonSwindle · 2018-03-23T17:38:09Z

No description provided.

Load and run agent 1.16.0 Update agent runtime configuration to support awsvpc requirements.

Amazon systemd unit files.

nmeyerhans · 2018-03-23T18:30:38Z

integration/systemd/amazon-ecs-agent.service

+
+# Load an updated ECS Agent, if it exists:
+ExecStartPre=-/bin/sh -c 'test -f /var/cache/ecs/desired-image && \
+    docker load $(cat /var/cache/ecs/desired-image) \


We should just docker load --input=/var/cache/ecs/desired-image No need to use cat.

We should also add --quiet to the docker load invocation.

nmeyerhans · 2018-03-23T18:30:59Z

integration/systemd/amazon-ecs-agent.service

+
+# If we don't have an ECS Agent, load from disk, if possible, or Docker Hub:
+ExecStartPre=/bin/sh -c "docker inspect amazon/amazon-ecs-agent \
+    || docker load --quiet < /var/cache/ecs/ecs-agent.tar \


Should also update this to use --input

nmeyerhans · 2018-03-23T18:32:26Z

integration/systemd/ecs.config

@@ -0,0 +1,2 @@
+ECS_LOGLEVEL=info
+ECS_CLUSTER=default


I'm not sure that it makes sense for this file to be here. There's nothing systemd-specific about it, so at the very least it probably shouldn't be in the systemd subdirectory.

Good call-out, fixing now.

... driver is now journald. The ECS Agent still logs into the amazon-ecs-agent unit and writes to the file system.

nmeyerhans

A couple details I noticed. Looks pretty good in general.

nmeyerhans · 2018-03-23T19:56:10Z

integration/systemd/amazon-ecs-agent.service

+# Load an updated ECS Agent, if it exists:
+ExecStartPre=-/bin/sh -c 'test -f /var/cache/ecs/desired-image && \
+    docker load --quiet --input=/var/cache/ecs/desired-image \
+    && rm -f $(cat /var/cache/ecs/desired-image) /var/cache/ecs/desired-image'


$(cat /var/cache/ecs/desired-image) does not belong here

nmeyerhans · 2018-03-23T19:57:37Z

integration/systemd/amazon-ecs-agent.service

+    && rm -f $(cat /var/cache/ecs/desired-image) /var/cache/ecs/desired-image'
+
+# If we don't have an ECS Agent, load from disk, if possible, or Docker Hub:
+ExecStartPre=/bin/sh -c "`docker inspect amazon/amazon-ecs-agent &>/dev/null` \


The &> redirection syntax is a bash extension. We don't want to assume that /bin/sh is bash, as this isn't the case on all systems. Please stick with POSIX-friendly redirections, so > /dev/null 2>&1

(Note that this issue is present in multiple places.)

nmeyerhans · 2018-03-23T21:38:53Z

integration/systemd/amazon-ecs-agent.service

+
+# Docker stop is used as it will send a SIGTEM and wait 10 seconds
+# before sending SIGKILL
+ExecStartPre=-/bin/sh -c "`docker stop --time 10 ecs-agent &>/dev/null`"


Why do we do this in ExecStartPre? Do we expect the service to already be running when we try to start it? Further, this occurs later in the file than the other ExecStartPre call to 'docker rm'.

jhaynes · 2018-03-23T21:27:26Z

integration/systemd/amazon-ecs-agent.service

+Restart=on-failure
+RestartSec=5s
+RestartPreventExitStatus=5
+StartLimitInterval=5min


Do we have data to back up this restart policy?

I do not. The default RestartSec time for systemd is 100ms, which I feel is way too fast and will clobber docker trying to restart the agent over and over.

jhaynes · 2018-03-23T21:27:28Z

integration/systemd/amazon-ecs-agent.service

+# attempts to duplicate the functionality of the `ecs-init` Golang package. The
+# notable differences currently are:
+#
+# 1. The unit file does an unconditional pull of the latest ECS Agent from


Are we really doing this instead of using a potentially cached agent?

jhaynes · 2018-03-23T21:30:29Z

integration/systemd/amazon-ecs-agent.service

+# Depending on the OS, it could be placed in 
+# /etc/systemd/system/amazon-ecs-agent.d , and named 
+# ecs_agent_version_override.conf
+Environment=ECS_AGENT_VERSION=v1.17.2


Does this mean we need to update this with each Agent release? If so, why not just pin this to "latest" and allow customers to pin to a version with the override described above?

jhaynes · 2018-03-23T21:33:59Z

integration/systemd/amazon-ecs-agent.service

+# If we don't have an ECS Agent, load from disk, if possible, or Docker Hub:
+ExecStartPre=/bin/sh -c "`docker inspect amazon/amazon-ecs-agent &>/dev/null` \
+    || docker load --quiet -input=/var/cache/ecs/ecs-agent.tar \
+    || docker pull amazon/amazon-ecs-agent:${ECS_AGENT_VERSION}"


Can we change this to "pull" from our S3 bucket instead of Docker Hub?

jhaynes · 2018-03-23T21:42:56Z

integration/systemd/amazon-ecs-agent.service

+    --publish=127.0.0.1:51678:51678 \
+    --env ECS_UPDATES_ENABLED=false \
+    --env ECS_DATADIR=/data \
+    --env ECS_ENABLE_TASK_IAM_ROLE=true \


It looks like (at least with Docker 1.12.6) if you set --env they override anything set in the --env-file. We should be cautious about setting anything that we expect customers to potentially want to configure.

➜ ~ echo FOO=foo > foo.env ➜ ~ docker run --rm -it --env-file foo.env amazonlinux bash bash-4.2# echo $FOO foo bash-4.2# exit ➜ ~ docker run --rm -it --env-file foo.env --env FOO=bar amazonlinux bash bash-4.2# echo $FOO bar bash-4.2# exit ➜ ~ docker run --rm -it --env FOO=bar --env-file foo.env amazonlinux bash bash-4.2# echo $FOO bar bash-4.2# exit

jhaynes · 2018-03-23T21:46:10Z

integration/systemd/amazon-ecs-agent.service

+ExecStartPre=-/bin/sh -c "`docker rm ecs-agent &>/dev/null`"
+
+# Create the directories needed for the ECS Agent.
+ExecStartPre=-/bin/mkdir -p /var/lib/ecs/dhclient /var/ecs-data


This should probably fail to start if it returns non-zero. mkdir -p will return zero even if the directories already exist.

- ECS Agent only downloads from S3. -- Will download from region as needed. - MD5 checks the downloaded file. - Moved ENV to ecs.config so they can over-written.

- Changed docs to HTTPS vs HTTP.

nmeyerhans · 2018-03-26T22:47:57Z

integration/systemd/amazon-ecs-agent.service

+# Download ECS Agent from S3.
+ExecStartPre=/usr/bin/echo "Downloading the ECS Agent from S3, if missing"
+ExecStartPre=/bin/sh -c '\
+case $$REGION in \


Does this actually work? REGION is set in a prior invocation of sh. That environment variable doesn't persist outside that shell.

Fixed. This lead to a few errors as well that have been fixed.

- A echo to work on Ubuntu as well. - Fixed the case logic (Tested in us-gov-west-1 on Ubuntu 16.04 instance) - Fixed test logic to be more POSIX. - Typo

ProgrammingAce · 2018-03-27T21:41:08Z

For loading the environment, we could keep the /etc/ecs/ecs.conf file optional by loading the environment through the unit file. Adding this before the ExecStart (although the env file portion might have to be part of the ExecStart callout). This would preserve the exiting functionality in ECS:

# Agent environment variables
Environment=ECS_UPDATES_ENABLEg=false
Environment=ECS_DATADIR=/data
Environment=ECS_ENABLE_TASK_IAM_ROLE=true
Environment=ECS_ENABLE_TASK_IAM_ROLE_NETWORK_HOST=true
Environment=ECS_ENABLE_TASK_ENI=true
Environment=ECS_LOGFILE=/log/ecs-agent.log
Environment=ECS_AVAILABLE_LOGGING_DRIVERS=["json-file","syslog","awslogs","none"]
Environment=ECS_CGROUP_PREFIX=ecs
Environment=ECS_CLUSTER=default

# Load the ECS configuration file into the agent environment
EnvironmentFile=-/etc/ecs/ecs.conf

nmeyerhans · 2018-03-27T22:10:44Z

@ProgrammingAce Setting the environment variables in the unit file doesn't pass them through to the container. Docker only passes variables explicitly set via the --env or --env-file command-line flags.

I don't really like the idea of moving the environment to an external file, but it seems like it might be the best approach...

@JasonSwindle If we need an external environment file, I'm also inclined to move all the shell code out of the unit file and into something standalone, too. As you've no doubt discovered, embedding complex logic in inline code in unit files is painful. The result is brittle and difficult to maintain.

JasonSwindle · 2018-03-27T22:28:22Z

My only concern with many small scripts is the complexity for customers to use it when not using the ECS AMI. I'm more than happy to break the shell code out of the unit file, as it was a huge pain to make everything work correctly. My other concern is how much is now mandatory in the ecs.config file. This changes the simple nature of ECS at the container instance, and will make customer usage way more error prone.

nmeyerhans · 2018-03-28T16:38:23Z

@JasonSwindle The alternative is that we ditch the env-file altogether and instead encourage customers to override the environment via a drop-in. It'll be more straightforward in the default case with no overrides, but I'm not sure it's ideal in terms of reducing complexity for people who want to make changes.

Noah Meyerhans and others added 10 commits December 20, 2017 16:12

Add a systemd unit file for running ECS Agent

0f30704

WIP: work on supporting agent updates

09aac00

Updates for agent 1.16.0 and awsvpc networking

4d7b43d

Load and run agent 1.16.0 Update agent runtime configuration to support awsvpc requirements.

Add an Install section to the systemd service file

c08574c

Update to load ecs agent 1.16.1

5b8ba6e

systemd: update to load agent 1.16.2

57a060b

Update unit file for agent 1.17.0

92ef111

Merging our systemd unit files.

3c04c5a

Example ecs.config for the systemd unit file.

72495ad

Rename to match the naming standard of other ...

67ef172

Amazon systemd unit files.

nmeyerhans requested changes Mar 23, 2018

View reviewed changes

Jason Swindle added 3 commits March 23, 2018 12:18

Cleaned up output of unit, and agent logging ...

51e10c7

... driver is now journald. The ECS Agent still logs into the amazon-ecs-agent unit and writes to the file system.

Delete ecs.config

cedc64b

Typo in restart logic comment.

1a2e194

nmeyerhans requested changes Mar 23, 2018

View reviewed changes

nmeyerhans reviewed Mar 23, 2018

View reviewed changes

jhaynes reviewed Mar 23, 2018

View reviewed changes

Jason Swindle added 3 commits March 26, 2018 15:13

Updated from feedback

9c8e50d

- ECS Agent only downloads from S3. -- Will download from region as needed. - MD5 checks the downloaded file. - Moved ENV to ecs.config so they can over-written.

Forgot to update some of the comments.

f469cc2

- Changed docs to HTTPS vs HTTP.

Added ecs.config, as the unit file needs it.

076e07a

nmeyerhans reviewed Mar 26, 2018

View reviewed changes

Fixed....

e233717

- A echo to work on Ubuntu as well. - Fixed the case logic (Tested in us-gov-west-1 on Ubuntu 16.04 instance) - Fixed test logic to be more POSIX. - Typo

nmeyerhans force-pushed the systemd branch from 92ef111 to 28ebd97 Compare September 12, 2019 19:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Merging our systemd unit files. #1

Merging our systemd unit files. #1

JasonSwindle commented Mar 23, 2018

nmeyerhans Mar 23, 2018

JasonSwindle Mar 23, 2018

nmeyerhans Mar 23, 2018

JasonSwindle Mar 23, 2018

nmeyerhans Mar 23, 2018

JasonSwindle Mar 23, 2018

nmeyerhans left a comment

nmeyerhans Mar 23, 2018

nmeyerhans Mar 23, 2018

nmeyerhans Mar 23, 2018

jhaynes Mar 23, 2018

JasonSwindle Mar 26, 2018

jhaynes Mar 23, 2018

jhaynes Mar 23, 2018

jhaynes Mar 23, 2018

jhaynes Mar 23, 2018

jhaynes Mar 23, 2018

nmeyerhans Mar 26, 2018

JasonSwindle Mar 27, 2018

ProgrammingAce commented Mar 27, 2018 •

edited

Loading

nmeyerhans commented Mar 27, 2018

JasonSwindle commented Mar 27, 2018

nmeyerhans commented Mar 28, 2018

		@@ -0,0 +1,2 @@
		ECS_LOGLEVEL=info
		ECS_CLUSTER=default

Merging our systemd unit files. #1

Are you sure you want to change the base?

Merging our systemd unit files. #1

Conversation

JasonSwindle commented Mar 23, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nmeyerhans left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ProgrammingAce commented Mar 27, 2018 • edited Loading

nmeyerhans commented Mar 27, 2018

JasonSwindle commented Mar 27, 2018

nmeyerhans commented Mar 28, 2018

ProgrammingAce commented Mar 27, 2018 •

edited

Loading