Implement Storage interface for OpenStack

Implement Storage interface for OpenStack

This branch implements the environs.Storage interface for OpenStack.
The OpenStack config is also updated to allow the specification of public and control bucket/containers.

https://code.launchpad.net/~wallyworld/juju-core/openstack-storage/+merge/140127

Requires: https://code.launchpad.net/~wallyworld/juju-core/goose-api-catchup/+merge/139378

(do not edit description out of merge proposal)

[wallyworld, 2012-12-17 02:50:56]

Please take a look.

[dave_cheney.net, 2012-12-17 07:16:19]

This looks good, the only major concern i have is the parallel delete operation.

https://codereview.appspot.com/6940069/diff/1/environs/openstack/provider.go
File environs/openstack/provider.go (right):

https://codereview.appspot.com/6940069/diff/1/environs/openstack/provider.go#...
environs/openstack/provider.go:206: swift:         swift.New(storageClient)}
have you run go fmt ? I'm pretty sure it will reformat this line.

https://codereview.appspot.com/6940069/diff/1/environs/openstack/provider.go#...
environs/openstack/provider.go:211: swift:        
swift.New(publicBucketClient)}
same

https://codereview.appspot.com/6940069/diff/1/environs/openstack/provider.go#...
environs/openstack/provider.go:392: err = st.deleteAll()
return st.deleteAll()

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

https://codereview.appspot.com/6940069/diff/1/environs/openstack/storage.go#n...
environs/openstack/storage.go:17: containerMutex sync.Mutex
you can drop the name, then this becomes an embedded field.

https://codereview.appspot.com/6940069/diff/1/environs/openstack/storage.go#n...
environs/openstack/storage.go:28: s.containerMutex.Lock()
^ from above
s.Lock()

https://codereview.appspot.com/6940069/diff/1/environs/openstack/storage.go#n...
environs/openstack/storage.go:35: err := s.swift.CreateContainer(containerName)
madeContainer is process local, what happens when two processes try to
CreateContainer concurrently ?

https://codereview.appspot.com/6940069/diff/1/environs/openstack/storage.go#n...
environs/openstack/storage.go:65: return r, nil
you can just say 
   return 

here

The rule is, you check the error value before assuming anything about the other
values, so it is fine to drop the last err != nil check here.

https://codereview.appspot.com/6940069/diff/1/environs/openstack/storage.go#n...
environs/openstack/storage.go:70: expires := time.Now().AddDate(1, 0, 0)
Should this be 10 years from a known point ? I don't know how URL is used, but
it looks like it doesn't work idempotently

https://codereview.appspot.com/6940069/diff/1/environs/openstack/storage.go#n...
environs/openstack/storage.go:81: }
could be shorter if you say

if err, ok := maybeNotFound(err); !ok { 
   return err
}
return nil

https://codereview.appspot.com/6940069/diff/1/environs/openstack/storage.go#n...
environs/openstack/storage.go:91: err, ok := maybeNotFound(err)
see above, also helps with the scoping of err

https://codereview.appspot.com/6940069/diff/1/environs/openstack/storage.go#n...
environs/openstack/storage.go:112: // of concurrent operations.
How will we know when customers have passed the 100s object limit ?

Is it worth adding a check here ?

if len(names) > N { return errors.New("Too many files to remove") }

https://codereview.appspot.com/6940069/diff/1/environs/openstack/storage.go#n...
environs/openstack/storage.go:119: if err := s.Remove(name); err != nil {
Is this going to kick off len(names) goroutines (which is cool), that could
consume len(names) file descriptors (not cool) ?

It might be better to turn this around and start a small number of goroutines
that consume jobs from a channel of names. 

I'm also worried about rate limiting, or black banning that might happen if
these jobs keep pumeling the openstack endpoint once errors start to be
reported.

[fwereade, 2012-12-17 08:27:31]

General +1 to davechecey's comments; would appreciate your thoughts re public
storage.

https://codereview.appspot.com/6940069/diff/1/environs/openstack/provider.go
File environs/openstack/provider.go (right):

https://codereview.appspot.com/6940069/diff/1/environs/openstack/provider.go#...
environs/openstack/provider.go:90: publicStorageUnlocked *storage // optional.
Optional for now, I suppose, but we should at least be considering how we
distribute new tools to people on private openstack clouds. Is it unreasonable
to fall back to the usual S3 bucket here, I wonder..?

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

https://codereview.appspot.com/6940069/diff/1/environs/openstack/storage.go#n...
environs/openstack/storage.go:35: err := s.swift.CreateContainer(containerName)
On 2012/12/17 07:16:19, dfc wrote:
> madeContainer is process local, what happens when two processes try to
> CreateContainer concurrently ?

I think that's up to s.swift to handle correctly -- the madeContainer field is
AIUI just a local optimization to avoid extra roundtrips as mentioned in the
comment.

https://codereview.appspot.com/6940069/diff/1/environs/openstack/storage.go#n...
environs/openstack/storage.go:112: // of concurrent operations.
On 2012/12/17 07:16:19, dfc wrote:
> How will we know when customers have passed the 100s object limit ?
> 
> Is it worth adding a check here ?
> 
> if len(names) > N { return errors.New("Too many files to remove") }

+1 (I share dave's other concerns, but a suitably conservative limit here would
make me comfortable enough to accept this as a first step)

[jameinel, 2012-12-17 13:03:44]

I like William's "rather than implement a worker pool to land this, just have it
fail if too many are given, and then we can implement a worker pool in the
future". (Assuming I understood William's request.)

Otherwise some small comments, but LGTM.

https://codereview.appspot.com/6940069/diff/1/environs/openstack/provider.go
File environs/openstack/provider.go (right):

https://codereview.appspot.com/6940069/diff/1/environs/openstack/provider.go#...
environs/openstack/provider.go:90: publicStorageUnlocked *storage // optional.
I think the goal we discussed was having a tool/script to make it easy to copy
the official tools into your new provider (be it ec2 us-west, or your new
openstack cloud). That also avoids the accidental "oh it has been working, but
now canonistack can no longer get to ec2".
Also, I'm thinking canonistack may have some pretty strong firewalls preventing
reaching stuff like ec2 directly.

https://codereview.appspot.com/6940069/diff/1/environs/openstack/provider.go#...
environs/openstack/provider.go:211: swift:        
swift.New(publicBucketClient)}
Doesn't lbox propose require go fmt to be happy with all files?

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

https://codereview.appspot.com/6940069/diff/1/environs/openstack/storage.go#n...
environs/openstack/storage.go:70: expires := time.Now().AddDate(1, 0, 0)
Openstack public URLs work by specifying an expiration time. (Make this public
for 10 days, etc).
The comment says '10 years from now' but it looks to be 1 year from now.

I don't think we want 10 years from a known date, because that means upload
tools stops working at that date.

I believe URL() is used for things like uploaded tools. And for a "stable"
release series, you might have them supported for 5-ish years. So we may need
something longer than 1 year.

https://codereview.appspot.com/6940069/diff/1/environs/openstack/storage.go#n...
environs/openstack/storage.go:119: if err := s.Remove(name); err != nil {
This does look like a case where you'd want a worker pool construct. Is that
straightforward in go? (Is there a standard package/idiom to be used?)

As for specifically rate limiting, openstack does do limiting (limit of 10
POSTs/min by default), but goose is intended to hide some of the retry aspects.
(If we get a retry-later request, we sleep and try again later, for example).

I agree that a high-but-not-infinite pool of workers seems ideal. ~10 ?

https://codereview.appspot.com/6940069/diff/1/environs/openstack/storage.go#n...
environs/openstack/storage.go:151: error, ok := err.(errors.Error)
Because of the common type assertion idiom, you probably want:

var error errors.Error
if error, ok := err.(errors.Error); !ok {
  return err
}

I realize it isn't shorter, but it does avoid leaking the 'ok' variable to the
function scope, and it looks more like other type assertions that are done.

https://codereview.appspot.com/6940069/diff/1/environs/openstack/storage.go#n...
environs/openstack/storage.go:158: }
A comment about why 412 Precondition Failed => NotFound is probably useful here.

[wallyworld, 2012-12-18 02:16:31]

Please take a look.

https://codereview.appspot.com/6940069/diff/1/environs/openstack/provider.go
File environs/openstack/provider.go (right):

https://codereview.appspot.com/6940069/diff/1/environs/openstack/provider.go#...
environs/openstack/provider.go:206: swift:         swift.New(storageClient)}
On 2012/12/17 07:16:19, dfc wrote:
> have you run go fmt ? I'm pretty sure it will reformat this line.

Yeah, I did run it. Otherwise lbox propose fails.

https://codereview.appspot.com/6940069/diff/1/environs/openstack/provider.go#...
environs/openstack/provider.go:211: swift:        
swift.New(publicBucketClient)}
On 2012/12/17 13:03:44, jameinel wrote:
> Doesn't lbox propose require go fmt to be happy with all files?

Yes. go fmt was happy

https://codereview.appspot.com/6940069/diff/1/environs/openstack/provider.go#...
environs/openstack/provider.go:392: err = st.deleteAll()
On 2012/12/17 07:16:19, dfc wrote:
> return st.deleteAll()

Fair call. I just copied this code from the ec2 provider. I changed that code
also.

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

https://codereview.appspot.com/6940069/diff/1/environs/openstack/storage.go#n...
environs/openstack/storage.go:17: containerMutex sync.Mutex
On 2012/12/17 07:16:19, dfc wrote:
> you can drop the name, then this becomes an embedded field. 

I just copied the ec2 implementation for this. I'll make the change here and the
ec2 stuff can be fixed separately.

https://codereview.appspot.com/6940069/diff/1/environs/openstack/storage.go#n...
environs/openstack/storage.go:28: s.containerMutex.Lock()
On 2012/12/17 07:16:19, dfc wrote:
> ^ from above
> s.Lock()

Done.

https://codereview.appspot.com/6940069/diff/1/environs/openstack/storage.go#n...
environs/openstack/storage.go:35: err := s.swift.CreateContainer(containerName)
On 2012/12/17 07:16:19, dfc wrote:
> madeContainer is process local, what happens when two processes try to
> CreateContainer concurrently ?

AFAIUI, if the container already exists, a status of Accepted is returned rather
than Ok, and the swift client handles this.

https://codereview.appspot.com/6940069/diff/1/environs/openstack/storage.go#n...
environs/openstack/storage.go:65: return r, nil
On 2012/12/17 07:16:19, dfc wrote:
> you can just say 
>    return 
> 
> here
> 
> The rule is, you check the error value before assuming anything about the
other
> values, so it is fine to drop the last err != nil check here.

I need to keep the code as is I think, because the API requires that nil be
returned if there is an error. See line 199 of environs/jujutests/tests.go

https://codereview.appspot.com/6940069/diff/1/environs/openstack/storage.go#n...
environs/openstack/storage.go:70: expires := time.Now().AddDate(1, 0, 0)
On 2012/12/17 07:16:19, dfc wrote:
> Should this be 10 years from a known point ? I don't know how URL is used, but
> it looks like it doesn't work idempotently 

This is a direct cut and paste copy of the ec2 implementation. You are right
that there is a typo in the years value. I'll assume the comment is correct and
change the code from 1 to 10, and also fix the ec2 implementation.

https://codereview.appspot.com/6940069/diff/1/environs/openstack/storage.go#n...
environs/openstack/storage.go:81: }
On 2012/12/17 07:16:19, dfc wrote:
> could be shorter if you say
> 
> if err, ok := maybeNotFound(err); !ok { 
>    return err
> }
> return nil

Done.

https://codereview.appspot.com/6940069/diff/1/environs/openstack/storage.go#n...
environs/openstack/storage.go:91: err, ok := maybeNotFound(err)
On 2012/12/17 07:16:19, dfc wrote:
> see above, also helps with the scoping of err

Done.

https://codereview.appspot.com/6940069/diff/1/environs/openstack/storage.go#n...
environs/openstack/storage.go:112: // of concurrent operations.
On 2012/12/17 08:27:31, fwereade wrote:
> On 2012/12/17 07:16:19, dfc wrote:
> > How will we know when customers have passed the 100s object limit ?
> > 
> > Is it worth adding a check here ?
> > 
> > if len(names) > N { return errors.New("Too many files to remove") }
> 
> +1 (I share dave's other concerns, but a suitably conservative limit here
would
> make me comfortable enough to accept this as a first step)

This is a direct copy of the ec2 implementation. I'll tweak it here but the ec2
version should be checked also.

https://codereview.appspot.com/6940069/diff/1/environs/openstack/storage.go#n...
environs/openstack/storage.go:119: if err := s.Remove(name); err != nil {
On 2012/12/17 13:03:44, jameinel wrote:
> This does look like a case where you'd want a worker pool construct. Is that
> straightforward in go? (Is there a standard package/idiom to be used?)
> 
> As for specifically rate limiting, openstack does do limiting (limit of 10
> POSTs/min by default), but goose is intended to hide some of the retry
aspects.
> (If we get a retry-later request, we sleep and try again later, for example).
> 
> I agree that a high-but-not-infinite pool of workers seems ideal. ~10 ?

Since this code is a direct copy of the ec2 implementation, I'd like to leave it
as is for now and tackle the problem holistically as a separate unit of work. It
has been bothering me that a fair chunk of the OpenStack provider implementation
is more or less a copy of what's been done in ec2. This suggests we need to
re-look at the abstractions used and we do really need to try and extract this
common code and share it.

https://codereview.appspot.com/6940069/diff/1/environs/openstack/storage.go#n...
environs/openstack/storage.go:151: error, ok := err.(errors.Error)
On 2012/12/17 13:03:44, jameinel wrote:
> Because of the common type assertion idiom, you probably want:
> 
> var error errors.Error
> if error, ok := err.(errors.Error); !ok {
>   return err
> }
> 
> I realize it isn't shorter, but it does avoid leaking the 'ok' variable to the
> function scope, and it looks more like other type assertions that are done.

Sadly your suggestion won't work as written because := means that the error in
"if error, ok" is a separate variable to the one defined outside the scope of
the if. If I change := to =, then ok becomes undefined. So I moved the block of
code after the if to inside.

https://codereview.appspot.com/6940069/diff/1/environs/openstack/storage.go#n...
environs/openstack/storage.go:158: }
On 2012/12/17 13:03:44, jameinel wrote:
> A comment about why 412 Precondition Failed => NotFound is probably useful
here.

Done.

[fwereade, 2012-12-18 14:57:41]

LGTM -- and thanks for the ec2 consistency fixes. Probably best to wait for dave
before you land this though.

[wallyworld, 2012-12-19 07:05:13]

*** Submitted:

Implement Storage interface for OpenStack

This branch implements the environs.Storage interface for OpenStack.
The OpenStack config is also updated to allow the specification of public and
control bucket/containers.

R=dfc, fwereade, jameinel
CC=
https://codereview.appspot.com/6940069