environ.StateInfo() for Azure provider.

environ.StateInfo() for Azure provider.

This also required an implementation of loadState and saveState.  All of this
looks like it belongs outside of the provider implementation; we just copy it
from the rest.  Tests are also similar to what we found elsewhere.  It looks
like it could do with some refactorings but we try to avoid that, on the
assumption that such code will be generalized at some point and the job will
only get worse if the various redundant implementations look too different.

In a new twist, I found that the loading/saving of state can be unit-tested.
The dummy provider has an in-memory storage implementation that we can re-use.
No need to put up a test double for Azure's storage service or anything like
that.  All it took was to export two constructors from the dummy provider.

https://code.launchpad.net/~jtv/juju-core/stateinfo/+merge/171081

(do not edit description out of merge proposal)

[jtv.canonical, 2013-06-24 13:27:40]

Please take a look.

[mue, 2013-06-25 13:47:30]

Not yet sure about the role of bootstrapState. A better documentation there may
help.

https://codereview.appspot.com/10500043/diff/1/environs/azure/environ.go
File environs/azure/environ.go (right):

https://codereview.appspot.com/10500043/diff/1/environs/azure/environ.go#newc...
environs/azure/environ.go:37: storage environs.Storage
Comment.

https://codereview.appspot.com/10500043/diff/1/environs/azure/environ.go#newc...
environs/azure/environ.go:43: func NewEnviron(cfg *config.Config)
(*azureEnviron, error) {
Comment.

https://codereview.appspot.com/10500043/diff/1/environs/azure/environ.go#newc...
environs/azure/environ.go:104: log.Debugf("environs/azure: error  getting state
instance: %v", err.Error())
Space in ... error getting ... ;)

https://codereview.appspot.com/10500043/diff/1/environs/azure/state.go
File environs/azure/state.go (right):

https://codereview.appspot.com/10500043/diff/1/environs/azure/state.go#newcode15
environs/azure/state.go:15: 
Why does it need to know them and how does this solution handles large number of
instances?

https://codereview.appspot.com/10500043/diff/1/environs/azure/state.go#newcode16
environs/azure/state.go:16: // Persistent environment state.  An environment
needs to know what resources
// bootstrapState stores the managed resources
// of the Azure cloud.

[fwereade, 2013-06-26 13:10:53]

good in essence, but I don;t want the dummy provider infecting real code. (if
you've spotted it elsewhere, let me know, please ;))

https://codereview.appspot.com/10500043/diff/1/environs/azure/environ.go
File environs/azure/environ.go (right):

https://codereview.appspot.com/10500043/diff/1/environs/azure/environ.go#newc...
environs/azure/environ.go:84: // This code is cargo-culted from the
ec2/maas/openstack providers.
If this stuff is really the same in all 4 places, can we please factor it out
somewhere? It'll want an Environ interface change -- ManagerInstanceIds() or
something, that hides the potential differences in individual loadState()s --
but that seems like a good tradeoff to me.

I don't want to put you guys under a heavy refactoring burden -- but this looks
like it's begging to be a global func StateInfo(Environ) (*state.Info,
*api.Info, error).

https://codereview.appspot.com/10500043/diff/1/environs/azure/environ_test.go
File environs/azure/environ_test.go (right):

https://codereview.appspot.com/10500043/diff/1/environs/azure/environ_test.go...
environs/azure/environ_test.go:132: }
You could then write these tests directly against the StateInfo func, and delete
them elsewhere, at the cost of writing much simpler tests for
ManagerInstanceIds() on each provider.

(I don't *think* we want to make ManagerInstanceIds common, though, I think
there remains reasonable doubt over whether we'll end up needing
provider-specific differences there.)

https://codereview.appspot.com/10500043/diff/1/environs/azure/state.go
File environs/azure/state.go (right):

https://codereview.appspot.com/10500043/diff/1/environs/azure/state.go#newcode15
environs/azure/state.go:15: 
On 2013/06/25 13:47:30, mue wrote:
> Why does it need to know them and how does this solution handles large number
of
> instances?

For the same reason all the other providers do. I'm pretty sure it's fine
handling one instance, and will also be fine with 3; if we go giga-HA at some
point we'll deal with it then.

https://codereview.appspot.com/10500043/diff/1/environs/azure/state.go#newcode16
environs/azure/state.go:16: // Persistent environment state.  An environment
needs to know what resources
On 2013/06/25 13:47:30, mue wrote:
> // bootstrapState stores the managed resources
> // of the Azure cloud.

bootstrapState stores the instance id(s) of the manager node(s) in an
environment.

Wouldn't complain if you fixed the name :).

https://codereview.appspot.com/10500043/diff/1/environs/azure/state_test.go
File environs/azure/state_test.go (right):

https://codereview.appspot.com/10500043/diff/1/environs/azure/state_test.go#n...
environs/azure/state_test.go:28: env.storage = dummy.NewStorage(dummyState,
"/dummy-storage")
I really don't want the dummy provider used in this package. How about moving
the dummy storage out into environs/testing and severing whatever its links with
the dummy state type might be?

Or, hmm, no, it's really tied up with some ugly stuff. I wouldn't have a problem
with you patching in the local provider storage though...

[jtv.canonical, 2013-06-26 16:19:07]

Please take a look.

[fwereade, 2013-06-26 23:44:38]

original comments still apply, just a couple more notes

https://codereview.appspot.com/10500043/diff/10001/environs/azure/environ.go
File environs/azure/environ.go (right):

https://codereview.appspot.com/10500043/diff/10001/environs/azure/environ.go#...
environs/azure/environ.go:13: "launchpad.net/juju-core/log"
loggo logging is appreciated these days fwiw.

https://codereview.appspot.com/10500043/diff/10001/environs/azure/environ.go#...
environs/azure/environ.go:107: // of the provider code.
I'm not going to force it this branch, but can I extract a promise to extract
this out to a func(Environ) (...) in a separate branch please?

I'm not going to be asking for this all the time, but it's a complex enough loop
that it hurts me inside to see it duped in 4 places -- and this one, unlike some
of the others, is almost 100% implemented in terms of the Environ interface, so
it really shouldn't be a massive hassle.

Let me know if it's weirder than I think, though, I might be missing something.

[jtv.canonical, 2013-06-27 10:32:43]

Please take a look.

[jtv.canonical, 2013-06-27 10:34:27]

Please take a look.

[jtv.canonical, 2013-06-27 10:54:02]

(Reproducing a message from memory that got lost in the catacombs of Rietveld
somewhere)

Whew.  It was a lot of work just updating this to deal with divergence from
trunk during review.  In particular, the creation of storage objects had become
on-the-fly but after checking with others I moved back to creating them as the
environ is initialized.  The process got a bit more complicated with the storage
struct's need for dependency injection.

As requested, switched over from using the dummy provider's storage
implementation to the local provider's.  A lot of make-work was generated by the
need to export and un-export symbols: in particular, I had to remove
local.Listen and turn local.listen into local.Listen (which is what the old
local.Listen was an alias for anyway) in order to be able to invoke it from my
tests.  The old local.Listen was in export_test.go, and therefore, was exported
but not actually to outside packages.  There have been requests for me to factor
out the local provider's storage into a separate package, but I'll have to leave
that for a separate branch as this one is getting out of hand.  It started with
a small change but it's a lot like pulling a loose thread on a sweater: before
you know it you've got two hands full of wool string and no sweater!

As for the things that are identical between providers: there are many of these,
and I suspect it's indicative of a missing, or insufficiently developed,
centralized layer of abstraction over the providers.  Either that or there's
been too much friction for code to move to the right place.  I don't think it
would do any good for me to muddle around that and refactor on an ad-hoc basis,
from a limited "view from below" perspective of working on provider
implementations only.  This is a job deserving of its own refactoring drive, by
people with a broader view of the codebase — and preferably while we're not also
in a rush to get a new provider off the ground.

I'm sorry to stubborn about this.  I'd love to start making these changes right
away, because clean code is close to my heart, but this is a part of the
provider where it's still essential to avoid blocking the rest of the team.  We
can't afford to let branches get out of hand, and refactoring works much better
from a stable and functional situation.

[dimitern, 2013-06-27 14:24:37]

On 2013/06/27 10:54:02, jtv.canonical wrote:
> (Reproducing a message from memory that got lost in the catacombs of Rietveld
> somewhere)
> 
> Whew.  It was a lot of work just updating this to deal with divergence from
> trunk during review.  In particular, the creation of storage objects had
become
> on-the-fly but after checking with others I moved back to creating them as the
> environ is initialized.  The process got a bit more complicated with the
storage
> struct's need for dependency injection.
> 
> As requested, switched over from using the dummy provider's storage
> implementation to the local provider's.  A lot of make-work was generated by
the
> need to export and un-export symbols: in particular, I had to remove
> local.Listen and turn local.listen into local.Listen (which is what the old
> local.Listen was an alias for anyway) in order to be able to invoke it from my
> tests.  The old local.Listen was in export_test.go, and therefore, was
exported
> but not actually to outside packages.  There have been requests for me to
factor
> out the local provider's storage into a separate package, but I'll have to
leave
> that for a separate branch as this one is getting out of hand.  It started
with
> a small change but it's a lot like pulling a loose thread on a sweater: before
> you know it you've got two hands full of wool string and no sweater!
> 
> As for the things that are identical between providers: there are many of
these,
> and I suspect it's indicative of a missing, or insufficiently developed,
> centralized layer of abstraction over the providers.  Either that or there's
> been too much friction for code to move to the right place.  I don't think it
> would do any good for me to muddle around that and refactor on an ad-hoc
basis,
> from a limited "view from below" perspective of working on provider
> implementations only.  This is a job deserving of its own refactoring drive,
by
> people with a broader view of the codebase — and preferably while we're not
also
> in a rush to get a new provider off the ground.
> 
> I'm sorry to stubborn about this.  I'd love to start making these changes
right
> away, because clean code is close to my heart, but this is a part of the
> provider where it's still essential to avoid blocking the rest of the team. 
We
> can't afford to let branches get out of hand, and refactoring works much
better
> from a stable and functional situation.

Please take a look and use Roger's CL factoring out the local provider's
storage:
https://codereview.appspot.com/10678043/ (already landed).

[jtv.canonical, 2013-06-27 15:12:30]

> Please take a look and use Roger's CL factoring out the local provider's
> storage:
> https://codereview.appspot.com/10678043/ (already landed).

Christ.  More conflicts, more changes, more delay, more blockage.  Very nice to
extract this, but couldn't it have waited until my code was at least landed?

I'm trying to fix things up now.  Where's the documentation for localstorage?  I
can't find it in the code.

[dimitern, 2013-06-27 15:45:52]

On 2013/06/27 15:12:30, jtv.canonical wrote:
> > Please take a look and use Roger's CL factoring out the local provider's
> > storage:
> > https://codereview.appspot.com/10678043/ (already landed).
> 
> Christ.  More conflicts, more changes, more delay, more blockage.  Very nice
to
> extract this, but couldn't it have waited until my code was at least landed?
> 
> I'm trying to fix things up now.  Where's the documentation for localstorage? 
I
> can't find it in the code.

Sorry about that, Roger probably thought it'll help you to have this first,
since you needed a local storage implementation.
There are 2 methods you probably need: Serve(addr, dir string) (net.Listener,
error) and Client(addr string) environs.Storage.
You can always try godoc launchpad.net/juju-core/environs/localstorage/ (as with
any package) to see the generated documentation.

[jtv.canonical, 2013-06-27 18:20:51]

> There are 2 methods you probably need: Serve(addr, dir string) (net.Listener,
> error) and Client(addr string) environs.Storage.
> You can always try godoc launchpad.net/juju-core/environs/localstorage/ (as
with
> any package) to see the generated documentation.

Thanks for being patient with me.  I looked through all the source-code
comments, but didn't find much.  It looks like the format of "addr" changed to
include a port number, but I don't know what the port number should be.  I tried
"grep -rIi port environs/localstorage" but didn't find any mention.  On the
other hand it's entirely unclear to me why an IP address is needed, because for
this usage ISTM it's always going to be 127.0.0.1.  So I feel that I'm missing a
lot of background knowledge.

Also, don't I need to clean anything up after I'm done with the fake storage?

[jtv.canonical, 2013-06-28 08:01:21]

Please take a look.

[jtv.canonical, 2013-06-28 08:26:27]

Okay, I got it working again after the conflicts and API change from
localstorage.  I don't know why the port number of ":0" really needs to be
included in the address, or what that means.  That was just copied from a test
inside the localstorage package.  Please let me know if it's incorrect.

I'm really hoping to get this branch landed today.  It was really just adding
some code that all the other providers already have, verbatim, and it shouldn't
take more than a week.  Yes, the repetitive code should be factored out, and it
should probably have happened a long time ago.  But let's at least leave that
until this branch has landed!

[fwereade, 2013-06-28 11:29:06]

On 2013/06/28 08:26:27, jtv.canonical wrote:
> Okay, I got it working again after the conflicts and API change from
> localstorage.  I don't know why the port number of ":0" really needs to be
> included in the address, or what that means.  That was just copied from a test
> inside the localstorage package.  Please let me know if it's incorrect.

AIUI, it means "listen on whatever port you can find", and the chosen port will
be part of the Listener's address. Someone correct me if I'm wrong please :).

> I'm really hoping to get this branch landed today.  It was really just adding
> some code that all the other providers already have, verbatim, and it
shouldn't
> take more than a week.  Yes, the repetitive code should be factored out, and
it
> should probably have happened a long time ago.  But let's at least leave that
> until this branch has landed!

OK, somewhat grudgingly, LGTM; but please add a bug for this particularly
egregious duplication, and tag it "tech-debt". I'd particularly appreciate it if
you'd add comments with the bug number in each implementation of the method :).

[jtv.canonical, 2013-06-28 12:34:36]

Thanks.  I filed bug 1195721, and added comments to all the StateInfo
implementations and the state.go files (which are also entirely duplicated) in
the EC2, OpenStack, MAAS, and Azure providers.  This branch is getting pretty
big!

[jtv.canonical, 2013-06-28 12:47:03]

Please take a look.

[allenap, 2013-06-28 14:27:34]

LGTM