state: deprecate JobManageState

state: deprecate JobManageState

It doesn't really make sense to have the two kinds
of job. There shouldn't be a compatibility problem
as all existing environments have a single state
server machine with both jobs configured.

https://code.launchpad.net/~rogpeppe/juju-core/487-remove-job-managestate/+merge/202916

(do not edit description out of merge proposal)

[rog, 2014-01-23 19:04:35]

Please take a look.

[mue, 2014-01-24 13:05:50]

Mostly LGTM from my side, pretty straightforward change. But would like a second
review to be sure.

https://codereview.appspot.com/54230044/diff/20001/cmd/jujud/machine_test.go
File cmd/jujud/machine_test.go (right):

https://codereview.appspot.com/54230044/diff/20001/cmd/jujud/machine_test.go#...
cmd/jujud/machine_test.go:159: m, _, _ := s.primeAgent(c, state.JobHostUnits)
Why here not JobManageEnviron?

https://codereview.appspot.com/54230044/diff/20001/state/machine.go
File state/machine.go (right):

https://codereview.appspot.com/54230044/diff/20001/state/machine.go#newcode205
state/machine.go:205: // IsManager returns true if the machine has
JJobManageEnviron.
s/JJobManageEnviron/JobManageEnviron

https://codereview.appspot.com/54230044/diff/20001/worker/firewaller/firewall...
File worker/firewaller/firewaller.go (right):

https://codereview.appspot.com/54230044/diff/20001/worker/firewaller/firewall...
worker/firewaller/firewaller.go:178: delete(fw.machineds, tag)
How is this related? Or just a catch?

[rog, 2014-01-24 13:10:34]

https://codereview.appspot.com/54230044/diff/20001/cmd/jujud/machine_test.go
File cmd/jujud/machine_test.go (right):

https://codereview.appspot.com/54230044/diff/20001/cmd/jujud/machine_test.go#...
cmd/jujud/machine_test.go:159: m, _, _ := s.primeAgent(c, state.JobHostUnits)
On 2014/01/24 13:05:51, mue wrote:
> Why here not JobManageEnviron?

Because we can't EnsureDead an environ manager.
The fact that we could previously EnsureDead
a machine with JobManageState was a bug.

https://codereview.appspot.com/54230044/diff/20001/state/machine.go
File state/machine.go (right):

https://codereview.appspot.com/54230044/diff/20001/state/machine.go#newcode205
state/machine.go:205: // IsManager returns true if the machine has
JJobManageEnviron.
On 2014/01/24 13:05:51, mue wrote:
> s/JJobManageEnviron/JobManageEnviron

I've fixed that in a downstream branch; will leave alone here to save the
recommit effort, if that's ok.

https://codereview.appspot.com/54230044/diff/20001/worker/firewaller/firewall...
File worker/firewaller/firewaller.go (right):

https://codereview.appspot.com/54230044/diff/20001/worker/firewaller/firewall...
worker/firewaller/firewaller.go:178: delete(fw.machineds, tag)
On 2014/01/24 13:05:51, mue wrote:
> How is this related? Or just a catch?

It made the tests pass for me. And it definitely seems like a bug.

[mue, 2014-01-24 13:25:11]

On 2014/01/24 13:10:34, rog wrote:
> https://codereview.appspot.com/54230044/diff/20001/cmd/jujud/machine_test.go
> File cmd/jujud/machine_test.go (right):
> 
>
https://codereview.appspot.com/54230044/diff/20001/cmd/jujud/machine_test.go#...
> cmd/jujud/machine_test.go:159: m, _, _ := s.primeAgent(c, state.JobHostUnits)
> On 2014/01/24 13:05:51, mue wrote:
> > Why here not JobManageEnviron?
> 
> Because we can't EnsureDead an environ manager.
> The fact that we could previously EnsureDead
> a machine with JobManageState was a bug.
> 
> https://codereview.appspot.com/54230044/diff/20001/state/machine.go
> File state/machine.go (right):
> 
> https://codereview.appspot.com/54230044/diff/20001/state/machine.go#newcode205
> state/machine.go:205: // IsManager returns true if the machine has
> JJobManageEnviron.
> On 2014/01/24 13:05:51, mue wrote:
> > s/JJobManageEnviron/JobManageEnviron
> 
> I've fixed that in a downstream branch; will leave alone here to save the
> recommit effort, if that's ok.
> 
>
https://codereview.appspot.com/54230044/diff/20001/worker/firewaller/firewall...
> File worker/firewaller/firewaller.go (right):
> 
>
https://codereview.appspot.com/54230044/diff/20001/worker/firewaller/firewall...
> worker/firewaller/firewaller.go:178: delete(fw.machineds, tag)
> On 2014/01/24 13:05:51, mue wrote:
> > How is this related? Or just a catch?
> 
> It made the tests pass for me. And it definitely seems like a bug.

Thanks for the answers.

[dimitern, 2014-01-24 13:31:38]

Excellent! LGTM, with just a few comments.

https://codereview.appspot.com/54230044/diff/20001/state/assign_test.go
File state/assign_test.go (left):

https://codereview.appspot.com/54230044/diff/20001/state/assign_test.go#oldco...
state/assign_test.go:794: // Add a state management machine which can host units
and check it is not chosen.
Why remove this, rather than just change JobManageState to JobManageEnviron and
the comment?

https://codereview.appspot.com/54230044/diff/20001/state/machine.go
File state/machine.go (right):

https://codereview.appspot.com/54230044/diff/20001/state/machine.go#newcode205
state/machine.go:205: // IsManager returns true if the machine has
JJobManageEnviron.
On 2014/01/24 13:10:35, rog wrote:
> On 2014/01/24 13:05:51, mue wrote:
> > s/JJobManageEnviron/JobManageEnviron
> 
> I've fixed that in a downstream branch; will leave alone here to save the
> recommit effort, if that's ok.

As long as it doesn't land like this eventually..

https://codereview.appspot.com/54230044/diff/20001/worker/firewaller/firewall...
File worker/firewaller/firewaller.go (right):

https://codereview.appspot.com/54230044/diff/20001/worker/firewaller/firewall...
worker/firewaller/firewaller.go:178: delete(fw.machineds, tag)
On 2014/01/24 13:10:35, rog wrote:
> On 2014/01/24 13:05:51, mue wrote:
> > How is this related? Or just a catch?
> 
> It made the tests pass for me. And it definitely seems like a bug.

Please, make sure to test this live as well.

[rog, 2014-01-24 13:34:41]

https://codereview.appspot.com/54230044/diff/20001/state/assign_test.go
File state/assign_test.go (left):

https://codereview.appspot.com/54230044/diff/20001/state/assign_test.go#oldco...
state/assign_test.go:794: // Add a state management machine which can host units
and check it is not chosen.
On 2014/01/24 13:31:38, dimitern wrote:
> Why remove this, rather than just change JobManageState to JobManageEnviron
and
> the comment?

Because it would then be exactly the same as the following test.

[rog, 2014-01-24 14:44:54]

Please take a look.

[fwereade, 2014-01-24 14:49:28]

This is delightful. To add to the chorus, LGTM.

https://codereview.appspot.com/54230044/diff/20001/worker/firewaller/firewall...
File worker/firewaller/firewaller.go (right):

https://codereview.appspot.com/54230044/diff/20001/worker/firewaller/firewall...
worker/firewaller/firewaller.go:178: delete(fw.machineds, tag)
On 2014/01/24 13:31:38, dimitern wrote:
> On 2014/01/24 13:10:35, rog wrote:
> > On 2014/01/24 13:05:51, mue wrote:
> > > How is this related? Or just a catch?
> > 
> > It made the tests pass for me. And it definitely seems like a bug.
> 
> Please, make sure to test this live as well.

Looks solid to me, but yes, I am +1 on live tests of obvious driveby fixes.