code review 4965061: gobuilder: timeout support, for flaky/hangy builders

gobuilder: timeout support, for flaky/hangy builders

The Windows builder likes to hang or spin.

[bradfitz, 2011-09-01 22:13:27]

Hello rsc@golang.org (cc: golang-dev@googlegroups.com),

I'd like you to review this change to
https://go.googlecode.com/hg/

[bradfitz, 2011-09-01 22:31:07]

Ignore the failures on the public dashboard here:

http://godashboard.appspot.com/

(linux-amd64-alt)

That was my testing this CL with a 5 second timeout (so it'd intentionally
fail).  I didn't realize they'd be published there.

Which leads to next question:  should timeouts count as failures?  Or should
they be retried once first?


On Thu, Sep 1, 2011 at 3:13 PM, <bradfitz@golang.org> wrote:

> Reviewers: rsc,
>
> Message:
> Hello rsc@golang.org (cc: golang-dev@googlegroups.com),
>
> I'd like you to review this change to
> https://go.googlecode.com/hg/
>
>
> Description:
> gobuilder: timeout support, for flaky/hangy builders
>
> The Windows builder likes to hang or spin.
>
> Please review this at
http://codereview.appspot.com/**4965061/<http://codereview.appspot.com/4965061/>
>
> Affected files:
>  M misc/dashboard/builder/exec.go
>  M misc/dashboard/builder/main.go
>
>
> Index: misc/dashboard/builder/exec.go
> ==============================**==============================**=======
> --- a/misc/dashboard/builder/exec.**go
> +++ b/misc/dashboard/builder/exec.**go
> @@ -32,6 +32,23 @@
>  // The error returned is nil, if process is started successfully,
>  // even if exit status is not 0.
>  func runLog(envv []string, logfile, dir string, argv ...string) (string,
> int, os.Error) {
> +       return runLogArgs{envv: envv, logfile: logfile, dir:
> dir}.run(argv...)
> +}
> +
> +// runLogArgs is like exec.Command, but with a few more
> +// gobuilder-specific fields, like logfile and procc.
> +type runLogArgs struct {
> +       envv []string // optional environment
> +       dir  string   // optional directory to run in
> +
> +       logfile string           // optional filename to write logs to
> +       procc   chan *os.Process // optional chan to send process once
> started, before finished
> +}
> +
> +func (ra runLogArgs) run(argv ...string) (string, int, os.Error) {
> +       if ra.procc != nil {
> +               defer close(ra.procc)
> +       }
>        if *verbose {
>                log.Println("runLog", argv)
>        }
> @@ -39,8 +56,8 @@
>
>        b := new(bytes.Buffer)
>        var w io.Writer = b
> -       if logfile != "" {
> -               f, err := os.OpenFile(logfile,
> os.O_WRONLY|os.O_CREATE|os.O_**APPEND, 0666)
> +       if ra.logfile != "" {
> +               f, err := os.OpenFile(ra.logfile,
> os.O_WRONLY|os.O_CREATE|os.O_**APPEND, 0666)
>                if err != nil {
>                        return "", 0, err
>                }
> @@ -49,12 +66,16 @@
>        }
>
>        cmd := exec.Command(argv[0], argv[1:]...)
> -       cmd.Dir = dir
> -       cmd.Env = envv
> +       cmd.Dir = ra.dir
> +       cmd.Env = ra.envv
>        cmd.Stdout = w
>        cmd.Stderr = w
>
> -       err := cmd.Run()
> +       err := cmd.Start()
> +       if err == nil && ra.procc != nil {
> +               ra.procc <- cmd.Process
> +       }
> +       err = cmd.Wait()
>        if err != nil {
>                if ws, ok := err.(*os.Waitmsg); ok {
>                        return b.String(), ws.ExitStatus(), nil
> Index: misc/dashboard/builder/main.go
> ==============================**==============================**=======
> --- a/misc/dashboard/builder/main.**go
> +++ b/misc/dashboard/builder/main.**go
> @@ -54,6 +54,7 @@
>        buildRelease  = flag.Bool("release", false, "Build and upload binary
> release archives")
>        buildRevision = flag.String("rev", "", "Build specified revision and
> exit")
>        buildCmd      = flag.String("cmd", "./all.bash", "Build command
> (specify absolute or relative to go/src/)")
> +       buildTimeout  = flag.Int("timeout", 0, "Optional build timeout, in
> seconds")
>        external      = flag.Bool("external", false, "Build external
> packages")
>        parallel      = flag.Bool("parallel", false, "Build multiple targets
> in parallel")
>        verbose       = flag.Bool("v", false, "verbose")
> @@ -292,8 +293,31 @@
>        srcDir := path.Join(workpath, "go", "src")
>
>        // build
> +       var procc chan *os.Process
> +       donec := make(chan bool, 1)
> +       if *buildTimeout != 0 {
> +               procc = make(chan *os.Process)
> +               go func() {
> +                       proc, ok := <-procc
> +                       if !ok {
> +                               return
> +                       }
> +                       select {
> +                       case <-donec:
> +                               return
> +                       case <-time.After(int64(***buildTimeout) * 1e9):
> +                               proc.Kill()
> +                       }
> +               }()
> +       }
>        logfile := path.Join(workpath, "build.log")
> -       buildLog, status, err := runLog(b.envv(), logfile, srcDir,
> *buildCmd)
> +       buildLog, status, err := runLogArgs{
> +               envv:    b.envv(),
> +               logfile: logfile,
> +               dir:     srcDir,
> +               procc:   procc,
> +       }.run(*buildCmd)
> +       donec <- true
>        if err != nil {
>                return fmt.Errorf("%s: %s", *buildCmd, err)
>        }
>
>
>

[brainman, 2011-09-01 23:58:02]

http://codereview.appspot.com/4965061/diff/8001/misc/dashboard/builder/main.go
File misc/dashboard/builder/main.go (right):

http://codereview.appspot.com/4965061/diff/8001/misc/dashboard/builder/main.g...
misc/dashboard/builder/main.go:310: proc.Kill()
I do not think this will work reliably on Windows. Your proc.Kill will kill top
level bash process (that waits for others and does nothing much else anyways),
but it might not be able to kill processes at fault (make). So you will end up
with some "left over" processes. These will prevent you from deleting file tree
they were building. It means you won't be able to create that tree again
(directory names are predefined by "hg id") to try and build it again. So, you
might decide to skip that "hg id" build. But then you will not be able to
restart gobuilder, because gobuilder recreates its complete tree in /tmp at
start-up.

I will try and test your change over the weekend, to see if my theory stands.

[adg, 2011-09-02 00:05:21]

http://codereview.appspot.com/4965061/diff/8001/misc/dashboard/builder/main.go
File misc/dashboard/builder/main.go (right):

http://codereview.appspot.com/4965061/diff/8001/misc/dashboard/builder/main.g...
misc/dashboard/builder/main.go:310: proc.Kill()
Does this actually work?

When I played with this before, proc.Kill would kill all.bash but not its child
processes. That's kinda the point when you're trying to stop a spinning test,
for example.

[bradfitz, 2011-09-02 00:17:23]

Alex,

Can we get a list of all processes on Windows?

Do processes have parents on Windows?

Can we get the working directory of a process on Windows? (is there such a
concept?)

Perhaps on timeout we can just brute-force it, going hunting for all the
things we need to kill.

Do you want to take this CL over, or work with me to add more os/Process
APIs to do process enumeration on Windows (and Linux, etc) too?

- Brad

On Thu, Sep 1, 2011 at 4:58 PM, <alex.brainman@gmail.com> wrote:

>
> http://codereview.appspot.com/**4965061/diff/8001/misc/**
>
dashboard/builder/main.go<http://codereview.appspot.com/4965061/diff/8001/misc/dashboard/builder/main.go>
> File misc/dashboard/builder/main.go (right):
>
> http://codereview.appspot.com/**4965061/diff/8001/misc/**
>
dashboard/builder/main.go#**newcode310<http://codereview.appspot.com/4965061/diff/8001/misc/dashboard/builder/main.go#newcode310>
> misc/dashboard/builder/main.**go:310: proc.Kill()
> I do not think this will work reliably on Windows. Your proc.Kill will
> kill top level bash process (that waits for others and does nothing much
> else anyways), but it might not be able to kill processes at fault
> (make). So you will end up with some "left over" processes. These will
> prevent you from deleting file tree they were building. It means you
> won't be able to create that tree again (directory names are predefined
> by "hg id") to try and build it again. So, you might decide to skip that
> "hg id" build. But then you will not be able to restart gobuilder,
> because gobuilder recreates its complete tree in /tmp at start-up.
>
> I will try and test your change over the weekend, to see if my theory
> stands.
>
>
>
http://codereview.appspot.com/**4965061/<http://codereview.appspot.com/4965061/>
>

[brainman, 2011-09-02 00:25:43]

On 2011/09/02 00:17:23, bradfitz wrote:
> Alex,
> 

I do not have answers to you questions. Let me look at them over the weekend.

Alex

[Joe Poirier, 2011-09-02 04:06:25]

On 2011/09/02 00:25:43, brainman wrote:
> On 2011/09/02 00:17:23, bradfitz wrote:
> > Alex,
> > 
> 
> I do not have answers to you questions. Let me look at them over the weekend.
> 
> Alex

FYI 
I've had pretty good luck avoiding the spins/hangs by rebuilding the gmake
executable on the windows machine it's being run on. 
ftp://ftp.gnu.org/gnu/make

-joe

[peterGo, 2011-09-02 04:50:46]

Brad,

What version of make are you using? I never have this problem. I use make
version 3.82-5.

$ make -v
GNU Make 3.82
Built for i386-pc-mingw32
Copyright (C) 2010  Free Software Foundation, Inc.
License GPLv3+: GNU GPL version 3 or later <http://gnu.org/licenses/gpl.html>
This is free software: you are free to change and redistribute it.
There is NO WARRANTY, to the extent permitted by law.

Peter

[bradfitz, 2011-09-02 05:11:35]

3.81

On Thu, Sep 1, 2011 at 9:50 PM, <go.peter.90@gmail.com> wrote:

> Brad,
>
> What version of make are you using? I never have this problem. I use
> make version 3.82-5.
>
> $ make -v
> GNU Make 3.82
> Built for i386-pc-mingw32
> Copyright (C) 2010  Free Software Foundation, Inc.
> License GPLv3+: GNU GPL version 3 or later
> <http://gnu.org/licenses/gpl.**html <http://gnu.org/licenses/gpl.html>>
> This is free software: you are free to change and redistribute it.
> There is NO WARRANTY, to the extent permitted by law.
>
> Peter
>
>
>
http://codereview.appspot.com/**4965061/<http://codereview.appspot.com/4965061/>
>

[Joe Poirier, 2011-09-02 05:16:57]

On Fri, Sep 2, 2011 at 12:11 AM, Brad Fitzpatrick <bradfitz@golang.org> wrote:
> 3.81
> On Thu, Sep 1, 2011 at 9:50 PM, <go.peter.90@gmail.com> wrote:
>>
>> Brad,
>>
>> What version of make are you using? I never have this problem. I use
>> make version 3.82-5.
>>
>> $ make -v
>> GNU Make 3.82
>> Built for i386-pc-mingw32
>> Copyright (C) 2010  Free Software Foundation, Inc.
>> License GPLv3+: GNU GPL version 3 or later
>> <http://gnu.org/licenses/gpl.html>
>> This is free software: you are free to change and redistribute it.
>> There is NO WARRANTY, to the extent permitted by law.
>>
>> Peter
>>
>> http://codereview.appspot.com/4965061/
>
>

Keep in mind that msys' modified version of make
(path magic) is recommended if you're using an msys shell
and/or environment; mingw's make != msys' make.

The link to the msys-make source is
http://sourceforge.net/projects/mingw/files/MSYS/make
if you decide to rebuild it.

-joe

[bradfitz, 2011-09-02 05:20:16]

On Thu, Sep 1, 2011 at 10:16 PM, Joseph Poirier <jdpoirier@gmail.com> wrote:

> On Fri, Sep 2, 2011 at 12:11 AM, Brad Fitzpatrick <bradfitz@golang.org>
> wrote:
> > 3.81
> > On Thu, Sep 1, 2011 at 9:50 PM, <go.peter.90@gmail.com> wrote:
> >>
> >> Brad,
> >>
> >> What version of make are you using? I never have this problem. I use
> >> make version 3.82-5.
> >>
> >> $ make -v
> >> GNU Make 3.82
> >> Built for i386-pc-mingw32
> >> Copyright (C) 2010  Free Software Foundation, Inc.
> >> License GPLv3+: GNU GPL version 3 or later
> >> <http://gnu.org/licenses/gpl.html>
> >> This is free software: you are free to change and redistribute it.
> >> There is NO WARRANTY, to the extent permitted by law.
> >>
> >> Peter
> >>
> >> http://codereview.appspot.com/4965061/
> >
> >
>
> Keep in mind that msys' modified version of make
> (path magic) is recommended if you're using an msys shell
> and/or environment; mingw's make != msys' make.
>

If it's recommended, why is whatever version I have (presumably the
default--- I followed the wiki instructions) wrong?

I don't know what "path magic" is.  Is that what makes it hang?

[peterGo, 2011-09-02 05:33:04]

Brad,

Copy the following three files from your C:\MinGW\bin directory to your
C:\MinGW\msys\1.0\bin directory:

mingw32-make.exe
libiconv-2.dll
libintl-8.dll

In your C:\MinGW\msys\1.0\bin directory, rename make.exe to msys-make.exe to
save it. Rename mingw32-make.exe to make.exe to replace the msys version. Run
make -v. You should see:

$ make -v
GNU Make 3.82
Built for i386-pc-mingw32
Copyright (C) 2010  Free Software Foundation, Inc.
License GPLv3+: GNU GPL version 3 or later <http://gnu.org/licenses/gpl.html>
This is free software: you are free to change and redistribute it.
There is NO WARRANTY, to the extent permitted by law.

Peter

On 2011/09/02 05:11:35, bradfitz wrote:
> 3.81
> 
> On Thu, Sep 1, 2011 at 9:50 PM, <mailto:go.peter.90@gmail.com> wrote:
> 
> > Brad,
> >
> > What version of make are you using? I never have this problem. I use
> > make version 3.82-5.
> >
> > $ make -v
> > GNU Make 3.82
> > Built for i386-pc-mingw32
> > Copyright (C) 2010  Free Software Foundation, Inc.
> > License GPLv3+: GNU GPL version 3 or later
> > <http://gnu.org/licenses/gpl.**html <http://gnu.org/licenses/gpl.html>>
> > This is free software: you are free to change and redistribute it.
> > There is NO WARRANTY, to the extent permitted by law.
> >
> > Peter
> >
> >
> >
>
http://codereview.appspot.com/**4965061/%3Chttp://codereview.appspot.com/4965...>

[Joe Poirier, 2011-09-02 05:36:36]

On Fri, Sep 2, 2011 at 12:20 AM, Brad Fitzpatrick <bradfitz@golang.org> wrote:
>
>
> On Thu, Sep 1, 2011 at 10:16 PM, Joseph Poirier <jdpoirier@gmail.com> wrote:
>>
>> On Fri, Sep 2, 2011 at 12:11 AM, Brad Fitzpatrick <bradfitz@golang.org>
>> wrote:
>> > 3.81
>> > On Thu, Sep 1, 2011 at 9:50 PM, <go.peter.90@gmail.com> wrote:
>> >>
>> >> Brad,
>> >>
>> >> What version of make are you using? I never have this problem. I use
>> >> make version 3.82-5.
>> >>
>> >> $ make -v
>> >> GNU Make 3.82
>> >> Built for i386-pc-mingw32
>> >> Copyright (C) 2010  Free Software Foundation, Inc.
>> >> License GPLv3+: GNU GPL version 3 or later
>> >> <http://gnu.org/licenses/gpl.html>
>> >> This is free software: you are free to change and redistribute it.
>> >> There is NO WARRANTY, to the extent permitted by law.
>> >>
>> >> Peter
>> >>
>> >> http://codereview.appspot.com/4965061/
>> >
>> >
>>
>> Keep in mind that msys' modified version of make
>> (path magic) is recommended if you're using an msys shell
>> and/or environment; mingw's make != msys' make.
>
> If it's recommended, why is whatever version I have (presumably the
> default--- I followed the wiki instructions) wrong?
> I don't know what "path magic" is.  Is that what makes it hang?
>

msys is a separate package from mingw and they each have a bin
folder where they their respective executables live. mingw/bin is
where the compiler tools live along with a plain vanilla make. The
msys bin folder contains a bunch of ported unix tools. Some of the
msys unix tools tools have been specifically modified to do
windows<->unix path conversions (path magic) under the hood.

[brainman, 2011-09-02 05:40:28]

On 2011/09/02 05:20:16, bradfitz wrote:
> 
> I don't know what "path magic" is.  Is that what makes it hang?

I use Joe's bundle. And make version is 

# make --version
GNU Make 3.81
Copyright (C) 2006  Free Software Foundation, Inc.
This is free software; see the source for copying conditions.
There is NO warranty; not even for MERCHANTABILITY or FITNESS FOR A
PARTICULAR PURPOSE.

This program built for i686-pc-msys

Reading this http://goo.gl/8undr, I decided to try and upgrade it. So I did 

mingw-get update
mingw-get install mingw32-make

But after upgrade my make version message didn't change. It appears, we use
C:\MinGW\msys\1.0\bin\make.exe, but mingw-get program installs new make into
C:\MinGW\bin\mingw32-make.exe.

If I ran

$ mingw32-make --version
GNU Make 3.82
Built for i386-pc-mingw32
Copyright (C) 2010  Free Software Foundation, Inc.
License GPLv3+: GNU GPL version 3 or later <http://gnu.org/licenses/gpl.html>
This is free software: you are free to change and redistribute it.
There is NO WARRANTY, to the extent permitted by law.

I have copied C:\MinGW\bin\mingw32-make.exe into C:\MinGW\msys\1.0\bin\make.exe,
and now I get

$ make --version
GNU Make 3.82
Built for i386-pc-mingw32
Copyright (C) 2010  Free Software Foundation, Inc.
License GPLv3+: GNU GPL version 3 or later <http://gnu.org/licenses/gpl.html>
This is free software: you are free to change and redistribute it.
There is NO WARRANTY, to the extent permitted by law.

Perhaps you will need more files as per peterGo's instructions.

I do not know, if this will fix our hanging. But I will be able to try it at
home on weekend and will let you know. You should backup your c:\MinGW directory
if you plan to fiddle with it.

Alex

[Joe Poirier, 2011-09-02 06:37:03]

An example of mingw's make failing.

Create a Makefile with the following rule

test:
    /c/MinGW/msys/1.0/bin/ls Makefile

then run the two from an msys shell:

$ /bin/make test
$ /mingw/bin/mingw32-make test


From what I've read, make's parallel build problem goes back several
years and as of early 2011 that hadn't change with the 3.82 builds at
the time, but that may not be the case with newer builds. In any case,
I've had good luck rebuilding the binary... YMMV.

-joe

On Fri, Sep 2, 2011 at 12:40 AM,  <alex.brainman@gmail.com> wrote:
> On 2011/09/02 05:20:16, bradfitz wrote:
>
>> I don't know what "path magic" is.  Is that what makes it hang?
>
> I use Joe's bundle. And make version is
>
> # make --version
> GNU Make 3.81
> Copyright (C) 2006  Free Software Foundation, Inc.
> This is free software; see the source for copying conditions.
> There is NO warranty; not even for MERCHANTABILITY or FITNESS FOR A
> PARTICULAR PURPOSE.
>
> This program built for i686-pc-msys
>
> Reading this http://goo.gl/8undr, I decided to try and upgrade it. So I
> did
>
> mingw-get update
> mingw-get install mingw32-make
>
> But after upgrade my make version message didn't change. It appears, we
> use C:\MinGW\msys\1.0\bin\make.exe, but mingw-get program installs new
> make into C:\MinGW\bin\mingw32-make.exe.
>
> If I ran
>
> $ mingw32-make --version
> GNU Make 3.82
> Built for i386-pc-mingw32
> Copyright (C) 2010  Free Software Foundation, Inc.
> License GPLv3+: GNU GPL version 3 or later
> <http://gnu.org/licenses/gpl.html>
> This is free software: you are free to change and redistribute it.
> There is NO WARRANTY, to the extent permitted by law.
>
> I have copied C:\MinGW\bin\mingw32-make.exe into
> C:\MinGW\msys\1.0\bin\make.exe, and now I get
>
> $ make --version
> GNU Make 3.82
> Built for i386-pc-mingw32
> Copyright (C) 2010  Free Software Foundation, Inc.
> License GPLv3+: GNU GPL version 3 or later
> <http://gnu.org/licenses/gpl.html>
> This is free software: you are free to change and redistribute it.
> There is NO WARRANTY, to the extent permitted by law.
>
> Perhaps you will need more files as per peterGo's instructions.
>
> I do not know, if this will fix our hanging. But I will be able to try
> it at home on weekend and will let you know. You should backup your
> c:\MinGW directory if you plan to fiddle with it.
>
> Alex
>
> http://codereview.appspot.com/4965061/
>

[rsc, 2011-09-02 15:32:52]

On Thu, Sep 1, 2011 at 20:05,  <adg@golang.org> wrote:
> Does this actually work?

Unlikely.  Kill is just a 'kill -9'.  What you want for
Unix is more like sending SIGTERM to the entire
process group / session / whatever they're called.

Russ

[brainman, 2011-09-03 00:57:44]

On 2011/09/02 05:40:28, brainman wrote:
> 
> 
> ... But I will be able to try it at
> home on weekend and will let you know. ...
> 

I tried new make, and it seems to be working OK. I have set MAKEFLAGS="-j8" (I
have 8 core PC) to stress our build process, but it works fine. I ran it ~20
times now and yet to see it fail. With old make I could break it on ~2-3
attempt.

I think you should update as per my instructions. It is not a certain solution,
but better then what we have. Thank you.

Alex

[peterGo, 2011-09-03 01:35:10]

Alex,

> I tried new make, and it seems to be working OK. I have set MAKEFLAGS="-j8" (I
> have 8 core PC) to stress our build process, but it works fine.

I run with MAKEFLAGS="-j". Why don't you?

Peter

On 2011/09/03 00:57:44, brainman wrote:
> On 2011/09/02 05:40:28, brainman wrote:
> > 
> > 
> > ... But I will be able to try it at
> > home on weekend and will let you know. ...
> > 
> 
> I tried new make, and it seems to be working OK. I have set MAKEFLAGS="-j8" (I
> have 8 core PC) to stress our build process, but it works fine. I ran it ~20
> times now and yet to see it fail. With old make I could break it on ~2-3
> attempt.
> 
> I think you should update as per my instructions. It is not a certain
solution,
> but better then what we have. Thank you.
> 
> Alex

[Mike.Rosset, 2011-09-03 02:35:42]

> I run with MAKEFLAGS="-j". Why don't you?

If the -j option is given without an argument, make will not limit the number of
jobs that can run simultaneously. (taken from gnu manual)

this may work in some situations but generally people are more explicit about
how many parallel makes they use. generally CPU or CORES

[peterGo, 2011-09-03 03:05:30]

Mike,

I've read the manuals.

The objective is to do a stress test. Setting n for the -j flag to the number of
CPU cores doesn't do that; there's I/O to consider. Setting it to -j does.

Peter

On 2011/09/03 02:35:42, Mike.Rosset wrote:
> > I run with MAKEFLAGS="-j". Why don't you?
> 
> If the -j option is given without an argument, make will not limit the number
of
> jobs that can run simultaneously. (taken from gnu manual)
> 
> this may work in some situations but generally people are more explicit about
> how many parallel makes they use. generally CPU or CORES