add `supervise` subcommand (supervisor mode) #327

Songmu · 2017-02-06T15:57:04Z

Usage

mackerel-agent supervise -conf ...

Description

The supervisor mode launches the mackerel-agent as a child process and controls it.
In paticular, it does the following things.

reloading mackerel-agent with HUP signal
crash recovery
- However, if the child process dies within 30 seconds, it is regarded as launching failure and terminate the process without crash recovery

mattn

[super nit] Let's discuss how we implement this on Windows in later.

mattn · 2017-02-06T16:04:21Z

fork.go

+	signal.Notify(c, os.Interrupt, syscall.SIGTERM, syscall.SIGQUIT, syscall.SIGHUP)
+	go func() {
+		for sig := range c {
+			if sig == syscall.SIGHUP {


syscall.SIGHUP, SIGTERM, SIGQUIT, SIGHUP doesn't work on Windows.

mattn · 2017-02-06T16:07:34Z

fork.go

+func (cm *cmdManager) reload() error {
+	// TODO configtest
+	cm.hupped = true
+	return cm.cmd.Process.Signal(syscall.SIGTERM)


Songmu · 2017-02-07T02:07:28Z

@mattn I have no plan to support supervise mode on windows.
Because wix/wrapper should do supervise on windows.

mattn · 2017-02-07T02:09:27Z

Okay, then please put build-constraint to avoid compilation errors.

mattn · 2017-02-07T04:12:44Z

Ah, sorry. I thought Windows didn't have definition of SIGQUIT or some signals (which doesn't work on Windows). Then, your change is best for fixing my issue. Thanks!

… behavior)

https://golang.org/pkg/os/signal/#Notify

haya14busa · 2017-02-21T03:34:03Z

supervisor.go

+	cmd      *exec.Cmd
+	startAt  time.Time
+	signaled bool
+	hupped   bool


Isn't it better to guard signaled and hupped with mutex?

and we should run go test with -race in CI, i guess...

fixed at 1356a53

haya14busa · 2017-02-21T03:39:17Z

supervisor.go

+}
+
+func (sv *supervisor) supervise() error {
+	sv.start()


maybe it's better to check err from sv.start() here in case the command is something wrong.

fixed at a225f87

haya14busa · 2017-02-21T03:40:20Z

supervisor_test.go

+
+	err := <-done
+	if err != nil {
+		t.Errorf("error should be nil but: %s", err)


[nit] should be %v for err format?

fixed at 52d2cb1

haya14busa · 2017-02-21T05:06:59Z

supervisor_test.go

+	time.Sleep(50 * time.Millisecond)
+	ch <- os.Interrupt
+
+	err := <-done


we can remove done and just use <- sv.wait() instead.

I changed interface of supervisor.supervise() at a85cbd2.

How about it?

haya14busa

I left some comments, but it works great!

I confirmed child process will be restarted with kill -HUP {parent process} or kill -INT {child process}, kill -INT {parent process} successfully terminates the supervisor processes... and so on.

haya14busa · 2017-02-21T12:27:23Z

util/pid.go

+package util
+
+// ExistsPid check if pid exists
+func ExistsPid(pid int) bool {


[nit] s/check/checks

And comment sentences is preferred to be end with period. It may help godoc formatting or other tools. but, nevermind.

https://github.com/golang/go/wiki/CodeReviewComments#comment-sentences

fixed at e13d0fd

haya14busa · 2017-02-21T12:38:46Z

util/pid.go

@@ -0,0 +1,6 @@
+package util


Hmm... there were already "util" package, so I think it's ok to some extent...
But, how about creating "pidutil" package which only handles pid files?
Maybe we can puts ExistsPid, createPidFile and removePidFile funcs into pidutil package.

We should avoid almost-meaning less package name and do not make meaning less package bigger as much as possible, i think.
https://github.com/golang/go/wiki/CodeReviewComments#package-names

Second option. How about putting back existsPid to main package and just little copy-pasting existsPid func to supervisor package? supervisor package uses it in only test files and it's really small function.

A little copying is better than a little dependency.
https://go-proverbs.github.io/

fixed at bc8bba7.

haya14busa · 2017-02-21T12:45:23Z

supervisor/supervisor.go

+
+// Supervise the mackerel-agent
+func (sv *Supervisor) Supervise(c chan os.Signal) error {
+	err := sv.start()


[nit] Since if accepts an initialization statement, it's common to see one used to set up a local variable. We can keep "err" variable scope short.

if err := sv.start(); err != nil { return err }

https://golang.org/doc/effective_go.html#if

fixed at e13d0fd

haya14busa · 2017-02-21T13:30:42Z

supervisor/supervisor.go

+		return err
+	}
+	if c == nil {
+		c = make(chan os.Signal, 1)


it's better to close c not to leak go sv.handleSignal(c) goroutine.

defer close(c)

I also confirmed there is a leak with https://github.com/fortytw2/leaktest

Also, it's better to document c chan os.Signal arg can be nil and if it's not nil, the caller has responsibility to close it. Or, we can just say "do not use this package as library", i guess?

e.g.

// Supervise starts a child mackerel-agent process and supervises it. // 'c' can be nil and it's tipically nil. When you pass signal channel to this // method, you have to close the channel to stop internal goroutine. func (sv *Supervisor) Supervise(c chan os.Signal) error {

fixed at 5966779.

haya14busa · 2017-02-21T13:35:24Z

supervisor/supervisor.go

+}
+
+// Supervise the mackerel-agent
+func (sv *Supervisor) Supervise(c chan os.Signal) error {


Interface can be something like this and it looks simple...?

func Supervise(agentProg string, argv []string, c chan os.Signal) error {

Also, with this interface, we don't have to care the caller side modifies sv.Prog or sv.ArgV.

But, maybe.... it's ok as it is since we uses *Supervisor in test and we don't have to care caller side so much because we expects it's not used as library (right?)....

fixed at 5966779.

astj

Looks good, but I wrote some questions and comments.

astj · 2017-02-22T06:04:29Z

pidfile/pidfile_test.go

@@ -0,0 +1,61 @@
+// +build !windows


For which reason this test is skipped on windows?
Create and Remove is performed in windows too, so it's better test them on windows

I guess this skip is due to missing implementation of existsPid, but even so it's better there's some tests for Windows (maybe in another file like pidfile_windows_test.go).

astj · 2017-02-22T06:23:01Z

supervisor/supervisor.go

+				logger.Warningf("failed to reload: %s", err.Error())
+			}
+		} else {
+			sv.stop(sig)


[imo]
Isn't it better supervisor outputs a log here like L149?

(Especially here, logging which signal the supervisor received is maybe good)

astj · 2017-02-22T06:28:38Z

supervisor/supervisor.go

+}
+
+func (sv *supervisor) supervise(c chan os.Signal) error {
+	if err := sv.start(); err != nil {


[imo]
same as L155

astj · 2017-02-22T06:32:36Z

supervisor/supervisor.go

@@ -0,0 +1,171 @@
+package supervisor


Since supervisor.go is not used in Windows, so is it possible to add // +build !windows and add dummy supervisor_windows.go?
(I don't know the best way. One might think current way is better because of simplicity).

astj · 2017-02-22T06:33:12Z

supervisor/testdata/stub-agent.go

@@ -0,0 +1,45 @@
+package main


Cute stub ♥️

The comment meant following code if c == nil { c = make(chan os.Signal, 1) defer close(c) } but it actually looks like this if c == nil { c = make(chan os.Signal, 1) } defer close(c) I think both are fine but comment should be true. ref: mackerelio#327 (comment)

Songmu added 10 commits February 4, 2017 22:40

start to implement fork mode. define doFork

6c9fddf

add signal handler

56cc736

define cmdManager

9b0cc93

implement reloader

64a9b7b

configtest before reloading

1208a9c

rename subcommand s/fork/supervise/g

a24e8cc

output precise error message when configtest failed on reloading

fb251c9

add -child option for child process of supervise mode

28a7a56

create pidfile on supervise process

ec695e2

adjustment

5dadfc9

mattn reviewed Feb 6, 2017

View reviewed changes

supervise mode is not supported on windows

43aa150

Songmu added 15 commits February 10, 2017 23:51

define sv.handleSignal

feae6fa

TestSupervise

7322273

add TestSuperviseReload

862320d

adjust tests

d676e79

update .goxc.json

154dd13

add TestSupervisorReloadFail

81d9bfc

update .goxc.json

d274c5d

stub-agent normally exit when signal received (same as mackerel-agent…

7a28b76

… behavior)

enhance test cases

ae62675

add TestSupervisor_crashRecovery

288de1c

sleep 50 msec in TestSupervisor

923fe11

remove Config.Child field

601f518

We must use a buffered channel on signal handling

3eb19a5

https://golang.org/pkg/os/signal/#Notify

comment and log enhancement

cd53169

care loglevel in supervisor mode

fd05b57

haya14busa reviewed Feb 21, 2017

View reviewed changes

Songmu added 13 commits February 21, 2017 15:12

care race conditions

1356a53

change interface of supervisor.supervise()

a85cbd2

fix format string of Errorf

52d2cb1

add error handling of sv.start()

a225f87

Merge remote-tracking branch 'origin/master' into mackerel-agent_safe

6305c9f

fix lock sequence

b2cecb8

fix comment

30e6197

adjustment

0cb5e85

separate supervisor package from main

a3c9abe

add document comment of util.ExistsPid

219c802

add document comment

02d373e

Merge branch 'packagize_supervisor' into mackerel-agent_safe

c29ef61

upadte .goxc.json

46b93f4

haya14busa reviewed Feb 21, 2017

View reviewed changes

Songmu added 6 commits February 22, 2017 01:23

change interface of supervisor. expose only the Supervise function.

5966779

fix code styles

e13d0fd

separate pidfile package from main

bc8bba7

add tests for pidfile

27cac77

s/Testsupervisor/TestSupervisor/g

65e296e

TestSupervise

4061e7d

astj reviewed Feb 22, 2017

View reviewed changes

Songmu merged commit 01eab3d into master Feb 22, 2017

Songmu deleted the mackerel-agent_safe branch February 22, 2017 08:09

haya14busa mentioned this pull request Feb 24, 2017

[nit] fix comment of supervisor #345

Merged

This was referenced Mar 7, 2017

Release version 0.40.1 #349

Closed

Release version 0.41.0 #350

Merged

astj mentioned this pull request Oct 19, 2017

Use mackerel-agent supervisor mode mackerelio/homebrew-mackerel-agent#21

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add `supervise` subcommand (supervisor mode) #327

add `supervise` subcommand (supervisor mode) #327

Songmu commented Feb 6, 2017 •

edited

Loading

mattn left a comment

mattn Feb 6, 2017

mattn Feb 6, 2017

Songmu commented Feb 7, 2017

mattn commented Feb 7, 2017

mattn commented Feb 7, 2017 •

edited

Loading

haya14busa Feb 21, 2017

haya14busa Feb 21, 2017

Songmu Feb 21, 2017

haya14busa Feb 21, 2017

Songmu Feb 21, 2017

haya14busa Feb 21, 2017

Songmu Feb 21, 2017

haya14busa Feb 21, 2017

Songmu Feb 21, 2017

haya14busa left a comment

haya14busa Feb 21, 2017

Songmu Feb 21, 2017

haya14busa Feb 21, 2017

Songmu Feb 21, 2017

haya14busa Feb 21, 2017

Songmu Feb 21, 2017

haya14busa Feb 21, 2017

Songmu Feb 21, 2017

haya14busa Feb 21, 2017

Songmu Feb 21, 2017

astj left a comment

astj Feb 22, 2017

astj Feb 22, 2017

astj Feb 22, 2017

astj Feb 22, 2017

astj Feb 22, 2017

astj Feb 22, 2017

astj Feb 22, 2017

add supervise subcommand (supervisor mode) #327

add supervise subcommand (supervisor mode) #327

Conversation

Songmu commented Feb 6, 2017 • edited Loading

Usage

Description

mattn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Songmu commented Feb 7, 2017

mattn commented Feb 7, 2017

mattn commented Feb 7, 2017 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

haya14busa 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

astj 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

add `supervise` subcommand (supervisor mode) #327

add `supervise` subcommand (supervisor mode) #327

Songmu commented Feb 6, 2017 •

edited

Loading

mattn commented Feb 7, 2017 •

edited

Loading