state: create stateServers document

state: create stateServers document There's no way for it to change currently, and it will be out of date the moment anyone adds or removes a machine with a state server job, but that will come in subsequent CLs, where we'll implement EnsureAvailability and remove the jobs argument from AddMachine and friends. https://code.launchpad.net/~rogpeppe/juju-core/462-state-stateservers/+merge/195382 (do not edit description out of merge proposal)

[rog, 2013-11-15 13:19:59]

Please take a look. https://codereview.appspot.com/26880043/diff/1/state/environ.go File state/environ.go (right): https://codereview.appspot.com/26880043/diff/1/state/environ.go#newcode34 state/environ.go:34: err := st.environments.Find(nil).One(&doc) This is a drive-by fix.

[gz, 2013-11-15 14:55:54]

LGTM. https://codereview.appspot.com/26880043/diff/20001/state/environ.go File state/environ.go (right): https://codereview.appspot.com/26880043/diff/20001/state/environ.go#newcode34 state/environ.go:34: err := st.environments.Find(nil).One(&doc) Don't understand the drive-by fix here, but will trust it's correct. https://codereview.appspot.com/26880043/diff/20001/state/open.go File state/open.go (right): https://codereview.appspot.com/26880043/diff/20001/state/open.go#newcode287 state/open.go:287: if _, err := st.StateServerMachineIds(); err == nil { This expects all error returns from StateServerMachineIds to be signals that the doc doesn't exist, rather than other kinds of breakage. Mentioning that in the comment above (and perhaps in the StateServerMachineIds function doc comment) may be useful? https://codereview.appspot.com/26880043/diff/20001/state/open.go#newcode322 state/open.go:322: // is already done. Okay, this comment is nice and clear. Do we expect this to ever happen? Seems like a funny race, that I guess we'd expect the stateServers doc updating code to make irrelevent anyway?

[william.reade, 2013-11-15 14:58:19]

I'm not quite sure where this is going. Just having a document, with contents that may or not be correct or relevant, doesn't seem like much of a win. We can find out exactly the same information any time, with at least as much chance of being correct, just by running the same Find you use to construct the document in Open... https://codereview.appspot.com/26880043/diff/20001/state/environ.go File state/environ.go (right): https://codereview.appspot.com/26880043/diff/20001/state/environ.go#newcode34 state/environ.go:34: err := st.environments.Find(nil).One(&doc) I'm not sure this is actually a fix, is it? it moves away from, not towards, multitenancy. https://codereview.appspot.com/26880043/diff/20001/state/open.go File state/open.go (right): https://codereview.appspot.com/26880043/diff/20001/state/open.go#newcode149 state/open.go:149: Id: "", Can't we at least give this the environGlobalKey or something? https://codereview.appspot.com/26880043/diff/20001/state/state.go File state/state.go (right): https://codereview.appspot.com/26880043/diff/20001/state/state.go#newcode1141 state/state.go:1141: sort.Strings(doc.MachineIds) why sorted? in particular, why sorted like that?

[natefinch, 2013-11-15 15:27:25]

https://codereview.appspot.com/26880043/diff/20001/state/open.go File state/open.go (right): https://codereview.appspot.com/26880043/diff/20001/state/open.go#newcode149 state/open.go:149: Id: "", On 2013/11/15 14:58:19, william.reade wrote: > Can't we at least give this the environGlobalKey or something? Yeah. empty string id seems bad, unless that makes it generate a new Id? https://codereview.appspot.com/26880043/diff/20001/state/open.go#newcode287 state/open.go:287: if _, err := st.StateServerMachineIds(); err == nil { On 2013/11/15 14:55:54, gz wrote: > This expects all error returns from StateServerMachineIds to be signals that the > doc doesn't exist, rather than other kinds of breakage. Mentioning that in the > comment above (and perhaps in the StateServerMachineIds function doc comment) > may be useful? That seems like a bad assumption, or are we sure the only error will be a not found error? I admit to not knowing what other errors it might return. https://codereview.appspot.com/26880043/diff/20001/state/state.go File state/state.go (right): https://codereview.appspot.com/26880043/diff/20001/state/state.go#newcode1139 state/state.go:1139: return nil, fmt.Errorf("cannot get state servers document: %v", err) seems like we're obscuring the original error here... at worst, I'd prefer if this were an errors.NotFoundF() call... but it would also be nice to know if the error actually was not found or something else.

[rog, 2013-11-15 16:17:18]

Please take a look. https://codereview.appspot.com/26880043/diff/20001/state/environ.go File state/environ.go (right): https://codereview.appspot.com/26880043/diff/20001/state/environ.go#newcode34 state/environ.go:34: err := st.environments.Find(nil).One(&doc) On 2013/11/15 14:58:19, william.reade wrote: > I'm not sure this is actually a fix, is it? it moves away from, not towards, > multitenancy. It's a fix because the original code is plain wrong (there is no "uuid" field). I'm not sure what it was trying to do, as there would never have been an environment with uuid=="", would there? The eventual right fix will probably to let the state know its own UUID, but that's future fuzziness. https://codereview.appspot.com/26880043/diff/20001/state/open.go File state/open.go (right): https://codereview.appspot.com/26880043/diff/20001/state/open.go#newcode149 state/open.go:149: Id: "", On 2013/11/15 14:58:19, william.reade wrote: > Can't we at least give this the environGlobalKey or something? Done. https://codereview.appspot.com/26880043/diff/20001/state/open.go#newcode287 state/open.go:287: if _, err := st.StateServerMachineIds(); err == nil { On 2013/11/15 14:55:54, gz wrote: > This expects all error returns from StateServerMachineIds to be signals that the > doc doesn't exist, rather than other kinds of breakage. Mentioning that in the > comment above (and perhaps in the StateServerMachineIds function doc comment) > may be useful? It doesn't care what the error is - it's just a preliminary check. If it happened to be some other kind of error (for example the state server connection is broken), that will be picked up below. If it succeeds, however, we know definitely that we don't need to create the document so we can return. I could make the code more complex, but I think this is ok. https://codereview.appspot.com/26880043/diff/20001/state/state.go File state/state.go (right): https://codereview.appspot.com/26880043/diff/20001/state/state.go#newcode1139 state/state.go:1139: return nil, fmt.Errorf("cannot get state servers document: %v", err) On 2013/11/15 15:27:26, nate.finch wrote: > seems like we're obscuring the original error here... at worst, I'd prefer if > this were an errors.NotFoundF() call... but it would also be nice to know if the > error actually was not found or something else. How are we obscuring the original error? We include its message inside the returned error. I considered panicking if we got a not-found error (it should never happen) but I think it's ok to have a slightly more descriptive error message. https://codereview.appspot.com/26880043/diff/20001/state/state.go#newcode1141 state/state.go:1141: sort.Strings(doc.MachineIds) On 2013/11/15 14:58:19, william.reade wrote: > why sorted? in particular, why sorted like that? to make it easy to test. that said, i can sort it in the test. i've done that instead.

[natefinch, 2013-11-15 17:27:38]

On 2013/11/15 16:17:18, rog wrote: > https://codereview.appspot.com/26880043/diff/20001/state/state.go > File state/state.go (right): > > https://codereview.appspot.com/26880043/diff/20001/state/state.go#newcode1139 > state/state.go:1139: return nil, fmt.Errorf("cannot get state servers document: > %v", err) > On 2013/11/15 15:27:26, nate.finch wrote: > > seems like we're obscuring the original error here... at worst, I'd prefer if > > this were an errors.NotFoundF() call... but it would also be nice to know if > the > > error actually was not found or something else. > > How are we obscuring the original error? We include its > message inside the returned error. > > I considered panicking if we got a not-found error (it should > never happen) but I think it's ok to have a slightly more > descriptive error message. I just meant we're obscuring the error type into a generic "error" with the string from the original appended in the message. It's not a huge deal, it just means we can't ever programmatically handle different types of errors returned from the function, since they're all the same thing modulo part of the string.

[natefinch, 2013-11-15 17:28:06]

LGTM