Migrate to use etcd client v3 for CoreDNS provider #686

shashidharatd · 2018-08-22T13:07:37Z

This PR is for migrating CoreDNS provider which is based on etcd client v2 to etcd client v3. (Fixes #659 )
Also fixed ~~couple of~~ below issue:

external-dns CoreDNS provider is not deleting records in sync mode. #669 external-dns CoreDNS provider is not deleting records in sync mode.
~~Endpoint Labels are getting lost in TXT registry. 8424d40~~

/cc @njuettner @linki @ideahitme

njuettner · 2018-09-05T15:08:49Z

endpoint/labels.go

@@ -52,8 +52,8 @@ func NewLabels() Labels {
 // NewLabelsFromString constructs endpoints labels from a provided format string
 // if heritage set to another value is found then error is returned
 // no heritage automatically assumes is not owned by external-dns and returns invalidHeritage error
-func NewLabelsFromString(labelText string) (Labels, error) {
-	endpointLabels := map[string]string{}
+func NewLabelsFromString(existingLabels map[string]string, labelText string) (Labels, error) {


Why do you have to pass in a map string? TBH I don't like this idea.

njuettner

Thanks for your PR 👍 ! I made a comment above.

Forget it

pigmej · 2018-09-19T10:12:54Z

@njuettner @shashidharatd any updates?

shashidharatd · 2018-09-19T10:19:09Z

I am planning to work on it in few days !!

shashidharatd · 2018-09-24T06:49:56Z

I am stuck now. stable etcd release version is 3.3.9 and it has dependency on google.golang.org/grpc with version v1.7.5. external-dns also had indirect dependency on google.golang.org/grpc and it is already updated to version v1.14.0. Because of this incompatibility we cannot use the stable etcd version now.
we have 2 choice.

wait for new stable etcd version.
use unstable master branch commit.

shashidharatd · 2018-10-04T04:49:19Z

Here is the exact issue, i am stuck with and not able to bump etcd

root@laptop:~/gopath/src/github.com/kubernetes-incubator/external-dns# make verify
go test -v -race github.com/kubernetes-incubator/external-dns github.com/kubernetes-incubator/external-dns/controller github.com/kubernetes-incubator/external-dns/endpoint github.com/kubernetes-incubator/external-dns/internal/testutils github.com/kubernetes-incubator/external-dns/pkg/apis/externaldns github.com/kubernetes-incubator/external-dns/pkg/apis/externaldns/validation github.com/kubernetes-incubator/external-dns/pkg/tlsutils github.com/kubernetes-incubator/external-dns/plan github.com/kubernetes-incubator/external-dns/provider github.com/kubernetes-incubator/external-dns/registry github.com/kubernetes-incubator/external-dns/source
# github.com/kubernetes-incubator/external-dns/vendor/go.etcd.io/etcd/clientv3/balancer/picker
vendor/go.etcd.io/etcd/clientv3/balancer/picker/err.go:25:20: cannot use errPicker literal (type *errPicker) as type Picker in return argument:
	*errPicker does not implement Picker (wrong type for Pick method)
		have Pick("context".Context, balancer.PickOptions) (balancer.SubConn, func(balancer.DoneInfo), error)
		want Pick("github.com/kubernetes-incubator/external-dns/vendor/golang.org/x/net/context".Context, balancer.PickOptions) (balancer.SubConn, func(balancer.DoneInfo), error)
vendor/go.etcd.io/etcd/clientv3/balancer/picker/roundrobin_balanced.go:38:3: cannot use rrBalanced literal (type *rrBalanced) as type Picker in return argument:
	*rrBalanced does not implement Picker (wrong type for Pick method)
		have Pick("context".Context, balancer.PickOptions) (balancer.SubConn, func(balancer.DoneInfo), error)
		want Pick("github.com/kubernetes-incubator/external-dns/vendor/golang.org/x/net/context".Context, balancer.PickOptions) (balancer.SubConn, func(balancer.DoneInfo), error)
FAIL	github.com/kubernetes-incubator/external-dns/controller [build failed]

grpc/grpc-go#711

jremond · 2018-10-18T16:30:39Z

Hello, we are trying to test external-dns with coredns 1.2.X / etcdv3 backend, any idea of the next step ?

jremond · 2018-10-26T14:58:10Z

@shashidharatd I have been able to build a working external-dns binary when merging this PR into the master branch and make it work with our coredns/etcdv3 cluster.

shashidharatd · 2018-10-26T15:48:07Z

@shashidharatd I have been able to build a working external-dns binary when merging this PR into the master branch and make it work with our coredns/etcdv3 cluster.

That's awesome, what is the etcd version you used, @jremond ?

jremond · 2018-10-26T16:15:52Z

@shashidharatd
etcd Version: 3.3.8
Git SHA: 33245c6b5
Go Version: go1.9.7
Go OS/Arch: linux/amd64

coredns 1.2.1

I merged your code after 5ff808f in master

shashidharatd · 2018-10-26T16:46:09Z

I am still getting the same error, when i do make verify
google.golang.org/grpc version used by etcd v3.3.8 is v1.7.5 https://github.com/etcd-io/etcd/blob/v3.3.8/glide.yaml#L113

google.golang.org/grpc version used in this project is v1.14.0 https://github.com/kubernetes-incubator/external-dns/blob/master/Gopkg.lock#L852

google.golang.org/grpc version v1.7.5 and v1.14.0 are not compatible.

This issue will be resolved by grpc/grpc-go#711 in 5 days. Then etcd should pick this change in their development branch, and we can use it in this project. a long shot.

shashidharatd · 2018-10-26T16:49:47Z

@jremond, i suggest you create a new pr with your changes. that would trigger all tests and not just the binary build. lets see how it goes.

jremond · 2018-10-26T17:28:50Z

See #761

shashidharatd · 2018-10-26T17:35:46Z

@jremond, recent changes in this repo seemed to fix the issue in this pr. I rebased this pr on latest changes and then did dep ensure. Now make verify works. Thanks for your prodding.

shashidharatd · 2018-10-26T17:49:30Z

@njuettner @linki, this pr is now unblocked. can you please check it again. @njuettner, i have removed the commit on which you have objection. That problem should have been fixed by #713. Thanks !

shashidharatd · 2018-10-26T18:02:03Z

/assign @njuettner

njuettner · 2018-10-31T15:37:33Z

provider/coredns.go

+	var sx []*Service
+	bx := make(map[Service]bool)
+	for _, n := range r.Kvs {
+		serv := new(Service)


wouldn't it be better to name it sor svc. servsounds more like server to me 😄

njuettner · 2018-10-31T15:44:43Z

provider/coredns.go

-				queue.PushBack(childNode)
-			}
-			continue
+	var sx []*Service


maybe svcs?

njuettner · 2018-10-31T15:46:01Z

provider/coredns.go

-			}
-			continue
+	var sx []*Service
+	bx := make(map[Service]bool)


For someone which reads the code it's hard to tell what it does, could you find a better name? or just comment what it does :)

I have added comments now. sorry for the issue in readability. i referred to the etcd plugin in coredns here and didn't do too much of my own modifications.

shashidharatd

Thanks @njuettner, can you ptal once again. have addressed your comments.

njuettner · 2018-11-14T14:06:14Z

/LGTM

njuettner · 2018-11-14T14:06:21Z

/APPROVE

k8s-ci-robot · 2018-11-14T14:06:33Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: njuettner

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [njuettner]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

k8s-ci-robot requested review from ideahitme, linki and njuettner August 22, 2018 13:07

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Aug 22, 2018

shashidharatd force-pushed the etcdcv3 branch 4 times, most recently from 99a78cd to 87ee5cf Compare August 29, 2018 12:47

njuettner reviewed Sep 5, 2018

View reviewed changes

njuettner requested changes Sep 5, 2018

View reviewed changes

njuettner previously approved these changes Sep 5, 2018

View reviewed changes

njuettner self-requested a review September 5, 2018 15:13

shashidharatd force-pushed the etcdcv3 branch from 87ee5cf to 5a62a6d Compare September 6, 2018 05:37

fturib mentioned this pull request Sep 23, 2018

Add etcd3 support for CoreDNS provider #716

Closed

jremond mentioned this pull request Oct 26, 2018

Etcdv3 pr686 #761

Closed

shashidharatd force-pushed the etcdcv3 branch from 5a62a6d to 66c6a57 Compare October 26, 2018 17:37

k8s-ci-robot assigned njuettner Oct 26, 2018

njuettner reviewed Oct 31, 2018

View reviewed changes

shashidharatd added 2 commits October 31, 2018 21:34

Update CoreDNS provider to use etcd v3 client

c668865

Update Gopkg.* vendor management files for github.com/coreos/etcd

a1563f0

shashidharatd force-pushed the etcdcv3 branch from 66c6a57 to a1563f0 Compare October 31, 2018 16:05

shashidharatd commented Oct 31, 2018

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 14, 2018

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 14, 2018

k8s-ci-robot merged commit 5fea753 into kubernetes-sigs:master Nov 14, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate to use etcd client v3 for CoreDNS provider #686

Migrate to use etcd client v3 for CoreDNS provider #686

shashidharatd commented Aug 22, 2018 •

edited

Loading

njuettner Sep 5, 2018

njuettner left a comment •

edited

Loading

pigmej commented Sep 19, 2018

shashidharatd commented Sep 19, 2018

shashidharatd commented Sep 24, 2018

shashidharatd commented Oct 4, 2018

jremond commented Oct 18, 2018

jremond commented Oct 26, 2018

shashidharatd commented Oct 26, 2018

jremond commented Oct 26, 2018 •

edited

Loading

shashidharatd commented Oct 26, 2018

shashidharatd commented Oct 26, 2018

jremond commented Oct 26, 2018

shashidharatd commented Oct 26, 2018

shashidharatd commented Oct 26, 2018 •

edited

Loading

shashidharatd commented Oct 26, 2018

njuettner Oct 31, 2018

shashidharatd Oct 31, 2018

njuettner Oct 31, 2018

shashidharatd Oct 31, 2018

njuettner Oct 31, 2018

shashidharatd Oct 31, 2018 •

edited

Loading

shashidharatd left a comment

njuettner commented Nov 14, 2018

njuettner commented Nov 14, 2018

k8s-ci-robot commented Nov 14, 2018

Migrate to use etcd client v3 for CoreDNS provider #686

Migrate to use etcd client v3 for CoreDNS provider #686

Conversation

shashidharatd commented Aug 22, 2018 • edited Loading

njuettner Sep 5, 2018

Choose a reason for hiding this comment

njuettner left a comment • edited Loading

Choose a reason for hiding this comment

pigmej commented Sep 19, 2018

shashidharatd commented Sep 19, 2018

shashidharatd commented Sep 24, 2018

shashidharatd commented Oct 4, 2018

jremond commented Oct 18, 2018

jremond commented Oct 26, 2018

shashidharatd commented Oct 26, 2018

jremond commented Oct 26, 2018 • edited Loading

shashidharatd commented Oct 26, 2018

shashidharatd commented Oct 26, 2018

jremond commented Oct 26, 2018

shashidharatd commented Oct 26, 2018

shashidharatd commented Oct 26, 2018 • edited Loading

shashidharatd commented Oct 26, 2018

njuettner Oct 31, 2018

Choose a reason for hiding this comment

shashidharatd Oct 31, 2018

Choose a reason for hiding this comment

njuettner Oct 31, 2018

Choose a reason for hiding this comment

shashidharatd Oct 31, 2018

Choose a reason for hiding this comment

njuettner Oct 31, 2018

Choose a reason for hiding this comment

shashidharatd Oct 31, 2018 • edited Loading

Choose a reason for hiding this comment

shashidharatd left a comment

Choose a reason for hiding this comment

njuettner commented Nov 14, 2018

njuettner commented Nov 14, 2018

k8s-ci-robot commented Nov 14, 2018

shashidharatd commented Aug 22, 2018 •

edited

Loading

njuettner left a comment •

edited

Loading

jremond commented Oct 26, 2018 •

edited

Loading

shashidharatd commented Oct 26, 2018 •

edited

Loading

shashidharatd Oct 31, 2018 •

edited

Loading