Merge lp:~thumper/juju-core/upstart-retry-start-on-install into lp:~go-bot/juju-core/trunk

[Tim Penhey]

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://codereview.appspot.com/14260043/

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://codereview.appspot.com/14260043/

[Tim Penhey (thumper)]

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...

[Andrew Wilkins (axwalk)]

On 2013/10/02 03:48:39, thumper wrote:
> Please take a look.

LGTM

https://codereview.appspot.com/14260043/

=== modified file 'upstart/upstart.go'
--- upstart/upstart.go	2013-10-01 21:09:59 +0000
+++ upstart/upstart.go	2013-10-02 03:47:13 +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.

=== 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:47:13 +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