Verify storage writing on bootstrap

Verify storage writing on bootstrap

Replicates a mechanism used by python juju to test if private bucket storage is writeable by juju on bootstrap.  It, however, uses a different content inside the bootstrap-verify file since we want to use the differences to detect a Python environment and fail on it (this is coming in a follow-up branch).

This has been tested with ec2, but not with openstack or maas.  Any suggestions on how to best do that are welcome.

https://code.launchpad.net/~danilo/juju-core/bootstrap-verify/+merge/166481

(do not edit description out of merge proposal)

[Danilo, 2013-05-30 11:16:48]

Please take a look.

[fwereade, 2013-05-30 16:03:43]

Since you *did* implement this for the dummy provider, you could add a jujutest
test [0] that something actually is written to environ storage. Wouldn't fix
MAAS, which doesn't use those, but that should I think be testable directly?

[0] these (mostly?) get run against ec2, openstack, dummy. But they're also
somewhat crazy and confusing. I don't really mind how this is tested, so long as
it is :).

PS are we expecting a followup that checks there's no pyjuju check-writable
file?

https://codereview.appspot.com/9876043/diff/1/environs/dummy/environs.go
File environs/dummy/environs.go (right):

https://codereview.appspot.com/9876043/diff/1/environs/dummy/environs.go#newc...
environs/dummy/environs.go:437: return fmt.Errorf("Provider storage is not
writeable.")
FWIW, this is extending an antipattern -- the dummy provider doesn't do the same
stuff as the others anyway, and faking it up doesn't seem that helpful -- but
I'm not suggesting you change this today.

[Danilo, 2013-06-04 12:24:24]

Please take a look.

[Danilo, 2013-06-04 12:31:29]

On 2013/05/30 16:03:43, fwereade wrote:
> Since you *did* implement this for the dummy provider, you could add a
jujutest
> test [0] that something actually is written to environ storage. Wouldn't fix
> MAAS, which doesn't use those, but that should I think be testable directly?

At the very least, I've extended the dummy provider's OpPutFile to log the
filename as well and I watch out for that operation in the bootstrap jujutest. 
I can add in the file contents as well, which might make sense for the follow up
branch (but if I am doing it for it, I'd rather do it there).

I'd add a separate test for bootstrap verify, but it would be running exactly
the same code and I am uncertain if extending the test suite run for this makes
sense.

[Danilo, 2013-06-04 12:31:38]

https://codereview.appspot.com/9876043/diff/1/environs/dummy/environs.go
File environs/dummy/environs.go (right):

https://codereview.appspot.com/9876043/diff/1/environs/dummy/environs.go#newc...
environs/dummy/environs.go:437: return fmt.Errorf("Provider storage is not
writeable.")
What would you suggest I do instead?  Ignore the error?

[mue, 2013-06-04 12:42:51]

LGTM with some minor comments.

https://codereview.appspot.com/9876043/diff/4001/environs/ec2/ec2.go
File environs/ec2/ec2.go (right):

https://codereview.appspot.com/9876043/diff/4001/environs/ec2/ec2.go#newcode268
environs/ec2/ec2.go:268: return fmt.Errorf("Provider storage is not writeable.")
Lowercase, "provider storage ...", and not dot at the end.

https://codereview.appspot.com/9876043/diff/4001/environs/maas/environ.go
File environs/maas/environ.go (right):

https://codereview.appspot.com/9876043/diff/4001/environs/maas/environ.go#new...
environs/maas/environ.go:117: return fmt.Errorf("Provider storage is not
writeable.")
Dito, lowercase and dot.

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

https://codereview.appspot.com/9876043/diff/4001/environs/openstack/provider....
environs/openstack/provider.go:494: return fmt.Errorf("Provider storage is not
writeable.")
And again lowercase and dot.

https://codereview.appspot.com/9876043/diff/4001/environs/storage.go
File environs/storage.go (right):

https://codereview.appspot.com/9876043/diff/4001/environs/storage.go#newcode18
environs/storage.go:18: const VERIFICATION_FILENAME string = "bootstrap-verify"
Typical camel-case is used. And it only starts with an uppercase char if it
shall be exported.

[jameinel, 2013-06-04 17:15:34]

I think this is reasonable, but is it ok to just write something and look for an
error code rather than also trying to read it back in (or seeing if one already
exists)?

https://codereview.appspot.com/9876043/diff/4001/environs/storage.go
File environs/storage.go (right):

https://codereview.appspot.com/9876043/diff/4001/environs/storage.go#newcode19
environs/storage.go:19: const VERIFICATION_CONTENT = "juju-core storage writing
verified: ok"
If you are writing it out for long-term storage, it is usually nice to put a
'\n' on the end of it.

[fwereade, 2013-06-05 11:50:02]

On 2013/06/04 12:31:29, Danilo wrote:
> On 2013/05/30 16:03:43, fwereade wrote:
> > Since you *did* implement this for the dummy provider, you could add a
> jujutest
> > test [0] that something actually is written to environ storage. Wouldn't fix
> > MAAS, which doesn't use those, but that should I think be testable directly?
> 
> At the very least, I've extended the dummy provider's OpPutFile to log the
> filename as well and I watch out for that operation in the bootstrap jujutest.

> I can add in the file contents as well, which might make sense for the follow
up
> branch (but if I am doing it for it, I'd rather do it there).
> 
> I'd add a separate test for bootstrap verify, but it would be running exactly
> the same code and I am uncertain if extending the test suite run for this
makes
> sense.

I don't see an environs/jujutest change anywhere... if we're not syncing the
check against all providers, it doesn't help.

[fwereade, 2013-06-05 11:50:10]

https://codereview.appspot.com/9876043/diff/1/environs/dummy/environs.go
File environs/dummy/environs.go (right):

https://codereview.appspot.com/9876043/diff/1/environs/dummy/environs.go#newc...
environs/dummy/environs.go:437: return fmt.Errorf("Provider storage is not
writeable.")
On 2013/06/04 12:31:38, Danilo wrote:
> What would you suggest I do instead?  Ignore the error?

Sorry, I was commenting a bit obliquely on the fundamental issues with the dummy
provider. Implementing this functionality here is only helpful if it's to
support a jujutest test case, which would then be able to verify that all
providers (except MAAS... different discussion) fulfil the same expectations.
That is: without a jujutest case, I don't think it's meaningful to do this.

[dimitern, 2013-06-07 12:35:20]

Almost there and nice! Some minor suggestions, but big +1 on fwereade's comments
about changing jujutest.

https://codereview.appspot.com/9876043/diff/4001/environs/dummy/environs.go
File environs/dummy/environs.go (right):

https://codereview.appspot.com/9876043/diff/4001/environs/dummy/environs.go#n...
environs/dummy/environs.go:440: return fmt.Errorf("Provider storage is not
writeable.")
s/Provider/provider/ and no dot at the end.

https://codereview.appspot.com/9876043/diff/4001/environs/ec2/ec2.go
File environs/ec2/ec2.go (right):

https://codereview.appspot.com/9876043/diff/4001/environs/ec2/ec2.go#newcode268
environs/ec2/ec2.go:268: return fmt.Errorf("Provider storage is not writeable.")
On 2013/06/04 12:42:52, mue wrote:
> Lowercase, "provider storage ...", and not dot at the end.

+1

https://codereview.appspot.com/9876043/diff/4001/environs/maas/environ.go
File environs/maas/environ.go (right):

https://codereview.appspot.com/9876043/diff/4001/environs/maas/environ.go#new...
environs/maas/environ.go:117: return fmt.Errorf("Provider storage is not
writeable.")
On 2013/06/04 12:42:52, mue wrote:
> Dito, lowercase and dot.

+1

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

https://codereview.appspot.com/9876043/diff/4001/environs/openstack/provider....
environs/openstack/provider.go:494: return fmt.Errorf("Provider storage is not
writeable.")
On 2013/06/04 12:42:52, mue wrote:
> And again lowercase and dot.

+1

https://codereview.appspot.com/9876043/diff/4001/environs/storage.go
File environs/storage.go (right):

https://codereview.appspot.com/9876043/diff/4001/environs/storage.go#newcode18
environs/storage.go:18: const VERIFICATION_FILENAME string = "bootstrap-verify"
Please, this is not C :)
const VerificationFilename string = "bootstrap-verify"

https://codereview.appspot.com/9876043/diff/4001/environs/storage.go#newcode19
environs/storage.go:19: const VERIFICATION_CONTENT = "juju-core storage writing
verified: ok"
ditto: VerificationContent

https://codereview.appspot.com/9876043/diff/4001/environs/storage_test.go
File environs/storage_test.go (right):

https://codereview.appspot.com/9876043/diff/4001/environs/storage_test.go#new...
environs/storage_test.go:36: type MockStorage struct {
does it have to be exported?

https://codereview.appspot.com/9876043/diff/4001/environs/storage_test.go#new...
environs/storage_test.go:44: "Put('%s', '%s', %d)", filename, content, length)
if you're doing the call multiline, please:
log_message := fmt.Sprintf(
  "...",
  filename,
  content,
  length,
)

https://codereview.appspot.com/9876043/diff/4001/environs/storage_test.go#new...
environs/storage_test.go:67: type VerifyStorageSuite struct{}
again, the suite doesn't need to be exported (verifyStorageSuite or even just
storageSuite)

[Danilo, 2013-06-13 11:12:54]

Please take a look.

https://codereview.appspot.com/9876043/diff/1/environs/dummy/environs.go
File environs/dummy/environs.go (right):

https://codereview.appspot.com/9876043/diff/1/environs/dummy/environs.go#newc...
environs/dummy/environs.go:437: return fmt.Errorf("Provider storage is not
writeable.")
I've added a minimal jujutest that bootstraps the environment, and then makes
sure that the bootstrap-verify file is written in.  It has been tested with at
least ec2 and openstack providers.

https://codereview.appspot.com/9876043/diff/4001/environs/dummy/environs.go
File environs/dummy/environs.go (right):

https://codereview.appspot.com/9876043/diff/4001/environs/dummy/environs.go#n...
environs/dummy/environs.go:440: return fmt.Errorf("Provider storage is not
writeable.")
On 2013/06/07 12:35:20, dimitern wrote:
> s/Provider/provider/ and no dot at the end.

Fixed already after review by Frank.

https://codereview.appspot.com/9876043/diff/4001/environs/ec2/ec2.go
File environs/ec2/ec2.go (right):

https://codereview.appspot.com/9876043/diff/4001/environs/ec2/ec2.go#newcode268
environs/ec2/ec2.go:268: return fmt.Errorf("Provider storage is not writeable.")
Fixed.

https://codereview.appspot.com/9876043/diff/4001/environs/maas/environ.go
File environs/maas/environ.go (right):

https://codereview.appspot.com/9876043/diff/4001/environs/maas/environ.go#new...
environs/maas/environ.go:117: return fmt.Errorf("Provider storage is not
writeable.")
Fixed.

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

https://codereview.appspot.com/9876043/diff/4001/environs/openstack/provider....
environs/openstack/provider.go:494: return fmt.Errorf("Provider storage is not
writeable.")
And fixed.

https://codereview.appspot.com/9876043/diff/4001/environs/storage.go
File environs/storage.go (right):

https://codereview.appspot.com/9876043/diff/4001/environs/storage.go#newcode18
environs/storage.go:18: const VERIFICATION_FILENAME string = "bootstrap-verify"
On 2013/06/07 12:35:20, dimitern wrote:
> Please, this is not C :)
> const VerificationFilename string = "bootstrap-verify"

This is actually quite common naming pattern in Python ;) Naming patterns are
there to make it easier for us to see this in an arbitrary place in code and
understand that this is a constant.  I know that Go makes everything starting
with a capital exported, but that means that there's no good naming pattern for
constants.

Anyway, changed to verificationFilename, as Frank suggested originally.

https://codereview.appspot.com/9876043/diff/4001/environs/storage.go#newcode19
environs/storage.go:19: const VERIFICATION_CONTENT = "juju-core storage writing
verified: ok"
On 2013/06/04 17:15:34, jameinel wrote:
> If you are writing it out for long-term storage, it is usually nice to put a
> '\n' on the end of it.

Good point, fixed.

https://codereview.appspot.com/9876043/diff/4001/environs/storage.go#newcode19
environs/storage.go:19: const VERIFICATION_CONTENT = "juju-core storage writing
verified: ok"
On 2013/06/07 12:35:20, dimitern wrote:
> ditto: VerificationContent

Fixed to verificationContent.

https://codereview.appspot.com/9876043/diff/4001/environs/storage_test.go
File environs/storage_test.go (right):

https://codereview.appspot.com/9876043/diff/4001/environs/storage_test.go#new...
environs/storage_test.go:36: type MockStorage struct {
On 2013/06/07 12:35:20, dimitern wrote:
> does it have to be exported?

Got rid of it (replaced with calls to dummy provider storage class instead).

https://codereview.appspot.com/9876043/diff/4001/environs/storage_test.go#new...
environs/storage_test.go:67: type VerifyStorageSuite struct{}
Fixed.

[jameinel, 2013-06-13 11:40:40]

LGTM

[fwereade, 2013-06-13 16:02:22]

LGTM, just one thought.

https://codereview.appspot.com/9876043/diff/21001/environs/storage.go
File environs/storage.go (right):

https://codereview.appspot.com/9876043/diff/21001/environs/storage.go#newcode38
environs/storage.go:38: return err
Might be nicer to log the actual error here, and return fmt.Errorf("provider
storage is not writable"), so the clients can all just return non-nil errors
unmodified.

[Danilo, 2013-06-18 10:16:57]

https://codereview.appspot.com/9876043/diff/21001/environs/storage.go
File environs/storage.go (right):

https://codereview.appspot.com/9876043/diff/21001/environs/storage.go#newcode38
environs/storage.go:38: return err
On 2013/06/13 16:02:22, fwereade wrote:
> Might be nicer to log the actual error here, and return fmt.Errorf("provider
> storage is not writable"), so the clients can all just return non-nil errors
> unmodified.

I've done this (had to change the unit test for failure to match on this error
now).