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

[Tim Penhey]

Further break up cloudinit.

Move the definitions of the mongo and machine agent
services into the upstart package. These have been
parameterized to allow reuse in the local provider.

https://codereview.appspot.com/10952043/

Further break up cloudinit.

Move the definitions of the mongo and machine agent
services into the upstart package. These have been
parameterized to allow reuse in the local provider.

There definitions have been moved from the cloudinit
function, and as you can see there are two different
ways of defining the command. I have left it up to
the reviewers to decide which is better. Personally
I'm leaning towards the style used to define the
mongo service.

Also, wondering about specific tests for these. They
don't really do much except format the parameters
for a new object.

https://codereview.appspot.com/10952043/

[Tim Penhey (thumper)]

Reviewers: mp+173121_code.launchpad.net,

Message:
Please take a look.

Description:
Further break up cloudinit.

Move the definitions of the mongo and machine agent
services into the upstart package. These have been
parameterized to allow reuse in the local provider.

There definitions have been moved from the cloudinit
function, and as you can see there are two different
ways of defining the command. I have left it up to
the reviewers to decide which is better. Personally
I'm leaning towards the style used to define the
mongo service.

Also, wondering about specific tests for these. They
don't really do much except format the parameters
for a new object.

https://code.launchpad.net/~thumper/juju-core/upstart-services/+merge/173121

Requires:
https://code.launchpad.net/~thumper/juju-core/find-ipv4-address/+merge/173119

(do not edit description out of merge proposal)

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

Affected files:
   A [revision details]
   M environs/all/all.go
   M environs/cloudinit/cloudinit.go
   M environs/local/config.go
   M environs/local/config_test.go
   M environs/local/environ.go
   M environs/local/environprovider.go
   M environs/local/environprovider_test.go
   M environs/local/export_test.go
   A upstart/service.go

[Tim Penhey (thumper)]

Must have switched before lbox had finished making the diff, trying to regenerate.

[Tim Penhey (thumper)]

Please take a look.

https://codereview.appspot.com/10952043/

[Frank Mueller (themue)]

I really like the approach, but would also like isolated tests for the
new upstart functions.

https://codereview.appspot.com/10952043/

[Ian Booth (wallyworld)]

LGTM. I prefer the string concatenation as it's easier to read. I also
don't think tests for the struct creation will add too much value. I see
this as a minor, internal implementation detail. So long as there are
other tests that check things like the contents of the upstart files,
that to me is sufficient.

https://codereview.appspot.com/10952043/

[Tim Penhey (thumper)]

On 2013/07/08 01:38:17, wallyworld wrote:
> LGTM. I prefer the string concatenation as it's easier to read. I also
don't
> think tests for the struct creation will add too much value. I see
this as a
> minor, internal implementation detail. So long as there are other
tests that
> check things like the contents of the upstart files, that to me is
sufficient.

Restructured.

https://codereview.appspot.com/10952043/

[Tim Penhey (thumper)]

Please take a look.

https://codereview.appspot.com/10952043/

=== modified file 'environs/cloudinit/cloudinit.go'
--- environs/cloudinit/cloudinit.go	2013-07-01 01:09:16 +0000
+++ environs/cloudinit/cloudinit.go	2013-07-08 22:36:27 +0000
@@ -6,7 +6,10 @@
 import (
 	"encoding/base64"
 	"fmt"
+	"path/filepath"
+
 	"launchpad.net/goyaml"
+
 	"launchpad.net/juju-core/cloudinit"
 	"launchpad.net/juju-core/constraints"
 	"launchpad.net/juju-core/environs/agent"
@@ -17,12 +20,6 @@
 	"launchpad.net/juju-core/state/api"
 	"launchpad.net/juju-core/upstart"
 	"launchpad.net/juju-core/utils"
-	"path"
-)
-
-const (
-	maxMongoFiles = 65000
-	maxAgentFiles = 20000
 )
 
 // MachineConfig represents initialization information for a new juju machine.
@@ -190,9 +187,7 @@
 		)
 	}
 
-	if err := cfg.addAgentToBoot(c, "machine",
-		machineTag,
-		fmt.Sprintf("--machine-id %s "+debugFlag, cfg.MachineId)); err != nil {
+	if err := cfg.addMachineAgentToBoot(c, machineTag, cfg.MachineId, debugFlag); err != nil {
 		return nil, err
 	}
 
@@ -232,7 +227,7 @@
 }
 
 func (cfg *MachineConfig) dataFile(name string) string {
-	return path.Join(cfg.DataDir, name)
+	return filepath.Join(cfg.DataDir, name)
 }
 
 func (cfg *MachineConfig) agentConfig(tag string) *agent.Conf {
@@ -273,7 +268,7 @@
 	return acfg, nil
 }
 
-func (cfg *MachineConfig) addAgentToBoot(c *cloudinit.Config, kind, tag, args string) error {
+func (cfg *MachineConfig) addMachineAgentToBoot(c *cloudinit.Config, tag, machineId, logConfig string) error {
 	// Make the agent run via a symbolic link to the actual tools
 	// directory, so it can upgrade itself without needing to change
 	// the upstart script.
@@ -281,27 +276,8 @@
 	// TODO(dfc) ln -nfs, so it doesn't fail if for some reason that the target already exists
 	addScripts(c, fmt.Sprintf("ln -s %v %s", cfg.Tools.Binary, shquote(toolsDir)))
 
-	svc := upstart.NewService("jujud-" + tag)
-	logPath := fmt.Sprintf("/var/log/juju/%s.log", tag)
-	cmd := fmt.Sprintf(
-		"%s/jujud %s"+
-			" --log-file %s"+
-			" --data-dir '%s'"+
-			" %s",
-		toolsDir, kind,
-		logPath,
-		cfg.DataDir,
-		args,
-	)
-	conf := &upstart.Conf{
-		Service: *svc,
-		Desc:    fmt.Sprintf("juju %s agent", tag),
-		Limit: map[string]string{
-			"nofile": fmt.Sprintf("%d %d", maxAgentFiles, maxAgentFiles),
-		},
-		Cmd: cmd,
-		Out: logPath,
-	}
+	name := "jujud-" + tag
+	conf := upstart.MachineAgentUpstartService(name, toolsDir, cfg.DataDir, "/var/log/juju/", tag, machineId, logConfig)
 	cmds, err := conf.InstallCommands()
 	if err != nil {
 		return fmt.Errorf("cannot make cloud-init upstart script for the %s agent: %v", tag, err)
@@ -311,33 +287,16 @@
 }
 
 func (cfg *MachineConfig) addMongoToBoot(c *cloudinit.Config) error {
+	dbDir := filepath.Join(cfg.DataDir, "db")
 	addScripts(c,
-		"mkdir -p /var/lib/juju/db/journal",
+		"mkdir -p "+dbDir+"/journal",
 		// Otherwise we get three files with 100M+ each, which takes time.
-		"dd bs=1M count=1 if=/dev/zero of=/var/lib/juju/db/journal/prealloc.0",
-		"dd bs=1M count=1 if=/dev/zero of=/var/lib/juju/db/journal/prealloc.1",
-		"dd bs=1M count=1 if=/dev/zero of=/var/lib/juju/db/journal/prealloc.2",
+		"dd bs=1M count=1 if=/dev/zero of="+dbDir+"/journal/prealloc.0",
+		"dd bs=1M count=1 if=/dev/zero of="+dbDir+"/journal/prealloc.1",
+		"dd bs=1M count=1 if=/dev/zero of="+dbDir+"/journal/prealloc.2",
 	)
-	svc := upstart.NewService("juju-db")
-	conf := &upstart.Conf{
-		Service: *svc,
-		Desc:    "juju state database",
-		Limit: map[string]string{
-			"nofile": fmt.Sprintf("%d %d", maxMongoFiles, maxMongoFiles),
-			"nproc":  fmt.Sprintf("%d %d", maxAgentFiles, maxAgentFiles),
-		},
-		Cmd: "/usr/bin/mongod" +
-			" --auth" +
-			" --dbpath=/var/lib/juju/db" +
-			" --sslOnNormalPorts" +
-			" --sslPEMKeyFile " + shquote(cfg.dataFile("server.pem")) +
-			" --sslPEMKeyPassword ignored" +
-			" --bind_ip 0.0.0.0" +
-			" --port " + fmt.Sprint(cfg.StatePort) +
-			" --noprealloc" +
-			" --syslog" +
-			" --smallfiles",
-	}
+
+	conf := upstart.MongoUpstartService("juju-db", cfg.DataDir, dbDir, cfg.StatePort)
 	cmds, err := conf.InstallCommands()
 	if err != nil {
 		return fmt.Errorf("cannot make cloud-init upstart script for the state database: %v", err)
@@ -350,8 +309,8 @@
 // to use as a directory for storing the tools executables in
 // by using the last element stripped of its extension.
 func versionDir(toolsURL string) string {
-	name := path.Base(toolsURL)
-	ext := path.Ext(name)
+	name := filepath.Base(toolsURL)
+	ext := filepath.Ext(name)
 	return name[:len(name)-len(ext)]
 }
 

=== modified file 'environs/cloudinit/cloudinit_test.go'
--- environs/cloudinit/cloudinit_test.go	2013-07-01 12:15:52 +0000
+++ environs/cloudinit/cloudinit_test.go	2013-07-08 22:36:27 +0000
@@ -103,7 +103,7 @@
 /var/lib/juju/tools/1\.2\.3-precise-amd64/jujud bootstrap-state --data-dir '/var/lib/juju' --env-config '[^']*' --constraints 'mem=2048M' --debug
 rm -rf '/var/lib/juju/agents/bootstrap'
 ln -s 1\.2\.3-precise-amd64 '/var/lib/juju/tools/machine-0'
-cat >> /etc/init/jujud-machine-0\.conf << 'EOF'\\ndescription "juju machine-0 agent"\\nauthor "Juju Team <juju@lists\.ubuntu\.com>"\\nstart on runlevel \[2345\]\\nstop on runlevel \[!2345\]\\nrespawn\\nnormal exit 0\\n\\nlimit nofile 20000 20000\\n\\nexec /var/lib/juju/tools/machine-0/jujud machine --log-file /var/log/juju/machine-0\.log --data-dir '/var/lib/juju' --machine-id 0  --debug >> /var/log/juju/machine-0\.log 2>&1\\nEOF\\n
+cat >> /etc/init/jujud-machine-0\.conf << 'EOF'\\ndescription "juju machine-0 agent"\\nauthor "Juju Team <juju@lists\.ubuntu\.com>"\\nstart on runlevel \[2345\]\\nstop on runlevel \[!2345\]\\nrespawn\\nnormal exit 0\\n\\nlimit nofile 20000 20000\\n\\nexec /var/lib/juju/tools/machine-0/jujud machine --log-file '/var/log/juju/machine-0\.log' --data-dir '/var/lib/juju' --machine-id 0  --debug >> /var/log/juju/machine-0\.log 2>&1\\nEOF\\n
 start jujud-machine-0
 `,
 	}, {
@@ -158,7 +158,7 @@
 /var/lib/juju/tools/1\.2\.3-raring-amd64/jujud bootstrap-state --data-dir '/var/lib/juju' --env-config '[^']*' --constraints 'mem=2048M' --debug
 rm -rf '/var/lib/juju/agents/bootstrap'
 ln -s 1\.2\.3-raring-amd64 '/var/lib/juju/tools/machine-0'
-cat >> /etc/init/jujud-machine-0\.conf << 'EOF'\\ndescription "juju machine-0 agent"\\nauthor "Juju Team <juju@lists\.ubuntu\.com>"\\nstart on runlevel \[2345\]\\nstop on runlevel \[!2345\]\\nrespawn\\nnormal exit 0\\n\\nlimit nofile 20000 20000\\n\\nexec /var/lib/juju/tools/machine-0/jujud machine --log-file /var/log/juju/machine-0\.log --data-dir '/var/lib/juju' --machine-id 0  --debug >> /var/log/juju/machine-0\.log 2>&1\\nEOF\\n
+cat >> /etc/init/jujud-machine-0\.conf << 'EOF'\\ndescription "juju machine-0 agent"\\nauthor "Juju Team <juju@lists\.ubuntu\.com>"\\nstart on runlevel \[2345\]\\nstop on runlevel \[!2345\]\\nrespawn\\nnormal exit 0\\n\\nlimit nofile 20000 20000\\n\\nexec /var/lib/juju/tools/machine-0/jujud machine --log-file '/var/log/juju/machine-0\.log' --data-dir '/var/lib/juju' --machine-id 0  --debug >> /var/log/juju/machine-0\.log 2>&1\\nEOF\\n
 start jujud-machine-0
 `,
 	}, {
@@ -196,7 +196,7 @@
 echo 'datadir: /var/lib/juju\\noldpassword: arble\\nmachinenonce: FAKE_NONCE\\nstateinfo:\\n  addrs:\\n  - state-addr\.example\.com:12345\\n  cacert:\\n[^']+  tag: machine-99\\n  password: ""\\noldapipassword: ""\\napiinfo:\\n  addrs:\\n  - state-addr\.example\.com:54321\\n  cacert:\\n[^']+  tag: machine-99\\n  password: ""\\n' > '/var/lib/juju/agents/machine-99/agent\.conf'
 chmod 600 '/var/lib/juju/agents/machine-99/agent\.conf'
 ln -s 1\.2\.3-linux-amd64 '/var/lib/juju/tools/machine-99'
-cat >> /etc/init/jujud-machine-99\.conf << 'EOF'\\ndescription "juju machine-99 agent"\\nauthor "Juju Team <juju@lists\.ubuntu\.com>"\\nstart on runlevel \[2345\]\\nstop on runlevel \[!2345\]\\nrespawn\\nnormal exit 0\\n\\nlimit nofile 20000 20000\\n\\nexec /var/lib/juju/tools/machine-99/jujud machine --log-file /var/log/juju/machine-99\.log --data-dir '/var/lib/juju' --machine-id 99  --debug >> /var/log/juju/machine-99\.log 2>&1\\nEOF\\n
+cat >> /etc/init/jujud-machine-99\.conf << 'EOF'\\ndescription "juju machine-99 agent"\\nauthor "Juju Team <juju@lists\.ubuntu\.com>"\\nstart on runlevel \[2345\]\\nstop on runlevel \[!2345\]\\nrespawn\\nnormal exit 0\\n\\nlimit nofile 20000 20000\\n\\nexec /var/lib/juju/tools/machine-99/jujud machine --log-file '/var/log/juju/machine-99\.log' --data-dir '/var/lib/juju' --machine-id 99  --debug >> /var/log/juju/machine-99\.log 2>&1\\nEOF\\n
 start jujud-machine-99
 `,
 	}, {
@@ -235,7 +235,7 @@
 echo 'datadir: /var/lib/juju\\noldpassword: arble\\nmachinenonce: FAKE_NONCE\\nstateinfo:\\n  addrs:\\n  - state-addr\.example\.com:12345\\n  cacert:\\n[^']+  tag: machine-2-lxc-1\\n  password: ""\\noldapipassword: ""\\napiinfo:\\n  addrs:\\n  - state-addr\.example\.com:54321\\n  cacert:\\n[^']+  tag: machine-2-lxc-1\\n  password: ""\\n' > '/var/lib/juju/agents/machine-2-lxc-1/agent\.conf'
 chmod 600 '/var/lib/juju/agents/machine-2-lxc-1/agent\.conf'
 ln -s 1\.2\.3-linux-amd64 '/var/lib/juju/tools/machine-2-lxc-1'
-cat >> /etc/init/jujud-machine-2-lxc-1\.conf << 'EOF'\\ndescription "juju machine-2-lxc-1 agent"\\nauthor "Juju Team <juju@lists\.ubuntu\.com>"\\nstart on runlevel \[2345\]\\nstop on runlevel \[!2345\]\\nrespawn\\nnormal exit 0\\n\\nlimit nofile 20000 20000\\n\\nexec /var/lib/juju/tools/machine-2-lxc-1/jujud machine --log-file /var/log/juju/machine-2-lxc-1\.log --data-dir '/var/lib/juju' --machine-id 2/lxc/1  --debug >> /var/log/juju/machine-2-lxc-1\.log 2>&1\\nEOF\\n
+cat >> /etc/init/jujud-machine-2-lxc-1\.conf << 'EOF'\\ndescription "juju machine-2-lxc-1 agent"\\nauthor "Juju Team <juju@lists\.ubuntu\.com>"\\nstart on runlevel \[2345\]\\nstop on runlevel \[!2345\]\\nrespawn\\nnormal exit 0\\n\\nlimit nofile 20000 20000\\n\\nexec /var/lib/juju/tools/machine-2-lxc-1/jujud machine --log-file '/var/log/juju/machine-2-lxc-1\.log' --data-dir '/var/lib/juju' --machine-id 2/lxc/1  --debug >> /var/log/juju/machine-2-lxc-1\.log 2>&1\\nEOF\\n
 start jujud-machine-2-lxc-1
 `,
 	},

=== added file 'upstart/service.go'
--- upstart/service.go	1970-01-01 00:00:00 +0000
+++ upstart/service.go	2013-07-08 22:36:27 +0000
@@ -0,0 +1,62 @@
+// Copyright 2013 Canonical Ltd.
+// Licensed under the AGPLv3, see LICENCE file for details.
+
+package upstart
+
+import (
+	"fmt"
+	"path/filepath"
+
+	"launchpad.net/juju-core/utils"
+)
+
+const (
+	maxMongoFiles = 65000
+	maxAgentFiles = 20000
+)
+
+// MongoUpstartService returns the upstart config for the mongo state service.
+func MongoUpstartService(name, dataDir, dbDir string, port int) *Conf {
+	keyFile := filepath.Join(dataDir, "server.pem")
+	svc := NewService(name)
+	return &Conf{
+		Service: *svc,
+		Desc:    "juju state database",
+		Limit: map[string]string{
+			"nofile": fmt.Sprintf("%d %d", maxMongoFiles, maxMongoFiles),
+			"nproc":  fmt.Sprintf("%d %d", maxAgentFiles, maxAgentFiles),
+		},
+		Cmd: "/usr/bin/mongod" +
+			" --auth" +
+			" --dbpath=" + dbDir +
+			" --sslOnNormalPorts" +
+			" --sslPEMKeyFile " + utils.ShQuote(keyFile) +
+			" --sslPEMKeyPassword ignored" +
+			" --bind_ip 0.0.0.0" +
+			" --port " + fmt.Sprint(port) +
+			" --noprealloc" +
+			" --syslog" +
+			" --smallfiles",
+	}
+}
+
+// MachineAgentUpstartService returns the upstart config for a machine agent
+// based on the tag and machineId passed in.
+func MachineAgentUpstartService(name, toolsDir, dataDir, logDir, tag, machineId, logConfig string) *Conf {
+	svc := NewService(name)
+	logFile := filepath.Join(logDir, tag+".log")
+	return &Conf{
+		Service: *svc,
+		Desc:    fmt.Sprintf("juju %s agent", tag),
+		Limit: map[string]string{
+			"nofile": fmt.Sprintf("%d %d", maxAgentFiles, maxAgentFiles),
+		},
+		Cmd: filepath.Join(toolsDir, "jujud") +
+			" machine" +
+			" --log-file " + utils.ShQuote(logFile) +
+			" --data-dir " + utils.ShQuote(dataDir) +
+			" --machine-id " + machineId +
+			" " + logConfig,
+		Out: logFile,
+	}
+}