Joyent Provider - Storage impl

Joyent Provider - Storage impl



https://code.launchpad.net/~dstroppa/juju-core/joyent-provider-storage/+merge/200851

(do not edit description out of merge proposal)

[dstroppa, 2014-01-08 15:13:22]

Please take a look.

[dstroppa, 2014-01-08 15:59:26]

Please take a look.

[gz, 2014-01-09 17:50:34]

Thanks for getting this next step up for review! Several things in need of
fixing:

https://codereview.appspot.com/49050044/diff/20001/dependencies.tsv
File dependencies.tsv (right):

https://codereview.appspot.com/49050044/diff/20001/dependencies.tsv#newcode7
dependencies.tsv:7:
launchpad.net/gojoyent	bzr	daniele.stroppa@joyent.com-20140108145637-xg6p7yewt10z7e2f	47
It seems gojoyent in turn depends on github.com/nu7hatch/gouuid for tests in
localservices/manta/service.go

That would also need adding here - do we want it as a dep or should the tests
just generate sample uuids through some other method?

https://codereview.appspot.com/49050044/diff/20001/provider/joyent/config_tes...
File provider/joyent/config_test.go (right):

https://codereview.appspot.com/49050044/diff/20001/provider/joyent/config_tes...
provider/joyent/config_test.go:4: package joyent
What's the reason to change this from external to internal testing?

https://codereview.appspot.com/49050044/diff/20001/provider/joyent/config_tes...
provider/joyent/config_test.go:26: "sdc-key-id":  
"12:c3:a7:cb:a2:29:e2:90:88:3f:04:53:3b:4e:75:40",
Not changed here, but duplicated later, so mentioning. These fake values should
be faker, rather than being for name and a real-looking key id. Better to spot
in test assertions and such like if it's testuser and 00:11:22:... or similar
obvious made up values.

https://codereview.appspot.com/49050044/diff/20001/provider/joyent/joyent_tes...
File provider/joyent/joyent_test.go (right):

https://codereview.appspot.com/49050044/diff/20001/provider/joyent/joyent_tes...
provider/joyent/joyent_test.go:57: "sdc-key-id":  
"12:c3:a7:cb:a2:29:e2:90:88:3f:04:53:3b:4e:75:40",
These values look very similar to the earlier ones, perhaps they need to be in a
shared location and accessed from there. Otherwise, same comment applies, user
and key-id should be more obviously fake.

https://codereview.appspot.com/49050044/diff/20001/provider/joyent/storage.go
File provider/joyent/storage.go (right):

https://codereview.appspot.com/49050044/diff/20001/provider/joyent/storage.go...
provider/joyent/storage.go:83: // deleteContainer deletes the named comtainer
from the storage account.
Typo, 'coNtainer'.

https://codereview.appspot.com/49050044/diff/20001/provider/joyent/storage.go...
provider/joyent/storage.go:107: //return something that a random wget can
retrieve the object at, without any credentials
This may as well be an actual function doc comment.

https://codereview.appspot.com/49050044/diff/20001/provider/joyent/storage.go...
provider/joyent/storage.go:109: return s.manta.SignURL(path,
time.Now().Add(time.Minute*5))
I think the EC2 provider uses a mcuh longer duration, what a sensible value is
I'm less sure.

https://codereview.appspot.com/49050044/diff/20001/provider/joyent/storage.go...
provider/joyent/storage.go:125: //obj := r.Read()
Remove.

https://codereview.appspot.com/49050044/diff/20001/provider/joyent/storage.go...
provider/joyent/storage.go:141: func (s *joyentStorage) RemoveAll() error {
This is logic I'm not wild about having done (and slightly differently) in all
providers. The openstack version for instance has logic for limiting the number
of concurrent requests...

https://codereview.appspot.com/49050044/diff/20001/provider/joyent/storage_te...
File provider/joyent/storage_test.go (right):

https://codereview.appspot.com/49050044/diff/20001/provider/joyent/storage_te...
provider/joyent/storage_test.go:1: // Copyright 2013 Joyent Inc.
All the tests in this file bar TestURL fail when I run them here, with
variations on:

FAIL: storage_test.go:140: storageSuite.TestCreateContainer

storage_test.go:143:
    s.assertContainer(mantaStorage, c)
storage_test.go:43:
    c.Assert(err, gc.IsNil)
... value *errors.gojoyentError =
&errors.gojoyentError{error:(*errors.errorString)(0xc2002f33b0),
errcode:"UnkownError", cause:(*errors.gojoyentError)(0xc200304510)} ("failed to
create directory: juju-test\ncaused by: Bad request
https://us-east.manta.joyent.com/dstroppa/stor/juju-test\ncaused by: request
(https://us-east.manta.joyent.com/dstroppa/stor/juju-test) returned unexpected
status: 400; error info: {\"code\":\"InvalidHeader\",\"message\":\"Authorization
header invalid: signature was not specified\"}")

It seems they're escaping isolation, rather than using the test services?

[dstroppa, 2014-01-13 12:42:15]

https://codereview.appspot.com/49050044/diff/20001/provider/joyent/storage.go
File provider/joyent/storage.go (right):

https://codereview.appspot.com/49050044/diff/20001/provider/joyent/storage.go...
provider/joyent/storage.go:83: // deleteContainer deletes the named comtainer
from the storage account.
On 2014/01/09 17:50:34, gz wrote:
> Typo, 'coNtainer'.

Done.

https://codereview.appspot.com/49050044/diff/20001/provider/joyent/storage.go...
provider/joyent/storage.go:107: //return something that a random wget can
retrieve the object at, without any credentials
On 2014/01/09 17:50:34, gz wrote:
> This may as well be an actual function doc comment.

Done.

https://codereview.appspot.com/49050044/diff/20001/provider/joyent/storage.go...
provider/joyent/storage.go:109: return s.manta.SignURL(path,
time.Now().Add(time.Minute*5))
On 2014/01/09 17:50:34, gz wrote:
> I think the EC2 provider uses a mcuh longer duration, what a sensible value is
> I'm less sure.

5 minutes is the value used in the Node.js SDK (used as reference). EC2, Azure
and OpenStack providers use 10 years.  Changed to 10 years to be consistent.

https://codereview.appspot.com/49050044/diff/20001/provider/joyent/storage.go...
provider/joyent/storage.go:125: //obj := r.Read()
On 2014/01/09 17:50:34, gz wrote:
> Remove.

Done.

[dstroppa, 2014-01-13 13:03:01]

https://codereview.appspot.com/49050044/diff/20001/dependencies.tsv
File dependencies.tsv (right):

https://codereview.appspot.com/49050044/diff/20001/dependencies.tsv#newcode7
dependencies.tsv:7:
launchpad.net/gojoyent	bzr	daniele.stroppa@joyent.com-20140108145637-xg6p7yewt10z7e2f	47
On 2014/01/09 17:50:34, gz wrote:
> It seems gojoyent in turn depends on github.com/nu7hatch/gouuid for tests in
> localservices/manta/service.go
> 
> That would also need adding here - do we want it as a dep or should the tests
> just generate sample uuids through some other method?

Replaced the gouuid dep with a custom method implemented with std libs.

[dstroppa, 2014-01-13 14:23:38]

https://codereview.appspot.com/49050044/diff/20001/provider/joyent/config_tes...
File provider/joyent/config_test.go (right):

https://codereview.appspot.com/49050044/diff/20001/provider/joyent/config_tes...
provider/joyent/config_test.go:4: package joyent
On 2014/01/09 17:50:34, gz wrote:
> What's the reason to change this from external to internal testing?

No reason actually, it was just a mistake. Changed it to external testing.

[dstroppa, 2014-01-14 13:09:54]

https://codereview.appspot.com/49050044/diff/20001/provider/joyent/config_tes...
File provider/joyent/config_test.go (right):

https://codereview.appspot.com/49050044/diff/20001/provider/joyent/config_tes...
provider/joyent/config_test.go:26: "sdc-key-id":  
"12:c3:a7:cb:a2:29:e2:90:88:3f:04:53:3b:4e:75:40",
On 2014/01/09 17:50:34, gz wrote:
> Not changed here, but duplicated later, so mentioning. These fake values
should
> be faker, rather than being for name and a real-looking key id. Better to spot
> in test assertions and such like if it's testuser and 00:11:22:... or similar
> obvious made up values.

Changed to obvious made up values.

[dstroppa, 2014-01-14 13:11:12]

https://codereview.appspot.com/49050044/diff/20001/provider/joyent/joyent_tes...
File provider/joyent/joyent_test.go (right):

https://codereview.appspot.com/49050044/diff/20001/provider/joyent/joyent_tes...
provider/joyent/joyent_test.go:57: "sdc-key-id":  
"12:c3:a7:cb:a2:29:e2:90:88:3f:04:53:3b:4e:75:40",
On 2014/01/09 17:50:34, gz wrote:
> These values look very similar to the earlier ones, perhaps they need to be in
a
> shared location and accessed from there. Otherwise, same comment applies, user
> and key-id should be more obviously fake.

Changed to valid fake values.

[dstroppa, 2014-01-14 15:33:51]

https://codereview.appspot.com/49050044/diff/20001/provider/joyent/storage_te...
File provider/joyent/storage_test.go (right):

https://codereview.appspot.com/49050044/diff/20001/provider/joyent/storage_te...
provider/joyent/storage_test.go:1: // Copyright 2013 Joyent Inc.
On 2014/01/09 17:50:34, gz wrote:
> All the tests in this file bar TestURL fail when I run them here, with
> variations on:
> 
> FAIL: storage_test.go:140: storageSuite.TestCreateContainer
> 
> storage_test.go:143:
>     s.assertContainer(mantaStorage, c)
> storage_test.go:43:
>     c.Assert(err, gc.IsNil)
> ... value *errors.gojoyentError =
> &errors.gojoyentError{error:(*errors.errorString)(0xc2002f33b0),
> errcode:"UnkownError", cause:(*errors.gojoyentError)(0xc200304510)} ("failed
to
> create directory: juju-test\ncaused by: Bad request
> https://us-east.manta.joyent.com/dstroppa/stor/juju-test%5Cncaused by: request
> (https://us-east.manta.joyent.com/dstroppa/stor/juju-test) returned unexpected
> status: 400; error info:
{\"code\":\"InvalidHeader\",\"message\":\"Authorization
> header invalid: signature was not specified\"}")
> 
> It seems they're escaping isolation, rather than using the test services?

Changed the storage test to run against the local services instead of the Joyent
Live environment

[dstroppa, 2014-01-14 15:36:38]

Please take a look.

[gz, 2014-01-23 16:23:42]

LGTM, should be ready to go with a settled gojoyent version.

[jameinel, 2014-02-27 07:39:42]

On 2014/01/23 16:23:42, gz wrote:
> LGTM, should be ready to go with a settled gojoyent version.

We're still hanging on this 10 days later. It does seem like we should just land
it so that we can start ratcheting forward rather than delaying it.

[dstroppa, 2014-02-27 13:35:18]

Please take a look.