Code review comment for lp:~axwalk/juju-core/filestorage-write-tmpdir

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
   }