Merge lp:~themue/juju-core/012-local-storage into lp:~juju/juju-core/trunk

[Frank Mueller]

local: add storage

Added environ.Storage implementation using the local
storage backend. The tests so far are a copy from
environs/jujutest to check if the behavior is adequate.

https://codereview.appspot.com/7346052/

[Dimiter Naydenov (dimitern)]

Overall LGTM, modulo some trivial suggestions.

https://codereview.appspot.com/7346052/diff/1/environs/local/storage.go
File environs/local/storage.go (right):

https://codereview.appspot.com/7346052/diff/1/environs/local/storage.go#newcode11
environs/local/storage.go:11:
d

https://codereview.appspot.com/7346052/diff/1/environs/local/storage.go#newcode14
environs/local/storage.go:14:
var _ environ.Storage = (*storage)(nil) ?

https://codereview.appspot.com/7346052/diff/1/environs/local/storage.go#newcode30
environs/local/storage.go:30: // exist, it should return a
*NotFoundError.
s/*NotFoundError/NotFoundError - or even use roger's NotFoundf() ?

https://codereview.appspot.com/7346052/diff/1/environs/local/storage.go#newcode68
environs/local/storage.go:68: if string(body) == "" {
len(body) == 0 ?

https://codereview.appspot.com/7346052/diff/1/environs/local/storage.go#newcode76
environs/local/storage.go:76: // URL returns a URL that can be used to
access the given storage file.
s/a URL/an URL

https://codereview.appspot.com/7346052/diff/1/environs/local/storage.go#newcode82
environs/local/storage.go:82: // The length must give the total length
of the file.
s/must give/must be set to/

https://codereview.appspot.com/7346052/diff/1/environs/local/storage.go#newcode98
environs/local/storage.go:98: return errors.New(resp.Status)
Include the StatusCode in the error text?

https://codereview.appspot.com/7346052/diff/1/environs/local/storage.go#newcode120
environs/local/storage.go:120: return errors.New(resp.Status)
as above

https://codereview.appspot.com/7346052/diff/1/environs/local/storage_test.go
File environs/local/storage_test.go (right):

https://codereview.appspot.com/7346052/diff/1/environs/local/storage_test.go#newcode70
environs/local/storage_test.go:70: c.Assert(err, IsNil)
if it's check*, use c.Check, rather than c.Assert - like you're doing
above. Otherwise, call it assert*

https://codereview.appspot.com/7346052/diff/1/environs/local/storage_test.go#newcode83
environs/local/storage_test.go:83: for a := attempt.Start(); a.Next(); {
If you're not retrying (attempt strategy is zeroed out), why do you do
that here?

https://codereview.appspot.com/7346052/diff/1/environs/local/storage_test.go#newcode121
environs/local/storage_test.go:121: c.Check(data, DeepEquals, contents)
Mixing Assert and Check seems a bit random here, care to explain or
unify these?

https://codereview.appspot.com/7346052/

[Dimiter Naydenov (dimitern)]

LGTM

https://codereview.appspot.com/7346052/

[William Reade (fwereade)]

LGTM with the AttemptStrategy stuff dropped.

https://codereview.appspot.com/7346052/diff/12001/environs/local/storage_test.go
File environs/local/storage_test.go (right):

https://codereview.appspot.com/7346052/diff/12001/environs/local/storage_test.go#newcode18
environs/local/storage_test.go:18: var noRetry =
trivial.AttemptStrategy{}
There's no need for an AttemptStrategy here.

https://codereview.appspot.com/7346052/diff/12001/environs/local/storage_test.go#newcode22
environs/local/storage_test.go:22: // be removed again.
These tests should be written properly, and should not be earmarked for
deletion. Do we really write these sorts of tests against an *Environ*
in the other cases? If so, what have we been smoking?

https://codereview.appspot.com/7346052/diff/12001/environs/local/storage_test.go#newcode85
environs/local/storage_test.go:85: func checkFileDoesNotExist(c *C,
storage environs.StorageReader, name string, attempt
trivial.AttemptStrategy) {
AttemptStrategy not needed.

https://codereview.appspot.com/7346052/diff/12001/environs/local/storage_test.go#newcode99
environs/local/storage_test.go:99: func checkFileHasContents(c *C,
storage environs.StorageReader, name string, contents []byte, attempt
trivial.AttemptStrategy) {
AttemptStrategy not needed.

https://codereview.appspot.com/7346052/

=== modified file 'environs/local/export_test.go'
--- environs/local/export_test.go	2013-02-20 15:00:28 +0000
+++ environs/local/export_test.go	2013-02-20 15:00:28 +0000
@@ -2,8 +2,14 @@
 
 import (
 	"net"
+
+	"launchpad.net/juju-core/environs"
 )
 
 func Listen(basepath, environName, ip string, port int) (net.Listener, error) {
 	return listen(basepath, environName, ip, port)
 }
+
+func NewStorage(address string, port int, environName string) environs.Storage {
+	return newStorage(address, port, environName)
+}

=== added file 'environs/local/storage.go'
--- environs/local/storage.go	1970-01-01 00:00:00 +0000
+++ environs/local/storage.go	2013-02-20 15:00:28 +0000
@@ -0,0 +1,123 @@
+package local
+
+import (
+	"fmt"
+	"io"
+	"io/ioutil"
+	"launchpad.net/juju-core/environs"
+	"net/http"
+	"sort"
+	"strings"
+)
+
+// storage implements the environs.Storage interface.
+type storage struct {
+	baseURL string
+}
+
+var _ environs.Storage = (*storage)(nil)
+
+// newStorage returns a new local storage.
+func newStorage(address string, port int, environName string) *storage {
+	return &storage{
+		baseURL: fmt.Sprintf("http://%s:%d", address, port),
+	}
+}
+
+// Get opens the given storage file and returns a ReadCloser
+// that can be used to read its contents. It is the caller's
+// responsibility to close it after use. If the name does not
+// exist, it should return a *NotFoundError.
+func (s *storage) Get(name string) (io.ReadCloser, error) {
+	url, err := s.URL(name)
+	if err != nil {
+		return nil, err
+	}
+	resp, err := http.Get(url)
+	if err != nil {
+		return nil, err
+	}
+	if resp.StatusCode != 200 {
+		return nil, &environs.NotFoundError{fmt.Errorf("file %q not found", name)}
+	}
+	return resp.Body, nil
+}
+
+// List lists all names in the storage with the given prefix, in
+// alphabetical order. The names in the storage are considered
+// to be in a flat namespace, so the prefix may include slashes
+// and the names returned are the full names for the matching
+// entries.
+func (s *storage) List(prefix string) ([]string, error) {
+	url, err := s.URL(prefix)
+	if err != nil {
+		return nil, err
+	}
+	resp, err := http.Get(url + "*")
+	if err != nil {
+		return nil, err
+	}
+	if resp.StatusCode != 200 {
+		return nil, fmt.Errorf("%d %s", resp.StatusCode, resp.Status)
+	}
+	defer resp.Body.Close()
+	body, err := ioutil.ReadAll(resp.Body)
+	if err != nil {
+		return nil, err
+	}
+	if len(body) == 0 {
+		return nil, nil
+	}
+	names := strings.Split(string(body), "\n")
+	sort.Strings(names)
+	return names, nil
+}
+
+// URL returns an URL that can be used to access the given storage file.
+func (s *storage) URL(name string) (string, error) {
+	return fmt.Sprintf("%s/%s", s.baseURL, name), nil
+}
+
+// Put reads from r and writes to the given storage file.
+// The length must be set to the total length of the file.
+func (s *storage) Put(name string, r io.Reader, length int64) error {
+	url, err := s.URL(name)
+	if err != nil {
+		return err
+	}
+	req, err := http.NewRequest("PUT", url, r)
+	if err != nil {
+		return err
+	}
+	req.Header.Set("Content-Type", "application/octet-stream")
+	resp, err := http.DefaultClient.Do(req)
+	if err != nil {
+		return err
+	}
+	if resp.StatusCode != 201 {
+		return fmt.Errorf("%d %s", resp.StatusCode, resp.Status)
+	}
+	return nil
+}
+
+// Remove removes the given file from the environment's
+// storage. It should not return an error if the file does
+// not exist.
+func (s *storage) Remove(name string) error {
+	url, err := s.URL(name)
+	if err != nil {
+		return err
+	}
+	req, err := http.NewRequest("DELETE", url, nil)
+	if err != nil {
+		return err
+	}
+	resp, err := http.DefaultClient.Do(req)
+	if err != nil {
+		return err
+	}
+	if resp.StatusCode != 200 {
+		return fmt.Errorf("%d %s", resp.StatusCode, resp.Status)
+	}
+	return nil
+}

=== added file 'environs/local/storage_test.go'
--- environs/local/storage_test.go	1970-01-01 00:00:00 +0000
+++ environs/local/storage_test.go	2013-02-20 15:00:28 +0000
@@ -0,0 +1,127 @@
+package local_test
+
+import (
+	"bytes"
+	"io"
+	"io/ioutil"
+	. "launchpad.net/gocheck"
+	"launchpad.net/juju-core/environs"
+	"launchpad.net/juju-core/environs/local"
+	"launchpad.net/juju-core/trivial"
+	"net/http"
+)
+
+type storageSuite struct{}
+
+var _ = Suite(&storageSuite{})
+
+var noRetry = trivial.AttemptStrategy{}
+
+// TestPersistence and the helpers are borrowed from environs/jujutest
+// te check the storage behavior before integration. It will later
+// be removed again.
+func (s *storageSuite) TestPersistence(c *C) {
+	// Non-standard port to avoid conflict with not-yet full 
+	// closed listener in backend test.
+	portNo := 60007
+	listener, err := local.Listen(c.MkDir(), environName, "127.0.0.1", portNo)
+	c.Assert(err, IsNil)
+	defer listener.Close()
+	storage := local.NewStorage("127.0.0.1", portNo, environName)
+
+	names := []string{
+		"aa",
+		"zzz/aa",
+		"zzz/bb",
+	}
+	for _, name := range names {
+		checkFileDoesNotExist(c, storage, name, noRetry)
+		checkPutFile(c, storage, name, []byte(name))
+	}
+	checkList(c, storage, "", names)
+	checkList(c, storage, "a", []string{"aa"})
+	checkList(c, storage, "zzz/", []string{"zzz/aa", "zzz/bb"})
+
+	storage2 := local.NewStorage("127.0.0.1", portNo, environName)
+	for _, name := range names {
+		checkFileHasContents(c, storage2, name, []byte(name), noRetry)
+	}
+
+	// remove the first file and check that the others remain.
+	err = storage2.Remove(names[0])
+	c.Check(err, IsNil)
+
+	// check that it's ok to remove a file twice.
+	err = storage2.Remove(names[0])
+	c.Check(err, IsNil)
+
+	// ... and check it's been removed in the other environment
+	checkFileDoesNotExist(c, storage, names[0], noRetry)
+
+	// ... and that the rest of the files are still around
+	checkList(c, storage2, "", names[1:])
+
+	for _, name := range names[1:] {
+		err := storage2.Remove(name)
+		c.Assert(err, IsNil)
+	}
+
+	// check they've all gone
+	checkList(c, storage2, "", nil)
+}
+
+func checkList(c *C, storage environs.StorageReader, prefix string, names []string) {
+	lnames, err := storage.List(prefix)
+	c.Assert(err, IsNil)
+	c.Assert(lnames, DeepEquals, names)
+}
+
+func checkPutFile(c *C, storage environs.StorageWriter, name string, contents []byte) {
+	c.Logf("check putting file %s ...", name)
+	err := storage.Put(name, bytes.NewBuffer(contents), int64(len(contents)))
+	c.Assert(err, IsNil)
+}
+
+func checkFileDoesNotExist(c *C, storage environs.StorageReader, name string, attempt trivial.AttemptStrategy) {
+	var r io.ReadCloser
+	var err error
+	for a := attempt.Start(); a.Next(); {
+		r, err = storage.Get(name)
+		if err != nil {
+			break
+		}
+	}
+	c.Assert(r, IsNil)
+	var notFoundError *environs.NotFoundError
+	c.Assert(err, FitsTypeOf, notFoundError)
+}
+
+func checkFileHasContents(c *C, storage environs.StorageReader, name string, contents []byte, attempt trivial.AttemptStrategy) {
+	r, err := storage.Get(name)
+	c.Assert(err, IsNil)
+	c.Check(r, NotNil)
+	defer r.Close()
+
+	data, err := ioutil.ReadAll(r)
+	c.Check(err, IsNil)
+	c.Check(data, DeepEquals, contents)
+
+	url, err := storage.URL(name)
+	c.Assert(err, IsNil)
+
+	var resp *http.Response
+	for a := attempt.Start(); a.Next(); {
+		resp, err = http.Get(url)
+		c.Assert(err, IsNil)
+		if resp.StatusCode != 404 {
+			break
+		}
+		c.Logf("get retrying after earlier get succeeded. *sigh*.")
+	}
+	c.Assert(err, IsNil)
+	data, err = ioutil.ReadAll(resp.Body)
+	c.Assert(err, IsNil)
+	defer resp.Body.Close()
+	c.Assert(resp.StatusCode, Equals, 200, Commentf("error response: %s", data))
+	c.Check(data, DeepEquals, contents)
+}