Merge pull request #2305 from natefinch/fix-1452285-master

Merge pull request #2303 from natefinch/fix-1452285-1.24

Forward port of pull request #2303 from natefinch/fix-1452285-1.24

revamp log rotation

This fixes https://bugs.launchpad.net/juju-core/+bug/1452285

The problem was that we had moved the log rotation code earlier in the startup code, and then other code overwrote what loggo used as a default writer.  This code consolidates the loggo-modification code to a single place, so we don't have to worry about that anymore.


(Review request: http://reviews.vapour.ws/r/1668/)

agent/agent.go

@@ -191,6 +191,11 @@ type ConfigSetterOnly interface {
 	SetStateServingInfo(info params.StateServingInfo)
 }
 
+// LogFileName returns the filename for the Agent's log file.
+func LogFilename(c Config) string {
+	return filepath.Join(c.LogDir(), c.Tag().String()+".log")
+}
+
 type ConfigMutator func(ConfigSetter) error
 
 type ConfigWriter interface {

cmd/jujud/agent/agent.go

@@ -49,45 +49,77 @@ func openAPIForAgent(info *api.Info, opts api.DialOpts) (*api.State, error) {
 	return api.Open(info, opts)
 }
 
-// AgentConf handles command-line flags shared by all agents.
-type AgentConf struct {
-	DataDir string
+type AgentConf interface {
+	// AddFlags injects common agent flags into f.
+	AddFlags(f *gnuflag.FlagSet)
+	// CheckArgs reports whether the given args are valid for this agent.
+	CheckArgs(args []string) error
+	// ReadConfig reads the agent's config from its config file.
+	ReadConfig(tag string) error
+	// ChangeConfig modifies this configuration using the given mutator.
+	ChangeConfig(change agent.ConfigMutator) error
+	// CurrentConfig returns the agent config for this agent.
+	CurrentConfig() agent.Config
+	// SetAPIHostPorts satisfies worker/apiaddressupdater/APIAddressSetter.
+	SetAPIHostPorts(servers [][]network.HostPort) error
+	// SetStateServingInfo satisfies worker/certupdater/SetStateServingInfo.
+	SetStateServingInfo(info params.StateServingInfo) error
+	// DataDir returns the directory where this agent should store its data.
+	DataDir() string
+}
+
+// NewAgentConf returns a new value that satisfies AgentConf
+func NewAgentConf(dataDir string) AgentConf {
+	return &agentConf{dataDir: dataDir}
+}
+
+// agentConf handles command-line flags shared by all agents.
+type agentConf struct {
+	dataDir string
 	mu      sync.Mutex
 	_config agent.ConfigSetterWriter
 }
 
 // AddFlags injects common agent flags into f.
-func (c *AgentConf) AddFlags(f *gnuflag.FlagSet) {
+func (c *agentConf) AddFlags(f *gnuflag.FlagSet) {
 	// TODO(dimitern) 2014-02-19 bug 1282025
 	// We need to pass a config location here instead and
 	// use it to locate the conf and the infer the data-dir
 	// from there instead of passing it like that.
-	f.StringVar(&c.DataDir, "data-dir", util.DataDir, "directory for juju data")
+	f.StringVar(&c.dataDir, "data-dir", util.DataDir, "directory for juju data")
 }
 
-func (c *AgentConf) CheckArgs(args []string) error {
-	if c.DataDir == "" {
+// CheckArgs reports whether the given args are valid for this agent.
+func (c *agentConf) CheckArgs(args []string) error {
+	if c.dataDir == "" {
 		return util.RequiredError("data-dir")
 	}
 	return cmd.CheckEmpty(args)
 }
 
-func (c *AgentConf) ReadConfig(tag string) error {
+// DataDir returns the directory where this agent should store its data.
+func (c *agentConf) DataDir() string {
+	return c.dataDir
+}
+
+// ReadConfig reads the agent's config from its config file.
+func (c *agentConf) ReadConfig(tag string) error {
 	t, err := names.ParseTag(tag)
 	if err != nil {
 		return err
 	}
 	c.mu.Lock()
 	defer c.mu.Unlock()
-	conf, err := agent.ReadConfig(agent.ConfigPath(c.DataDir, t))
+	conf, err := agent.ReadConfig(agent.ConfigPath(c.dataDir, t))
 	if err != nil {
 		return err
 	}
 	c._config = conf
 	return nil
 }
 
-func (ch *AgentConf) ChangeConfig(change agent.ConfigMutator) error {
+// ChangeConfig modifies this configuration using the given mutator.
+func (ch *agentConf) ChangeConfig(change agent.ConfigMutator) error {
 	ch.mu.Lock()
 	defer ch.mu.Unlock()
 	if err := change(ch._config); err != nil {
@@ -99,22 +131,23 @@ func (ch *AgentConf) ChangeConfig(change agent.ConfigMutator) error {
 	return nil
 }
 
-func (ch *AgentConf) CurrentConfig() agent.Config {
+// CurrentConfig returns the agent config for this agent.
+func (ch *agentConf) CurrentConfig() agent.Config {
 	ch.mu.Lock()
 	defer ch.mu.Unlock()
 	return ch._config.Clone()
 }
 
 // SetAPIHostPorts satisfies worker/apiaddressupdater/APIAddressSetter.
-func (a *AgentConf) SetAPIHostPorts(servers [][]network.HostPort) error {
+func (a *agentConf) SetAPIHostPorts(servers [][]network.HostPort) error {
 	return a.ChangeConfig(func(c agent.ConfigSetter) error {
 		c.SetAPIHostPorts(servers)
 		return nil
 	})
 }
 
 // SetStateServingInfo satisfies worker/certupdater/SetStateServingInfo.
-func (a *AgentConf) SetStateServingInfo(info params.StateServingInfo) error {
+func (a *agentConf) SetStateServingInfo(info params.StateServingInfo) error {
 	return a.ChangeConfig(func(c agent.ConfigSetter) error {
 		c.SetStateServingInfo(info)
 		return nil

cmd/jujud/agent/agent_test.go

@@ -104,7 +104,7 @@ func (s *apiOpenSuite) TestOpenAPIStateWaitsProvisionedGivesUp(c *gc.C) {
 	c.Assert(called, gc.Equals, checkProvisionedStrategy.Min+1)
 }
 
-type acCreator func() (cmd.Command, *AgentConf)
+type acCreator func() (cmd.Command, AgentConf)
 
 // CheckAgentCommand is a utility function for verifying that common agent
 // options are handled by a Command; it returns an instance of that
@@ -114,7 +114,7 @@ func CheckAgentCommand(c *gc.C, create acCreator, args []string) cmd.Command {
 	err := coretesting.InitCommand(com, args)
 	dataDir, err := paths.DataDir(version.Current.Series)
 	c.Assert(err, jc.ErrorIsNil)
-	c.Assert(conf.DataDir, gc.Equals, dataDir)
+	c.Assert(conf.DataDir(), gc.Equals, dataDir)
 	badArgs := append(args, "--data-dir", "")
 	com, _ = create()
 	err = coretesting.InitCommand(com, badArgs)
@@ -123,7 +123,7 @@ func CheckAgentCommand(c *gc.C, create acCreator, args []string) cmd.Command {
 	args = append(args, "--data-dir", "jd")
 	com, conf = create()
 	c.Assert(coretesting.InitCommand(com, args), gc.IsNil)
-	c.Assert(conf.DataDir, gc.Equals, "jd")
+	c.Assert(conf.DataDir(), gc.Equals, "jd")
 	return com
 }
 

cmd/jujud/agent/agentconf_test.go

@@ -21,8 +21,8 @@ type agentConfSuite struct {
 func (s *agentConfSuite) TestChangeConfigSuccess(c *gc.C) {
 	mcsw := &mockConfigSetterWriter{}
 
-	conf := AgentConf{
-		DataDir: c.MkDir(),
+	conf := agentConf{
+		dataDir: c.MkDir(),
 		_config: mcsw,
 	}
 
@@ -37,8 +37,8 @@ func (s *agentConfSuite) TestChangeConfigSuccess(c *gc.C) {
 func (s *agentConfSuite) TestChangeConfigMutateFailure(c *gc.C) {
 	mcsw := &mockConfigSetterWriter{}
 
-	conf := AgentConf{
-		DataDir: c.MkDir(),
+	conf := agentConf{
+		dataDir: c.MkDir(),
 		_config: mcsw,
 	}
 
@@ -55,8 +55,8 @@ func (s *agentConfSuite) TestChangeConfigWriteFailure(c *gc.C) {
 		WriteError: errors.New("boom"),
 	}
 
-	conf := AgentConf{
-		DataDir: c.MkDir(),
+	conf := agentConf{
+		dataDir: c.MkDir(),
 		_config: mcsw,
 	}
 

cmd/jujud/agent/machine.go

@@ -160,11 +160,13 @@ type AgentConfigWriter interface {
 // command-line arguments and instantiating and running a
 // MachineAgent.
 func NewMachineAgentCmd(
+	ctx *cmd.Context,
 	machineAgentFactory func(string) *MachineAgent,
 	agentInitializer AgentInitializer,
 	configFetcher AgentConfigWriter,
 ) cmd.Command {
 	return &machineAgentCmd{
+		ctx:                 ctx,
 		machineAgentFactory: machineAgentFactory,
 		agentInitializer:    agentInitializer,
 		currentConfig:       configFetcher,
@@ -178,6 +180,7 @@ type machineAgentCmd struct {
 	agentInitializer    AgentInitializer
 	currentConfig       AgentConfigWriter
 	machineAgentFactory func(string) *MachineAgent
+	ctx                 *cmd.Context
 
 	// This group is for debugging purposes.
 	logToStdErr bool
@@ -212,15 +215,15 @@ func (a *machineAgentCmd) Init(args []string) error {
 		return errors.Annotate(err, "cannot read agent configuration")
 	}
 	agentConfig := a.currentConfig.CurrentConfig()
-	filename := filepath.Join(agentConfig.LogDir(), agentConfig.Tag().String()+".log")
 
-	log := &lumberjack.Logger{
-		Filename:   filename,
+	// the context's stderr is set as the loggo writer in github.com/juju/cmd/logging.go
+	a.ctx.Stderr = &lumberjack.Logger{
+		Filename:   agent.LogFilename(agentConfig),
 		MaxSize:    300, // megabytes
 		MaxBackups: 2,
 	}
 
-	return cmdutil.SwitchProcessToRollingLogs(log)
+	return nil
 }
 
 // Run instantiates a MachineAgent and runs it.

cmd/jujud/agent/machine_test.go

@@ -25,6 +25,7 @@ import (
 	gc "gopkg.in/check.v1"
 	"gopkg.in/juju/charm.v5"
 	"gopkg.in/juju/charm.v5/charmrepo"
+	"gopkg.in/natefinch/lumberjack.v2"
 
 	"github.com/juju/juju/agent"
 	"github.com/juju/juju/api"
@@ -60,6 +61,7 @@ import (
 	sshtesting "github.com/juju/juju/utils/ssh/testing"
 	"github.com/juju/juju/version"
 	"github.com/juju/juju/worker"
+	"github.com/juju/juju/worker/apiaddressupdater"
 	"github.com/juju/juju/worker/authenticationworker"
 	"github.com/juju/juju/worker/certupdater"
 	"github.com/juju/juju/worker/deployer"
@@ -208,16 +210,17 @@ func (s *commonMachineSuite) configureMachine(c *gc.C, machineId string, vers ve
 
 // newAgent returns a new MachineAgent instance
 func (s *commonMachineSuite) newAgent(c *gc.C, m *state.Machine) *MachineAgent {
-	agentConf := AgentConf{DataDir: s.DataDir()}
+	agentConf := agentConf{dataDir: s.DataDir()}
 	agentConf.ReadConfig(names.NewMachineTag(m.Id()).String())
 	machineAgentFactory := MachineAgentFactoryFn(&agentConf, &agentConf)
 	return machineAgentFactory(m.Id())
 }
 
 func (s *MachineSuite) TestParseSuccess(c *gc.C) {
-	create := func() (cmd.Command, *AgentConf) {
-		agentConf := AgentConf{DataDir: s.DataDir()}
+	create := func() (cmd.Command, AgentConf) {
+		agentConf := agentConf{dataDir: s.DataDir()}
 		a := NewMachineAgentCmd(
+			nil,
 			MachineAgentFactoryFn(&agentConf, &agentConf),
 			&agentConf,
 			&agentConf,
@@ -260,14 +263,14 @@ func (s *MachineSuite) TestParseNonsense(c *gc.C) {
 		{},
 		{"--machine-id", "-4004"},
 	} {
-		var agentConf AgentConf
+		var agentConf agentConf
 		err := ParseAgentCommand(&machineAgentCmd{agentInitializer: &agentConf}, args)
 		c.Assert(err, gc.ErrorMatches, "--machine-id option must be set, and expects a non-negative integer")
 	}
 }
 
 func (s *MachineSuite) TestParseUnknown(c *gc.C) {
-	var agentConf AgentConf
+	var agentConf agentConf
 	a := &machineAgentCmd{agentInitializer: &agentConf}
 	err := ParseAgentCommand(a, []string{"--machine-id", "42", "blistering barnacles"})
 	c.Assert(err, gc.ErrorMatches, `unrecognized args: \["blistering barnacles"\]`)
@@ -280,6 +283,83 @@ func (s *MachineSuite) TestRunInvalidMachineId(c *gc.C) {
 	c.Assert(err, gc.ErrorMatches, "some error")
 }
 
+type FakeConfig struct {
+	agent.Config
+}
+
+func (FakeConfig) LogDir() string {
+	return "/var/log/juju/"
+}
+
+func (FakeConfig) Tag() names.Tag {
+	return names.NewMachineTag("42")
+}
+
+type FakeAgentConfig struct {
+	AgentConfigWriter
+	apiaddressupdater.APIAddressSetter
+	AgentInitializer
+}
+
+func (FakeAgentConfig) ReadConfig(string) error { return nil }
+
+func (FakeAgentConfig) CurrentConfig() agent.Config {
+	return FakeConfig{}
+}
+
+func (FakeAgentConfig) CheckArgs([]string) error { return nil }
+
+func (s *MachineSuite) TestUseLumberjack(c *gc.C) {
+	ctx, err := cmd.DefaultContext()
+	c.Assert(err, gc.IsNil)
+
+	agentConf := FakeAgentConfig{}
+
+	a := NewMachineAgentCmd(
+		ctx,
+		MachineAgentFactoryFn(agentConf, agentConf),
+		agentConf,
+		agentConf,
+	)
+	// little hack to set the data that Init expects to already be set
+	a.(*machineAgentCmd).machineId = "42"
+
+	err = a.Init(nil)
+	c.Assert(err, gc.IsNil)
+
+	l, ok := ctx.Stderr.(*lumberjack.Logger)
+	c.Assert(ok, jc.IsTrue)
+	c.Check(l.MaxAge, gc.Equals, 0)
+	c.Check(l.MaxBackups, gc.Equals, 2)
+	c.Check(l.Filename, gc.Equals, "/var/log/juju/machine-42.log")
+	c.Check(l.MaxSize, gc.Equals, 300)
+}
+
+func (s *MachineSuite) TestDontUseLumberjack(c *gc.C) {
+	ctx, err := cmd.DefaultContext()
+	c.Assert(err, gc.IsNil)
+
+	agentConf := FakeAgentConfig{}
+
+	a := NewMachineAgentCmd(
+		ctx,
+		MachineAgentFactoryFn(agentConf, agentConf),
+		agentConf,
+		agentConf,
+	)
+	// little hack to set the data that Init expects to already be set
+	a.(*machineAgentCmd).machineId = "42"
+
+	// set the value that normally gets set by the flag parsing
+	a.(*machineAgentCmd).logToStdErr = true
+
+	err = a.Init(nil)
+	c.Assert(err, gc.IsNil)
+
+	_, ok := ctx.Stderr.(*lumberjack.Logger)
+	c.Assert(ok, jc.IsFalse)
+}
+
 func (s *MachineSuite) TestRunStop(c *gc.C) {
 	m, ac, _ := s.primeAgent(c, version.Current, state.JobHostUnits)
 	a := s.newAgent(c, m)

cmd/jujud/bootstrap.go

@@ -61,6 +61,13 @@ type BootstrapCommand struct {
 	ImageMetadataDir string
 }
 
+// NewBootstrapCommand returns a new BootstrapCommand that has been initialized.
+func NewBootstrapCommand() *BootstrapCommand {
+	return &BootstrapCommand{
+		AgentConf: agentcmd.NewAgentConf(""),
+	}
+}
+
 // Info returns a decription of the command.
 func (c *BootstrapCommand) Info() *cmd.Info {
 	return &cmd.Info{

cmd/jujud/bootstrap_test.go

@@ -179,7 +179,7 @@ func (s *BootstrapSuite) initBootstrapCommand(c *gc.C, jobs []multiwatcher.Machi
 	err = machineConf.Write()
 	c.Assert(err, jc.ErrorIsNil)
 
-	cmd = &BootstrapCommand{}
+	cmd = NewBootstrapCommand()
 
 	err = testing.InitCommand(cmd, append([]string{"--data-dir", s.dataDir}, args...))
 	return machineConf, cmd, err