Windows service #1070

samuelkarp · 2017-11-12T01:40:30Z

Summary

This set of commits enables the agent to be run as a Windows service and write logs into the Windows event log.

Note: This is marked WIP as it will fail to build because I haven't vendored the new dependencies (two more packages inside golang.org/x/sys/windows). I'm waiting for #1009 to be merged so I can manage dependencies with dep.

Implementation details

Added a new command-line argument to run as a Windows service
Refactored the termination handler to use a function provided as part of the agent struct
Implemented a new log receiver for seelog

TODO

Vendor dependencies
Set restart settings
Set dependency on Docker
Determine correct recovery settings
Determine if these need to be set in Go code or in PowerShell startup?

Testing

New tests cover the changes: yes

Description for the changelog

Feature - The Amazon ECS agent can now be installed as a Windows service

Licensing

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

tehshwann

This is awesome. Thanks!

Few comments... mostly text nits.

tehshwann · 2017-11-13T14:42:14Z

agent/logger/eventlog_windows.go

+
+const (
+	eventLogName = "AmazonECSAgent"
+	eventLogID   = 999


Can we add a comment here explaining that this is a default value so others know its overridden later on a case-by-case basis?

Right now it's a constant without being overridden. I'm going to leave it as-is for now and we can come back to making it play more nicely with Windows later.

It would help to list the eventid in the Readme or in a comment so that it is easy to search. Even though windows event log id's are specific to the application and not always well documented, administrators like to be able to find the id quickly in a search.

tehshwann · 2017-11-13T14:43:15Z

agent/logger/log.go

@@ -86,7 +87,7 @@ func SetLevel(logLevel string) {
 // GetLevel gets the log level
 func GetLevel() string {
 	levelLock.RLock()
-	defer levelLock.RLock()
+	defer levelLock.RUnlock()


Nit: RUnlock -> Runlock or RunLock??

This look like it's a https://godoc.org/sync#RWMutex, so RUnlock is a reader taking a lock.

RUnlock is unlocking the read-lock. This is unrelated to Windows; I just saw gometalinter complain about it while I was working in the package.

tehshwann · 2017-11-13T14:45:21Z

README.md

+```powershell
+PS C:\> # Set up directories the agent uses
+PS C:\> New-Item -Type directory -Path $ProgramFiles\Amazon\ECS
+PS C:\> New-Item -Type directory -Path $ProgramData\Amazon\ECS


New-Item -Type directory -Path **$ENV:**ProgramData\Amazon\ECS -Force

tehshwann · 2017-11-13T14:45:34Z

README.md

+
+```powershell
+PS C:\> # Set up directories the agent uses
+PS C:\> New-Item -Type directory -Path $ProgramFiles\Amazon\ECS


New-Item -Type directory -Path **$ENV:**ProgramFiles\Amazon\ECS -Force

Is New-Item -Type dir -Path ... -Force like mkdir -p?

Assuming you meant -d, (dir) yes.

tehshwann · 2017-11-13T14:50:47Z

README.md

+PS C:\> Invoke-RestMethod -OutFile $zipFile -Uri $agentZipUri
+PS C:\> # Put the executables in the executable directory.
+PS C:\> Expand-Archive -Path $zipFile -DestinationPath $ecsExeDir -Force
+PS C:\> # Uncomment the following line to enable task IAM roles.


This isnt super clear cause the following line is a comment. Maybe add a bool here for users to change so its more clear? Also... I think you want to cd into the exec dir in either case??

Example:

Set-Location $ecsExeDir
/# Set '$EnableTaskIAMRoles' to '$true' if you wish to use Task IAM Roles.
[bool]$EnableTaskIAMRoles = $false
if ($EnableTaskIAMRoles) {
.\hostsetup.ps1
}

tehshwann · 2017-11-13T14:57:26Z

agent/app/args/flag.go

 	acceptInsecureCertUsage  = "Disable SSL certificate verification. We do not recommend setting this option"
 	licenseUsage             = "Print the LICENSE and NOTICE files and exit"
 	blacholeEC2MetadataUsage = "Blackhole the EC2 Metadata requests. Setting this option can cause the ECS Agent to fail to work properly.  We do not recommend setting this option"
-	ecsAttributesUsage       = "Print the Agent's ECS Attributes based on its environment"


Why are we removing this?

This got reordered.

tehshwann · 2017-11-13T14:58:20Z

agent/logger/eventlog_windows.go

+// ReceiveMessage receives a log line from seelog and emits it to the Windows event log
+func (r *eventLogReceiver) ReceiveMessage(message string, level seelog.LogLevel, context seelog.LogContextInterface) error {
+	switch level {
+	case seelog.DebugLvl, seelog.InfoLvl:


nit: should this be indented?

Nope, this is Go's spec :)

gofmt does this. I recommend running gofmt on save every time you modify a Go file.

tehshwann · 2017-11-13T14:59:27Z

agent/sighandlers/termination_handler.go

@@ -61,11 +66,11 @@ func FinalSave(saver statemanager.Saver, taskEngine engine.TaskEngine) error {
 	engineDisabled := make(chan error)

 	disableTimer := time.AfterFunc(engineDisableTimeout, func() {
-		engineDisabled <- errors.New("Timed out waiting for TaskEngine to settle")
+		engineDisabled <- errors.New("final save: timed out waiting for TaskEngine to settle")


nit: Final save:...

This is conventional idiomatic go casing for the error message to be lower cased. Here's an example:

agent error: this package errored fetching data: I couldn't reach the server.

This can come from 3 different sources and would look weird if it was capitalized after each :.

The convention is described here.

tehshwann · 2017-11-13T14:59:47Z

agent/sighandlers/termination_handler.go

@@ -75,10 +80,10 @@ func FinalSave(saver statemanager.Saver, taskEngine engine.TaskEngine) error {

 	stateSaved := make(chan error)
 	saveTimer := time.AfterFunc(finalSaveTimeout, func() {
-		stateSaved <- errors.New("Timed out trying to save to disk")
+		stateSaved <- errors.New("final save: timed out trying to save to disk")


nit: Final...

See above: idiomatic error messages.

tehshwann · 2017-11-13T15:01:16Z

agent/sighandlers/termination_handler.go

 	})
 	go func() {
-		log.Debug("Saving state before shutting down")
+		seelog.Debug("Saving state before shutting down")
 		stateSaved <- saver.ForceSave()


Excuse my not-Go-savviness... Should this have a trap/catch with a log so in the event that the state was not saved correctly... its at least logged?

The saver.ForceSave sends its results on down the stateSaved channel and its up to the other side to determine what to do about that. However.. we could be better on how we log failures and its context.

The log line is above, on line 83. That function will get triggered on a timeout so we have a record that the save failed.

adnxn · 2017-11-13T17:01:25Z

@samuelkarp #1009 merged =]

jahkeup · 2017-11-13T17:09:37Z

agent/app/agent_windows.go

+	case <-agentDone:
+		// This means that the agent stopped on its own.  This is where it's appropriate to light the event log on fire
+		// and set off all the alarms.
+		seelog.Error("Exiting")


Can we add more context in this exit log message?

There isn't a ton of context here except that the agent stopped on its own... What would you like me to add here?

jahkeup · 2017-11-13T17:18:08Z

agent/app/agent_windows.go

+}
+
+// sleepCtx provides a cancelable sleep
+func sleepCtx(ctx context.Context, duration time.Duration) {


This can instead use the stdlib (or golang.org/x/net) context.WithDeadline function and wait on ctx.Done() of that context.

golang.org/x/sys/windows/svc golang.org/x/sys/windows/svc/eventlog

tehshwann

Do the things!

cabbruzzese

Looks good. Found some small suggestions and nits.

cabbruzzese · 2017-11-14T18:23:41Z

README.md


 ```powershell
 PS C:\> # Set up directories the agent uses
-PS C:\> New-Item -Type directory -Path $ProgramFiles\Amazon\ECS
-PS C:\> New-Item -Type directory -Path $ProgramData\Amazon\ECS
+PS C:\> New-Item -Type directory -Path ${env:ProgramFiles}\Amazon\ECS -Force


nit: Even thought it's case insensitive, powershell docs capitalize Env

Example: https://docs.microsoft.com/en-us/powershell/module/microsoft.powershell.core/about/about_environment_variables?view=powershell-5.1

I'm going to leave this as-is for now; we can come back and fix it later.

cabbruzzese · 2017-11-14T18:54:15Z

agent/app/agent_windows_test.go

+	wg.Wait()
+}
+
+func TestHandler_Execute_WindowsStops(t *testing.T) {


A service has 30 seconds to report back a stopped status, and a 20 seconds to respond to a shutdown event. Can you add an assert to make sure it does not exceed these timeouts?
https://msdn.microsoft.com/en-us/library/windows/desktop/ms685149(v=vs.85).aspx

Unit tests are run with a 25s timeout right now. This completes in about a second. Our unit tests aren't really able to simulate the worst-case right now, so I'd rather not add an assert that won't tell us anything or try to make the unit test different. Instead, I think this can be addressed in a higher-level acceptance test (probably outside the agent itself).

cabbruzzese · 2017-11-14T18:57:51Z

agent/logger/eventlog_windows.go

+
+const (
+	eventLogName = "AmazonECSAgent"
+	eventLogID   = 999


It would help to list the eventid in the Readme or in a comment so that it is easy to search. Even though windows event log id's are specific to the application and not always well documented, administrators like to be able to find the id quickly in a search.

samuelkarp · 2017-11-14T19:14:17Z

@cabbruzzese Not sure why I can't seem to reply inline to your comment about the event log ID. Is it important to include it in the README even though it's a constant right now?

cabbruzzese · 2017-11-14T20:25:24Z

Was just looking for the event id to have better visibility on search engines for future debugging. Not a blocker.

cabbruzzese

LGTM

ofiliz · 2017-11-14T20:39:45Z

README.md

+>> .\hostsetup.ps1
+>> }
+PS C:\> # Install the agent service
+PS C:\> New-Service -Name "AmazonECS" `


Isn't it confusing the call the ECS Agent service "AmazonECS"? This is the agent. Would it be better to call it "AmazonECSAgent"?

Sorry if this topic came up before, feel free to ignore & point me to any existing discussion.

I named it "AmazonECS" so that we'd have the flexibility to swap out a bit in terms of implementation without requiring customers to change the name that they're using or making the name confusing. Specifically, this enables us to potentially transition to running the agent inside a container and using a service on the host to manage the container (similar to what we do with ecs-init on Linux). If we called it "AmazonECSAgent", we could still do that but it could create confusion because the service is separate from the agent.

AmazonECSService?

samuelkarp · 2017-11-14T22:20:15Z

Service starts correctly on boot when service is set to automatic startup or delayed start

I had success when setting the service Startup Type to "Automatic (Delayed Start)". Setting it to "Automatic" caused it to try to start before networking was available and then fail because it couldn't retrieve the instance identity document. For now, I would say that we want to recommend "Automatic (Delayed Start)" and as a follow up later we should implement retries for getting the instance identity document.

petderek

Nice! Had some questions but overall it looks good

petderek · 2017-11-14T21:27:13Z

agent/app/agent.go

 	// start starts the Agent execution
 	start() int
+	// setTerminationHandler sets the termination handler
+	setTerminationHandler(sighandlers.TerminationHandler)


petderek · 2017-11-14T21:53:50Z

agent/app/agent_windows.go

+		h.ecsAgent.start() // should block forever, unless there is an error
+		// TODO: distinguish between recoverable and unrecoverable errors
+		indicator.agentStopped()
+		cancel()


should this be a defered cancel?

Possibly. I will be changing some of this in the next PR.

petderek · 2017-11-14T22:02:27Z

agent/app/agent_windows.go

+	<-derivedCtx.Done()
+}
+
+type termHandlerIndicator struct {


I like the way this indicator struct is being used. Out of curiosity -- is this implementing anything?

Not that I'm aware of, but I didn't do a very deep search yet.

petderek · 2017-11-14T22:15:25Z

agent/app/agent_windows.go

+
+func (t *termHandlerIndicator) wait() {
+	t.mu.Lock()
+	invoked := t.handlerInvoked


I don't completely understand how the 'invoked' state is consumed. Is this the only place 'invoked' is used?

Yep. This is just to determine whether we need to wait on the channel or not.

petderek · 2017-11-15T02:07:07Z

README.md

+>> .\hostsetup.ps1
+>> }
+PS C:\> # Install the agent service
+PS C:\> New-Service -Name "AmazonECS" `


AmazonECSService?

petderek · 2017-11-15T02:08:59Z

agent/logger/eventlog_windows.go

+
+// ReceiveMessage receives a log line from seelog and emits it to the Windows event log
+func (r *eventLogReceiver) ReceiveMessage(message string, level seelog.LogLevel, context seelog.LogContextInterface) error {
+	switch level {


How tightly is this coupled to seelog? Are we adding significant work for if we want to swap out logging implementations?

This is Seelog's interface, so it's somewhat tightly coupled. However, the Windows event log interface is pretty simple so it should be easy to change this if we switch to a different logger as long as that logger has a pluggable backend.

petderek · 2017-11-15T02:24:01Z

agent/app/agent_windows.go

+	t.agentRunning = false
+}
+
+func (t *termHandlerIndicator) done() {


Would recommend this be called 'finish' or 'end'. This isn’t a strong preference, just that 'done()' is used by the context class for listening instead of terminating.

Sure, will change in the next PR.

samuelkarp · 2017-11-15T19:18:44Z

Merging this into the windows branch for now. I will have a follow-up PR to adjust what we're doing with the failure/recovery policy and address any additional comments.

samuelkarp requested review from aaithal, petderek, ofiliz and tehshwann November 12, 2017 01:40

samuelkarp added the os/windows label Nov 12, 2017

tehshwann reviewed Nov 13, 2017

View reviewed changes

jahkeup reviewed Nov 13, 2017

View reviewed changes

samuelkarp added 7 commits November 13, 2017 14:16

vendor: Vendor new packages for Windows service

2954177

golang.org/x/sys/windows/svc golang.org/x/sys/windows/svc/eventlog

logger: send logs to the Windows event log

6eadfe7

app: Implement Windows service for agent

c3d7f6c

app: tests for handler.runAgent

d3d57cd

app: tests for handling service events

2f7dc6d

app: Add tests for handler.Execute

b87ab7e

readme: Update readme for Windows service

4aea88d

samuelkarp force-pushed the windows-service branch from 09ea837 to 4aea88d Compare November 13, 2017 22:18

app: fix race condition in Windows service

903ec56

samuelkarp force-pushed the windows-service branch from e1ea0c7 to 903ec56 Compare November 14, 2017 01:14

samuelkarp changed the title ~~[WIP] Windows service~~ Windows service Nov 14, 2017

samuelkarp added 3 commits November 14, 2017 02:11

readme: Minor changes to Windows scripts

9ce866b

app: fix a comment

d8c4796

app: simplify sleepCtx

d83b49a

tehshwann approved these changes Nov 14, 2017

View reviewed changes

cabbruzzese reviewed Nov 14, 2017

View reviewed changes

cabbruzzese approved these changes Nov 14, 2017

View reviewed changes

ofiliz reviewed Nov 14, 2017

View reviewed changes

petderek approved these changes Nov 15, 2017

View reviewed changes

samuelkarp changed the base branch from dev to windows November 15, 2017 19:17

samuelkarp merged commit d83b49a into aws:windows Nov 15, 2017

This was referenced Nov 15, 2017

Windows ScheduledJob does not start on reboot until RDP #809

Closed

Windows merge #1106

Merged

samuelkarp added this to the 1.16.0 milestone Nov 22, 2017

Windows service #1070

Windows service #1070

Conversation

samuelkarp commented Nov 12, 2017 • edited Loading

Summary

Implementation details

TODO

Testing

Description for the changelog

Licensing

tehshwann 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

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

adnxn commented Nov 13, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jahkeup Nov 13, 2017 • edited Loading

Choose a reason for hiding this comment

tehshwann left a comment

Choose a reason for hiding this comment

cabbruzzese 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

samuelkarp commented Nov 14, 2017

cabbruzzese commented Nov 14, 2017

cabbruzzese 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

samuelkarp commented Nov 14, 2017

petderek 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

samuelkarp commented Nov 15, 2017

samuelkarp commented Nov 12, 2017 •

edited

Loading

jahkeup Nov 13, 2017 •

edited

Loading