Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add etcd SRV discovery support #1086

Merged
merged 2 commits into from May 28, 2019

Conversation

Projects
None yet
3 participants
@jsok
Copy link
Contributor

commented May 23, 2019

Description

Adds the configuration options to support specifying a domain name that
the etcd SRV discovery library will use to enumerate the current member
endpoints.

Not having this capability is becoming more inconvenient when running calico-node and the calico-kube-controllers via Kubernetes.

Another attempt at getting #388 updated and merged.

Todos

  • Tests
  • Documentation
  • Release note

Release Note

Adds [etcd DNS discovery](https://coreos.com/etcd/docs/latest/op-guide/clustering.html#dns-discovery) support for specifying etcdv3 endpoint(s).

@jsok jsok force-pushed the jsok:etcd-srv branch 2 times, most recently from f19715d to e09708c May 23, 2019

@caseydavenport
Copy link
Member

left a comment

@jsok thanks for the PR - this looks good to me, though I haven't tried it out.

Is there a good way to test this?

@jsok

This comment has been minimized.

Copy link
Contributor Author

commented May 24, 2019

@caseydavenport implementing a functional test would be pretty involved:

  • Start a DNS server (CoreDNS is a good candidate) with a static zone config specify the SRV records (make run-coredns)
  • Somehow set the make fv test container's DNS server, probably via docker run --dns=... ?
  • Write a [Datastore] test case for etcdv3

The code and functionality is tested fairly well in the etcd test suite:

I'd be inclined to trust that the implementation is sound - but yes, my plumbing of the SRV client to the endpoint enumeration in the calico client is still untested from a functional perspective.

@caseydavenport

This comment has been minimized.

Copy link
Member

commented May 24, 2019

I'd be inclined to trust that the implementation is sound - but yes, my plumbing of the SRV client to the endpoint enumeration in the calico client is still untested from a functional perspective.

Yeah, I agree - I assume you have at least tested this yourself for real? My only worry is that we'll not have a way to make sure this keeps working, or resolve any issues found in it.

Add etcd SRV discovery support
Adds the configuration options to support specifying a domain name that
the etcd SRV discovery library will use to enumerate the current member
endpoints.

https://coreos.com/etcd/docs/latest/op-guide/clustering.html#dns-discovery

@jsok jsok force-pushed the jsok:etcd-srv branch 2 times, most recently from d5b25c2 to 5577bac May 27, 2019

Add functional tests for etcd SRV discovery
Use CoreDNS to serve the zone `etcd.local` which contains the SRV records
for the etcd client service. This points to the non-TLS instance of etcd
running on port 2379.

Adds tests to ensure the etcdv3 client handles SRV discovery correctly.

@jsok jsok force-pushed the jsok:etcd-srv branch from 5577bac to e24e921 May 27, 2019

@jsok

This comment has been minimized.

Copy link
Contributor Author

commented May 27, 2019

@caseydavenport I've implemented the functional tests now to help maintain this feature. Please re-review when ready.

@jsok

This comment has been minimized.

Copy link
Contributor Author

commented May 27, 2019

If this gets merged I'll prepare subsequent PRs for:

  • Documentation
  • cni-plugin
  • kube-controllers
@caseydavenport
Copy link
Member

left a comment

@jsok awesome, thanks for the extra effort on the tests :D

LGTM

@caseydavenport

This comment has been minimized.

Copy link
Member

commented May 28, 2019

Our CI should automatically update the pins in downstream repos overnight, so shouldn't be a need to submit manual updates for that.

@caseydavenport caseydavenport merged commit 5015d7d into projectcalico:master May 28, 2019

2 checks passed

license/cla Contributor License Agreement is signed.
Details
semaphoreci The build passed on Semaphore.
Details

@caseydavenport caseydavenport added this to the Calico v3.8.0 milestone May 28, 2019

@jsok jsok deleted the jsok:etcd-srv branch May 28, 2019

jsok added a commit to jsok/kube-controllers that referenced this pull request May 29, 2019

Add etcd SRV discovery support
etcdDiscoverySrv option added to configuration in
projectcalico/libcalico-go#1086

jsok added a commit to jsok/calico that referenced this pull request Jun 3, 2019

Add etcd DNS discovery documentation
Documents functionality added in
projectcalico/libcalico-go#1086

The capability of discovering etcd members via SRV records has been
added to:
 * calicoctl
 * cni-plugin
 * kube-controllers

@jsok jsok referenced this pull request Jun 3, 2019

Merged

Add etcd DNS discovery documentation #2643

3 of 3 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.