|
|
|
APIWorker should only connect to API on localhost
In some cases, APIWorker might try to connect
to a different address than localhost in State
Servers, for this APIInfo was modified to add
localhost with the configured StateServerPort
to the Addrs list if not present. To make sure
localhost option will be picked first Open now
sorts the Addrs list before trying to connect
to make sure local ones will be tried first.
https://code.launchpad.net/~hduran-8/juju-core/apiworker_force_local_connection/+merge/221221
(do not edit description out of merge proposal)
Patch Set 1 #Patch Set 2 : APIWorker should only connect to API on localhost #
Total comments: 4
Patch Set 3 : APIWorker should only connect to API on localhost #
Total comments: 5
Total messages: 7
|
hduran
Please take a look.
|
11 years, 7 months ago (2014年05月28日 19:06:50 UTC) #1 | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Please take a look.
I think we need to test that we're actually passing the results of the address picker into apiOpen. https://codereview.appspot.com/100810045/diff/20001/cmd/jujud/agent_test.go File cmd/jujud/agent_test.go (right): https://codereview.appspot.com/100810045/diff/20001/cmd/jujud/agent_test.go#n... cmd/jujud/agent_test.go:114: type replaceErrors struct { You can put this type definition right in the test function, and I think you should :) https://codereview.appspot.com/100810045/diff/20001/cmd/jujud/agent_test.go#n... cmd/jujud/agent_test.go:362: type providedExpectedAddresses struct { move this into the test, and you can give it a less long name. Also, please give each test a unique description and log that description inside the test's loop, that way it's clear what test fails (otherwise there's no way to know which of the tests in the loop failed). It can be something like "State machine should always use localhost" and "non-state machine should use the address given".
Forgive the pontification, but I think this demands a bit more thought. As it is it feel like just slapping another layer on top to cover the deficiencies below, and that rarely ends well... https://codereview.appspot.com/100810045/diff/20001/cmd/jujud/agent.go File cmd/jujud/agent.go (right): https://codereview.appspot.com/100810045/diff/20001/cmd/jujud/agent.go#newcod... cmd/jujud/agent.go:196: func pickBestHosts(apiInfo *api.Info, jobs []params.MachineJob) error { On general principles, I think I'd prefer it if we copied and returned a new *api.Info, rather than mutating the one we passed in. It might be the case that nobody else has a reference to this one, but then it also might not ;). https://codereview.appspot.com/100810045/diff/20001/cmd/jujud/agent.go#newcod... cmd/jujud/agent.go:233: } Hmm. This feels like a band-aid-style solution... I'd prefer to record the addresses we mean to connect to in the first place. But then that involves throwing away information we'd need if we were ever demoted, so it's probably not viable. And we can't have state servers reject API connections from other state servers, or how will we ever promote a machine to a state server? Bah. However... apiOpen *does* try different servers if it can't connect, right? What would you think of the combination of: 1) APIInfo() uses the presence of a StateServingInfo to *also* return localhost:port-this-machine-is-configured-to-serve-the-api-on (so we don't ever actually lie, but we do return what's likely to be the best address to use.) ...and... 2) apiOpen (or the layer below) prioritises localhost: addresses (which STM like a good idea anyway..?) ..? The other option would be to have the API call that returns state server addresses figure out the correct addresses from the POV of the affected entity, and return localhost where appropriate in the first place. I think that'd be best, but I'm not sure it's best enough to be worth the effort. I *am* really bothered about encoding the one-api-port-only assumption in this snippet of code, though -- picking an (effectively) random address from which to take the port feels all kinds of wrong. Thoughts?
Please take a look.
LGTM with a couple minor modifications. https://codereview.appspot.com/100810045/diff/40001/agent/agent.go File agent/agent.go (right): https://codereview.appspot.com/100810045/diff/40001/agent/agent.go#newcode615 agent/agent.go:615: if eachAddress == localApiAddr { I don't really like this naming convention. I get that you're making it "for eachAddress in addresses"... but then the if statement looks like it's saying "if each address equal local" which in English means "if *all* addresses equal local". Plus, the variable is used directly under where it is declared, so it's not like you need a long variable name to remember that this is one item in the list. I'd prefer for _, addr := range addrs { if addr == localApiAddr { each here doesn't really add anything, except give me more words to look at and try to understand. https://codereview.appspot.com/100810045/diff/40001/agent/agent_test.go File agent/agent_test.go (right): https://codereview.appspot.com/100810045/diff/40001/agent/agent_test.go#newco... agent/agent_test.go:461: c.Assert(localhostAddressFound, gc.Equals, true) There's a jc.IsTrue, by the way. https://codereview.appspot.com/100810045/diff/40001/state/api/apiclient.go File state/api/apiclient.go (right): https://codereview.appspot.com/100810045/diff/40001/state/api/apiclient.go#ne... state/api/apiclient.go:112: func SortLocalHostFirst(addrs []string) []string { you should just implement a sort type: type localFirst []string func (l localFirst) Len() int { return len(l) } func (l LocalFirst) Swap(i, j int) { l[i], l[j] = l[j], l[i] } func Less(i, j int) bool { return strings.HasPrefix(l[i]) } then you can just do addrs := sort.Sort(localFirst(info.Addrs)) And that way I don't have to debug your sorting algorithm ;) https://codereview.appspot.com/100810045/diff/40001/state/api/apiclient_test.go File state/api/apiclient_test.go (right): https://codereview.appspot.com/100810045/diff/40001/state/api/apiclient_test.... state/api/apiclient_test.go:76: listener, err := net.Listen("tcp", "localhost:27017") Don't use the same port as built in mongo port, since that may conflict with mongo if it's running.
https://codereview.appspot.com/100810045/diff/40001/state/api/apiclient.go File state/api/apiclient.go (right): https://codereview.appspot.com/100810045/diff/40001/state/api/apiclient.go#ne... state/api/apiclient.go:111: We should use the built-in sort here. http://golang.org/pkg/sort/ type LocalhostFirst []string func (a LocalhostFirst) Len() int { return len(a) } func (a LocalhostFirst) Swap(i int, j int) { a[i], a[j] = a[j], a[i] } func (a LocalhostFirst) Less(i int, j int) bool { return strings.HasPrefix(a[i], "localhost") } return sort.Sort(LocalhostFirst(addrs))
On 2014年05月30日 14:12:32, wwitzel3 wrote: > https://codereview.appspot.com/100810045/diff/40001/state/api/apiclient.go > File state/api/apiclient.go (right): > > https://codereview.appspot.com/100810045/diff/40001/state/api/apiclient.go#ne... > state/api/apiclient.go:111: > We should use the built-in sort here. > > http://golang.org/pkg/sort/ > > type LocalhostFirst []string > > func (a LocalhostFirst) Len() int { return len(a) } > func (a LocalhostFirst) Swap(i int, j int) { a[i], a[j] = a[j], a[i] } > func (a LocalhostFirst) Less(i int, j int) bool { return strings.HasPrefix(a[i], > "localhost") } > > return sort.Sort(LocalhostFirst(addrs)) So I think the sort.Sort is actually incorrect, having worked in this code a bit. Specifically, this breaks the order for *everything else*. And we intentionally move the last-successful-connection to the beginning of the list, so the order isn't random. So for things like EC2, where you have public and private (etc) addresses, this now essentially randomizes which one we are going to try to connect to first. (Actually, it probably sorts the private addresses first because 10. comes before 172.). I can see the point of wanting to try localhost first, and in HA mode we really do want localhost. However, I think what we *really* want is to check "if JobManageEnviron" then *only* connect to localhost, right?