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