DNS with new API #1513

lyuxuan · 2017-09-12T21:08:36Z

Implement DNS resolver conforming to v2 resolver API. https://github.com/grpc/grpc-go/blob/master/resolver/resolver.go
Add service config support as specified here https://github.com/grpc/proposal/blob/master/A2-service-configs-in-dns.md

menghanl · 2017-09-12T22:21:24Z

resolver/dns/dns_resolver.go

+)
+
+// NewDNSBuilder creates a dnsBuilder which is used to factory DNS resolvers.
+func NewDNSBuilder() resolver.Builder {


Rename to NewBuilder.
Otherwise it will be dns.NewDNSBuilder().

menghanl · 2017-09-12T22:22:37Z

resolver/dns/dns_resolver.go

+		rn:     make(chan struct{}),
+	}
+
+	go dnsWatcher(d)


Why not d.watcher()?

menghanl · 2017-09-12T22:24:09Z

resolver/dns/dns_resolver.go

+		return nil, err
+	}
+
+	// IP address


Nit: add period at the end...

menghanl · 2017-09-12T22:24:20Z

resolver/dns/dns_resolver.go

+		return i, nil
+	}
+
+	// DNS address (non-IP)


Nit: add period at the end...

menghanl · 2017-09-12T22:25:13Z

resolver/dns/dns_resolver.go

+		}
+		host, _ = formatIP(host)
+		i.updateChan <- resolver.Address{Addr: host + ":" + port}
+		go ipWatcher(i)


Why not i.watcher()?

And, we don't need this goroutine here. It should be OK to call NewAddress() directly here.

done. we still need a go routine here: when user calls ResolveNow() on IPWatcher we can return the same IP again, so we need a go routine to monitor user calling ResolveNow() and send IP as needed.

Call NewAddress() in ipResolver.ResolveNow()?

menghanl · 2017-09-12T22:49:00Z

resolver/dns/dns_resolver.go

+	d.mu.Lock()
+	defer d.mu.Unlock()
+	d.t.Stop()
+	d.rn <- struct{}{}


Do an unblocking write?

sounds good.

menghanl · 2017-09-12T22:56:22Z

resolver/dns/dns_resolver.go

+		d.t.Reset(d.b.freq)
+		d.mu.Unlock()
+		d.cc.NewAddress(result)
+		d.cc.NewServiceConfig(string(sc))


Handle ServiceConfig before addresses, so if the ServiceConfig changes balancer, we can avoid a bit overhead.

menghanl · 2017-09-12T22:56:55Z

resolver/dns/dns_resolver.go

+		d.t.Reset(d.b.freq)
+		d.mu.Unlock()
+		d.cc.NewAddress(result)
+		d.cc.NewServiceConfig(string(sc))


And don't call NewServiceConfig if it's empty?

empty can mean reset, e.g. switching from previous service config to no service config. But I just realize that, Mark mentioned that we should have an Option to turn on/off ServiceConfig check. So I may add some check here later I guess.

menghanl · 2017-09-12T22:58:54Z

resolver/dns/dns_resolver.go

+	return true
+}
+
+func canaryingSC(js []byte) []byte {


If the reason this function takes a []byte and returns a []byte is that it calls json functions, how about making this function takes a string and returns a string, and doing the conversion between []byte and string within this function?

menghanl · 2017-09-12T23:03:18Z

resolver/dns/dns_resolver.go

+
+func (d *dnsResolver) lookup() ([]resolver.Address, []byte) {
+	newAddrs := d.lookupSRV()
+	if newAddrs == nil {


What if the result contains both balancer addresses and backend addresses?
There's a fallback mechanism that use backends directly if cannot connect to the remote balancers.

here it says future work here, but I guess we can do it now.

thinkerou · 2017-09-13T05:41:54Z

resolver/dns/go17.go

@@ -0,0 +1,35 @@
+// +build go1.6, !go1.8


the name of file is go17, but the tag of build is go1.6, the name of file should be go16.go?

This build tags says go1.6 <= build < go1.8, which essentially means go1.6 and go1.7. So the file name indicates that this build file is for build <= go1.7.

thinkerou · 2017-09-13T05:42:53Z

resolver/dns/go17.go

+import (
+	"net"
+
+	"golang.org/x/net/context"


context have been official lib on go1.7

Yes, but LookupHost, LookupSRV and LookupTXT taking in a context starts from go1.8. see here

…r_test.go

menghanl · 2017-09-19T22:31:52Z

resolver/dns/dns_resolver.go

+	}
+
+	// TXT record must have "grpc_config=" attribute in order to be used as service config.
+	if len(res) < len(txtAttribute) || res[:len(txtAttribute)] != txtAttribute {


https://golang.org/pkg/strings/#HasPrefix

menghanl · 2017-09-19T22:32:54Z

resolver/dns/dns_resolver.go

+	defaultPort  = "443"
+	defaultFreq  = time.Minute * 30
+	golang       = "GO"
+	txtAttribute = "grpc_config="


If it's not hard to explain, add a comment.

menghanl · 2017-09-19T22:35:48Z

resolver/dns/dns_resolver.go

+			q:  make(chan struct{}),
+		}
+		cc.NewAddress(addr)
+		go i.watcher()


Why do we need this goroutine?
Is it to support ResolveNow()? If so, we could call NewAddress() in ResolverNow(), right?

yes, and I am indirectly calling NewAddress() in ResolveNow() by sending an empty struct on a notification channel, so inside the watcher it will call NewAddress(). We can also making ResolveNow() running in a gorotine in place of watcher(), but I think our current way is more consistent and clearer.

menghanl · 2017-09-19T22:41:17Z

resolver/dns/dns_resolver.go

+		case <-d.rn:
+		}
+		result, sc := d.lookup()
+		// Next lookup should happen after an interval defined by d.b.freq.


Nit: comment still shows d.b.freq.

menghanl · 2017-09-19T22:48:41Z

resolver/dns/dns_resolver.go

+		// target doesn't have port
+		return host, port, nil
+	}
+	return "", "", fmt.Errorf("invalid target address %v", target)


Include the error returned by SplitHostPort in the return value.

menghanl · 2017-09-19T22:53:55Z

resolver/dns/dns_resolver.go

+		grpclog.Warningf("grpc: failed to parse service config json string due to %v.\n", err)
+		return ""
+	}
+	clihostname, err := os.Hostname()


cliHostName?

menghanl · 2017-09-19T23:09:13Z

resolver/dns/dns_resolver_test.go

+			addrs, ok = cc.getAddress()
+			if ok > 0 {
+				break
+			}


time.Sleep(time.Millisecond)?

menghanl · 2017-09-20T17:10:58Z

resolver/dns/dns_resolver.go

+ *
+ */
+
+package dns


Add an init() to register the builder.

menghanl

One more nit in tests. Otherwise LGTM.

menghanl · 2017-09-25T19:52:03Z

resolver/dns/dns_resolver_test.go

+	}
+}
+
+func newBool(b bool) *bool {


It seems nobody calls these two functions.

…lver

…d lookup

googlebot · 2017-09-27T22:32:57Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
If your company signed a CLA, they designated a Point of Contact who decides which employees are authorized to participate. You may need to contact the Point of Contact for your company and ask to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the project maintainer to go/cla#troubleshoot.
In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again.

lyuxuan · 2017-09-27T23:03:02Z

I signed it!

googlebot · 2017-09-28T03:25:47Z

CLAs look good, thanks!

lyuxuan added 4 commits September 7, 2017 18:27

initial commit with test passed

434fa81

implement service config canarying

401ffac

minor update and implement chosenByPercentage

f2fb0cc

SC contains a slice of MC

41f3bff

menghanl reviewed Sep 12, 2017

View reviewed changes

menghanl self-assigned this Sep 12, 2017

fix corner case

69fe9e1

lyuxuan force-pushed the dns_with_new_api branch from 35cccf1 to 69fe9e1 Compare September 12, 2017 23:08

lyuxuan added 2 commits September 12, 2017 17:34

fix review comments

157c0f9

support fallback to non-balancer address

fa215ed

thinkerou reviewed Sep 13, 2017

View reviewed changes

add attribute checking, unexport service config struct in dns_resolve…

0af96da

…r_test.go

lyuxuan force-pushed the dns_with_new_api branch from 4f80d41 to 0af96da Compare September 14, 2017 01:25

menghanl reviewed Sep 19, 2017

View reviewed changes

menghanl reviewed Sep 20, 2017

View reviewed changes

fix review comments

62b449b

lyuxuan force-pushed the dns_with_new_api branch from ef970f7 to 62b449b Compare September 21, 2017 00:58

dfawley assigned lyuxuan and unassigned menghanl Sep 21, 2017

lyuxuan added 3 commits September 21, 2017 18:20

change to json string in dns_resolver_test

6f66914

try resolving testrace for go1.6 and go1.7

0818a81

resolve go1.6 go1.7 test issue

04edd3c

menghanl approved these changes Sep 25, 2017

View reviewed changes

resolver/dns/dns_resolver_test.go Outdated

}

}

func newBool(b bool) *bool {

Copy link

Contributor

menghanl Sep 25, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems nobody calls these two functions.

lyuxuan added 6 commits September 25, 2017 15:06

polish dns_resolver_test.go

e0e2868

add service config ResolveNow() test, close rn chan when close a reso…

71720c5

…lver

remove close of rn chanel, localize lookup table lock

be7b25c

resolve race

3fd29be

another try to resolve race condition in test, between replaceFunc an…

c9af79e

…d lookup

stop timer when Close() the resolver

aa6e8fc

elaborte race condition

0e50324

lyuxuan merged commit eaf555a into grpc:master Sep 28, 2017

menghanl added this to the 1.7 Release milestone Sep 29, 2017

menghanl added the Type: Feature New features or improvements in behavior label Sep 29, 2017

lock bot locked as resolved and limited conversation to collaborators Jan 18, 2019

DNS with new API #1513

DNS with new API #1513

Conversation

lyuxuan commented Sep 12, 2017 • edited by menghanl Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lyuxuan Sep 13, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

menghanl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

googlebot commented Sep 27, 2017

lyuxuan commented Sep 27, 2017

googlebot commented Sep 28, 2017

lyuxuan commented Sep 12, 2017 •

edited by menghanl

Loading

lyuxuan Sep 13, 2017 •

edited

Loading