state: implement EnsureAvailability

state: implement EnsureAvailability This also changes the semantics of AddMachine so that it can only be used to add a state server machine if it's adding the bootstrap machine. After than, only EnsureAvailability can be used to add state servers. https://code.launchpad.net/~rogpeppe/juju-core/475-ensure-availability-2/+merge/202518 (do not edit description out of merge proposal)

[rog, 2014-01-21 19:02:11]

Please take a look.

[dimitern, 2014-01-22 09:29:06]

Looks great in general, just a few minor suggestions and questions below. https://codereview.appspot.com/55150043/diff/20001/state/addmachine.go File state/addmachine.go (right): https://codereview.appspot.com/55150043/diff/20001/state/addmachine.go#newcod... state/addmachine.go:464: // (although the machines themselves will remain). And if the come back up won't that be a problem? https://codereview.appspot.com/55150043/diff/20001/state/addmachine.go#newcod... state/addmachine.go:477: // TODO find machines which are down, clear Please follow the agreed upon format (and file a tech-debt bug): // TODO(rog) bug #... 2014-01-22 // Find machines which are down, clear their // WantVote flag and add new machines to replace // them. https://codereview.appspot.com/55150043/diff/20001/state/api/deployer/deploye... File state/api/deployer/deployer_test.go (right): https://codereview.appspot.com/55150043/diff/20001/state/api/deployer/deploye... state/api/deployer/deployer_test.go:235: addrs := []instance.Address{ Isn't this what AddStateServerMachine actually does (SetAddresses etc.) ? https://codereview.appspot.com/55150043/diff/20001/state/api/uniter/state_tes... File state/api/uniter/state_test.go (right): https://codereview.appspot.com/55150043/diff/20001/state/api/uniter/state_tes... state/api/uniter/state_test.go:25: d https://codereview.appspot.com/55150043/diff/20001/state/apiserver/client/des... File state/apiserver/client/destroy_test.go (left): https://codereview.appspot.com/55150043/diff/20001/state/apiserver/client/des... state/apiserver/client/destroy_test.go:56: s.setUpScenario(c) Why remove this? https://codereview.appspot.com/55150043/diff/20001/state/open.go File state/open.go (right): https://codereview.appspot.com/55150043/diff/20001/state/open.go#newcode312 state/open.go:312: // We remove the document before inserting it because It doesn't seem you're removing the document, just updating some fields and then inserting the whole document again. Is the update necessary to handle newer documents with votingmachineids? https://codereview.appspot.com/55150043/diff/20001/state/state.go File state/state.go (right): https://codereview.appspot.com/55150043/diff/20001/state/state.go#newcode1091 state/state.go:1091: // StateServerInfo returns returns information about s/StateServerInfo/stateServerInfo/ https://codereview.appspot.com/55150043/diff/20001/state/state.go#newcode1093 state/state.go:1093: // TODO(rog) export this method when the stateServers Is this TODO relevant anymore? https://codereview.appspot.com/55150043/diff/20001/state/state_test.go File state/state_test.go (right): https://codereview.appspot.com/55150043/diff/20001/state/state_test.go#newcod... state/state_test.go:2497: m, err := s.State.Machine(fmt.Sprint(i + 1)) Cool trick - instead of fmt.Sprintf("%d", i+1) or strconv.Itoa() :)

[fwereade, 2014-01-22 10:31:57]

Just a couple of quick comments, might be able to circle back and look more closely in a bit. https://codereview.appspot.com/55150043/diff/20001/state/api/uniter/state_tes... File state/api/uniter/state_test.go (right): https://codereview.appspot.com/55150043/diff/20001/state/api/uniter/state_tes... state/api/uniter/state_test.go:38: err := s.stateServerMachine.Destroy() I don't think this should be possible, should it? https://codereview.appspot.com/55150043/diff/20001/state/apiserver/client/des... File state/apiserver/client/destroy_test.go (left): https://codereview.appspot.com/55150043/diff/20001/state/apiserver/client/des... state/apiserver/client/destroy_test.go:56: s.setUpScenario(c) On 2014/01/22 09:29:07, dimitern wrote: > Why remove this? Why have it if it's not needed? It's not very cheap. https://codereview.appspot.com/55150043/diff/20001/state/open.go File state/open.go (right): https://codereview.appspot.com/55150043/diff/20001/state/open.go#newcode312 state/open.go:312: // We remove the document before inserting it because On 2014/01/22 09:29:07, dimitern wrote: > It doesn't seem you're removing the document, just updating some fields and then > inserting the whole document again. Is the update necessary to handle newer > documents with votingmachineids? Yeah, I think the technique is sane but the comment is wrong. If it exists, update it; if not, insert it. (if it doesn't exist, an assertion-free up date is a no-op; if it does, the insert is)

[natefinch, 2014-01-22 12:20:51]

LGTM with a couple small items. https://codereview.appspot.com/55150043/diff/20001/state/addmachine.go File state/addmachine.go (right): https://codereview.appspot.com/55150043/diff/20001/state/addmachine.go#newcod... state/addmachine.go:205: if jset[JobManageState] { I'm not really a fan of where this method is placed, and the fact that now everywhere we call this method, we have to pass in true or false. I'd rather just call hasJob(p.Jobs, MachineJob) and put the call at the beginning of the couple of functions where we need it. I think it would be cleaner that way. https://codereview.appspot.com/55150043/diff/20001/state/addmachine.go#newcod... state/addmachine.go:428: if len(mdocs) != 1 || mdocs[0].Id != "0" { Can you comment this so it's clear why we return the error? I know it's in the function comment, but it would be useful to put it here, too. https://codereview.appspot.com/55150043/diff/20001/state/addmachine.go#newcod... state/addmachine.go:436: if len(currentInfo.MachineIds) != 0 || len(currentInfo.VotingMachineIds) != 0 { Can you make these >0 ? I know len can't return negative numbers, but it makes my spidey sense twitch.

[fwereade, 2014-01-24 16:56:25]

there's a couple of other comments I made earlier I don't think you spotted https://codereview.appspot.com/55150043/diff/20001/state/addmachine.go File state/addmachine.go (right): https://codereview.appspot.com/55150043/diff/20001/state/addmachine.go#newcod... state/addmachine.go:205: if jset[JobManageState] { On 2014/01/22 12:20:52, nate.finch wrote: > I'm not really a fan of where this method is placed, and the fact that now > everywhere we call this method, we have to pass in true or false. I'd rather > just call hasJob(p.Jobs, MachineJob) and put the call at the beginning of the > couple of functions where we need it. I think it would be cleaner that way. Yeah, those bools do smell a little. https://codereview.appspot.com/55150043/diff/20001/state/addmachine.go#newcod... state/addmachine.go:464: // (although the machines themselves will remain). On 2014/01/22 09:29:07, dimitern wrote: > And if the come back up won't that be a problem? So long as there's a bug tracking this I'm ok making progress with this potential problem in play https://codereview.appspot.com/55150043/diff/20001/state/addmachine.go#newcod... state/addmachine.go:477: // TODO find machines which are down, clear On 2014/01/22 09:29:07, dimitern wrote: > Please follow the agreed upon format (and file a tech-debt bug): > > // TODO(rog) bug #... 2014-01-22 > // Find machines which are down, clear their > // WantVote flag and add new machines to replace > // them. +1 https://codereview.appspot.com/55150043/diff/20001/state/api/uniter/state_tes... File state/api/uniter/state_test.go (right): https://codereview.appspot.com/55150043/diff/20001/state/api/uniter/state_tes... state/api/uniter/state_test.go:38: err := s.stateServerMachine.Destroy() On 2014/01/22 10:31:57, fwereade wrote: > I don't think this should be possible, should it? poke

[rog, 2014-01-24 17:01:31]

I forgot to publish these. https://codereview.appspot.com/55150043/diff/20001/state/addmachine.go File state/addmachine.go (right): https://codereview.appspot.com/55150043/diff/20001/state/addmachine.go#newcod... state/addmachine.go:205: if jset[JobManageState] { On 2014/01/22 12:20:52, nate.finch wrote: > I'm not really a fan of where this method is placed, and the fact that now > everywhere we call this method, we have to pass in true or false. I'd rather > just call hasJob(p.Jobs, MachineJob) and put the call at the beginning of the > couple of functions where we need it. I think it would be cleaner that way. I'm not sure I agree. effectiveMachineTemplate is already a guardian of sanity in several ways, and it's called in 4 places, three of which need this check. Adding the extra argument means that I can be very certain that state servers are not added unwittingly (and if someone adds a call to effectiveMachineTemplate in the future, they will be less likely to make that non-obvious mistake). There's probably an argument for making it a non-bool value, say allowStateServer, but it's a private method so I don't think that's worth the hassle. https://codereview.appspot.com/55150043/diff/20001/state/addmachine.go#newcod... state/addmachine.go:428: if len(mdocs) != 1 || mdocs[0].Id != "0" { On 2014/01/22 12:20:52, nate.finch wrote: > Can you comment this so it's clear why we return the error? I know it's in the > function comment, but it would be useful to put it here, too. Done. https://codereview.appspot.com/55150043/diff/20001/state/addmachine.go#newcod... state/addmachine.go:436: if len(currentInfo.MachineIds) != 0 || len(currentInfo.VotingMachineIds) != 0 { On 2014/01/22 12:20:52, nate.finch wrote: > Can you make these >0 ? I know len can't return negative numbers, but it makes > my spidey sense twitch. Done. https://codereview.appspot.com/55150043/diff/20001/state/addmachine.go#newcod... state/addmachine.go:464: // (although the machines themselves will remain). On 2014/01/22 09:29:07, dimitern wrote: > And if the come back up won't that be a problem? Actually, the comment isn't quite accurate. They won't be removed from the current set of state servers - they'll just have NoVote set. That way if they come back up, they're still part of the peer set and can potentially resynchronise and be quickly added back to the voting set. I've fixed the comment. https://codereview.appspot.com/55150043/diff/20001/state/addmachine.go#newcod... state/addmachine.go:477: // TODO find machines which are down, clear On 2014/01/22 09:29:07, dimitern wrote: > Please follow the agreed upon format (and file a tech-debt bug): > > // TODO(rog) bug #... 2014-01-22 > // Find machines which are down, clear their > // WantVote flag and add new machines to replace > // them. Done. https://codereview.appspot.com/55150043/diff/20001/state/api/deployer/deploye... File state/api/deployer/deployer_test.go (right): https://codereview.appspot.com/55150043/diff/20001/state/api/deployer/deploye... state/api/deployer/deployer_test.go:235: addrs := []instance.Address{ On 2014/01/22 09:29:07, dimitern wrote: > Isn't this what AddStateServerMachine actually does (SetAddresses etc.) ? We can't use AddStateServerMachine here because it adds a machine, but the test suite has already created a machine and we're only allowed to use AddMachine to create machine 0 as a state server. https://codereview.appspot.com/55150043/diff/20001/state/api/uniter/state_tes... File state/api/uniter/state_test.go (right): https://codereview.appspot.com/55150043/diff/20001/state/api/uniter/state_tes... state/api/uniter/state_test.go:25: On 2014/01/22 09:29:07, dimitern wrote: > d Done. https://codereview.appspot.com/55150043/diff/20001/state/api/uniter/state_tes... state/api/uniter/state_test.go:38: err := s.stateServerMachine.Destroy() On 2014/01/22 10:31:57, fwereade wrote: > I don't think this should be possible, should it? you're right - i was surprised when this worked. it's a bug in Machine.Destroy - it doesn't fail when the machine only has JobManageState. https://codereview.appspot.com/55150043/diff/20001/state/apiserver/client/des... File state/apiserver/client/destroy_test.go (left): https://codereview.appspot.com/55150043/diff/20001/state/apiserver/client/des... state/apiserver/client/destroy_test.go:56: s.setUpScenario(c) On 2014/01/22 09:29:07, dimitern wrote: > Why remove this? Because it's actually not relevant to the test, and because it adds machines, it prevents the rest of this test working (it relies on add a state-server bootstrap machine) https://codereview.appspot.com/55150043/diff/20001/state/open.go File state/open.go (right): https://codereview.appspot.com/55150043/diff/20001/state/open.go#newcode312 state/open.go:312: // We remove the document before inserting it because On 2014/01/22 09:29:07, dimitern wrote: > It doesn't seem you're removing the document, just updating some fields and then > inserting the whole document again. Is the update necessary to handle newer > documents with votingmachineids? Ah, good catch. I *did* remove then add the document, but it didn't work (not sure why actually). The update is necessary to handle 1.17 upgrades which have the stateServers doc but not the votingmachineids field or a correctly up-to-date machineids field. https://codereview.appspot.com/55150043/diff/20001/state/state.go File state/state.go (right): https://codereview.appspot.com/55150043/diff/20001/state/state.go#newcode1093 state/state.go:1093: // TODO(rog) export this method when the stateServers On 2014/01/22 09:29:07, dimitern wrote: > Is this TODO relevant anymore? ha, no, good catch; i can export it now.

[rog, 2014-01-25 11:59:47]

Please take a look. https://codereview.appspot.com/55150043/diff/20001/state/state.go File state/state.go (right): https://codereview.appspot.com/55150043/diff/20001/state/state.go#newcode1093 state/state.go:1093: // TODO(rog) export this method when the stateServers On 2014/01/24 17:01:31, rog wrote: > On 2014/01/22 09:29:07, dimitern wrote: > > Is this TODO relevant anymore? > > ha, no, good catch; i can export it now. Done.