Generate masterN.mesos record for current leader #169
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -5,6 +5,7 @@ package records | |
| import ( | ||
| "encoding/json" | ||
| "errors" | ||
| "fmt" | ||
| "hash/fnv" | ||
| "io/ioutil" | ||
| "net" | ||
|
|
@@ -286,15 +287,19 @@ func (rg *RecordGenerator) masterRecord(domain string, masters []string, leader | |
| h := strings.Split(leader, "@") | ||
| if len(h) < 2 { | ||
| logging.Error.Println(leader) | ||
| return // avoid a panic later | ||
| } | ||
| leaderAddress := h[1] | ||
| ip, port, err := getProto(leaderAddress) | ||
| if err != nil { | ||
| logging.Error.Println(err) | ||
| return | ||
| } | ||
| arec := "leader." + domain + "." | ||
| rg.insertRR(arec, ip, "A") | ||
| arec = "master." + domain + "." | ||
| rg.insertRR(arec, ip, "A") | ||
|
|
||
| // SRV records | ||
| tcp := "_leader._tcp." + domain + "." | ||
| udp := "_leader._udp." + domain + "." | ||
|
|
@@ -303,22 +308,43 @@ func (rg *RecordGenerator) masterRecord(domain string, masters []string, leader | |
| rg.insertRR(udp, host, "SRV") | ||
|
There was a problem hiding this comment. I'd love to see this method have no side effects. It would make it easier to reason about and test. We'd isolate the data generation from the mutation effect. records := masterRecords(tc.domain, tc.masters...) // referentially transparent: same inputs, same outputs, no side effects
if err := rg.insert(records...); err != nil { // side-effect, mutates rg internal state
// handle error
}There was a problem hiding this comment. This can be tackled in a separate PR (good idea though!) |
||
|
|
||
| // if there is a list of masters, insert that as well | ||
| addedLeaderMasterN := false | ||
| idx := 0 | ||
| for _, master := range masters { | ||
|
|
||
| ip, _, err := getProto(master) | ||
| if err != nil { | ||
| logging.Error.Println(err) | ||
| continue | ||
| } | ||
|
|
||
| // A records (master and masterN) | ||
| if master != leaderAddress { | ||
| arec := "master." + domain + "." | ||
| added := rg.insertRR(arec, ip, "A") | ||
| if !added { | ||
| // duplicate master?! | ||
| continue | ||
| } | ||
| } | ||
|
|
||
| if master == leaderAddress && addedLeaderMasterN { | ||
| // duplicate leader in masters list?! | ||
| continue | ||
| } | ||
|
|
||
| arec := "master" + strconv.Itoa(idx) + "." + domain + "." | ||
| rg.insertRR(arec, ip, "A") | ||
| idx++ | ||
|
|
||
| if master == leaderAddress { | ||
| addedLeaderMasterN = true | ||
| } | ||
| } | ||
| // flake: we ended up with a leader that's not in the list of all masters? | ||
| if !addedLeaderMasterN { | ||
| logging.Error.Printf("warning: leader %q is not in master list", leader) | ||
| arec = "master" + strconv.Itoa(idx) + "." + domain + "." | ||
| rg.insertRR(arec, ip, "A") | ||
| } | ||
| } | ||
|
|
@@ -375,37 +401,47 @@ func (rg *RecordGenerator) setFromLocal(host string, ns string) { | |
| } | ||
| } | ||
|
|
||
| func (rg *RecordGenerator) exists(name string, host string, rtype string) bool { | ||
|
There was a problem hiding this comment. func (rg *RecordGenerator) exists(name, host, rtype string) bool |
||
| if rtype == "A" { | ||
|
There was a problem hiding this comment. I'd maintain an inverted index of host to names for this method. _, ok := rg.records[rtype][host][name]
return okThere was a problem hiding this comment. We should revisit the key datastructures holistically in order to support discoveryInfo, flexible naming, and http interface. There was a problem hiding this comment. @kozyraki agreed, in a separate PR |
||
| if val, ok := rg.As[name]; ok { | ||
| // check if A record already exists | ||
| // identical tasks on same slave | ||
| for _, b := range val { | ||
| if b == host { | ||
| return true | ||
| } | ||
| } | ||
| } | ||
| } else { | ||
| if val, ok := rg.SRVs[name]; ok { | ||
| // check if SRV record already exists | ||
| for _, b := range val { | ||
| if b == host { | ||
| return true | ||
| } | ||
| } | ||
| } | ||
| } | ||
| return false | ||
| } | ||
|
|
||
| // insertRR adds a record to the appropriate record map for the given name/host pair, | ||
| // but only if the pair is unique. returns true if added, false otherwise. | ||
| // TODO(???): REFACTOR when storage is updated | ||
| func (rg *RecordGenerator) insertRR(name string, host string, rtype string) bool { | ||
|
There was a problem hiding this comment. Argument types can be coalesced. func (rg *RecordGenerator) insertRR(name, host, rtype string) bool { |
||
| logging.VeryVerbose.Println("[" + rtype + "]\t" + name + ": " + host) | ||
|
|
||
| if rg.exists(name, host, rtype) { | ||
|
There was a problem hiding this comment. Why wouldn't we be able to change a record? Perhaps this check could be better placed outside this function in call sites where it's actually needed. There was a problem hiding this comment. i'm just maintaining existing functionality. i refactored so that I could use that said, there's probably a lot of room for refactoring in the system. there's also lots of work planned for features and the bulk of this implementation is going to change at that point. probably not worth optimizing this now. |
||
| return false | ||
| } | ||
| if rtype == "A" { | ||
| val := rg.As[name] | ||
| rg.As[name] = append(val, host) | ||
|
There was a problem hiding this comment. What do you think of making this a nested map? if _, ok := rg.records[rtype]; !ok {
rg.records[rtype] = map[string][]string{}
}
rg.records[rtype][name] = append(rg.records[rtype][name], host)There was a problem hiding this comment. Or even better: if _, ok := rg.records[rtype]; !ok {
rg.records[rtype] = map[string]map[string]struct{}{}
}
if _, ok := rg.records[rtype][name]; !ok {
rg.records[rtype][name] = map[string]struct{}{}
rg.records[rtype][host] = map[string]struct{}{}
}
rg.records[rtype][name][host] = struct{}{}
rg.records[rtype][host][name] = struct{}{}There was a problem hiding this comment. good suggestions. if the system wasn't about to undergo major re-implementation to support new features, i'd agree to an approach like this. |
||
| } else { | ||
| val := rg.SRVs[name] | ||
| rg.SRVs[name] = append(val, host) | ||
| } | ||
| return true | ||
| } | ||
|
|
||
| // returns an array of ports from a range | ||
|
|
@@ -448,6 +484,9 @@ func slaveIdTail(slaveID string) string { | |
| // zk://username:password@host1:port1,host2:port2,.../path | ||
| // file:///path/to/file (where file contains one of the above) | ||
| func getProto(pair string) (string, string, error) { | ||
|
There was a problem hiding this comment. I find it confusing we call this function getProto when we're using it with ip:port pairs too. We're essentially just splitting and returning the tokens as different return arguments so this is generalizable. func split(s, sep string) (string, string, error) {
ps := strings.SplitN(s, sep, 2)
if len(ps) != 2 {
return "", "", fmt.Errorf("%q not found in %q", sep, s)
}
return ps[0], ps[1], nil
}There was a problem hiding this comment. agree. There was a problem hiding this comment. instead of |
||
| h := strings.SplitN(pair, ":", 2) | ||
| if len(h) != 2 { | ||
| return "", "", fmt.Errorf("unable to parse proto from %q", pair) | ||
| } | ||
| return h[0], h[1], nil | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd make the leader be the first item of the masters slice.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The current func implementation makes an assumption about the order of
masters: it's the order in which you expect the ernumeratedmasterXrecords to be created. I think this is probably important: if a new leader is elected, you may not want it to becomemaster0simply because it's the leader. You probably want your DNS records to change as little as possible. And this func should have the least impact on enumeration order, or name/IP mappings - it's just creating the records. So let the caller do the work of ordering, sorting, whatever.. themasterslist if a different outcome is desired.Another consequence of the current overall mesos-dns app implementation is that the leader may not even be in the
masterslist at some point in time.mastersis really fallback-masters (only consider these to be masters if I can't find a leader via ZK). At some point in time, they may not actually be masters any more. Consider a cluster of 3 nodes that suffers the loss of a member, and gains a a new member (VM crashed, was replaced by another VM). And the cycle repeats several times. You end up with a set of running masters (and leader) that's different than the set of statically configured fallback masters.So the func tries to index the masters as they're listed and begrudgingly assigns the leading master an index out-of-band if it's not actually listed in the masters list. There are probably better ways to do it. But I'm really not trying to optimize that here. I'm just trying to fix #165 without introducing additional inconsistency into the system.
I should probably document some of this in the Go comments of the func.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've added my prior ramblings to the masterRecord godocs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍