Merge lp:~thumper/juju-core/logger-initial-state into lp:~go-bot/juju-core/trunk

[Tim Penhey]

A little clean up for initial agent logging.

Now that we have a logging worker that will set the log
levels to the system wide setting when it starts, we can
start all new agents with debug, and then as soon as the
agent starts, it will change it.

A drive by fix here to have the machine agent explicitly
remove the logfile writer from loggo if one has been set.
Machine agent upstart jobs that are around from before 1.16
will otherwise cause duplicate lines.

https://codereview.appspot.com/13717044/

A little clean up for initial agent logging.

Now that we have a logging worker that will set the log
levels to the system wide setting when it starts, we can
start all new agents with debug, and then as soon as the
agent starts, it will change it.

A drive by fix here to have the machine agent explicitly
remove the logfile writer from loggo if one has been set.
Machine agent upstart jobs that are around from before 1.16
will otherwise cause duplicate lines.

https://codereview.appspot.com/13717044/

[Tim Penhey (thumper)]

Reviewers: mp+185962_code.launchpad.net,

Message:
Please take a look.

Description:
A little clean up for initial agent logging.

Now that we have a logging worker that will set the log
levels to the system wide setting when it starts, we can
start all new agents with debug, and then as soon as the
agent starts, it will change it.

A drive by fix here to have the machine agent explicitly
remove the logfile writer from loggo if one has been set.
Machine agent upstart jobs that are around from before 1.16
will otherwise cause duplicate lines.

https://code.launchpad.net/~thumper/juju-core/logger-initial-state/+merge/185962

(do not edit description out of merge proposal)

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

Affected files (+22, -19 lines):
   A [revision details]
   M cmd/jujud/machine.go
   M environs/cloudinit/cloudinit.go
   M environs/cloudinit/cloudinit_test.go
   M provider/local/environ.go
   M upstart/service.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-20130916220230-s30jbjsc5zvfzii7
+New revision: <email address hidden>

Index: upstart/service.go
=== modified file 'upstart/service.go'
--- upstart/service.go 2013-09-13 05:20:56 +0000
+++ upstart/service.go 2013-09-16 22:02:24 +0000
@@ -42,9 +42,11 @@

  // 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, env map[string]string) *Conf {
+func MachineAgentUpstartService(name, toolsDir, dataDir, logDir, tag,
machineId string, env map[string]string) *Conf {
   svc := NewService(name)
   logFile := filepath.Join(logDir, tag+".log")
+ // The machine agent always starts with debug turned on. The logger
worker
+ // will update this to the system logging environment as soon as it
starts.
   return &Conf{
    Service: *svc,
    Desc: fmt.Sprintf("juju %s agent", tag),
@@ -55,7 +57,7 @@
     " machine" +
     " --data-dir " + utils.ShQuote(dataDir) +
     " --machine-id " + machineId +
- " " + logConfig,
+ " --debug",
    Out: logFile,
    Env: env,
   }

Index: cmd/jujud/machine.go
=== modified file 'cmd/jujud/machine.go'
--- cmd/jujud/machine.go 2013-09-12 22:18:10 +0000
+++ cmd/jujud/machine.go 2013-09-17 01:58:35 +0000
@@ -10,6 +10,7 @@
   "time"

   "launchpad.net/gnuflag"
+ "launchpad.net/loggo"
   "launchpad.net/tomb"

   "launchpad.net/juju-core/agent"
@@ -97,6 +98,11 @@

  // Run runs a machine agent.
  func (a *MachineAgent) Run(_ *cmd.Context) error {
+ // Due to changes in the logging, and needing to care about old
+ // environments that have been upgraded, we need to explicitly remove the
+ // file writer if one has been added, otherwise we will get duplicate
+ // lines of all logging in the log file.
+ loggo.RemoveWriter("logfile")
   defer a.tomb.Done()
   log.Infof("machine agent %v start", a.Tag())
   if err := a.Conf.read(a.Tag()); err != nil {

Index: environs/cloudinit/cl...

[Ian Booth (wallyworld)]

LGTM, although I'd prefer to see an integration style test that confirms
that debug is used and then turned off as expected. We do this sort of
thing elsewhere for other similar "live" sets of tests.

https://codereview.appspot.com/13717044/

=== modified file 'cmd/jujud/machine.go'
--- cmd/jujud/machine.go	2013-09-12 22:18:10 +0000
+++ cmd/jujud/machine.go	2013-09-17 02:01:38 +0000
@@ -10,6 +10,7 @@
 	"time"
 
 	"launchpad.net/gnuflag"
+	"launchpad.net/loggo"
 	"launchpad.net/tomb"
 
 	"launchpad.net/juju-core/agent"
@@ -97,6 +98,11 @@
 
 // Run runs a machine agent.
 func (a *MachineAgent) Run(_ *cmd.Context) error {
+	// Due to changes in the logging, and needing to care about old
+	// environments that have been upgraded, we need to explicitly remove the
+	// file writer if one has been added, otherwise we will get duplicate
+	// lines of all logging in the log file.
+	loggo.RemoveWriter("logfile")
 	defer a.tomb.Done()
 	log.Infof("machine agent %v start", a.Tag())
 	if err := a.Conf.read(a.Tag()); err != nil {

=== modified file 'environs/cloudinit/cloudinit.go'
--- environs/cloudinit/cloudinit.go	2013-09-16 02:30:27 +0000
+++ environs/cloudinit/cloudinit.go	2013-09-17 02:01:38 +0000
@@ -147,13 +147,6 @@
 		fmt.Sprintf("echo -n %s > $bin/downloaded-url.txt", shquote(cfg.Tools.URL)),
 	)
 
-	// TODO (thumper): work out how to pass the logging config to the children
-	debugFlag := ""
-	// TODO: disable debug mode by default when the system is stable.
-	if true {
-		debugFlag = " --debug"
-	}
-
 	if err := cfg.addLogging(c); err != nil {
 		return nil, err
 	}
@@ -189,16 +182,17 @@
 		}
 		c.AddScripts(
 			fmt.Sprintf("echo %s > %s", shquote(cfg.StateInfoURL), BootstrapStateURLFile),
+			// The bootstrapping is always run with debug on.
 			cfg.jujuTools()+"/jujud bootstrap-state"+
 				" --data-dir "+shquote(cfg.DataDir)+
 				" --env-config "+shquote(base64yaml(cfg.Config))+
 				" --constraints "+shquote(cfg.Constraints.String())+
-				debugFlag,
+				" --debug",
 			"rm -rf "+shquote(acfg.Dir()),
 		)
 	}
 
-	if err := cfg.addMachineAgentToBoot(c, machineTag, cfg.MachineId, debugFlag); err != nil {
+	if err := cfg.addMachineAgentToBoot(c, machineTag, cfg.MachineId); err != nil {
 		return nil, err
 	}
 
@@ -279,7 +273,7 @@
 	return acfg, nil
 }
 
-func (cfg *MachineConfig) addMachineAgentToBoot(c *cloudinit.Config, tag, machineId, logConfig string) error {
+func (cfg *MachineConfig) addMachineAgentToBoot(c *cloudinit.Config, tag, machineId 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.
@@ -288,7 +282,7 @@
 	c.AddScripts(fmt.Sprintf("ln -s %v %s", cfg.Tools.Version, shquote(toolsDir)))
 
 	name := "jujud-" + tag
-	conf := upstart.MachineAgentUpstartService(name, toolsDir, cfg.DataDir, "/var/log/juju/", tag, machineId, logConfig, nil)
+	conf := upstart.MachineAgentUpstartService(name, toolsDir, cfg.DataDir, "/var/log/juju/", tag, machineId, nil)
 	cmds, err := conf.InstallCommands()
 	if err != nil {
 		return fmt.Errorf("cannot make cloud-init upstart script for the %s agent: %v", tag, err)

=== modified file 'environs/cloudinit/cloudinit_test.go'
--- environs/cloudinit/cloudinit_test.go	2013-09-16 02:30:27 +0000
+++ environs/cloudinit/cloudinit_test.go	2013-09-17 02:01:38 +0000
@@ -110,7 +110,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 --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 --data-dir '/var/lib/juju' --machine-id 0 --debug >> /var/log/juju/machine-0\.log 2>&1\\nEOF\\n
 start jujud-machine-0
 `,
 	}, {
@@ -174,7 +174,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 --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 --data-dir '/var/lib/juju' --machine-id 0 --debug >> /var/log/juju/machine-0\.log 2>&1\\nEOF\\n
 start jujud-machine-0
 `,
 	}, {
@@ -216,7 +216,7 @@
 install -m 600 /dev/null '/var/lib/juju/agents/machine-99/agent\.conf'
 printf '%s\\n' '.*' > '/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 --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 --data-dir '/var/lib/juju' --machine-id 99 --debug >> /var/log/juju/machine-99\.log 2>&1\\nEOF\\n
 start jujud-machine-99
 `,
 	}, {
@@ -259,7 +259,7 @@
 install -m 600 /dev/null '/var/lib/juju/agents/machine-2-lxc-1/agent\.conf'
 printf '%s\\n' '.*' > '/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 --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 --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
 `,
 	},

=== modified file 'provider/local/environ.go'
--- provider/local/environ.go	2013-09-12 02:28:20 +0000
+++ provider/local/environ.go	2013-09-17 02:01:38 +0000
@@ -462,14 +462,13 @@
 	toolsDir := agenttools.ToolsDir(dataDir, tag)
 
 	logDir := env.config.logDir()
-	logConfig := "--debug" // TODO(thumper): specify loggo config
 	machineEnvironment := map[string]string{
 		"USER": env.config.user,
 		"HOME": osenv.Home(),
 	}
 	agentService := upstart.MachineAgentUpstartService(
 		env.machineAgentServiceName(),
-		toolsDir, dataDir, logDir, tag, machineId, logConfig, machineEnvironment)
+		toolsDir, dataDir, logDir, tag, machineId, machineEnvironment)
 
 	agentService.InitDir = upstartScriptLocation
 	logger.Infof("installing service %s to %s", env.machineAgentServiceName(), agentService.InitDir)

=== modified file 'upstart/service.go'
--- upstart/service.go	2013-09-13 05:20:56 +0000
+++ upstart/service.go	2013-09-17 02:01:38 +0000
@@ -42,9 +42,11 @@
 
 // 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, env map[string]string) *Conf {
+func MachineAgentUpstartService(name, toolsDir, dataDir, logDir, tag, machineId string, env map[string]string) *Conf {
 	svc := NewService(name)
 	logFile := filepath.Join(logDir, tag+".log")
+	// The machine agent always starts with debug turned on.  The logger worker
+	// will update this to the system logging environment as soon as it starts.
 	return &Conf{
 		Service: *svc,
 		Desc:    fmt.Sprintf("juju %s agent", tag),
@@ -55,7 +57,7 @@
 			" machine" +
 			" --data-dir " + utils.ShQuote(dataDir) +
 			" --machine-id " + machineId +
-			" " + logConfig,
+			" --debug",
 		Out: logFile,
 		Env: env,
 	}