launchpad-reviewers team mailing list archive

["Jeroen T. Vermeulen" <jtv@xxxxxxxxxxxxx>]

Jeroen T. Vermeulen has proposed merging lp:~jtv/juju-core/mpv-storage-list into lp:~maas-maintainers/juju-core/maas-provider-skeleton.

Commit message:
Foundations of MAAS provider's Storage implementation, and tests/code for List().

Requested reviews:
  MAAS Maintainers (maas-maintainers)

For more details, see:
https://code.launchpad.net/~jtv/juju-core/mpv-storage-list/+merge/151449

This was sliced out of a larger ongoing branch which implements file upload & retrieval.  Hence any “clairvoyance” in terms of unneeded (for now!) complexity: getSnapshot() for instance becomes useful in the other branch.

Speaking of getSnapshot: you may be wondering why I did that.  It's a bit of pioneering in the face of uncertainty.  We haven't really got a handle yet on what needs locking in the provider classes.  Meanwhile, Go's “defer” makes it easy to hold locks for longer than intended — e.g. because you're making an http request in a function that no longer really needs a lock, but still has the lock's release pending until the function returns.  And finally, it's hard to predict at what level in the call tree you need to do your locking.  When you're already holding a lock on an object, sync.Mutex won't grant you a second simultaneous lock on that same object, because the lock is meant to synchronize not just threads but also goroutines which may or may not be executed by the same thread.

And so I sidestepped the question by creating a source of simple, good-enough-for-now consistent snapshots.  You hold your lock very very briefly, and then you go on with an object that at some point may be outdated, but will never be inconsistent.  If you really need an up-to-date version, you'll have to hold a lock while you're working, or if you're an optimist, verify that your snapshot is still current before committing to anything.  But hopefully that won't be needed for the general case.  If your URL for the MAAS API suddenly changed, for instance, failure and retry will probably be about as good as trying to use the latest URL at all times.

Unfortunately there seems to be no good way to know the current state of a Mutex (held or available), or make an attempt to grab it without potentially blocking forever.  So to test that the mutex is available, I just compare it to a freshly-constructed one and that seems to work.  I feel duly guilty about it, but console myself with the thought that if this trick ever becomes inappropriate, the test suite will report a false positive rather than a false negative.  In other words, the compiler or test suite will tell you something's broken.  It won't give you a false impression that all is well.


Jeroen
-- 
https://code.launchpad.net/~jtv/juju-core/mpv-storage-list/+merge/151449
Your team MAAS Maintainers is requested to review the proposed merge of lp:~jtv/juju-core/mpv-storage-list into lp:~maas-maintainers/juju-core/maas-provider-skeleton.

=== modified file 'environs/maas/storage.go'
--- environs/maas/storage.go	2013-01-29 07:55:41 +0000
+++ environs/maas/storage.go	2013-03-04 10:24:25 +0000
@@ -1,20 +1,121 @@
 package maas
 
 import (
+	"fmt"
 	"io"
+	"launchpad.net/gomaasapi"
 	"launchpad.net/juju-core/environs"
+	"net/url"
+	"sort"
+	"strings"
+	"sync"
 )
 
-type maasStorage struct{}
+type maasStorage struct {
+	// Mutex protects the "*Unlocked" fields.
+	sync.Mutex
+
+	// Prefix for all files relevant to this Storage.  Immutable, so
+	// no need to lock.
+	namingPrefix string
+
+	// The Environ that this Storage is for.
+	environUnlocked *maasEnviron
+
+	// Reference to the URL on the API where files are stored.
+	maasClientUnlocked gomaasapi.MAASObject
+}
 
 var _ environs.Storage = (*maasStorage)(nil)
 
+// composeNamingPrefix generates a consistent naming prefix for all files
+// stored by this environment.
+func composeNamingPrefix(env *maasEnviron) string {
+	// Slashes are problematic as path separators: file names are
+	// sometimes included in URL paths, where we'd need to escape them
+	// but standard URL-escaping won't do so.  We can't escape them before
+	// they go into the URLs because subsequent URL escaping would escape
+	// the percentage signs in the escaping itself.
+	// Use a different separator instead.
+	const separator = "__"
+	return "juju" + separator + env.Name() + separator
+}
+
+func NewStorage(env *maasEnviron) environs.Storage {
+	env.ecfgMutex.Lock()
+	defer env.ecfgMutex.Unlock()
+
+	filesClient := env.maasClientUnlocked.GetSubObject("files")
+	storage := new(maasStorage)
+	storage.environUnlocked = env
+	storage.maasClientUnlocked = filesClient
+	storage.namingPrefix = composeNamingPrefix(env)
+	return storage
+}
+
+// getSnapshot returns a consistent copy of a maasStorage.  Use this if you
+// need a consistent view of the object's entire state, without having to
+// lock the object the whole time.
+//
+// An easy mistake to make with "defer" is to keep holding a lock without
+// realizing it, while you go on to block on http requests or other slow
+// things that don't actually require the lock.  In most cases you can just
+// create a snapshot first (releasing the lock immediately) and then do the
+// rest of the work with the snapshot.
+func (stor *maasStorage) getSnapshot() *maasStorage {
+	stor.Lock()
+	defer stor.Unlock()
+
+	return &maasStorage{
+		namingPrefix:       stor.namingPrefix,
+		environUnlocked:    stor.environUnlocked,
+		maasClientUnlocked: stor.maasClientUnlocked,
+	}
+}
+
 func (*maasStorage) Get(name string) (io.ReadCloser, error) {
 	panic("Not implemented.")
 }
 
-func (*maasStorage) List(prefix string) ([]string, error) {
-	panic("Not implemented.")
+// extractFilenames returns the filenames from a "list" operation on the
+// MAAS API, sorted by name.
+func (stor *maasStorage) extractFilenames(listResult gomaasapi.JSONObject) ([]string, error) {
+	list, err := listResult.GetArray()
+	if err != nil {
+		return nil, err
+	}
+	result := make([]string, len(list))
+	for index, entry := range list {
+		file, err := entry.GetMap()
+		if err != nil {
+			return nil, err
+		}
+		filename, err := file["filename"].GetString()
+		if err != nil {
+			return nil, err
+		}
+		if !strings.HasPrefix(filename, stor.namingPrefix) {
+			msg := fmt.Errorf("unexpected filename '%s' lacks environment prefix '%s'", filename, stor.namingPrefix)
+			return nil, msg
+		}
+		filename = filename[len(stor.namingPrefix):]
+		result[index] = filename
+	}
+	sort.Strings(result)
+	return result, nil
+}
+
+func (stor *maasStorage) List(prefix string) ([]string, error) {
+	snapshot := stor.getSnapshot()
+	params := make(url.Values)
+	if len(prefix) > 0 {
+		params.Add("prefix", snapshot.namingPrefix+prefix)
+	}
+	obj, err := snapshot.maasClientUnlocked.CallGet("list", params)
+	if err != nil {
+		return nil, err
+	}
+	return snapshot.extractFilenames(obj)
 }
 
 func (*maasStorage) URL(name string) (string, error) {

=== added file 'environs/maas/storage_test.go'
--- environs/maas/storage_test.go	1970-01-01 00:00:00 +0000
+++ environs/maas/storage_test.go	2013-03-04 10:24:25 +0000
@@ -0,0 +1,136 @@
+package maas
+
+import (
+	. "launchpad.net/gocheck"
+	"launchpad.net/gomaasapi"
+	"launchpad.net/juju-core/environs"
+	"math/rand"
+	"sync"
+)
+
+type StorageSuite struct {
+	ProviderSuite
+}
+
+var _ = Suite(new(StorageSuite))
+
+// makeStorage creates a MAAS storage object for the running test.
+func (s *StorageSuite) makeStorage(name string) *maasStorage {
+	maasobj := s.testMAASObject.MAASObject
+	env := maasEnviron{name: name, maasClientUnlocked: &maasobj}
+	return NewStorage(&env).(*maasStorage)
+}
+
+// makeRandomBytes returns an array of arbitrary byte values.
+func makeRandomBytes(length int) []byte {
+	data := make([]byte, length)
+	for index := range data {
+		data[index] = byte(rand.Intn(256))
+	}
+	return data
+}
+
+// fakeStoredFile creates a file directly in the (simulated) MAAS file store.
+// It will contain an arbitrary amount of random data.  The contents are also
+// returned.
+//
+// If you want properly random data here, initialize the randomizer first.
+// Or don't, if you want consistent (and debuggable) results.
+func (s *StorageSuite) fakeStoredFile(storage environs.Storage, name string) gomaasapi.MAASObject {
+	data := makeRandomBytes(rand.Intn(10))
+	fullName := storage.(*maasStorage).namingPrefix + name
+	return s.testMAASObject.TestServer.NewFile(fullName, data)
+}
+
+func (s *StorageSuite) TestNamingPrefixDiffersBetweenEnvironments(c *C) {
+	storA := s.makeStorage("A")
+	storB := s.makeStorage("B")
+	c.Check(storA.namingPrefix, Not(Equals), storB.namingPrefix)
+}
+
+func (s *StorageSuite) TestNamingPrefixIsConsistentForEnvironment(c *C) {
+	stor1 := s.makeStorage("name")
+	stor2 := s.makeStorage("name")
+	c.Check(stor1.namingPrefix, Equals, stor2.namingPrefix)
+}
+
+func (s *StorageSuite) TestGetSnapshotCreatesClone(c *C) {
+	original := s.makeStorage("storage-name")
+	snapshot := original.getSnapshot()
+	c.Check(snapshot.namingPrefix, Equals, original.namingPrefix)
+	c.Check(snapshot.environUnlocked, Equals, original.environUnlocked)
+	c.Check(snapshot.maasClientUnlocked.URL().String(), Equals, original.maasClientUnlocked.URL().String())
+	// Snapshotting locks the original internally, but does not leave
+	// either the original or the snapshot locked.
+	c.Check(original.Mutex, Equals, sync.Mutex{})
+	c.Check(snapshot.Mutex, Equals, sync.Mutex{})
+}
+
+func (s *StorageSuite) TestListReturnsEmptyIfNoFilesStored(c *C) {
+	storage := NewStorage(s.environ)
+	listing, err := storage.List("")
+	c.Assert(err, IsNil)
+	c.Check(listing, DeepEquals, []string{})
+}
+
+func (s *StorageSuite) TestListReturnsAllFilesIfPrefixEmpty(c *C) {
+	storage := NewStorage(s.environ)
+	files := []string{"1a", "2b", "3c"}
+	for _, name := range files {
+		s.fakeStoredFile(storage, name)
+	}
+
+	listing, err := storage.List("")
+	c.Assert(err, IsNil)
+	c.Check(listing, DeepEquals, files)
+}
+
+func (s *StorageSuite) TestListSortsResults(c *C) {
+	storage := NewStorage(s.environ)
+	files := []string{"4d", "1a", "3c", "2b"}
+	for _, name := range files {
+		s.fakeStoredFile(storage, name)
+	}
+
+	listing, err := storage.List("")
+	c.Assert(err, IsNil)
+	c.Check(listing, DeepEquals, []string{"1a", "2b", "3c", "4d"})
+}
+
+func (s *StorageSuite) TestListReturnsNoFilesIfNoFilesMatchPrefix(c *C) {
+	storage := NewStorage(s.environ)
+	s.fakeStoredFile(storage, "foo")
+
+	listing, err := storage.List("bar")
+	c.Assert(err, IsNil)
+	c.Check(listing, DeepEquals, []string{})
+}
+
+func (s *StorageSuite) TestListReturnsOnlyFilesWithMatchingPrefix(c *C) {
+	storage := NewStorage(s.environ)
+	s.fakeStoredFile(storage, "abc")
+	s.fakeStoredFile(storage, "xyz")
+
+	listing, err := storage.List("x")
+	c.Assert(err, IsNil)
+	c.Check(listing, DeepEquals, []string{"xyz"})
+}
+
+func (s *StorageSuite) TestListMatchesPrefixOnly(c *C) {
+	storage := NewStorage(s.environ)
+	s.fakeStoredFile(storage, "abc")
+	s.fakeStoredFile(storage, "xabc")
+
+	listing, err := storage.List("a")
+	c.Assert(err, IsNil)
+	c.Check(listing, DeepEquals, []string{"abc"})
+}
+
+func (s *StorageSuite) TestListOperatesOnFlatNamespace(c *C) {
+	storage := NewStorage(s.environ)
+	s.fakeStoredFile(storage, "a/b/c/d")
+
+	listing, err := storage.List("a/b")
+	c.Assert(err, IsNil)
+	c.Check(listing, DeepEquals, []string{"a/b/c/d"})
+}