Implement 1459203 (remove NSSM), 1436274 (use eventlog) #214

milescrabill · 2019-07-08T18:17:53Z

There's a ton here. Too much!

Continued from #210

adds dependency on gopkg.in/natefinch/lumberjack.v2 for logfile management
removes NSSM and modifies docs, userdata in worker_types, etc. as such
changes the way the main RunWorker function is called for flexibility, it now accepts a signal chan
split out config handling and exit code handling into functions
adds debug logic for panic scenario exits including writing out a crash log in scenarios where other logging might be failing
support for installing/removing the generic worker service using native go
support for specifying configuration location at install time
support for windows event log when run as a service
logging to logfile by default in windows
remove need for a bat script wrapper to change the service working directory, cwd is set to parent dir of worker exe

Usage testing:

verify: starting the service in interactive mode
verify: starting the service as a service works properly
verify: under expected use, can ctrl-c to properly exit from interactive mode
- caveat: not running as LocalUser sticks us into a loop that cannot be broken by interrupts!

TODO:

fix CI
actually write tests
niceify commits
test: install the service, verify the service exists, verify the event log source exists, remove the service
test: starting the service without the event log source in non-interactive mode should fail with exit code 78
test: adding a broken io.Writer to the logWriter MultiWriter will be caught (worth testing?)
should logfile location be configurable? (currently: generic-worker.log, rotated by lumberjack)
should logfile management be in scope or is that an infra concern?

Weird things:

running in interactive mode prints an extra blank newline in between log entries

coveralls · 2019-07-08T18:32:19Z

Coverage increased (+0.03%) to 77.126% when pulling 5dff72c on no-more-nssm-merge-20190619 into 6737dbf on master.

.gitattributes

petemoore

It looks awesome, thanks so much for all your hard work, especially your care and attention to detail, ensuring we have some solid tests, and configurable options where they make sense, over hardcoded values.

.gitattributes

README.md

build.cmd

petemoore · 2019-07-09T18:21:23Z

helper_test.go

@@ -225,7 +225,8 @@ func execute(t *testing.T, expectedExitCode ExitCode) {
 	if err != nil {
 		t.Fatalf("Test setup failure - could not write to tasks-resolved-count.txt file: %v", err)
 	}
-	exitCode := RunWorker()
+	interruptChan := make(chan os.Signal, 1)
+	exitCode := RunWorker(interruptChan)


Does this allow us to Ctrl-C out of tests now?

I believe it does, yes. Were we not able to before?

I changed this to match the signature of the function, which I had changed before running the tests (yikes!) :P

helper_windows_test.go

petemoore · 2019-07-09T18:46:35Z

simple_docker.go

+func remove(arguments map[string]interface{}) (err error) {
+	return nil
+}
+


Isn't the remove target only needed on Windows?

Yeah, I added this to match install() but I don't think either are needed now that we're doing things in platformTargets()

usage.go

usage_windows.go

petemoore · 2019-07-09T18:51:42Z

usage_windows.go

-                                            installed under. [default: Generic Worker]`
+                                            installed under. [default: Generic Worker]
+    --working-directory DIRECTORY           The working directory the Generic Worker
+                                            service will use. [default: C:\Windows\system32]`


Since the worker writes files to the working directory, can we change the default to e.g. C:\generic-worker? Perhaps the correct drive letter can be determined from the system, in case the operating system isn't installed on C:.

What would the behavior be if that directory doesn't exist?

I think I'm going to change the behavior here.

If no --working-directory is specified, we default to the parent directory of the generic-worker executable. If one is specified, we use it.

multiuser_service_windows.go

petemoore · 2019-07-11T15:51:40Z

multiuser_service_windows.go

+	}
+	log.Printf("Successfully configured eventlog source %q.", name)
+
+	// start service manually in order to fail fast


When we create a machine image with generic-worker on it, we typically want to install it without starting it up (since it is just the machine image that real workers will use, and is not a real worker itself), so I think we don't want to start it up here, otherwise it would run on the machine which is not a worker, but it used to create the machine image for the actual workers.

petemoore · 2019-07-12T07:34:15Z

multiuser_service_windows.go

+var logWriter = io.MultiWriter(ioutil.Discard)
+
+func init() {
+	// TODO make this work with --working-directory


I think that is done now, isn't it?

This always changes the working directiry to the parent directory of the generic worker binary.

petemoore · 2019-07-12T07:37:16Z

multiuser_service_windows.go

@@ -282,6 +284,8 @@ func removeService(name string) error {
 		return fmt.Errorf("service %s is not installed", name)
 	}
 	defer s.Close()
+	// throw away this error


Can you add a note to say why we ignore the error?

If this fails it's essentially because something went wrong interacting with the service, i.e. it's not running, it didn't respond, etc. We don't care about an error in this scenario, we're just trying to delete it.

Perfect. Could you include that as a code comment?

petemoore · 2019-07-12T07:39:29Z

multiuser_service_windows_test.go

 	// but not actually removed until reboot

-	// TODO
+	cleanupService(t, name)


What made you decide to move cleanupService out of the defer block?

The commented out blocks below, if uncommented, would happen before a deferred call to cleanupService()

petemoore · 2019-07-12T07:40:25Z

multiuser_windows.go

-			os.Chdir(cwd)
+		// default to generic-worker executable parent dir
+		if cwd == "" {
+			dir := path.Dir(os.Args[0])


Suggested change

dir := path.Dir(os.Args[0])

dir := filepath.Dir(os.Args[0])

petemoore · 2019-07-12T07:41:44Z

usage_windows.go

@@ -62,7 +62,7 @@ func platformCommandLineParameters() string {
    --service-name SERVICE-NAME             The name that the Windows service should be
                                            installed under. [default: Generic Worker]
    --working-directory DIRECTORY           The working directory the Generic Worker
-                                            service will use. [default: C:\Windows\system32]`
+                                            service will use.`


Can we determine the system default at runtime and include it in the usage text?

I think defaulting to the parent directory of the generic worker binary is a reasonable default. It's also trivial to implement in a cross platform manner.

That's fine, but we should describe what the default is in the docs.

build.cmd: use multiuser build tag keep existing gitattributes directive

logging working w/ multiwriter!

add TestRunServiceWithoutEventlogSource, TestRunServiceWithBrokenWriter, use SKIP_ADMINISTRATOR_TESTS env var to skip service install/remove tests

…s locally rewrite runservice to return ExitCodes properly

… out some logic

use io.MultiWriter() instead of ioutil.Discard

helper_windows_test.go

…_TESTS

petemoore

Hi Miles,

I had a play with the Windows binaries today, and found some issues:

The generic-worker creates a log file even when you run generic-worker --help. The Windows service should direct stderr/stdout to a log file, but when generic-worker is invoked any other way, it should write to stdout/stderr without redirecting them somewhere else.
The remove service subcommand is not documented in usage docs. Please include some help text.
Same for run-service.
Same for the crash file.
When I had a crash, the crash file content said Error loading configuration file %v. I suspect this might just be a log.Print somewhere that should be a log.Printf.
I'm not sure we have a use case for calling run-service from the command line with arguments (such as --config). My understanding was that running generic-worker install service ... would install the worker as a Windows Service, and from that point on, the service is configured, so doesn't need options to be passed to it. The user would just start it or stop it. I thought running generic-worker run-service would be equivalent to running net start "Generic Worker".

I think we can get away entirely without having a run-service subcommand, because net start "Generic Worker" is already available, and then the Windows service would invoke the run subcommand, running as LocalSystem, and redirect stdout/stderr to the log file. The run subcommand can test to see whether it is being run interactively or not, if it is necessary for switching the logging mode, etc. Alternatively, you could add a --log LOG-FILE option to run that gets passed when invoked as a Windows service.
generic-worker run isn't logging to stdout/stderr when run interactively.
run-service is named inconsistently with remove service and install service which use separate words (rather than a hyphen), but this issue should disappear when we remove run-service and adapt run instead (see point 6).
generic-worker install service is also directed to a log rather than stderr/stdout.
I get a panic running the install step as Administrator. If the panic is due to missing files or anything else, we should try to report that and avoid crashing the worker:

C:\Users\Peter James Moore\Desktop>.\generic-worker-windows-386.exe install service
panic: runtime error: invalid memory address or nil pointer dereference
[signal 0xc0000005 code=0x0 addr=0x8 pc=0x7a023c]

goroutine 1 [running]:
github.com/taskcluster/generic-worker/vendor/golang.org/x/sys/windows/svc/mgr.(*Service).Start(0x0, 0x1265ef60, 0x3, 0x4, 0x1, 0x0)
        Z:/task_1563291642/gopath1.10.8/src/github.com/taskcluster/generic-worker/vendor/golang.org/x/sys/windows/svc/mgr/service.go:45 +0xcc
main.deployService(0x126378a9, 0x15, 0x124cd32b, 0xe, 0x126ac190, 0x41, 0xaf000000, 0x0, 0x0)
        Z:/task_1563291642/gopath1.10.8/src/github.com/taskcluster/generic-worker/multiuser_service_windows.go:206 +0x334
main.install(0x1265ef40, 0x1265ef40, 0x8f7dda)
        Z:/task_1563291642/gopath1.10.8/src/github.com/taskcluster/generic-worker/multiuser_windows.go:242 +0x25a
main.platformTargets(0x1265ef40, 0x1265ef40)
        Z:/task_1563291642/gopath1.10.8/src/github.com/taskcluster/generic-worker/multiuser_windows.go:428 +0x6db
main.main()
        Z:/task_1563291642/gopath1.10.8/src/github.com/taskcluster/generic-worker/main.go:123 +0x1b5

If the generic-worker binary is being installed to a different location than it currently resides (e.g. if it is downloaded to Downloads or Desktop etc) then the binary should probably be copied to the target destination folder as part of the installation process.

Admittedly we didn't do this before, so this is minor scope creep. I think it would make sense to tackle this at the same time though. If we don't do this, we should at least highlight in the usage docs, that the Windows service will be configured to use the generic-worker binary from its current location, so the generic-worker binary should be moved to its desired installation location before running the install subcommand.

Thanks for all your hard work so far on this so far. It is looking really good!

petemoore · 2019-12-07T09:00:17Z

@milescrabill This has kind of bitrotted. Should we close it, or do you want to return to it?

petemoore reviewed Jul 9, 2019

View reviewed changes

.gitattributes Outdated Show resolved Hide resolved

petemoore suggested changes Jul 9, 2019

View reviewed changes

petemoore reviewed Jul 11, 2019

View reviewed changes

multiuser_service_windows.go Outdated Show resolved Hide resolved

petemoore reviewed Jul 11, 2019

View reviewed changes

petemoore reviewed Jul 12, 2019

View reviewed changes

petemoore force-pushed the no-more-nssm-merge-20190619 branch from 012d1ce to ef04a9b Compare July 12, 2019 08:09

Miles Crabill added 12 commits July 12, 2019 11:52

replace NSSM with x/sys/windows/svc

8334134

move multiuser_windows_service.go => multiuser_service_windows.go

1b5bf2c

add windows vendored modules

f074efd

build.cmd: use multiuser build tag keep existing gitattributes directive

use elogWrapper to wrap eventlog and set it as a log output

cd9f14a

logging working w/ multiwriter!

use CANT_LOG_PROPERLY for eventlog open fail

b438d47

windows: run go test with -tags multiuser

ecc18fc

change CANT_REMOVE_GENERIC_WORKER code to a fresh one

1cbeffa

add a test for install/remove service

99d9d7b

add TestRunServiceWithoutEventlogSource, TestRunServiceWithBrokenWriter, use SKIP_ADMINISTRATOR_TESTS env var to skip service install/remove tests

split out eventlog configuration from service configuration, fix test…

115afd3

…s locally rewrite runservice to return ExitCodes properly

fix race in Execute() / TestWindowsServiceInteraction

2a46fe7

cleanup service interaction test, more dodging race conditions, split…

9c85674

… out some logic

address review feedback

518722b

use io.MultiWriter() instead of ioutil.Discard

milescrabill force-pushed the no-more-nssm-merge-20190619 branch from 20b84d8 to 518722b Compare July 12, 2019 18:54

flesh out throwaway error comment

77f8dcb

milescrabill marked this pull request as ready for review July 16, 2019 14:54

milescrabill changed the title ~~WIP: Implement 1459203 (remove NSSM), 1436274 (use eventlog)~~ Implement 1459203 (remove NSSM), 1436274 (use eventlog) Jul 16, 2019

milescrabill requested a review from a team July 16, 2019 14:54

petemoore reviewed Jul 16, 2019

View reviewed changes

helper_windows_test.go Outdated Show resolved Hide resolved

rename env var to skip admin required tests to GW_SKIP_ADMIN_REQUIRED…

6787581

…_TESTS

add GW_SKIP_ADMIN_REQUIRED_TESTS doc to README

5dff72c

petemoore suggested changes Jul 22, 2019

View reviewed changes

petemoore mentioned this pull request Aug 23, 2019

Bug 1565215 - include timezone in log timestamps and set to UTC #221

Merged

petemoore force-pushed the master branch from b8819ca to 9083237 Compare December 6, 2019 18:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement 1459203 (remove NSSM), 1436274 (use eventlog) #214

Implement 1459203 (remove NSSM), 1436274 (use eventlog) #214

milescrabill commented Jul 8, 2019 •

edited

Loading

coveralls commented Jul 8, 2019 •

edited

Loading

petemoore left a comment

petemoore Jul 9, 2019

milescrabill Jul 10, 2019

petemoore Jul 9, 2019

milescrabill Jul 11, 2019

petemoore Jul 9, 2019

milescrabill Jul 11, 2019

milescrabill Jul 11, 2019

petemoore Jul 11, 2019

petemoore Jul 12, 2019

milescrabill Jul 12, 2019

petemoore Jul 12, 2019

milescrabill Jul 12, 2019

petemoore Jul 15, 2019

petemoore Jul 12, 2019 •

edited

Loading

milescrabill Jul 12, 2019

petemoore Jul 12, 2019

petemoore Jul 12, 2019

milescrabill Jul 12, 2019

petemoore Jul 22, 2019

petemoore left a comment •

edited

Loading

petemoore commented Dec 7, 2019

Implement 1459203 (remove NSSM), 1436274 (use eventlog) #214

Are you sure you want to change the base?

Implement 1459203 (remove NSSM), 1436274 (use eventlog) #214

Conversation

milescrabill commented Jul 8, 2019 • edited Loading

Usage testing:

TODO:

Weird things:

coveralls commented Jul 8, 2019 • edited Loading

petemoore 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

petemoore Jul 12, 2019 • edited Loading

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

petemoore left a comment • edited Loading

Choose a reason for hiding this comment

petemoore commented Dec 7, 2019

milescrabill commented Jul 8, 2019 •

edited

Loading

coveralls commented Jul 8, 2019 •

edited

Loading

petemoore Jul 12, 2019 •

edited

Loading

petemoore left a comment •

edited

Loading