juju-core

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

  1. upstart-retry-start-on-install
  2. Merge into trunk
Proposed by Tim Penhey
Status: Merged
Approved by: Tim Penhey
Approved revision: no longer in the source branch.
Merged at revision: 1918
Proposed branch: lp:~thumper/juju-core/upstart-retry-start-on-install
Merge into: lp:~go-bot/juju-core/trunk
Diff against target: 81 lines (+22/-8)
2 files modified
upstart/upstart.go (+16/-1)
upstart/upstart_test.go (+6/-7)
To merge this branch: bzr merge lp:~thumper/juju-core/upstart-retry-start-on-install
Reviewer Review Type Date Requested Status
Juju Engineering Pending
Review via email: mp+188761@code.launchpad.net

Commit message

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/

Description of the change

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/

To post a comment you must log in.
Revision history for this message
Tim Penhey (thumper) wrote :
Download full text (3.2 KiB)

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

Read more...

Revision history for this message
Andrew Wilkins (axwalk) wrote :

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

LGTM

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

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== 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 @@
13 "path"13 "path"
14 "regexp"14 "regexp"
15 "text/template"15 "text/template"
16 "time"
17
18 "launchpad.net/juju-core/utils"
16)19)
1720
18var startedRE = regexp.MustCompile(`^.* start/running, process (\d+)\n$`)21var startedRE = regexp.MustCompile(`^.* start/running, process (\d+)\n$`)
1922
23var InstallStartRetryAttempts = utils.AttemptStrategy{
24 Total: 1 * time.Second,
25 Delay: 250 * time.Millisecond,
26}
27
20// Service provides visibility into and control over an upstart service.28// Service provides visibility into and control over an upstart service.
21type Service struct {29type Service struct {
22 Name string30 Name string
@@ -181,7 +189,14 @@
181 if err := ioutil.WriteFile(c.confPath(), conf, 0644); err != nil {189 if err := ioutil.WriteFile(c.confPath(), conf, 0644); err != nil {
182 return err190 return err
183 }191 }
184 return c.Start()192 // On slower disks, upstart may take a short time to realise
193 // that there is a service there.
194 for attempt := InstallStartRetryAttempts.Start(); attempt.Next(); {
195 if err = c.Start(); err == nil {
196 break
197 }
198 }
199 return err
185}200}
186201
187// InstallCommands returns shell commands to install and start the service.202// InstallCommands returns shell commands to install and start the service.
188203
=== 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 @@
13 gc "launchpad.net/gocheck"13 gc "launchpad.net/gocheck"
1414
15 jc "launchpad.net/juju-core/testing/checkers"15 jc "launchpad.net/juju-core/testing/checkers"
16 "launchpad.net/juju-core/testing/testbase"
16 "launchpad.net/juju-core/upstart"17 "launchpad.net/juju-core/upstart"
18 "launchpad.net/juju-core/utils"
17)19)
1820
19func Test(t *testing.T) { gc.TestingT(t) }21func Test(t *testing.T) { gc.TestingT(t) }
2022
21type UpstartSuite struct {23type UpstartSuite struct {
22 origPath string24 testbase.LoggingSuite
23 testPath string25 testPath string
24 service *upstart.Service26 service *upstart.Service
25}27}
@@ -27,18 +29,15 @@
27var _ = gc.Suite(&UpstartSuite{})29var _ = gc.Suite(&UpstartSuite{})
2830
29func (s *UpstartSuite) SetUpTest(c *gc.C) {31func (s *UpstartSuite) SetUpTest(c *gc.C) {
30 s.origPath = os.Getenv("PATH")32 origPath := os.Getenv("PATH")
31 s.testPath = c.MkDir()33 s.testPath = c.MkDir()
32 os.Setenv("PATH", s.testPath+":"+s.origPath)34 s.PatchEnvironment("PATH", s.testPath+":"+origPath)
35 s.PatchValue(&upstart.InstallStartRetryAttempts, utils.AttemptStrategy{})
33 s.service = &upstart.Service{Name: "some-service", InitDir: c.MkDir()}36 s.service = &upstart.Service{Name: "some-service", InitDir: c.MkDir()}
34 _, err := os.Create(filepath.Join(s.service.InitDir, "some-service.conf"))37 _, err := os.Create(filepath.Join(s.service.InitDir, "some-service.conf"))
35 c.Assert(err, gc.IsNil)38 c.Assert(err, gc.IsNil)
36}39}
3740
38func (s *UpstartSuite) TearDownTest(c *gc.C) {
39 os.Setenv("PATH", s.origPath)
40}
41
42var checkargs = `41var checkargs = `
43#!/bin/bash42#!/bin/bash
44if [ "$1" != "--system" ]; then43if [ "$1" != "--system" ]; then

Subscribers

People subscribed via source and target branches

to status/vote changes: