Merge lp:~axwalk/juju-core/filestorage-write-tmpdir into lp:~go-bot/juju-core/trunk

[Andrew Wilkins]

environs/filestorage: optional atomic Put

As requested in review of manual-bootstrap:
https://code.launchpad.net/~axwalk/juju-core/manual-bootstrap/+merge/184714

A new parameter is added to NewFileStorageWriter
to specify a temporary directory. Put will now
write to a temporary file in this location, and
then move it to the final destination; as long
as the temporary directory exists on the same
filesystem as the storage directory, this should
be atomic.

If the temporary directory is unspecified/blank,
then storagedir+".tmp" will be used to ensure
a "default" of colocation on the storage directory's
filesystem.

While it is not strictly necessary, the temporary
directory location will match that of sshstorage
(merge proposal pending).

https://codereview.appspot.com/13727043/

environs/filestorage: optional atomic Put

As requested in review of manual-bootstrap:
https://code.launchpad.net/~axwalk/juju-core/manual-bootstrap/+merge/184714

A new parameter is added to NewFileStorageWriter
to specify a temporary directory. Put will now
write to a temporary file in this location, and
then move it to the final destination; as long
as the temporary directory exists on the same
filesystem as the storage directory, this should
be atomic.

If the temporary directory is unspecified/blank,
then storagedir+".tmp" will be used to ensure
a "default" of colocation on the storage directory's
filesystem.

While it is not strictly necessary, the temporary
directory location will match that of sshstorage
(merge proposal pending).

https://codereview.appspot.com/13727043/

[Andrew Wilkins (axwalk)]

Reviewers: mp+185715_code.launchpad.net,

Message:
Please take a look.

Description:
environs/filestorage: optional atomic Put

As requested in review of manual-bootstrap:
https://code.launchpad.net/~axwalk/juju-core/manual-bootstrap/+merge/184714

A new parameter is added to NewFileStorageWriter
to specify a temporary directory. Put will now
write to a temporary file in this location, and
then move it to the final destination; as long
as the temporary directory exists on the same
filesystem as the storage directory, this should
be atomic.

As with io/ioutil TemporaryFile, if the temporary
directory is blank, os.TempDir will be used to
get the default temporary directory.

https://code.launchpad.net/~axwalk/juju-core/filestorage-write-tmpdir/+merge/185715

(do not edit description out of merge proposal)

Please review this at https://codereview.appspot.com/13727043/
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-20130916061421-7r11uoyztquho14o
+New revision: <email address hidden>

Index: cmd/juju/synctools.go
=== modified file 'cmd/juju/synctools.go'
--- cmd/juju/synctools.go 2013-09-13 04:16:42 +0000
+++ cmd/juju/synctools.go 2013-09-16 06:41:18 +0000
@@ -83,7 +83,7 @@

   target := environ.Storage()
   if c.destination != "" {
- target, err = filestorage.NewFileStorageWriter(c.destination)
+ target, err = filestorage.NewFileStorageWriter(c.destination, "")
    if err != nil {
     return err
    }

Index: environs/filestorage/filestorage.go
=== modified file 'environs/filestorage/filestorage.go'
--- environs/filestorage/filestorage.go 2013-09-12 05:04:40 +0000
+++ environs/filestorage/filestorage.go 2013-09-16 06:41:18 +0000
@@ -21,7 +21,7 @@
   path string
  }

-// newFileStorageReader returns a new storage reader for
+// NewFileStorageReader returns a new storage reader for
  // a directory inside the local file system.
  func NewFileStorageReader(path string) (environs.StorageReader, error) {
   p := filepath.Clean(path)
@@ -85,14 +85,22 @@

  type fileStorageWriter struct {
   fileStorageReader
+ tmpdir string
  }

-func NewFileStorageWriter(path string) (environs.Storage, error) {
+// NewFileStorageWriter returns a new read/write storag for
+// a directory inside the local file system.
+//
+// A temporary directory may be specified, in which files will be written
+// to before moving to the final destination. If specified, the temporary
+// directory should be on the same filesystem as the storage directory
+// to ensure atomicity. If left unspecified (blank), $TMPDIR will be used.
+func NewFileStorageWriter(path, tmpdir string) (environs.Storage, error) {
   reader, err := NewFileStorageReader(path)
   if err != nil {
    return nil, err
   }
- return &fileStorageWriter{*reader.(*fileStorageReader)}, nil
+ return &fileStorageWriter{*reader.(*fileStorageReader), tmpdir}, nil
  }

  func (f *fileStorageWriter) Put(name string, r io.Reader, length int64)
error {
@@ -101,11 +109,18 @@
   if err := os.MkdirAll(dir, 0755); err != nil && !os.IsExist...

[Andrew Wilkins (axwalk)]

Please take a look.

https://codereview.appspot.com/13727043/

[Andrew Wilkins (axwalk)]

Please take a look.

https://codereview.appspot.com/13727043/

[Tim Penhey (thumper)]

https://codereview.appspot.com/13727043/diff/7001/environs/filestorage/filestorage.go
File environs/filestorage/filestorage.go (right):

https://codereview.appspot.com/13727043/diff/7001/environs/filestorage/filestorage.go#newcode107
environs/filestorage/filestorage.go:107: if err := os.MkdirAll(tmpdir,
0755); err != nil && !os.IsExist(err) {
Do you ever remove the tmpdir?

https://codereview.appspot.com/13727043/diff/7001/environs/filestorage/filestorage_test.go
File environs/filestorage/filestorage_test.go (right):

https://codereview.appspot.com/13727043/diff/7001/environs/filestorage/filestorage_test.go#newcode39
environs/filestorage/filestorage_test.go:39: s.writer, err =
filestorage.NewFileStorageWriter(s.dir, "")
how about a constant in filestorage
   const UseDefaultTmpDir = ""

then

s.writer, err = filestorage.NewFileStorageWriter(s.dir,
filestorage.UseDefaultTmpDir)

It means I don't have to go and read NewFileStorageWriter to work out
what "" means.

https://codereview.appspot.com/13727043/

[Andrew Wilkins (axwalk)]

https://codereview.appspot.com/13727043/diff/7001/environs/filestorage/filestorage.go
File environs/filestorage/filestorage.go (right):

https://codereview.appspot.com/13727043/diff/7001/environs/filestorage/filestorage.go#newcode107
environs/filestorage/filestorage.go:107: if err := os.MkdirAll(tmpdir,
0755); err != nil && !os.IsExist(err) {
On 2013/09/18 04:09:23, thumper wrote:
> Do you ever remove the tmpdir?

No, never. I wasn't sure about this when I implemented it, but thinking
about it some more I think it should create/remove it iff
tmpdir==UseDefaultTmpDir. If a directory is explicitly specified, it
must exist and won't be removed. Then juju sync-tools won't leave tmpdir
turds.

I've updated the implementation, docstring and tests to match this.

https://codereview.appspot.com/13727043/diff/7001/environs/filestorage/filestorage_test.go
File environs/filestorage/filestorage_test.go (right):

https://codereview.appspot.com/13727043/diff/7001/environs/filestorage/filestorage_test.go#newcode39
environs/filestorage/filestorage_test.go:39: s.writer, err =
filestorage.NewFileStorageWriter(s.dir, "")
On 2013/09/18 04:09:23, thumper wrote:
> how about a constant in filestorage
> const UseDefaultTmpDir = ""

> then

> s.writer, err = filestorage.NewFileStorageWriter(s.dir,
> filestorage.UseDefaultTmpDir)

> It means I don't have to go and read NewFileStorageWriter to work out
what ""
> means.

Good idea. Done.

https://codereview.appspot.com/13727043/

[Andrew Wilkins (axwalk)]

Please take a look.

https://codereview.appspot.com/13727043/

[Tim Penhey (thumper)]

On 2013/09/18 07:12:02, axw wrote:
> Please take a look.

LGTM

https://codereview.appspot.com/13727043/

[Tim Penhey (thumper)]

review: Approve

[Go Bot (go-bot)]

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

text conflict in environs/filestorage/filestorage.go
text conflict in environs/filestorage/filestorage_test.go

[Go Bot (go-bot)]

The attempt to merge lp:~axwalk/juju-core/filestorage-write-tmpdir into lp:juju-core failed. Below is the output from the failed tests.

# launchpad.net/juju-core/provider/local
provider/local/environ.go:169: not enough arguments in call to filestorage.NewFileStorageWriter
# launchpad.net/juju-core/provider/local/storage
provider/local/storage/worker.go:44: not enough arguments in call to filestorage.NewFileStorageWriter
provider/local/storage/worker.go:60: not enough arguments in call to filestorage.NewFileStorageWriter
# launchpad.net/juju-core/environs/testing
environs/testing/storage.go:31: not enough arguments in call to filestorage.NewFileStorageWriter

=== modified file 'cmd/juju/synctools.go'
--- cmd/juju/synctools.go	2013-09-13 04:16:42 +0000
+++ cmd/juju/synctools.go	2013-09-19 03:28:43 +0000
@@ -83,7 +83,7 @@
 
 	target := environ.Storage()
 	if c.destination != "" {
-		target, err = filestorage.NewFileStorageWriter(c.destination)
+		target, err = filestorage.NewFileStorageWriter(c.destination, filestorage.UseDefaultTmpDir)
 		if err != nil {
 			return err
 		}
 
=== modified file 'cmd/plugins/juju-metadata/toolsmetadata.go'
--- cmd/plugins/juju-metadata/toolsmetadata.go	2013-09-16 10:47:28 +0000
+++ cmd/plugins/juju-metadata/toolsmetadata.go	2013-09-19 03:28:43 +0000
@@ -69,7 +69,7 @@
 		return err
 	}
 
-	targetStorage, err := filestorage.NewFileStorageWriter(c.metadataDir)
+	targetStorage, err := filestorage.NewFileStorageWriter(c.metadataDir, filestorage.UseDefaultTmpDir)
 	if err != nil {
 		return err
 	}
 
=== modified file 'environs/filestorage/filestorage.go'
--- environs/filestorage/filestorage.go	2013-09-19 00:22:15 +0000
+++ environs/filestorage/filestorage.go	2013-09-19 03:28:43 +0000
@@ -23,7 +23,7 @@
 	path string
 }
 
-// newFileStorageReader returns a new storage reader for
+// NewFileStorageReader returns a new storage reader for
 // a directory inside the local file system.
 func NewFileStorageReader(path string) (storage.StorageReader, error) {
 	p := filepath.Clean(path)
@@ -98,27 +98,60 @@
 
 type fileStorageWriter struct {
 	fileStorageReader
+	tmpdir string
 }
 
-func NewFileStorageWriter(path string) (storage.Storage, error) {
+// UseDefaultTmpDir may be passed into NewFileStorageWriter
+// for the tmpdir argument, to signify that the default
+// value should be used. See NewFileStorageWriter for more.
+const UseDefaultTmpDir = ""
+
+// NewFileStorageWriter returns a new read/write storag for
+// a directory inside the local file system.
+//
+// A temporary directory may be specified, in which files will be written
+// to before moving to the final destination. If specified, the temporary
+// directory should be on the same filesystem as the storage directory
+// to ensure atomicity. If tmpdir == UseDefaultTmpDir (""), then path+".tmp"
+// will be used.
+//
+// If tmpdir == UseDefaultTmpDir, it will be created when Put is invoked,
+// and will be removed afterwards. If tmpdir != UseDefaultTmpDir, it must
+// already exist, and will never be removed.
+func NewFileStorageWriter(path, tmpdir string) (storage.Storage, error) {
 	reader, err := NewFileStorageReader(path)
 	if err != nil {
 		return nil, err
 	}
-	return &fileStorageWriter{*reader.(*fileStorageReader)}, nil
+	return &fileStorageWriter{*reader.(*fileStorageReader), tmpdir}, nil
 }
 
 func (f *fileStorageWriter) Put(name string, r io.Reader, length int64) error {
 	fullpath := f.fullPath(name)
 	dir := filepath.Dir(fullpath)
+	tmpdir := f.tmpdir
+	if tmpdir == UseDefaultTmpDir {
+		tmpdir = f.path + ".tmp"
+		if err := os.MkdirAll(tmpdir, 0755); err != nil && !os.IsExist(err) {
+			return err
+		}
+		defer os.Remove(tmpdir)
+	}
 	if err := os.MkdirAll(dir, 0755); err != nil && !os.IsExist(err) {
 		return err
 	}
-	data, err := ioutil.ReadAll(r)
-	if err != nil {
-		return err
-	}
-	return ioutil.WriteFile(fullpath, data, 0644)
+	// Write to a temporary file first, and then move (atomically).
+	file, err := ioutil.TempFile(tmpdir, "juju-filestorage-")
+	if err != nil {
+		return err
+	}
+	_, err = io.CopyN(file, r, length)
+	file.Close()
+	if err != nil {
+		os.Remove(file.Name())
+		return err
+	}
+	return os.Rename(file.Name(), fullpath)
 }
 
 func (f *fileStorageWriter) Remove(name string) error {
 
=== modified file 'environs/filestorage/filestorage_test.go'
--- environs/filestorage/filestorage_test.go	2013-09-19 00:22:15 +0000
+++ environs/filestorage/filestorage_test.go	2013-09-19 03:28:43 +0000
@@ -39,7 +39,7 @@
 	var err error
 	s.reader, err = filestorage.NewFileStorageReader(s.dir)
 	c.Assert(err, gc.IsNil)
-	s.writer, err = filestorage.NewFileStorageWriter(s.dir)
+	s.writer, err = filestorage.NewFileStorageWriter(s.dir, filestorage.UseDefaultTmpDir)
 	c.Assert(err, gc.IsNil)
 }
 
@@ -135,3 +135,35 @@
 	_, err = ioutil.ReadFile(expectedpath)
 	c.Assert(err, gc.Not(gc.IsNil))
 }
+
+func (s *filestorageSuite) TestPutTmpDir(c *gc.C) {
+	// Put should create and clean up the temporary directory if
+	// tmpdir==UseDefaultTmpDir.
+	err := s.writer.Put("test-write", bytes.NewReader(nil), 0)
+	c.Assert(err, gc.IsNil)
+	_, err = os.Stat(s.dir + ".tmp")
+	c.Assert(err, jc.Satisfies, os.IsNotExist)
+
+	// To deal with recovering from hard failure, UseDefaultTmpDir
+	// doesn't care if the temporary directory already exists. It
+	// still removes it, though.
+	err = os.Mkdir(s.dir+".tmp", 0755)
+	c.Assert(err, gc.IsNil)
+	err = s.writer.Put("test-write", bytes.NewReader(nil), 0)
+	c.Assert(err, gc.IsNil)
+	_, err = os.Stat(s.dir + ".tmp")
+	c.Assert(err, jc.Satisfies, os.IsNotExist)
+
+	// If we explicitly set the temporary directory, it must already exist.
+	s.writer, err = filestorage.NewFileStorageWriter(s.dir, s.dir+".tmp")
+	c.Assert(err, gc.IsNil)
+	err = s.writer.Put("test-write", bytes.NewReader(nil), 0)
+	c.Assert(err, jc.Satisfies, os.IsNotExist)
+	err = os.Mkdir(s.dir+".tmp", 0755)
+	c.Assert(err, gc.IsNil)
+	err = s.writer.Put("test-write", bytes.NewReader(nil), 0)
+	c.Assert(err, gc.IsNil)
+	// Temporary directory should not have been moved.
+	_, err = os.Stat(s.dir + ".tmp")
+	c.Assert(err, gc.IsNil)
+}
 
=== modified file 'environs/httpstorage/backend_test.go'
--- environs/httpstorage/backend_test.go	2013-09-18 05:32:18 +0000
+++ environs/httpstorage/backend_test.go	2013-09-19 03:28:43 +0000
@@ -36,7 +36,7 @@
 // a base URL for the server and the directory path.
 func startServer(c *gc.C) (listener net.Listener, url, dataDir string) {
 	dataDir = c.MkDir()
-	embedded, err := filestorage.NewFileStorageWriter(dataDir)
+	embedded, err := filestorage.NewFileStorageWriter(dataDir, filestorage.UseDefaultTmpDir)
 	c.Assert(err, gc.IsNil)
 	listener, err = httpstorage.Serve("localhost:0", embedded)
 	c.Assert(err, gc.IsNil)
 
=== modified file 'environs/testing/storage.go'
--- environs/testing/storage.go	2013-09-19 00:22:15 +0000
+++ environs/testing/storage.go	2013-09-19 03:28:43 +0000
@@ -28,7 +28,7 @@
 // directory.
 func CreateLocalTestStorage(c *gc.C) (closer io.Closer, stor storage.Storage, dataDir string) {
 	dataDir = c.MkDir()
-	underlying, err := filestorage.NewFileStorageWriter(dataDir)
+	underlying, err := filestorage.NewFileStorageWriter(dataDir, filestorage.UseDefaultTmpDir)
 	c.Assert(err, gc.IsNil)
 	listener, err := httpstorage.Serve("localhost:0", underlying)
 	c.Assert(err, gc.IsNil)
 
=== modified file 'environs/tools/simplestreams_test.go'
--- environs/tools/simplestreams_test.go	2013-09-13 02:02:59 +0000
+++ environs/tools/simplestreams_test.go	2013-09-19 03:28:43 +0000
@@ -284,7 +284,7 @@
 		},
 	}
 	dir := c.MkDir()
-	writer, err := filestorage.NewFileStorageWriter(dir)
+	writer, err := filestorage.NewFileStorageWriter(dir, filestorage.UseDefaultTmpDir)
 	c.Assert(err, gc.IsNil)
 	err = tools.WriteMetadata(toolsList, false, writer)
 	c.Assert(err, gc.IsNil)
@@ -311,7 +311,7 @@
 			URL:     "file://" + filepath.Join(dir, "tools/releases/juju-2.0.1-raring-amd64.tgz"),
 		},
 	}
-	writer, err := filestorage.NewFileStorageWriter(dir)
+	writer, err := filestorage.NewFileStorageWriter(dir, filestorage.UseDefaultTmpDir)
 	c.Assert(err, gc.IsNil)
 	err = tools.WriteMetadata(toolsList, true, writer)
 	c.Assert(err, gc.IsNil)
@@ -332,7 +332,7 @@
 			SHA256:  "xyz",
 		},
 	}
-	writer, err := filestorage.NewFileStorageWriter(dir)
+	writer, err := filestorage.NewFileStorageWriter(dir, filestorage.UseDefaultTmpDir)
 	c.Assert(err, gc.IsNil)
 	err = tools.WriteMetadata(existingToolsList, true, writer)
 	c.Assert(err, gc.IsNil)
 
=== modified file 'provider/local/environ.go'
--- provider/local/environ.go	2013-09-19 00:22:15 +0000
+++ provider/local/environ.go	2013-09-19 03:28:43 +0000
@@ -166,7 +166,7 @@
 	} else if !info.Mode().IsDir() {
 		return nil, fmt.Errorf("%q exists but is not a directory (and it needs to be)", dir)
 	}
-	storage, err := filestorage.NewFileStorageWriter(dir)
+	storage, err := filestorage.NewFileStorageWriter(dir, filestorage.UseDefaultTmpDir)
 	if err != nil {
 		return nil, err
 	}
 
=== modified file 'provider/local/storage/worker.go'
--- provider/local/storage/worker.go	2013-09-17 01:33:41 +0000
+++ provider/local/storage/worker.go	2013-09-19 03:28:43 +0000
@@ -41,7 +41,7 @@
 	storageAddr := s.config.Value(agent.StorageAddr)
 	logger.Infof("serving %s on %s", storageDir, storageAddr)
 
-	storage, err := filestorage.NewFileStorageWriter(storageDir)
+	storage, err := filestorage.NewFileStorageWriter(storageDir, filestorage.UseDefaultTmpDir)
 	if err != nil {
 		logger.Errorf("error with local storage: %v", err)
 		return err
@@ -57,7 +57,7 @@
 	sharedStorageAddr := s.config.Value(agent.SharedStorageAddr)
 	logger.Infof("serving %s on %s", sharedStorageDir, sharedStorageAddr)
 
-	sharedStorage, err := filestorage.NewFileStorageWriter(sharedStorageDir)
+	sharedStorage, err := filestorage.NewFileStorageWriter(sharedStorageDir, filestorage.UseDefaultTmpDir)
 	if err != nil {
 		logger.Errorf("error with local storage: %v", err)
 		return err