juju-core

Merge lp:~thumper/juju-core/upstart-system into lp:~go-bot/juju-core/trunk

  1. upstart-system
  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: 1917
Proposed branch: lp:~thumper/juju-core/upstart-system
Merge into: lp:~go-bot/juju-core/trunk
Diff against target: 66 lines (+17/-10)
3 files modified
environs/jujutest/livetests.go (+1/-1)
upstart/upstart.go (+9/-2)
upstart/upstart_test.go (+7/-7)
To merge this branch: bzr merge lp:~thumper/juju-core/upstart-system
Reviewer Review Type Date Requested Status
Juju Engineering Pending
Review via email: mp+188739@code.launchpad.net

Commit message

Really fix the local provider startup.

sudo -E causes upstart on saucy to default to use
the user bus. --system was added to start and stop,
but status was missed. Race conditions during startup
masked this before.

I've added another check in start so if the actual
start fails, check to see if it is started. When the
file is written to disk, upstart may start it before
we do, particularly, upstart may start it between when
we checked it wasn't running, and when we try to start it.

A drive by fix to the livetests was added as version.Current
was being used to source information from test metadata,
and there weren't saucy images defined. The tests pass wtih
the version specified with no other alterations.

https://codereview.appspot.com/14243043/

Description of the change

Really fix the local provider startup.

sudo -E causes upstart on saucy to default to use
the user bus. --system was added to start and stop,
but status was missed. Race conditions during startup
masked this before.

I've added another check in start so if the actual
start fails, check to see if it is started. When the
file is written to disk, upstart may start it before
we do, particularly, upstart may start it between when
we checked it wasn't running, and when we try to start it.

A drive by fix to the livetests was added as version.Current
was being used to source information from test metadata,
and there weren't saucy images defined. The tests pass wtih
the version specified with no other alterations.

https://codereview.appspot.com/14243043/

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

Reviewers: mp+188739_code.launchpad.net,

Message:
Please take a look.

Description:
Really fix the local provider startup.

sudo -E causes upstart on saucy to default to use
the user bus. --system was added to start and stop,
but status was missed. Race conditions during startup
masked this before.

I've added another check in start so if the actual
start fails, check to see if it is started. When the
file is written to disk, upstart may start it before
we do, particularly, upstart may start it between when
we checked it wasn't running, and when we try to start it.

A drive by fix to the livetests was added as version.Current
was being used to source information from test metadata,
and there weren't saucy images defined. The tests pass wtih
the version specified with no other alterations.

https://code.launchpad.net/~thumper/juju-core/upstart-system/+merge/188739

(do not edit description out of merge proposal)

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

Affected files (+934, -762 lines):
   A [revision details]
   M agent/agent.go
   M agent/agent_test.go
   A agent/export_test.go
   M cmd/juju/bootstrap.go
   M cmd/juju/bootstrap_test.go
   M cmd/juju/cmd_test.go
   M cmd/juju/destroyenvironment.go
   M cmd/juju/endpoint.go
   M cmd/juju/main_test.go
   M cmd/juju/ssh_test.go
   M cmd/juju/status_test.go
   M cmd/juju/synctools_test.go
   M cmd/jujud/agent.go
   M cmd/jujud/agent_test.go
   M cmd/jujud/bootstrap.go
   M cmd/jujud/bootstrap_test.go
   M cmd/jujud/machine.go
   M cmd/jujud/machine_test.go
   M cmd/jujud/unit_test.go
   M cmd/plugins/juju-metadata/validateimagemetadata.go
   M cmd/plugins/juju-metadata/validatetoolsmetadata.go
   M container/lxc/export_test.go
   M container/lxc/lxc.go
   M container/lxc/lxc_test.go
   M environs/bootstrap/bootstrap.go
   D environs/cert.go
   D environs/cert_internal_test.go
   D environs/cert_test.go
   M environs/jujutest/livetests.go
   M environs/jujutest/tests.go
   M environs/manual/bootstrap.go
   M environs/manual/bootstrap_test.go
   M environs/open.go
   M environs/open_test.go
   M environs/testing/polling.go
   M environs/testing/polling_test.go
   M juju/conn.go
   M juju/testing/conn.go
   A juju/testing/instance.go
   M provider/azure/environ.go
   M provider/azure/environ_test.go
   M provider/azure/instance.go
   M provider/common/export_test.go
   M provider/common/instance.go
   M provider/common/polling.go
   M provider/common/polling_test.go
   M provider/common/provider_test.go
   M provider/common/state.go
   M provider/common/state_test.go
   M provider/dummy/environs.go
   M provider/dummy/storage.go
   M provider/ec2/ec2.go
   M provider/ec2/live_test.go
   M provider/ec2/local_test.go
   M provider/local/environ.go
   M provider/local/instance.go
   M provider/maas/environ.go
   M provider/maas/environ_test.go
   M provider/maas/instance.go
   M provider/null/environ.go
   M provider/openstack/local_test.go
   M provider/openstack/provider.go
   M provider/provider.go
   M state/api/agent/machine_test.go
   M state/api/params/constants.go
   M state/apiserver/common/password.go
   M state/apiserver/common/password_test.go
   M state/machin...

Read more...

Revision history for this message
Tim Penhey (thumper) wrote :

Please take a look.

https://codereview.appspot.com/14243043/

Revision history for this message
Tim Penhey (thumper) wrote :

Please take a look.

https://codereview.appspot.com/14243043/

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

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

LGTM

https://codereview.appspot.com/14243043/

Revision history for this message
Go Bot (go-bot) wrote :
Download full text (104.8 KiB)

The attempt to merge lp:~thumper/juju-core/upstart-system into lp:juju-core failed. Below is the output from the failed tests.

ok launchpad.net/juju-core/agent 0.316s
ok launchpad.net/juju-core/agent/tools 0.247s
ok launchpad.net/juju-core/bzr 6.939s
ok launchpad.net/juju-core/cert 3.805s
ok launchpad.net/juju-core/charm 0.539s
? launchpad.net/juju-core/charm/hooks [no test files]
ok launchpad.net/juju-core/cloudinit 0.027s
ok launchpad.net/juju-core/cmd 0.199s
? launchpad.net/juju-core/cmd/charmd [no test files]
? launchpad.net/juju-core/cmd/charmload [no test files]
ok launchpad.net/juju-core/cmd/juju 163.854s
ok launchpad.net/juju-core/cmd/jujud 40.986s
ok launchpad.net/juju-core/cmd/plugins/juju-metadata 1.759s
ok launchpad.net/juju-core/constraints 0.033s
ok launchpad.net/juju-core/container/lxc 0.314s
? launchpad.net/juju-core/container/lxc/mock [no test files]
ok launchpad.net/juju-core/downloader 5.258s
ok launchpad.net/juju-core/environs 2.526s
ok launchpad.net/juju-core/environs/bootstrap 5.672s
ok launchpad.net/juju-core/environs/cloudinit 0.477s
ok launchpad.net/juju-core/environs/config 0.726s
ok launchpad.net/juju-core/environs/configstore 0.031s
ok launchpad.net/juju-core/environs/filestorage 0.029s
ok launchpad.net/juju-core/environs/httpstorage 0.726s
ok launchpad.net/juju-core/environs/imagemetadata 0.481s
ok launchpad.net/juju-core/environs/instances 0.051s
ok launchpad.net/juju-core/environs/jujutest 0.282s
ok launchpad.net/juju-core/environs/manual 4.895s
ok launchpad.net/juju-core/environs/simplestreams 0.331s
? launchpad.net/juju-core/environs/simplestreams/testing [no test files]
ok launchpad.net/juju-core/environs/sshstorage 0.650s
ok launchpad.net/juju-core/environs/storage 1.025s
ok launchpad.net/juju-core/environs/sync 27.790s
ok launchpad.net/juju-core/environs/testing 0.213s
ok launchpad.net/juju-core/environs/tools 5.866s
? launchpad.net/juju-core/environs/tools/testing [no test files]
ok launchpad.net/juju-core/errors 0.015s
ok launchpad.net/juju-core/instance 0.021s
? launchpad.net/juju-core/instance/testing [no test files]
ok launchpad.net/juju-core/juju 22.457s
ok launchpad.net/juju-core/juju/osenv 0.017s
? launchpad.net/juju-core/juju/testing [no test files]
ok launchpad.net/juju-core/log 0.015s
ok launchpad.net/juju-core/log/syslog 0.023s
ok launchpad.net/juju-core/names 0.031s
? launchpad.net/juju-core/provider [no test files]
? launchpad.net/juju-core/provider/all [no test files]
ok launchpad.net/juju-core/provider/azure 6.716s
ok launchpad.net/juju-core/provider/common 0.273s
ok launchpad.net/juju-core/provider/dummy 21.001s
ok launchpad.net/juju-core/provider/ec2 4.894s
ok launchpad.net/juju-core/provider/ec2/httpstorage 0.198s
ok launchpad.net/juju-core/provider/local 1.651s
ok launchpad.net/juju-core/provider/maas 3.883s
ok launchpad.net/juju-core/provider/null 1.094s
ok launchpad.net/juju-core/provider/openstack 12.162s
ok launchpad.net/juju-core/rpc 0.075s
ok launchpad.net/juju-core/rpc/jsoncodec 0.027s
? launchpad.net/juju-core/rpc/rpcreflect [no test files]
ok launch...

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'environs/jujutest/livetests.go'
2--- environs/jujutest/livetests.go 2013-09-30 19:40:06 +0000
3+++ environs/jujutest/livetests.go 2013-10-02 00:16:49 +0000
4@@ -864,7 +864,7 @@
5 machineConfig := environs.NewMachineConfig(machineId, "", stateInfo, apiInfo)
6
7 t.PrepareOnce(c)
8- possibleTools := envtesting.AssertUploadFakeToolsVersions(c, t.Env.Storage(), version.Current)
9+ possibleTools := envtesting.AssertUploadFakeToolsVersions(c, t.Env.Storage(), version.MustParseBinary("5.4.5-precise-amd64"))
10 inst, _, err := t.Env.StartInstance(constraints.Value{}, possibleTools, machineConfig)
11 if inst != nil {
12 err := t.Env.StopInstances([]instance.Instance{inst})
13
14=== modified file 'upstart/upstart.go'
15--- upstart/upstart.go 2013-09-29 23:03:55 +0000
16+++ upstart/upstart.go 2013-10-02 00:16:49 +0000
17@@ -41,7 +41,7 @@
18
19 // Running returns true if the Service appears to be running.
20 func (s *Service) Running() bool {
21- cmd := exec.Command("status", s.Name)
22+ cmd := exec.Command("status", "--system", s.Name)
23 out, err := cmd.CombinedOutput()
24 if err != nil {
25 return false
26@@ -54,7 +54,14 @@
27 if s.Running() {
28 return nil
29 }
30- return runCommand("start", "--system", s.Name)
31+ err := runCommand("start", "--system", s.Name)
32+ if err != nil {
33+ // Double check to see if we were started before our command ran.
34+ if s.Running() {
35+ return nil
36+ }
37+ }
38+ return err
39 }
40
41 func runCommand(args ...string) error {
42
43=== modified file 'upstart/upstart_test.go'
44--- upstart/upstart_test.go 2013-09-29 23:12:23 +0000
45+++ upstart/upstart_test.go 2013-10-02 00:16:49 +0000
46@@ -41,13 +41,13 @@
47
48 var checkargs = `
49 #!/bin/bash
50-if [ "$1" == "--system" ]; then
51- shift
52-fi
53-if [ "$1" != "some-service" ]; then
54- exit 255
55-fi
56-if [ "$2" != "" ]; then
57+if [ "$1" != "--system" ]; then
58+ exit 255
59+fi
60+if [ "$2" != "some-service" ]; then
61+ exit 255
62+fi
63+if [ "$3" != "" ]; then
64 exit 255
65 fi
66 `[1:]

Subscribers

People subscribed via source and target branches

to status/vote changes: