state: remove ProposedTools.

state: remove ProposedTools.

This obviously has many knock-on effects!

https://code.launchpad.net/~rogpeppe/juju-core/038-remove-proposed-tools/+merge/122331

(do not edit description out of merge proposal)

[rog, 2012-08-31 18:15:22]

Please take a look.

[dave_cheney.net, 2012-09-04 07:32:35]

On 2012/08/31 18:15:22, rog wrote:
> Please take a look.

Can cmd/juju/cmd_test.go and cmd/jujud/provisioning_test.go be handled in a
different CL to reduce the size of this change ?

[rog, 2012-09-04 13:57:48]

Please take a look.

[rog, 2012-09-04 15:27:32]

Please take a look.

[niemeyer, 2012-09-04 16:06:20]

https://codereview.appspot.com/6490067/diff/3018/cmd/jujud/upgrade.go
File cmd/jujud/upgrade.go (right):

https://codereview.appspot.com/6490067/diff/3018/cmd/jujud/upgrade.go#newcode16
cmd/jujud/upgrade.go:16: // testing hooks
This should be lower down the file. The Upgrader is still what we care about the
most here.

https://codereview.appspot.com/6490067/diff/3018/cmd/jujud/upgrade.go#newcode158
cmd/jujud/upgrade.go:158: sameVersion()
I'd prefer to avoid this kind of tests when we can. The only assurance of this
kind of test is that the CPU has  seen a given address while executing. Not only
that, but it binds the test to the exact shape of the code. The better kind of
test is the one that verifies intended behavior and semantics. To differentiate
I try to ask myself: how easy is it to break this test without breaking the
application, and how easy is it to break the application without breaking this
test?

https://codereview.appspot.com/6490067/diff/3018/cmd/jujud/upgrade_test.go
File cmd/jujud/upgrade_test.go (right):

https://codereview.appspot.com/6490067/diff/3018/cmd/jujud/upgrade_test.go#ne...
cmd/jujud/upgrade_test.go:146: <-invalidVersionEvent
And what is supposed to happen when "same version event" happens? What is the
thing we care about as far as the application behavior goes?

I don't see what this test is doing. Ditto for similar cases below.

https://codereview.appspot.com/6490067/diff/3018/cmd/jujud/upgrade_test.go#ne...
cmd/jujud/upgrade_test.go:153: // Upload a two new versions of the tools. We'll
test upgrading to these tools.
s/a//

https://codereview.appspot.com/6490067/diff/3018/state/state.go
File state/state.go (right):

https://codereview.appspot.com/6490067/diff/3018/state/state.go#newcode108
state/state.go:108: // state's environment configuration.
Hmmm.. are we going to have such a function for each setting we have in the
environment configuration? What if we want to change two settings at once?
Doesn't sound like a good path.

https://codereview.appspot.com/6490067/diff/3018/state/watcher.go
File state/watcher.go (left):

https://codereview.appspot.com/6490067/diff/3018/state/watcher.go#oldcode480
state/watcher.go:480: type AgentToolsWatcher struct {
Nice to see such a lot of code going away in this CL.

https://codereview.appspot.com/6490067/diff/3018/worker/provisioner/provision...
File worker/provisioner/provisioner.go (right):

https://codereview.appspot.com/6490067/diff/3018/worker/provisioner/provision...
worker/provisioner/provisioner.go:70: case cfg, ok :=
<-environWatcher.Changes():
Cheers!

[rog, 2012-09-04 16:31:50]

https://codereview.appspot.com/6490067/diff/3018/cmd/jujud/upgrade.go
File cmd/jujud/upgrade.go (right):

https://codereview.appspot.com/6490067/diff/3018/cmd/jujud/upgrade.go#newcode16
cmd/jujud/upgrade.go:16: // testing hooks
On 2012/09/04 16:06:22, niemeyer wrote:
> This should be lower down the file. The Upgrader is still what we care about
the
> most here.

pending resolution of how to do the tests.

https://codereview.appspot.com/6490067/diff/3018/cmd/jujud/upgrade_test.go
File cmd/jujud/upgrade_test.go (right):

https://codereview.appspot.com/6490067/diff/3018/cmd/jujud/upgrade_test.go#ne...
cmd/jujud/upgrade_test.go:146: <-invalidVersionEvent
On 2012/09/04 16:06:22, niemeyer wrote:
> And what is supposed to happen when "same version event" happens? What is the
> thing we care about as far as the application behavior goes?
> 
> I don't see what this test is doing. Ditto for similar cases below.

I wanted to check that the upgrader doesn't crash when an invalid version is
proposed. Similarly for sameVersion - I wanted some way to check that the code
has seen the proposed version but not done anything because the version number
remains the same.

An alternative might be to hook into the log messages - they're user-visible, so
could be construed as application behaviour (the sameVersion and invalidVersion
hooks are actually correlated with log messages, but are easier to check).

Other than that, I can't see a way of checking that nothing happens other than
sleeping for a while and checking that nothing has happened, which I'm not keen
on.

Suggestions?

https://codereview.appspot.com/6490067/diff/3018/cmd/jujud/upgrade_test.go#ne...
cmd/jujud/upgrade_test.go:153: // Upload a two new versions of the tools. We'll
test upgrading to these tools.
On 2012/09/04 16:06:22, niemeyer wrote:
> s/a//

Done.

https://codereview.appspot.com/6490067/diff/3018/state/state.go
File state/state.go (right):

https://codereview.appspot.com/6490067/diff/3018/state/state.go#newcode108
state/state.go:108: // state's environment configuration.
On 2012/09/04 16:06:22, niemeyer wrote:
> Hmmm.. are we going to have such a function for each setting we have in the
> environment configuration? What if we want to change two settings at once?
> Doesn't sound like a good path.

I put this in for a couple of reasons:
1) it's done in a few places and it's not entirely trivial.
2) it gives us a placeholder for mutating the environConfig atomically if we
wish.

I agree about the specificity of it though. How about
something like this:

// ChangeEnvironConfig calls the given change function
// with the current EnvironConfig and atomically replaces
// the current configuration with the result. If the
// function returns an invalid configuration, an error
// results.
func (s *State) ChangeEnvironConfig(change func(*config.Config)
map[string]interface{})

Then we could set the agent version like this:

st.ChangeEnvironConfig(func(cfg *config.Config) map[string]interface{}{
    attrs := cfg.AllAttrs()
    attrs["agent-version"] = vers.String()
    return attrs
})

[rog, 2012-09-04 16:46:34]

Please take a look.

[niemeyer, 2012-09-04 19:00:42]

https://codereview.appspot.com/6490067/diff/3018/cmd/jujud/upgrade_test.go
File cmd/jujud/upgrade_test.go (right):

https://codereview.appspot.com/6490067/diff/3018/cmd/jujud/upgrade_test.go#ne...
cmd/jujud/upgrade_test.go:146: <-invalidVersionEvent
On 2012/09/04 16:31:50, rog wrote:
> I wanted to check that the upgrader doesn't crash when an invalid version is
> proposed. Similarly for sameVersion - I wanted some way to check that the code
> has seen the proposed version but not done anything because the version number
> remains the same.

Sure, but it hasn't even done that, for example. This test is ensuring that a
given code path is reached, but it doesn't seem to check that it doesn't "do
anything" for the same version, for example. In fact, "same version" has two
different code paths in the implementation:

    // Ignore the proposed tools if they haven't been set yet
    // or we're already running the proposed version.
    if vers == version.Current.Number {
        break
    }

This doesn't reach sameVersion. This demonstrates the level of attachment that
these assumptions have to one specific implementation format. It's not "the same
version".. it's "that code path that I'm thinking of".

> An alternative might be to hook into the log messages - they're user-visible,
so
> could be construed as application behaviour (the sameVersion and
invalidVersion
> hooks are actually correlated with log messages, but are easier to check).

Yeah, the pattern is similar: observe a side effect and assume it means
correctness. It feels slightly less bad because the side effect is already there
(cost closer to benefit), it mentions the version, and we'd not just hang the
test if it doesn't pass through that code path (which is awful to debug). You're
right that the polling isn't nice, though.

> Other than that, I can't see a way of checking that nothing happens other than
> sleeping for a while and checking that nothing has happened, which I'm not
keen
> on.

Agreed, that's not great either. I have some ideas, that perhaps need some
polishing with your help. Let's talk about this tomorrow.

https://codereview.appspot.com/6490067/diff/3018/state/state.go
File state/state.go (right):

https://codereview.appspot.com/6490067/diff/3018/state/state.go#newcode108
state/state.go:108: // state's environment configuration.
On 2012/09/04 16:31:50, rog wrote:
> I put this in for a couple of reasons:
> 1) it's done in a few places and it's not entirely trivial.

It's done in a single place ATM, and at least the implementation that is in
place right now looks trivial. It's built entirely out of public APIs: get,
change, set.

> 2) it gives us a placeholder for mutating the environConfig atomically if we
> wish.

I was planning on delaying that, but we can also fix that in a follow up if you
think that's relevant.

> I agree about the specificity of it though. How about
> something like this:
> 
> // ChangeEnvironConfig calls the given change function
> // with the current EnvironConfig and atomically replaces
> // the current configuration with the result. If the
> // function returns an invalid configuration, an error
> // results.
> func (s *State) ChangeEnvironConfig(change func(*config.Config)
> map[string]interface{})

This is going in the right direction, but has a couple of issues in terms of how
the delta is represented.

Imagine someone wants to apply username=foo password=bar. We'll need to delta
the attributes to apply them atomically against the whole set of attributes. If
the username hasn't changed, it'll end up as a change of password=bar only,
despite the fact that this wasn't the requested change and is potentially quite
wrong.

Then, there's a second problem: atomicity won't  necessarily mean correctness.
It is possible to corrupt the validity of an environment configuration by
correctly applying an atomic delta on top of an unknown base.

I suggest doing this change in-place where needed in a well-tested helper
function, so we can postpone the general solution to these problems to a future
moment. This also means we can conveniently migrate to mstate without having to
worry about that right now.

https://codereview.appspot.com/6490067/diff/3018/state/state_test.go
File state/state_test.go (right):

https://codereview.appspot.com/6490067/diff/3018/state/state_test.go#newcode171
state/state_test.go:171: func (s *StateSuite) SetAgentVersion(c *C) {
Btw, this test has never run.

[rog, 2012-09-05 17:59:16]

Please take a look.

https://codereview.appspot.com/6490067/diff/3018/cmd/jujud/upgrade.go
File cmd/jujud/upgrade.go (right):

https://codereview.appspot.com/6490067/diff/3018/cmd/jujud/upgrade.go#newcode158
cmd/jujud/upgrade.go:158: sameVersion()
On 2012/09/04 16:06:22, niemeyer wrote:
> I'd prefer to avoid this kind of tests when we can. The only assurance of this
> kind of test is that the CPU has  seen a given address while executing. Not
only
> that, but it binds the test to the exact shape of the code. The better kind of
> test is the one that verifies intended behavior and semantics. To
differentiate
> I try to ask myself: how easy is it to break this test without breaking the
> application, and how easy is it to break the application without breaking this
> test?

tests changed to use a timeout, as discussed online.

https://codereview.appspot.com/6490067/diff/3018/state/state.go
File state/state.go (right):

https://codereview.appspot.com/6490067/diff/3018/state/state.go#newcode108
state/state.go:108: // state's environment configuration.
On 2012/09/04 19:00:43, niemeyer wrote:
> On 2012/09/04 16:31:50, rog wrote:
> > I put this in for a couple of reasons:
> > 1) it's done in a few places and it's not entirely trivial.
> 
> It's done in a single place ATM, and at least the implementation that is in
> place right now looks trivial. It's built entirely out of public APIs: get,
> change, set.
> 
> > 2) it gives us a placeholder for mutating the environConfig atomically if we
> > wish.
> 
> I was planning on delaying that, but we can also fix that in a follow up if
you
> think that's relevant.
> 
> > I agree about the specificity of it though. How about
> > something like this:
> > 
> > // ChangeEnvironConfig calls the given change function
> > // with the current EnvironConfig and atomically replaces
> > // the current configuration with the result. If the
> > // function returns an invalid configuration, an error
> > // results.
> > func (s *State) ChangeEnvironConfig(change func(*config.Config)
> > map[string]interface{})
> 
> This is going in the right direction, but has a couple of issues in terms of
how
> the delta is represented.
> 
> Imagine someone wants to apply username=foo password=bar. We'll need to delta
> the attributes to apply them atomically against the whole set of attributes.
If
> the username hasn't changed, it'll end up as a change of password=bar only,
> despite the fact that this wasn't the requested change and is potentially
quite
> wrong.
> 
> Then, there's a second problem: atomicity won't  necessarily mean correctness.
> It is possible to corrupt the validity of an environment configuration by
> correctly applying an atomic delta on top of an unknown base.
> 
> I suggest doing this change in-place where needed in a well-tested helper
> function, so we can postpone the general solution to these problems to a
future
> moment. This also means we can conveniently migrate to mstate without having
to
> worry about that right now.

Done.

https://codereview.appspot.com/6490067/diff/3018/state/state_test.go
File state/state_test.go (right):

https://codereview.appspot.com/6490067/diff/3018/state/state_test.go#newcode171
state/state_test.go:171: func (s *StateSuite) SetAgentVersion(c *C) {
On 2012/09/04 19:00:43, niemeyer wrote:
> Btw, this test has never run.

it's gone

[niemeyer, 2012-09-05 21:07:07]

LGTM, thank you!

[rog, 2012-09-06 09:18:13]

*** Submitted:

state: remove ProposedTools.

This obviously has many knock-on effects!

R=dfc, niemeyer
CC=
https://codereview.appspot.com/6490067