Update for code review (refs #2292).

* Use ioutil.TempFile to write new blocks, renaming to the correct
  name upon success.

* PutBlock should overwrite corrupt blocks, not report a collision.

* Get free disk space from syscall.Statfs, not `df`.

diff --git a/services/keep/keep.go b/services/keep/keep.go
index 5083e690a14d041f146fae8f5704b8aaa072fefd..7cca5344d8a2abc074cfb0f0f4dbd66d4fd2f0af 100644 (file)
--- a/services/keep/keep.go
+++ b/services/keep/keep.go
@@ -7,12 +7,13 @@ import (
        "errors"
        "fmt"
        "github.com/gorilla/mux"
+       "io/ioutil"
        "log"
        "net/http"
        "os"
-       "os/exec"
        "strconv"
        "strings"
+       "syscall"
        "time"
 )
 
@@ -228,18 +229,16 @@ func PutBlock(block []byte, hash string) error {
        }
 
        // If we already have a block on disk under this identifier, return
-       // success (but check for MD5 collisions, which may signify on-disk corruption).
+       // success (but check for MD5 collisions).
+       // The only errors that GetBlock can return are ErrCorrupt and ErrNotFound.
+       // In either case, we want to write our new (good) block to disk, so there is
+       // nothing special to do if err != nil.
        if oldblock, err := GetBlock(hash); err == nil {
                if bytes.Compare(block, oldblock) == 0 {
                        return nil
                } else {
                        return &KeepError{ErrCollision, errors.New("Collision")}
                }
-       } else {
-               ke := err.(*KeepError)
-               if ke.HTTPCode == ErrCorrupt {
-                       return &KeepError{ErrCollision, errors.New("Collision")}
-               }
        }
 
        // Store the block on the first available Keep volume.
@@ -256,21 +255,28 @@ func PutBlock(block []byte, hash string) error {
                        continue
                }
 
-               blockFilename := fmt.Sprintf("%s/%s", blockDir, hash)
-
-               f, err := os.OpenFile(blockFilename, os.O_CREATE|os.O_WRONLY, 0644)
-               if err != nil {
-                       log.Printf("%s: creating %s: %s\n", vol, blockFilename, err)
+               tmpfile, tmperr := ioutil.TempFile(blockDir, "tmp"+hash)
+               if tmperr != nil {
+                       log.Printf("ioutil.TempFile(%s, tmp%s): %s", blockDir, hash, tmperr)
                        continue
                }
+               blockFilename := fmt.Sprintf("%s/%s", blockDir, hash)
 
-               if _, err := f.Write(block); err == nil {
-                       f.Close()
-                       return nil
-               } else {
+               if _, err := tmpfile.Write(block); err != nil {
                        log.Printf("%s: writing to %s: %s\n", vol, blockFilename, err)
                        continue
                }
+               if err := tmpfile.Close(); err != nil {
+                       log.Printf("closing %s: %s\n", tmpfile.Name(), err)
+                       os.Remove(tmpfile.Name())
+                       continue
+               }
+               if err := os.Rename(tmpfile.Name(), blockFilename); err != nil {
+                       log.Printf("rename %s %s: %s\n", tmpfile.Name(), blockFilename, err)
+                       os.Remove(tmpfile.Name())
+                       continue
+               }
+               return nil
        }
 
        if allFull {
@@ -314,32 +320,14 @@ func IsFull(volume string) (isFull bool) {
 //     Returns the amount of available disk space on VOLUME,
 //     as a number of 1k blocks.
 //
-func FreeDiskSpace(volume string) (free int, err error) {
-       // Run df to find out how much disk space is left.
-       cmd := exec.Command("df", "--block-size=1k", volume)
-       stdout, perr := cmd.StdoutPipe()
-       if perr != nil {
-               return 0, perr
-       }
-       scanner := bufio.NewScanner(stdout)
-       if perr := cmd.Start(); err != nil {
-               return 0, perr
-       }
-
-       scanner.Scan() // skip header line of df output
-       scanner.Scan()
-
-       f := strings.Fields(scanner.Text())
-       if avail, err := strconv.Atoi(f[3]); err == nil {
-               free = avail
-       } else {
-               err = errors.New("bad df format: " + scanner.Text())
-       }
-
-       // Flush the df output and shut it down cleanly.
-       for scanner.Scan() {
+func FreeDiskSpace(volume string) (free uint64, err error) {
+       var fs syscall.Statfs_t
+       err = syscall.Statfs(volume, &fs)
+       if err == nil {
+               // Statfs output is not guaranteed to measure free
+               // space in terms of 1K blocks.
+               free = fs.Bavail * uint64(fs.Bsize) / 1024
        }
-       cmd.Wait()
 
        return
 }

diff --git a/services/keep/keep_test.go b/services/keep/keep_test.go
index cb5e89cfb919d3bfdfe5f72d15f4717bcce7dfb0..3f09e90da2a1a50f7e3c5089195ce1569cfca3d0 100644 (file)
--- a/services/keep/keep_test.go
+++ b/services/keep/keep_test.go
@@ -1,6 +1,7 @@
 package main
 
 import (
+       "bytes"
        "fmt"
        "io/ioutil"
        "os"
@@ -12,6 +13,17 @@ var TEST_BLOCK = []byte("The quick brown fox jumps over the lazy dog.")
 var TEST_HASH = "e4d909c290d0fb1ca068ffaddf22cbd0"
 var BAD_BLOCK = []byte("The magic words are squeamish ossifrage.")
 
+// TODO(twp): Tests still to be written
+//
+//   * PutBlockCollision
+//       - test that PutBlock(BLOCK, HASH) reports a collision. HASH must
+//         be present in Keep and identify a block which sums to HASH but
+//         which does not match BLOCK. (Requires an interface to mock MD5.)
+//
+//   * PutBlockFull
+//       - test that PutBlock returns 503 Full if the filesystem is full.
+//         (must mock FreeDiskSpace or Statfs? use a tmpfs?)
+
 // ========================================
 // GetBlock tests.
 // ========================================
@@ -49,6 +61,11 @@ func TestGetBlockMissing(t *testing.T) {
        result, err := GetBlock(TEST_HASH)
        if err == nil {
                t.Errorf("GetBlock incorrectly returned success: ", result)
+       } else {
+               ke := err.(*KeepError)
+               if ke.HTTPCode != ErrNotFound {
+                       t.Errorf("GetBlock: %v", ke)
+               }
        }
 }
 
@@ -157,11 +174,11 @@ func TestPutBlockMD5Fail(t *testing.T) {
        }
 }
 
-// TestPutBlockCollision
-//     PutBlock must report a 400 Collision error when asked to store a block
-//     when a different block exists on disk under the same identifier.
+// TestPutBlockCorrupt
+//     PutBlock should overwrite corrupt blocks on disk when given
+//     a PUT request with a good block.
 //
-func TestPutBlockCollision(t *testing.T) {
+func TestPutBlockCorrupt(t *testing.T) {
        defer teardown()
 
        // Create two test Keep volumes.
@@ -169,20 +186,22 @@ func TestPutBlockCollision(t *testing.T) {
 
        // Store a corrupted block under TEST_HASH.
        store(t, KeepVolumes[0], TEST_HASH, BAD_BLOCK)
-
-       // Attempting to put TEST_BLOCK should produce a 400 Collision error.
-       if err := PutBlock(TEST_BLOCK, TEST_HASH); err == nil {
-               t.Error("Expected PutBlock error, but no error returned")
-       } else {
-               ke := err.(*KeepError)
-               if ke.HTTPCode != ErrCollision {
-                       t.Errorf("Expected 400 Collision error, got %v", ke)
-               }
+       if err := PutBlock(TEST_BLOCK, TEST_HASH); err != nil {
+               t.Errorf("PutBlock: %v", err)
        }
 
-       KeepVolumes = nil
+       // The block on disk should now match TEST_BLOCK.
+       if block, err := GetBlock(TEST_HASH); err != nil {
+               t.Errorf("GetBlock: %v", err)
+       } else if bytes.Compare(block, TEST_BLOCK) != 0 {
+               t.Errorf("GetBlock returned: '%s'", string(block))
+       }
 }
 
+// ========================================
+// FindKeepVolumes tests.
+// ========================================
+
 // TestFindKeepVolumes
 //     Confirms that FindKeepVolumes finds tmpfs volumes with "/keep"
 //     directories at the top level.
@@ -272,6 +291,7 @@ func teardown() {
        for _, vol := range KeepVolumes {
                os.RemoveAll(path.Dir(vol))
        }
+       KeepVolumes = nil
 }
 
 // store