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(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(f.tmpdir, "juju-filestorage-"+name)
+	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 {


Index: environs/filestorage/filestorage_test.go
=== modified file 'environs/filestorage/filestorage_test.go'
--- environs/filestorage/filestorage_test.go	2013-09-12 05:04:40 +0000
+++ environs/filestorage/filestorage_test.go	2013-09-16 06:41:18 +0000
@@ -36,7 +36,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, "")
  	c.Assert(err, gc.IsNil)
  }



Index: environs/tools/simplestreams_test.go
=== 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-16 06:41:18 +0000
@@ -284,7 +284,7 @@
  		},
  	}
  	dir := c.MkDir()
-	writer, err := filestorage.NewFileStorageWriter(dir)
+	writer, err := filestorage.NewFileStorageWriter(dir, "")
  	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, "")
  	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, "")
  	c.Assert(err, gc.IsNil)
  	err = tools.WriteMetadata(existingToolsList, true, writer)
  	c.Assert(err, gc.IsNil)


Index: cmd/plugins/juju-metadata/toolsmetadata.go
=== modified file 'cmd/plugins/juju-metadata/toolsmetadata.go'
--- cmd/plugins/juju-metadata/toolsmetadata.go	2013-09-12 05:04:40 +0000
+++ cmd/plugins/juju-metadata/toolsmetadata.go	2013-09-16 06:41:18 +0000
@@ -69,7 +69,7 @@
  		return err
  	}

-	targetStorage, err := filestorage.NewFileStorageWriter(c.metadataDir)
+	targetStorage, err := filestorage.NewFileStorageWriter(c.metadataDir, "")
  	if err != nil {
  		return err
  	}