Code review comment for lp:~thumper/juju-core/upstart-retry-start-on-install

Reviewers: mp+188761_code.launchpad.net,

Message:
Please take a look.

Description:
Use a retry strategy to start new services.

On slower disks, upstart doesn't necessarily notice
the file straight away. Use a retry strategy to
start the newly written upstart file.

The first attempt happens straight away, so no slow
down on faster systems.

https://code.launchpad.net/~thumper/juju-core/upstart-retry-start-on-install/+merge/188761

(do not edit description out of merge proposal)

Please review this at https://codereview.appspot.com/14260043/

Affected files (+24, -8 lines):
   A [revision details]
   M upstart/upstart.go
   M upstart/upstart_test.go

Index: [revision details]
=== added file '[revision details]'
--- [revision details] 2012-01-01 00:00:00 +0000
+++ [revision details] 2012-01-01 00:00:00 +0000
@@ -0,0 +1,2 @@
+Old revision: tarmac-20131002011707-z9ofwwxn0ra5xh9t
+New revision: <email address hidden>

Index: upstart/upstart.go
=== modified file 'upstart/upstart.go'
--- upstart/upstart.go 2013-10-01 21:09:59 +0000
+++ upstart/upstart.go 2013-10-02 03:44:40 +0000
@@ -13,10 +13,18 @@
   "path"
   "regexp"
   "text/template"
+ "time"
+
+ "launchpad.net/juju-core/utils"
  )

  var startedRE = regexp.MustCompile(`^.* start/running, process (\d+)\n$`)

+var InstallStartRetryAttempts = utils.AttemptStrategy{
+ Total: 1 * time.Second,
+ Delay: 250 * time.Millisecond,
+}
+
  // Service provides visibility into and control over an upstart service.
  type Service struct {
   Name string
@@ -181,7 +189,14 @@
   if err := ioutil.WriteFile(c.confPath(), conf, 0644); err != nil {
    return err
   }
- return c.Start()
+ // On slower disks, upstart may take a short time to realise
+ // that there is a service there.
+ for attempt := InstallStartRetryAttempts.Start(); attempt.Next(); {
+ if err = c.Start(); err == nil {
+ break
+ }
+ }
+ return err
  }

  // InstallCommands returns shell commands to install and start the service.

Index: upstart/upstart_test.go
=== modified file 'upstart/upstart_test.go'
--- upstart/upstart_test.go 2013-10-02 00:16:23 +0000
+++ upstart/upstart_test.go 2013-10-02 03:44:40 +0000
@@ -13,13 +13,15 @@
   gc "launchpad.net/gocheck"

   jc "launchpad.net/juju-core/testing/checkers"
+ "launchpad.net/juju-core/testing/testbase"
   "launchpad.net/juju-core/upstart"
+ "launchpad.net/juju-core/utils"
  )

  func Test(t *testing.T) { gc.TestingT(t) }

  type UpstartSuite struct {
- origPath string
+ testbase.LoggingSuite
   testPath string
   service *upstart.Service
  }
@@ -27,18 +29,15 @@
  var _ = gc.Suite(&UpstartSuite{})

  func (s *UpstartSuite) SetUpTest(c *gc.C) {
- s.origPath = os.Getenv("PATH")
+ origPath := os.Getenv("PATH")
   s.testPath = c.MkDir()
- os.Setenv("PATH", s.testPath+":"+s.origPath)
+ s.PatchEnvironment("PATH", s.testPath+":"+origPath)
+ s.PatchValue(&upstart.InstallStartRetryAttempts, utils.AttemptStrategy{})
   s.service = &upstart.Service{Name: "some-service", InitDir: c.MkDir()}
   _, err := os.Create(filepath.Join(s.service.InitDir, "some-service.conf"))
   c.Assert(err, gc.IsNil)
  }

-func (s *UpstartSuite) TearDownTest(c *gc.C) {
- os.Setenv("PATH", s.origPath)
-}
-
  var checkargs = `
  #!/bin/bash
  if [ "$1" != "--system" ]; then