environ: add SetConfig

environ: add SetConfig

Discussion:

This proposal adds the ability for Environs to update their configuration.
This facility replaces the previously proposed proxy Environ and provides
more explicit control over when an Environs configuration is updated.

After much discussion on IRC it is important to highlight that the values 
returned from Environ.{Public,}Storage are immutable with respect to any
configuration change via Environ.SetConfig. To use the updated Environ 
configuration, callers must obtain a new reference to Storages.

Additionally, in the dummy provider, the zookeeper value is still parsed
as a configuration item but is not used. It wasn't used before either, so
I've removed it from dummy.environConfig.

https://code.launchpad.net/~dave-cheney/juju/go-environs-setconfig/+merge/107357

(do not edit description out of merge proposal)

[rog, 2012-05-25 10:53:55]

LGTM apart from PublicStorage.

https://codereview.appspot.com/6256050/diff/1/environs/ec2/ec2.go
File environs/ec2/ec2.go (right):

https://codereview.appspot.com/6256050/diff/1/environs/ec2/ec2.go#newcode157
environs/ec2/ec2.go:157: return e.publicStorageUnlocked
you need to copy the logic of the old PublicStorage method here.

https://codereview.appspot.com/6256050/diff/1/environs/interface.go
File environs/interface.go (right):

https://codereview.appspot.com/6256050/diff/1/environs/interface.go#newcode111
environs/interface.go:111: // Calls to SetConfig do no affect the configuration
of
s/no/not/

https://codereview.appspot.com/6256050/diff/1/environs/interface.go#newcode112
environs/interface.go:112: // values previously retured from Storage and
PublicStorage.
s/retured/returned/

[dfc, 2012-05-25 11:10:21]

Please take a look.

[dfc, 2012-05-25 11:11:20]

On 2012/05/25 11:10:21, dfc wrote:
> Please take a look.

This proposal continues on from https://codereview.appspot.com/6229046/ which I
abandoned because I damaged my working copy.

[niemeyer, 2012-05-25 19:23:37]

Oh, okay.. I got totally confused with it. It might be nice to mention the fact
you've abandoned on it. :-)

[niemeyer, 2012-05-25 19:35:17]

LGTM! This is great. Sorry for the pain, Dave. Just a few trivial details for
you to consider:

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

https://codereview.appspot.com/6256050/diff/4001/environs/ec2/ec2.go#newcode422
environs/ec2/ec2.go:422: // take an immutable reference to the current Storage
This comment should be dropped or clarified (take an immutable reference +
delete all == huh?). That said, please have a look at the last comment.

https://codereview.appspot.com/6256050/diff/4001/environs/interface.go
File environs/interface.go (right):

https://codereview.appspot.com/6256050/diff/4001/environs/interface.go#newcod...
environs/interface.go:134: // The reference returned is immutable with respect
to SetConfig.
I'm not sure if it makes sense to document that in the interface. I don't see it
as specially bad that SetConfig changes an existing Storage, as long as nothing
seriously incorrect happens in such a case.

Your current implementation on EC2 looks fine too, FWIW. I'm just saying it's
not clear that we need to make such a guarantee yet.

[niemeyer, 2012-05-25 19:47:45]

https://codereview.appspot.com/6256050/diff/1/environs/ec2/ec2.go
File environs/ec2/ec2.go (right):

https://codereview.appspot.com/6256050/diff/1/environs/ec2/ec2.go#newcode157
environs/ec2/ec2.go:157: return e.publicStorageUnlocked
On 2012/05/25 10:53:55, rog wrote:
> you need to copy the logic of the old PublicStorage method here.

Btw, if tests pass with this, we're missing a test.

[dfc, 2012-05-26 00:12:20]

im really sorry i didn't indicate this more clearly, the old branch was to
messed up on my local machine by my cavitations. i probably could have fixed it
from the remote branch, but i just wanted a clean copy.

Sent from my iPad

On 26/05/2012, at 5:23, n13m3y3r@gmail.com wrote:

> Oh, okay.. I got totally confused with it. It might be nice to mention
> the fact you've abandoned on it. :-)
> 
> https://codereview.appspot.com/6256050/

[dfc, 2012-05-26 02:17:47]

Please take a look.

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

https://codereview.appspot.com/6256050/diff/4001/environs/ec2/ec2.go#newcode422
environs/ec2/ec2.go:422: // take an immutable reference to the current Storage
Reworded, what I actually mean to say is we need to make sure we pickup the
value of e.storageUnlocked while holding the mutex, which Storage().(*storage)
does

https://codereview.appspot.com/6256050/diff/4001/environs/interface.go
File environs/interface.go (right):

https://codereview.appspot.com/6256050/diff/4001/environs/interface.go#newcod...
environs/interface.go:134: // The reference returned is immutable with respect
to SetConfig.
On 2012/05/25 19:35:17, niemeyer wrote:
> I'm not sure if it makes sense to document that in the interface. I don't see
it
> as specially bad that SetConfig changes an existing Storage, as long as
nothing
> seriously incorrect happens in such a case.

Roger was very concerned that callers holding a reference to a storage be able
to rely on it's configuration not changing over time. Specifically in the case
of ec2/storage.go#deleteAll which spawns multiple goroutines to delete in
parallel, he felt that the danger of the value of the storage.bucket changing
while deleteAll was in progress was high enough that we should make the contract
that a Storage's configuration would not change once you got a reference too it.

On the flip side, there are other values parts of a storage's configuration, the
AWS credentials, which you may want to take effect as soon as SetConfig is
called, as your calls through that storage will be failing if your credentials
are invalid.

After a discussion on IRC, we decided, of the set of alternatives, making a
Storage immutable was the least surprising. It certainly was the least invasive
change to the code; storage.go is virtually untouched which lead me to feel that
this was a reasonable decision.

[niemeyer, 2012-05-26 05:27:03]

https://codereview.appspot.com/6256050/diff/4001/environs/interface.go
File environs/interface.go (right):

https://codereview.appspot.com/6256050/diff/4001/environs/interface.go#newcod...
environs/interface.go:134: // The reference returned is immutable with respect
to SetConfig.
On 2012/05/26 02:17:47, dfc wrote:
> Roger was very concerned that callers holding a reference to a storage be able
> to rely on it's configuration not changing over time. Specifically in the case
> of ec2/storage.go#deleteAll which spawns multiple goroutines to delete in
> parallel, he felt that the danger of the value of the storage.bucket changing
> while deleteAll was in progress was high enough that we should make the
contract
> that a Storage's configuration would not change once you got a reference too
it.

Sounds like a valid concern for the implementation of the ec2 provider. This
issue is not related to this interface, though. deleteAll, storage.bucket, etc,
are all implementation details.

> On the flip side, there are other values parts of a storage's configuration,
the
> AWS credentials, which you may want to take effect as soon as SetConfig is
> called, as your calls through that storage will be failing if your credentials
> are invalid.

Yep.

> After a discussion on IRC, we decided, of the set of alternatives, making a
> Storage immutable was the least surprising. It certainly was the least
invasive
> change to the code; storage.go is virtually untouched which lead me to feel
that
> this was a reasonable decision.

I'm surprised. The Storage is not immutable in any form of what this word
traditionally means. There's a weird note saying that it's immutable "with
respect to SetConfig", which makes no sense except if you dive in and understand
that the ec2 implementation could have issues in some very particular scenarios
due to how it's internally organized. That's not about this interface. I'm not
suggesting we change the implementation. I'm suggesting we drop the weird note
here.

[dfc, 2012-05-26 05:56:02]

Please take a look.

https://codereview.appspot.com/6256050/diff/4001/environs/interface.go
File environs/interface.go (right):

https://codereview.appspot.com/6256050/diff/4001/environs/interface.go#newcod...
environs/interface.go:134: // The reference returned is immutable with respect
to SetConfig.
> I'm surprised. The Storage is not immutable in any form of what this word
> traditionally means. 

Indeed you are correct, the configuration is not changeable, but the remote
storage it's acting on is immutable. This was poor wordsmithing on my part.

> There's a weird note saying that it's immutable "with
> respect to SetConfig", which makes no sense except if you dive in and
understand
> that the ec2 implementation could have issues in some very particular
scenarios
> due to how it's internally organized. That's not about this interface. I'm not
> suggesting we change the implementation. I'm suggesting we drop the weird note
> here.

Works for me :). Done.

[niemeyer, 2012-05-28 12:29:28]

LGTM!

[dfc, 2012-05-28 12:30:40]

Huzzah! 

On 28/05/2012, at 22:29, n13m3y3r@gmail.com wrote:

> LGTM!
> 
> https://codereview.appspot.com/6256050/

[dfc, 2012-05-28 21:23:17]

*** Submitted:

environ: add SetConfig

Discussion:

This proposal adds the ability for Environs to update their configuration.
This facility replaces the previously proposed proxy Environ and provides
more explicit control over when an Environs configuration is updated.

After much discussion on IRC it is important to highlight that the values 
returned from Environ.{Public,}Storage are immutable with respect to any
configuration change via Environ.SetConfig. To use the updated Environ 
configuration, callers must obtain a new reference to Storages.

Additionally, in the dummy provider, the zookeeper value is still parsed
as a configuration item but is not used. It wasn't used before either, so
I've removed it from dummy.environConfig.

R=rog, niemeyer
CC=
https://codereview.appspot.com/6256050