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.

Sign up for GitHub

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

Jump to bottom

Gossip backed DNS #2327

Merged
merged 5 commits into from
Apr 25, 2017
Merged

Gossip backed DNS #2327

merged 5 commits into from
Apr 25, 2017

Conversation

justinsb
Copy link
Member

@justinsb justinsb commented Apr 8, 2017 •
edited
Loading

Checklist:

  • Gate with feature flag using .k8s.internal extension as feature-flag
  • Use stable format for gossip using protobuf
  • Get weave mesh PR merged upstream so we're not on a fork done
  • Add AWS support (it's in a separate branch) done
  • Split out the LB support again I think to make the PR more reviewable
  • Review the CRDT logic - it relies on timestamps currently good enough for now IMO; we can switch now we are versioned & weave uses timestamps

This change is Reviewable

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Apr 8, 2017
@justinsb justinsb force-pushed the gossip_dns branch 6 times, most recently from 4d746a2 to 51f2012 Compare April 10, 2017 03:59
@justinsb justinsb changed the title WIP: Gossip backed DNS Gossip backed DNS Apr 10, 2017
@justinsb justinsb mentioned this pull request Apr 10, 2017
Closed
@justinsb justinsb force-pushed the gossip_dns branch 3 times, most recently from 0b62938 to bb74fc8 Compare April 10, 2017 04:32
@justinsb justinsb changed the title Gossip backed DNS WIP Gossip backed DNS Apr 10, 2017
@justinsb justinsb force-pushed the gossip_dns branch 3 times, most recently from 8f65c12 to 685cda0 Compare April 19, 2017 16:42
@justinsb justinsb changed the title WIP Gossip backed DNS Gossip backed DNS Apr 19, 2017
chrislovecnm
chrislovecnm reviewed Apr 21, 2017
View reviewed changes
Copy link
Contributor

@chrislovecnm chrislovecnm left a comment

Choose a reason for hiding this comment

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

Looks awesome. How are we testing this? Can we set the timeout? Is dnscontroller the right place for this? I am wondering if we want a seperate controller. Just thinking to the future when we replace dnscontroller.... if we do.

protokube/pkg/gossip/dns/provider/rrset.go
"k8s.io/kubernetes/federation/pkg/dnsprovider/rrstype"
)

const defaultTTL = 60
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this 60 seconds?

Copy link
Contributor

Choose a reason for hiding this comment

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

It is below... it is seconds. Can we override?

Copy link
Member Author

Choose a reason for hiding this comment

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

We have to return something, so we use 1 minute. There are no TTLs in /etc/hosts

@justinsb
Copy link
Member Author

So far I've tested this manually i.e. by bringing up a cluster.

dnscontroller is a client of dnsprovider, and we have a new dnsprovider that talks to the gossip state. We could use the same dnsprovider with external-dns.

The writing to /etc/hosts is done by protokube. It also writes a few hosts to the gossip state.

I'll rebase :-)

@justinsb
Copy link
Member Author

Plan for real testing is to test GCE using this, without needing a DNS name

chrislovecnm
chrislovecnm reviewed Apr 21, 2017
View reviewed changes
Copy link
Contributor

@chrislovecnm chrislovecnm left a comment

Choose a reason for hiding this comment

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

More questions.. This is a MONDO PR. I need some more time to process and re-read again.

dns-controller/cmd/dns-controller/main.go Outdated

id := os.Getenv("HOSTNAME")
if id == "" {
glog.Warningf("Unable to fetch HOSTNAME for use as node identifier")
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this a warning or a failure?

Copy link
Member Author

Choose a reason for hiding this comment

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

Apparently a warning :-)

nodeup/pkg/model/kubeproxy.go
@@ -190,6 +191,11 @@ func (b *KubeProxyBuilder) buildPod() (*v1.Pod, error) {
sslCertsHost.MountPath = "/etc/ssl/certs"
}

if dns.IsGossipHostname(b.Cluster.Name) {
// Map /etc/hosts from host, so that we see the updates that are made by protokube
addHostPathMapping(pod, container, "etchosts", "/etc/hosts")
Copy link
Contributor

Choose a reason for hiding this comment

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

Are we adding a comment to /etc/hosts that we are maintaining it? I am not sure if we are adding comments to all of the files we have. This would be an issue that I would file as a separate enhancement.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes we do add a comment

protokube/pkg/gossip/dns/dns.go Outdated
Records map[string]DNSRecord
}

func (s *DNSViewSnapshot) RecordsForZone(zoneInfo DNSZoneInfo) []DNSRecord {
Copy link
Contributor

Choose a reason for hiding this comment

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

Code comment ...pretty please

@chrislovecnm
Copy link
Contributor

I will need to take a third pass ...

chrislovecnm
chrislovecnm reviewed Apr 21, 2017
View reviewed changes
Copy link
Contributor

@chrislovecnm chrislovecnm left a comment

Choose a reason for hiding this comment

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

Question about documentation :)

docs/development/gossip.md
## DNS

* We implement a dnsprovider backed by our local gossip state
* We write to `/etc/hosts`; this is sort of hacky but avoids the need for a custom local resolver
Copy link
Contributor

Choose a reason for hiding this comment

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

How do we launch this? How do we debug this? Do we have other user documentation?

Copy link
Member Author

Choose a reason for hiding this comment

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

Use a .k8s.local internal name; you'll also need to use api: loadBalancer: {} as we need a way for kubelet to reach apiserver. More docs to follow once it is more baked.

@chrislovecnm
Copy link
Contributor

Let me see if I can fork this and test in GCE next week :)

@chrislovecnm chrislovecnm mentioned this pull request Apr 23, 2017
Closed
@krisnova krisnova mentioned this pull request Apr 25, 2017
Closed
@justinsb
Copy link
Member Author

I'll fix the 2 problems @chrislovecnm

chrislovecnm
chrislovecnm approved these changes Apr 25, 2017
View reviewed changes
Copy link
Contributor

@chrislovecnm chrislovecnm left a comment

Choose a reason for hiding this comment

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

awesomesauce

@chrislovecnm chrislovecnm merged commit 1eb9949 into kubernetes:master Apr 25, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@chrislovecnm chrislovecnm chrislovecnm approved these changes

Assignees
No one assigned
Labels
blocks-next cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@justinsb @chrislovecnm @k8s-ci-robot