Merge pull request #2458 from natefinch/fix-1370896

Fix 1370896 - juju has conf files in /var/log/juju on instances

Fixes https://bugs.launchpad.net/juju-core/+bug/1370896

All the rsyslog configuration files are now created in juju's data directory (by default, /var/lib/juju), instead of the log directory (/var/log/juju).  Also added an upgrade step to move the files from old location to new location.

I made a couple changes to how you create a SyslogConfig, mostly because it was using a function with a huge number of string arguments, which made it very difficult to read, and to understand what value was being assigned to what field.  This made updating the code almost impossible, because of minor positional differences in the function signatures.

I changed  syslog.NewAccumulateConfig and NewForwardConfig to just take a pointer to a SyslogConfig, since pretty much all they did was set exported fields on the type.  Now they just set the non-exported field, and the caller sets everything else in a struct literal.

I also changed it so that the Namespace field in SyslogConfig always stays just "Namespace", not "-Namespace", since it's an exported value, and having it be something different than what was set is really confusing.  Instead, we just add the dash when rendering to the template.

Changed the tests much the same way - just set up the struct and pass it in is so much easier to read than a huge function call with a ton of different string parameters in a random order.  Also changed the test template, so that there aren't hard-coded values in it, and instead actually use the values passed in.




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

cmd/jujud/util/util.go

@@ -237,7 +237,7 @@ var NewRsyslogConfigWorker = func(st *apirsyslog.State, agentConfig agent.Config
 	if err != nil {
 		return nil, err
 	}
-	return rsyslog.NewRsyslogConfigWorker(st, mode, tag, namespace, addrs)
+	return rsyslog.NewRsyslogConfigWorker(st, mode, tag, namespace, addrs, agentConfig.DataDir())
 }
 
 // ParamsStateServingInfoToStateStateServingInfo converts a

upgrades/export_test.go

@@ -42,4 +42,8 @@ var (
 	AddEnvironmentUUIDToAgentConfig = addEnvironmentUUIDToAgentConfig
 	AddDefaultStoragePools          = addDefaultStoragePools
 	MoveBlocksFromEnvironToState    = moveBlocksFromEnvironToState
+
+	// 124 upgrade functions
+	MoveSyslogConfig = moveSyslogConfig
+	CopyFile         = copyFile
 )

upgrades/operations.go

@@ -56,6 +56,10 @@ var upgradeOperations = func() []Operation {
 			version.MustParse("1.23.0"),
 			stepsFor123(),
 		},
+		upgradeToVersion{
+			version.MustParse("1.24.0"),
+			stepsFor124(),
+		},
 	}
 	return steps
 }

upgrades/steps124.go

@@ -3,12 +3,19 @@
 
 package upgrades
 
-import "github.com/juju/juju/state"
+import (
+	"fmt"
+	"io"
+	"os"
+	"path/filepath"
+	"strings"
+
+	"github.com/juju/juju/state"
+)
 
 // stateStepsFor124 returns upgrade steps for Juju 1.24 that manipulate state directly.
 func stateStepsFor124() []Step {
-	var steps []Step
-	steps = append(steps,
+	return []Step{
 		&upgradeStep{
 			description: "add block device documents for existing machines",
 			targets:     []Target{DatabaseMaster},
@@ -22,6 +29,80 @@ func stateStepsFor124() []Step {
 				return state.AddInstanceIdFieldOfIPAddresses(context.State())
 			},
 		},
-	)
-	return steps
+	}
+}
+
+// stepsFor124 returns upgrade steps for Juju 1.24 that only need the API.
+func stepsFor124() []Step {
+	return []Step{
+		&upgradeStep{
+			description: "move syslog config from LogDir to DataDir",
+			targets:     []Target{AllMachines},
+			run:         moveSyslogConfig,
+		},
+	}
+}
+
+func moveSyslogConfig(context Context) error {
+	config := context.AgentConfig()
+	logdir := config.LogDir()
+	datadir := config.DataDir()
+
+	// these values were copied from
+	// github.com/juju/juju/utils/syslog/config.go
+	// Yes this is bad, but it is only needed once every for an
+	// upgrade, so it didn't seem worth exporting those values.
+	files := []string{
+		"ca-cert.pem",
+		"rsyslog-cert.pem",
+		"rsyslog-key.pem",
+		"logrotate.conf",
+		"logrotate.run",
+	}
+	var errs []string
+	for _, f := range files {
+		oldpath := filepath.Join(logdir, f)
+		newpath := filepath.Join(datadir, f)
+		if err := copyFile(newpath, oldpath); err != nil {
+			errs = append(errs, err.Error())
+			continue
+		}
+		if err := os.Remove(oldpath); err != nil && !os.IsNotExist(err) {
+			errs = append(errs, err.Error())
+		}
+	}
+	if len(errs) > 0 {
+		return fmt.Errorf("error(s) while moving old syslog config files: %s", strings.Join(errs, "\n"))
+	}
+	return nil
+}
+
+// copyFile copies a file from one location to another.  It won't overwrite
+// existing files and will return nil in this case.  This is used instead of
+// os.Rename because os.Rename won't work across partitions.
+func copyFile(to, from string) error {
+	logger.Debugf("Copying %q to %q", from, to)
+	orig, err := os.Open(from)
+	if os.IsNotExist(err) {
+		logger.Debugf("Old file %q does not exist, skipping.", from)
+		// original doesn't exist, that's fine.
+		return nil
+	}
+	if err != nil {
+		return err
+	}
+	defer orig.Close()
+	info, err := orig.Stat()
+	if err != nil {
+		return err
+	}
+	target, err := os.OpenFile(to, os.O_CREATE|os.O_WRONLY|os.O_EXCL, info.Mode())
+	if os.IsExist(err) {
+		return nil
+	}
+	defer target.Close()
+	if _, err := io.Copy(target, orig); err != nil {
+		return err
+	}
+	return nil
 }

upgrades/steps124_test.go

@@ -4,9 +4,16 @@
 package upgrades_test
 
 import (
+	"io/ioutil"
+	"os"
+	"path/filepath"
+
+	jc "github.com/juju/testing/checkers"
 	gc "gopkg.in/check.v1"
 
+	"github.com/juju/juju/agent"
 	"github.com/juju/juju/testing"
+	"github.com/juju/juju/upgrades"
 	"github.com/juju/juju/version"
 )
 
@@ -23,3 +30,151 @@ func (s *steps124Suite) TestStateStepsFor124(c *gc.C) {
 	}
 	assertStateSteps(c, version.MustParse("1.24.0"), expected)
 }
+
+func (s *steps124Suite) TestStepsFor124(c *gc.C) {
+	expected := []string{
+		"move syslog config from LogDir to DataDir",
+	}
+	assertSteps(c, version.MustParse("1.24.0"), expected)
+}
+
+func (s *steps124Suite) TestCopyFileNew(c *gc.C) {
+	src := c.MkDir()
+	dest := c.MkDir()
+	srcdata := []byte("new data!")
+
+	// test that a file in src dir and not in dest dir gets copied.
+
+	newSrc := filepath.Join(src, "new")
+	err := ioutil.WriteFile(newSrc, srcdata, 0644)
+	c.Assert(err, gc.IsNil)
+
+	newDest := filepath.Join(dest, "new")
+
+	err = upgrades.CopyFile(newDest, newSrc)
+	c.Assert(err, gc.IsNil)
+
+	srcb, err := ioutil.ReadFile(newSrc)
+	c.Assert(err, gc.IsNil)
+	destb, err := ioutil.ReadFile(newDest)
+	c.Assert(err, gc.IsNil)
+	// convert to string and use Equals because we'll get a better failure message
+	c.Assert(string(destb), gc.Equals, string(srcb))
+}
+
+func (s *steps124Suite) TestCopyFileExisting(c *gc.C) {
+	src := c.MkDir()
+	dest := c.MkDir()
+	srcdata := []byte("new data!")
+	destdata := []byte("old data!")
+
+	exSrc := filepath.Join(src, "existing")
+	exDest := filepath.Join(dest, "existing")
+
+	err := ioutil.WriteFile(exSrc, srcdata, 0644)
+	c.Assert(err, gc.IsNil)
+	err = ioutil.WriteFile(exDest, destdata, 0644)
+	c.Assert(err, gc.IsNil)
+
+	err = upgrades.CopyFile(exDest, exSrc)
+	c.Assert(err, gc.IsNil)
+
+	// assert we haven't changed the destination
+	b, err := ioutil.ReadFile(exDest)
+
+	c.Assert(err, gc.IsNil)
+	// convert to string because we'll get a better failure message
+	c.Assert(string(b), gc.Equals, string(destdata))
+}
+
+func (s *steps124Suite) TestMoveSyslogConfigDefault(c *gc.C) {
+	logdir := c.MkDir()
+	datadir := c.MkDir()
+	data := []byte("data!")
+	files := []string{
+		"ca-cert.pem",
+		"rsyslog-cert.pem",
+		"rsyslog-key.pem",
+		"logrotate.conf",
+		"logrotate.run",
+	}
+	for _, f := range files {
+		err := ioutil.WriteFile(filepath.Join(logdir, f), data, 0644)
+		c.Assert(err, gc.IsNil)
+	}
+
+	ctx := fakeContext{cfg: fakeConfig{logdir: logdir, datadir: datadir}}
+	err := upgrades.MoveSyslogConfig(ctx)
+	c.Assert(err, gc.IsNil)
+
+	for _, f := range files {
+		_, err := os.Stat(filepath.Join(datadir, f))
+		c.Assert(err, gc.IsNil)
+		_, err = os.Stat(filepath.Join(logdir, f))
+		c.Assert(err, jc.Satisfies, os.IsNotExist)
+	}
+}
+
+func (s *steps124Suite) TestMoveSyslogConfig(c *gc.C) {
+	logdir := c.MkDir()
+	datadir := c.MkDir()
+	data := []byte("data!")
+	files := []string{
+		"logrotate.conf",
+		"logrotate.run",
+	}
+
+	// ensure that we don't overwrite an existing file in datadir, and don't
+	// error out if one of the files exists in datadir but not logdir.
+
+	err := ioutil.WriteFile(filepath.Join(logdir, "logrotate.conf"), data, 0644)
+	c.Assert(err, gc.IsNil)
+
+	err = ioutil.WriteFile(filepath.Join(datadir, "logrotate.run"), data, 0644)
+	c.Assert(err, gc.IsNil)
+
+	differentData := []byte("different")
+	existing := filepath.Join(datadir, "logrotate.conf")
+	err = ioutil.WriteFile(existing, differentData, 0644)
+	c.Assert(err, gc.IsNil)
+
+	ctx := fakeContext{cfg: fakeConfig{logdir: logdir, datadir: datadir}}
+	err = upgrades.MoveSyslogConfig(ctx)
+	c.Assert(err, gc.IsNil)
+
+	for _, f := range files {
+		_, err := os.Stat(filepath.Join(datadir, f))
+		c.Assert(err, gc.IsNil)
+		_, err = os.Stat(filepath.Join(logdir, f))
+		c.Assert(err, jc.Satisfies, os.IsNotExist)
+	}
+
+	b, err := ioutil.ReadFile(existing)
+	c.Assert(err, gc.IsNil)
+	// convert to string because we'll get a better failure message
+	c.Assert(string(b), gc.Not(gc.Equals), string(existing))
+
+}
+
+type fakeContext struct {
+	upgrades.Context
+	cfg fakeConfig
+}
+
+func (f fakeContext) AgentConfig() agent.ConfigSetter {
+	return f.cfg
+}
+
+type fakeConfig struct {
+	agent.ConfigSetter
+	logdir  string
+	datadir string
+}
+
+func (f fakeConfig) LogDir() string {
+	return f.logdir
+}
+
+func (f fakeConfig) DataDir() string {
+	return f.datadir
+}

upgrades/upgrade_test.go

@@ -671,7 +671,7 @@ func (s *upgradeSuite) TestStateUpgradeOperationsVersions(c *gc.C) {
 
 func (s *upgradeSuite) TestUpgradeOperationsVersions(c *gc.C) {
 	versions := extractUpgradeVersions(c, (*upgrades.UpgradeOperations)())
-	c.Assert(versions, gc.DeepEquals, []string{"1.18.0", "1.22.0", "1.23.0"})
+	c.Assert(versions, gc.DeepEquals, []string{"1.18.0", "1.22.0", "1.23.0", "1.24.0"})
 }
 
 func extractUpgradeVersions(c *gc.C, ops []upgrades.Operation) []string {