Merge lp:~jameinel/juju-core/log-mongo-version into lp:~go-bot/juju-core/trunk

[John A Meinel]

agent/mongo: log version of mongo

Just before we start mongo, log the version so we know what we're
running. This is mostly just to aid us in debugging log files from
other people.

https://codereview.appspot.com/87570043/

agent/mongo: log version of mongo

Just before we start mongo, log the version so we know what we're
running. This is mostly just to aid us in debugging log files from
other people.

https://codereview.appspot.com/87570043/

[John A Meinel (jameinel)]

Reviewers: mp+215656_code.launchpad.net,

Message:
Please take a look.

Description:
agent/mongo: log version of mongo

Just before we start mongo, log the version so we know what we're
running. This is mostly just to aid us in debugging log files from
other people.

https://code.launchpad.net/~jameinel/juju-core/log-mongo-version/+merge/215656

(do not edit description out of merge proposal)

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

Affected files (+22, -0 lines):
   A [revision details]
   M agent/mongo/mongo.go
   M agent/mongo/mongo_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-20140414112125-eouvcipf4rmjrahn
+New revision: <email address hidden>

Index: agent/mongo/mongo.go
=== modified file 'agent/mongo/mongo.go'
--- agent/mongo/mongo.go 2014-04-12 09:18:12 +0000
+++ agent/mongo/mongo.go 2014-04-14 12:39:38 +0000
@@ -251,6 +251,17 @@
   return nil
  }

+func logMongoVersion(mongopath string) {
+ logger.Debugf("found mongod at: %s", mongopath)
+ cmd := exec.Command(mongopath, "--version")
+ output, err := cmd.CombinedOutput()
+ if err != nil {
+ logger.Infof("failed to read the output from %s --version", mongopath)
+ return
+ }
+ logger.Debugf("mongod --version:\n%s", string(output))
+}
+
  // mongoUpstartService returns the upstart config for the mongo state
service.
  //
  // This method assumes there exist "server.pem" and "shared_secret"
keyfiles in dataDir.
@@ -267,6 +278,7 @@
   if err != nil {
    return nil, err
   }
+ logMongoVersion(mongopath)

   conf := &upstart.Conf{
    Service: *svc,

Index: agent/mongo/mongo_test.go
=== modified file 'agent/mongo/mongo_test.go'
--- agent/mongo/mongo_test.go 2014-04-11 19:48:39 +0000
+++ agent/mongo/mongo_test.go 2014-04-14 12:39:38 +0000
@@ -119,6 +119,14 @@
   err = EnsureMongoServer(dir, port, namespace)
   c.Assert(err, gc.IsNil)
   c.Assert(svc.Installed(), jc.IsTrue)
+
+ // make sure that we log the version of mongodb as we get ready to
+ // start it
+ tlog := c.GetTestLog()
+ any := `(.|\n)*`
+ start := "^" + any
+ tail := any + "$"
+ c.Assert(tlog, gc.Matches, start+`mongod --version:\ndb version
v2\.\d+\.\d+`+tail)
  }

  func (s *MongoSuite) TestNoMongoDir(c *gc.C) {

[Nate Finch (natefinch)]

lgtm, a couple minor suggestions.

https://codereview.appspot.com/87570043/diff/1/agent/mongo/mongo.go
File agent/mongo/mongo.go (right):

https://codereview.appspot.com/87570043/diff/1/agent/mongo/mongo.go#newcode255
agent/mongo/mongo.go:255: logger.Debugf("found mongod at: %s",
mongopath)
maybe put this after the err check, just on the off chance we *don't*
find mongo there? or change the message to "checking version of mongo
at <path>?

https://codereview.appspot.com/87570043/diff/1/agent/mongo/mongo.go#newcode259
agent/mongo/mongo.go:259: logger.Infof("failed to read the output from
%s --version", mongopath)
should we log the error? it could be "mongod not found"

https://codereview.appspot.com/87570043/

[Roger Peppe (rogpeppe)]

LGTM modulo the suggestions below.

https://codereview.appspot.com/87570043/diff/1/agent/mongo/mongo.go
File agent/mongo/mongo.go (right):

https://codereview.appspot.com/87570043/diff/1/agent/mongo/mongo.go#newcode259
agent/mongo/mongo.go:259: logger.Infof("failed to read the output from
%s --version", mongopath)
On 2014/04/16 10:47:20, nate.finch wrote:
> should we log the error? it could be "mongod not found"

the error is often not very useful actually.
it may be that the command returns a non-zero exit
status but actually produced some useful output.

worth logging the output too (with %q), I think, when there is an error.

https://codereview.appspot.com/87570043/diff/1/agent/mongo/mongo.go#newcode262
agent/mongo/mongo.go:262: logger.Debugf("mongod --version:\n%s",
string(output))
I think this should be an Infof.

Also, no need for the string conversion (%s formats []byte
correctly as a string)

Additionally I'm not that keen on log messages with embedded newlines.
How about stripping space and using %q? Then the log line will look
like:

mongod --version: "db version v2.4.9\nWed Apr 16 12:25:32.774 git
version: nogitversion"

which looks pretty good to me and is easily greppable.

https://codereview.appspot.com/87570043/diff/1/agent/mongo/mongo.go#newcode281
agent/mongo/mongo.go:281: logMongoVersion(mongopath)
I'm not sure this is a great place for this log message.
It seems like we're putting the log message here as
a side effect in a function that is otherwise side-effect
free.

Two possibilities:
1) make mongoUpstartService return the version string
2) make mongoUpstartService return the mongod executable
path and call logMongoVersion from EnsureMongoServer.

I think I prefer the latter.

https://codereview.appspot.com/87570043/

[John A Meinel (jameinel)]

Please take a look.

https://codereview.appspot.com/87570043/diff/1/agent/mongo/mongo.go
File agent/mongo/mongo.go (right):

https://codereview.appspot.com/87570043/diff/1/agent/mongo/mongo.go#newcode255
agent/mongo/mongo.go:255: logger.Debugf("found mongod at: %s",
mongopath)
On 2014/04/16 10:47:20, nate.finch wrote:
> maybe put this after the err check, just on the off chance we *don't*
find mongo
> there? or change the message to "checking version of mongo at <path>?

I originally had it as one message, but then I wanted to make sure to
log something on error. But I realize we'll have that data as long as we
put the path in the output, so I'll fix it to just at the end. (Nicer
not to have lots of lines anyway.

https://codereview.appspot.com/87570043/diff/1/agent/mongo/mongo.go#newcode259
agent/mongo/mongo.go:259: logger.Infof("failed to read the output from
%s --version", mongopath)
On 2014/04/16 10:47:20, nate.finch wrote:
> should we log the error? it could be "mongod not found"

logged

https://codereview.appspot.com/87570043/

[Go Bot (go-bot)]

Attempt to merge into lp:juju-core failed due to conflicts:

text conflict in agent/mongo/mongo.go
text conflict in agent/mongo/mongo_test.go

=== modified file 'agent/mongo/mongo.go'
--- agent/mongo/mongo.go	2014-04-15 15:12:49 +0000
+++ agent/mongo/mongo.go	2014-04-17 12:10:55 +0000
@@ -121,6 +121,7 @@
 
 	path, err := exec.LookPath("mongod")
 	if err != nil {
+		logger.Infof("could not find %v or mongod in $PATH", JujuMongodPath)
 		return "", err
 	}
 	return path, nil
@@ -185,10 +186,11 @@
 		return fmt.Errorf("cannot install mongod: %v", err)
 	}
 
-	upstartConf, err := mongoUpstartService(namespace, dataDir, dbDir, info.StatePort, withHA)
+	upstartConf, mongoPath, err := mongoUpstartService(namespace, dataDir, dbDir, info.StatePort, withHA)
 	if err != nil {
 		return err
 	}
+	logMongoVersion(mongoPath)
 
 	// TODO(natefinch) 2014-04-12 https://launchpad.net/bugs/1306902
 	// remove this once we support upgrading to HA
@@ -247,6 +249,16 @@
 	return nil
 }
 
+func logMongoVersion(mongoPath string) {
+	cmd := exec.Command(mongoPath, "--version")
+	output, err := cmd.CombinedOutput()
+	if err != nil {
+		logger.Infof("failed to read the output from %s --version: %v", mongoPath, err)
+		return
+	}
+	logger.Debugf("using mongod: %s --version: %q", mongoPath, output)
+}
+
 func sslKeyPath(dataDir string) string {
 	return filepath.Join(dataDir, "server.pem")
 }
@@ -256,14 +268,16 @@
 }
 
 // mongoUpstartService returns the upstart config for the mongo state service.
-//
-func mongoUpstartService(namespace, dataDir, dbDir string, port int, withHA bool) (*upstart.Conf, error) {
+// It also returns the path to the mongod executable that the upstart config
+// will be using.
+func mongoUpstartService(namespace, dataDir, dbDir string, port int, withHA bool) (*upstart.Conf, string, error) {
 	svc := upstart.NewService(ServiceName(namespace))
 
 	mongoPath, err := MongodPath()
 	if err != nil {
-		return nil, err
+		return nil, "", err
 	}
+
 	mongoCmd := mongoPath + " --auth" +
 		" --dbpath=" + utils.ShQuote(dbDir) +
 		" --sslOnNormalPorts" +
@@ -287,7 +301,7 @@
 		},
 		Cmd: mongoCmd,
 	}
-	return conf, nil
+	return conf, mongoPath, nil
 }
 
 func aptGetInstallMongod() error {
@@ -334,11 +348,11 @@
 func mongoNoauthCommand(dataDir string, port int) (*exec.Cmd, error) {
 	sslKeyFile := path.Join(dataDir, "server.pem")
 	dbDir := filepath.Join(dataDir, "db")
-	mongopath, err := MongodPath()
+	mongoPath, err := MongodPath()
 	if err != nil {
 		return nil, err
 	}
-	cmd := exec.Command(mongopath,
+	cmd := exec.Command(mongoPath,
 		"--noauth",
 		"--dbpath", dbDir,
 		"--sslOnNormalPorts",

=== modified file 'agent/mongo/mongo_test.go'
--- agent/mongo/mongo_test.go	2014-04-15 16:01:43 +0000
+++ agent/mongo/mongo_test.go	2014-04-17 12:10:55 +0000
@@ -49,11 +49,12 @@
 }
 
 func (s *MongoSuite) SetUpTest(c *gc.C) {
+	s.LoggingSuite.SetUpTest(c)
 	// Try to make sure we don't execute any commands accidentally.
 	s.PatchEnvironment("PATH", "")
 
 	s.mongodPath = filepath.Join(c.MkDir(), "mongod")
-	err := ioutil.WriteFile(s.mongodPath, nil, 0755)
+	err := ioutil.WriteFile(s.mongodPath, []byte("#!/bin/bash\n\nprintf %s 'db version v2.4.9'\n"), 0755)
 	c.Assert(err, gc.IsNil)
 	s.PatchValue(&JujuMongodPath, s.mongodPath)
 
@@ -158,16 +159,24 @@
 	err = EnsureMongoServer(dataDir, namespace, testInfo, WithHA)
 	c.Assert(err, gc.IsNil)
 	assertInstalled()
+
+	// make sure that we log the version of mongodb as we get ready to
+	// start it
+	tlog := c.GetTestLog()
+	any := `(.|\n)*`
+	start := "^" + any
+	tail := any + "$"
+	c.Assert(tlog, gc.Matches, start+`using mongod: .*/mongod --version: "db version v2\.4\.9`+tail)
 }
 
 func (s *MongoSuite) TestMongoUpstartServiceWithHA(c *gc.C) {
 	dataDir := c.MkDir()
 
-	svc, err := mongoUpstartService("", dataDir, dataDir, 1234, WithHA)
+	svc, _, err := mongoUpstartService("", dataDir, dataDir, 1234, WithHA)
 	c.Assert(err, gc.IsNil)
 	c.Assert(strings.Contains(svc.Cmd, "--replSet"), jc.IsTrue)
 
-	svc, err = mongoUpstartService("", dataDir, dataDir, 1234, WithoutHA)
+	svc, _, err = mongoUpstartService("", dataDir, dataDir, 1234, WithoutHA)
 	c.Assert(err, gc.IsNil)
 	c.Assert(strings.Contains(svc.Cmd, "--replSet"), jc.IsFalse)
 }

=== modified file 'provider/openstack/local_test.go'
--- provider/openstack/local_test.go	2014-04-08 15:51:39 +0000
+++ provider/openstack/local_test.go	2014-04-17 12:10:55 +0000
@@ -334,7 +334,7 @@
 			}
 		}
 		if !found {
-			c.Errorf("expected security group %g not found", name)
+			c.Errorf("expected security group %q not found", name)
 		}
 	}
 	for _, group := range groups {
@@ -346,7 +346,7 @@
 			}
 		}
 		if !found {
-			c.Errorf("existing security group %g is not expected", group.Name)
+			c.Errorf("existing security group %q is not expected", group.Name)
 		}
 	}
 }

=== modified file 'worker/provisioner/provisioner_task.go'
--- worker/provisioner/provisioner_task.go	2014-04-11 15:16:00 +0000
+++ worker/provisioner/provisioner_task.go	2014-04-17 12:10:55 +0000
@@ -481,7 +481,7 @@
 
 	err = machine.SetInstanceInfo(inst.Id(), nonce, metadata, networks, ifaces)
 	if err != nil && params.IsCodeNotImplemented(err) {
-		return fmt.Errorf("cannot provision instance %v for machine %q with networks: not implemented")
+		return fmt.Errorf("cannot provision instance %v for machine %q with networks: not implemented", inst.Id(), machine)
 	} else if err == nil {
 		logger.Infof("started machine %s as instance %s with hardware %q, networks %v, interfaces %v", machine, inst.Id(), metadata, networks, ifaces)
 		return nil