Add ecs systemd unit for al2 #171

jahkeup · 2018-05-31T21:07:44Z

Summary

This change wraps the existing ecs-init daemon (with minor semantic modifications) with a systemd unit to run under Amazon Linux 2.

Implementation details

Minor modifications have been made to the entrypoints in the ecs-init binary to include the stop verb to align with systemd semantics surrounding service lifecycle. Though there is no difference from post-stop from upstart land, this allows the two to coexist in code if a change in handling is required at a later time.

The updated RPM spec accommodates both Amazon Linux and Amazon Linux 2 (thanks @ilianaw!) and includes a unit file suitable for running ecs-init on Amazon Linux 2 (with systemctl start ecs).

Testing

The RPM was built both for Amazon Linux and Amazon Linux 2 and run in an ECS Cluster with some tasks to confirm no functional differences were present.

New tests cover the changes: no

Description for the changelog

Add Amazon Linux 2 systemd unit to RPM spec

Licensing

This contribution is under the terms of the Apache 2.0 License: yes

iliana · 2018-05-31T21:14:42Z

packaging/amazon-linux-ami/ecs.service

+ExecStopPost=/usr/libexec/amazon-ecs-init post-stop
+
+[Install]
+WantedBy=multi-user.target


Please add a newline

iliana · 2018-05-31T21:25:07Z

packaging/amazon-linux-ami/ecs-init.spec

 fi

-%post
-# Symlink the bundled ECS Agent at loadable path.
-ln -sf ecs-agent-v%{bundled_agent_version}.tar %{cache_dir}/ecs-agent.tar


Looks like this got dropped in the amzn1 section...

nmeyerhans · 2018-05-31T21:21:44Z

packaging/amazon-linux-ami/ecs-init.spec

 License:        Apache 2.0
 Summary:        Amazon Elastic Container Service initialization application
-BuildArch:      x86_64
-BuildRoot:      %{_tmppath}/%{name}-%{version}-%{release}-root-%(%{__id_u} -n)
+ExclusiveArch:  x86_64


Why change from BuildArch to ExclusiveArch?

https://fedoraproject.org/wiki/Packaging:Guidelines#Arch-Specific_Runtime_and_Build-Time_Dependencies

nmeyerhans · 2018-05-31T21:25:03Z

ecs-init/ecs-init.go

@@ -31,6 +31,7 @@ const (
 	PRESTART = "pre-start"
 	START    = "start"
 	PRESTOP  = "pre-stop"
+	STOP     = "stop"


You don't appear to have updated the manpage to reflect this change.

Indeed! I'll update and clarify how these targets should be used in the context of upstart and systemd.

nmeyerhans · 2018-05-31T21:30:40Z

packaging/amazon-linux-ami/ecs-init.spec

-touch $RPM_BUILD_ROOT/%{conf_dir}/ecs.config
-touch $RPM_BUILD_ROOT/%{conf_dir}/ecs.config.json
+install -D amazon-ecs-init %{buildroot}%{_libexecdir}/amazon-ecs-init
+install -D scripts/amazon-ecs-init.1 %{buildroot}%{_mandir}/man1/amazon-ecs-init.1


We were explicitly compressing this before, but aren't here. Why?

https://fedoraproject.org/wiki/Packaging:Guidelines#Manpages

When installing man pages, note that they should be installed uncompressed as the build system will compress them as needed.

nmeyerhans · 2018-05-31T21:38:25Z

packaging/amazon-linux-ami/ecs.service

+Requires=docker.service
+After=docker.service
+
+[Service]


I wouldn't mind seeing Type=simple set explicitly here. Otherwise people will need to understand the rules around the different defaults in order to understand this unit's behavior.

nmeyerhans · 2018-05-31T21:39:36Z

packaging/amazon-linux-ami/ecs-init.spec

 Name:           ecs-init
 Version:        1.18.0
-Release:        1%{?dist}
-Group:          System Environment/Base
-Vendor:         Amazon.com


Why do we no longer set Group or Vendor?

https://fedoraproject.org/wiki/Packaging:Guidelines#Tags_and_Sections

Why is that? Does that apply to all packages? It's not clear from that doc if there are fedora-specific reasons to avoid these tags or if they must not be present even in third-party packages.

AL2 inherits the packaging guidelines from Fedora / EPEL, except for the ones we are explicitly incompatible with (such as ones requiring changes in RPM 4.14) or otherwise simply don't like (none of these are documented yet).

The Group field, I believe, is not listed in yum repodata (deprecated in favor of yum's groups in comps.xml). The Vendor field gets overwritten by our build system anyway.

samuelkarp · 2018-06-01T00:56:29Z

packaging/amazon-linux-ami/ecs.service

@@ -0,0 +1,13 @@
+[Unit]


Can you add the license header to this file?

jahkeup · 2018-06-04T21:13:55Z

See rendered man page here. Please leave comments in the PR and file.

nmeyerhans

LGTM

jahkeup · 2018-06-04T23:32:08Z

Trimmed down the commits. @samuelkarp PTAL at the man page updates specifically.

The process executed by the ecs.service is going to remain running to supervise the ECS agent and handles lifecycle with the Exec* directives and also internally where required (eg: for agent upgrade functionality).

samuelkarp · 2018-06-06T17:39:25Z

packaging/amazon-linux-ami/ecs.service

+[Unit]
+Description=ECS Agent
+Requires=docker.service
+After=docker.service


We should probably have this run after cloud-init.target so that you can configure the ECS agent using user-data.

This was inadvertently merged directly into the master and needs to be restaged. This reverts commit 52d5792.

jahkeup and others added 7 commits May 22, 2018 16:18

Added unit file

ee85ef5

Use systemd macros for unit installation

5d03c8f

Fix go build ldflags usage

8b4a78b

Use stop as the action verb

9287a67

update clean target

4346934

AL1 + AL2 support in same spec; cleanup

0c6717b

Merge branch 'dev' into ecs-init-unit

fd99bb8

jahkeup requested review from nmeyerhans, aaithal and jhaynes May 31, 2018 21:07

iliana reviewed May 31, 2018

View reviewed changes

nmeyerhans reviewed May 31, 2018

View reviewed changes

samuelkarp reviewed Jun 1, 2018

View reviewed changes

packaging/amazon-linux-ami/ecs.service

@@ -0,0 +1,13 @@

[Unit]

Copy link

Contributor

samuelkarp Jun 1, 2018

Choose a reason for hiding this comment

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

Can you add the license header to this file?

aaithal requested a review from richardpen June 4, 2018 20:23

jahkeup force-pushed the ecs-init-unit branch from 4504c1e to 404719d Compare June 4, 2018 21:08

jahkeup force-pushed the ecs-init-unit branch from 404719d to 37d1bd3 Compare June 4, 2018 21:43

jhaynes approved these changes Jun 4, 2018

View reviewed changes

nmeyerhans approved these changes Jun 4, 2018

View reviewed changes

Fix post step to include symlink in both amzn1 and amzn2

1bf6df3

jahkeup force-pushed the ecs-init-unit branch from aed5880 to 47f1d3d Compare June 4, 2018 23:31

jahkeup added 2 commits June 5, 2018 10:41

Include new targets and init usage in man page

f8aaab5

Explicitly declare ecs.service unit as Type=simple

2dec447

The process executed by the ecs.service is going to remain running to supervise the ECS agent and handles lifecycle with the Exec* directives and also internally where required (eg: for agent upgrade functionality).

jahkeup force-pushed the ecs-init-unit branch from 47f1d3d to 2dec447 Compare June 5, 2018 17:41

jahkeup merged commit 8409936 into aws:master Jun 5, 2018

jahkeup mentioned this pull request Jun 5, 2018

Revert "Add ecs systemd unit for al2" #172

Merged

samuelkarp reviewed Jun 6, 2018

View reviewed changes

jahkeup added a commit to jahkeup/amazon-ecs-init that referenced this pull request Jun 6, 2018

Add ecs-init-unit from aws#171

5da7cda

This was inadvertently merged directly into the master and needs to be restaged. This reverts commit 52d5792.

jahkeup mentioned this pull request Jun 6, 2018

Add ecs-init-unit from #171 #174

Merged

jahkeup added a commit that referenced this pull request Jun 11, 2018

Add ecs-init-unit from #171 (#174)

fbbf834

This was inadvertently merged directly into the master and needs to be restaged. This reverts commit 52d5792.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add ecs systemd unit for al2 #171

Add ecs systemd unit for al2 #171

jahkeup commented May 31, 2018

iliana May 31, 2018

iliana May 31, 2018

nmeyerhans May 31, 2018

iliana May 31, 2018

nmeyerhans May 31, 2018

jahkeup May 31, 2018

nmeyerhans May 31, 2018

iliana May 31, 2018

nmeyerhans May 31, 2018

nmeyerhans May 31, 2018

iliana May 31, 2018

nmeyerhans May 31, 2018

iliana May 31, 2018

samuelkarp Jun 1, 2018

jahkeup commented Jun 4, 2018

nmeyerhans left a comment

jahkeup commented Jun 4, 2018

samuelkarp Jun 6, 2018

Add ecs systemd unit for al2 #171

Add ecs systemd unit for al2 #171

Conversation

jahkeup commented May 31, 2018

Summary

Implementation details

Testing

Description for the changelog

Licensing

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

Choose a reason for hiding this comment

jahkeup commented Jun 4, 2018

nmeyerhans left a comment

Choose a reason for hiding this comment

jahkeup commented Jun 4, 2018

Choose a reason for hiding this comment