Integrate federated service DNS record management #25991

ghost · 2016-05-20T21:34:38Z

Integrate federated service DNS record management.

googlebot · 2016-05-20T21:34:46Z

We found a Contributor License Agreement for you (the sender of this pull request) and all commit authors, but as best as we can tell these commits were authored by someone else. If that's the case, please add them to this pull request and have them confirm that they're okay with these commits being contributed to Google. If we're mistaken and you did author these commits, just reply here to confirm.

ghost · 2016-05-20T21:38:36Z

cc @mfanjie

This PR is based off @mfanje's PR #25517
The last 4 commits are my changes, mostly confined to service/dns.go

bccbd47
8c09f63
226d675
229e28b

Unit tests still to come.

I'm not sure how best to get this merged. Presumably merge #25517 and then this?

madhusudancs · 2016-05-20T21:46:52Z

Yeah, I think it is better to get #25517 merged before this one, but that doesn't have to necessarily block reviews on this one. Is this ready for review yet?

Also, when it is ready for review, could you please squash your commits into a single commit? I can either see a full-PR-diff view which includes @mfanjie's changes. Or review your changes commit-by-commit but that's painful because you might have something in a commit which you might have fixed 3-commits later and I won't be sure if I have to write the comment now or wait until I see all the commits and go back. Github's PR views don't help here either. Squashing into a single commit makes this somewhat easier.

madhusudancs · 2016-05-20T21:47:21Z

Also, please let me know (via Hangouts/email) when this is ready for review.

mfanjie · 2016-05-21T02:42:10Z

@quinton-hoole I checked you changes, and one general comment:
as dns.go and other helper files are in same package, if you change (s *ServiceController) ensureDNSRecords to ensureDNSRecords in dns.go, then you will not need pass the service controller into func like syncEndpoint, unless you need fields of ServiceController.

madhusudancs · 2016-05-21T03:53:47Z

pkg/controller/service/servicecontroller_test.go

@@ -1,5 +1,5 @@
 /*
-Copyright 2015 The Kubernetes Authors All rights reserved.
+Copyright 2016 The Kubernetes Authors All rights reserved.


I don't think this should change. This file was committed/created in 2015.

ghost · 2016-05-22T04:45:08Z

Something got screwed up in the merge here, because I've fixed this. Will have to check and retry...

Boilerplate header is wrong for: /go/src/k8s.io/kubernetes/federation/pkg/dnsprovider/providers/google/clouddns/internal/stubs/service.go
FAILED   ./hack/../hack/verify-boilerplate.sh   1s

ghost · 2016-05-22T21:06:03Z

The above error should resolve itself once #26020 merges, and then this gets rebased against that. So this is blocked until #26020 (and #26034) merge and are rebased against.

ghost · 2016-05-23T05:09:28Z

@nikhiljindal FYI, as you're doing such a marvellous job of shepherding the cluster federation PR's through the pipeline.

googlebot · 2016-05-29T00:11:16Z

CLAs look good, thanks!

ghost · 2016-05-29T00:15:58Z

@madhusudancs @mfanjie OK, I finally got to rebase this against @26020 and #26034 which merged today. There is still a small amount of cleanup remaining, but unit tests at least pass. I split it into two commits, the first of which is the guts of the logic, and the second is what I had to do to rebase it against #26034. @mfanjie I broke the encapsulation of your helper classes rather embarrassingly to get this working as quickly as possible (see second commit), and thereby unblock integration testing. I'll take a second pass over this and improve on the encapsulation again. I preferred your original :-)

ghost · 2016-05-29T03:08:51Z

# k8s.io/kubernetes/federation/pkg/dnsprovider/providers/google/clouddns
federation/pkg/dnsprovider/providers/google/clouddns/clouddns_test.go:37: NewFakeInterface redeclared in this block
    previous declaration at federation/pkg/dnsprovider/providers/google/clouddns/clouddns.go:105

ghost · 2016-06-01T04:00:53Z

I'll fix this...

Verifying ./hack/../hack/verify-gofmt.sh
!!! 'gofmt -s -w' needs to be run on the following files: 
./federation/pkg/federation-controller/service/endpoint_helper_test.go
FAILED   ./hack/../hack/verify-gofmt.sh 13s

madhusudancs · 2016-06-01T05:35:33Z

@quinton-hoole with a squashed commit, I now don't know what changed between the last review and now. Going through the PR again, it will take a while.

ghost · 2016-06-01T05:51:44Z

My apologies @madhusudancs. For the most part I followed your advice exactly, the changes for which should be fairly self-evident from the comment above. I replied to each one of your feedback comments individually, so it should be pretty clear exactly what changed.

ghost · 2016-06-01T06:08:06Z

Regarding the above gofmt issue, my local gofmt passes that file with flying colors, so I'm not too sure what's going on. I'll have to take a deeper look in the morning.

$ gofmt -l -d endpoint_helper_test.go
$ echo $?
0

$ git log | head
commit f24912e93604bf98b7aa87817649e5897eb2e899
Author: Quinton Hoole <quinton@google.com>
Date:   Fri May 20 13:21:33 2016 -0700


    Integrate federated service DNS record management

commit 38d5be4f3659d0abb28c5e3e19f6467731852661
Merge: ee412ef 011eac7
Author: k8s-merge-robot <k8s.production.user@gmail.com>
Date:   Tue May 31 17:06:12 2016 -0700

$ /usr/local/go/bin/go version
go version go1.6 darwin/amd64

madhusudancs · 2016-06-01T06:19:37Z

The problem is in this line here - https://github.com/kubernetes/kubernetes/pull/25991/files#diff-2a1f013302ed85314e4e9dc18cd91305R101

Remove &clusterCache and this error should go away. Something like this:

        clientMap: map[string]*clusterCache{
            clusterName: {
                cluster: &federation.Cluster{

Also

$ /usr/local/go/bin/go version
go version go1.6 darwin/amd64

Is that the go version that your gofmt is picking up? (I mean which gofmt)

madhusudancs · 2016-06-01T06:25:45Z

federation/pkg/federation-controller/service/dns.go

+func (s *ServiceController) getClusterZoneNames(clusterName string) (zones []string, region string, err error) {
+	if client, ok := s.clusterCache.clientMap[clusterName]; !ok {
+		return nil, "", fmt.Errorf("Cluster cache does not contain entry for cluster %s", clusterName)
+	} else {


Why do you need an else here? if returns anyway.

Because of golang's variable scoping. client is in the scope of the if/else block. With the else removed, it doesn't compile (client would be undefined). An alternative would be to declare client outside of the address block, but that's way messier than the else statement (and requires explicit typing, which makes the code more brittle).

Fixed as per convention discussed below.

madhusudancs · 2016-06-01T10:54:20Z

@quinton-hoole finished the second pass. A few questions, some of them at logic/architecture level.

ghost · 2016-06-01T17:56:01Z

OK, I figured out the cause of the gofmt error. Locally I had not been passing in the -s "simplify" flag:

$ gofmt -d -l endpoint_helper_test.go 
$ echo $?
0

$ gofmt -d -l -s endpoint_helper_test.go 
endpoint_helper_test.go
diff endpoint_helper_test.go gofmt/endpoint_helper_test.go
--- /var/folders/00/16c7r000h01000cxqpysvccm004shz/T/gofmt521183572 2016-06-01 10:48:22.000000000 -0700
+++ /var/folders/00/16c7r000h01000cxqpysvccm004shz/T/gofmt043372195 2016-06-01 10:48:22.000000000 -0700
@@ -98,7 +98,7 @@
    clusterName := "foo"
    cc := clusterClientCache{
        clientMap: map[string]*clusterCache{
-           clusterName: &clusterCache{
+           clusterName: {
                cluster: &federation.Cluster{
                    Status: federation.ClusterStatus{
                        Zones:  []string{"foozone"},

ghost · 2016-06-01T22:02:49Z

Thanks @madhusudancs. Addressed all feedback. PTAL.

madhusudancs · 2016-06-01T22:10:40Z

federation/pkg/federation-controller/service/dns.go

+   and returns a list of IPv4 addresses.  If any of the endpoints are neither valid IPv4 addresses nor resolvable DNS names,
+   non-nil error is also returned (possibly along with a partially complete list of resolved endpoints.
+*/
+func getResolvedEndpoints(endpoints []string) ([]string, error) {


Thanks for extracting this into a function!

madhusudancs · 2016-06-01T22:22:18Z

@quinton-hoole Only one outstanding comment. Looks good otherwise since some of the comments will be addressed in the follow up PRs. However, thanks for addressing most of the comments. Please feel free to apply the LGTM label after addressing the only remaining comment.

ghost · 2016-06-01T23:23:30Z

@madhusudancs Thanks. I pushed an additional commit to fix your last comment. Will squash all commits and add LGTM now.

k8s-bot · 2016-06-02T00:21:06Z

GCE e2e build/test passed for commit 80e13ef798a83e1e4bf1548d520e5f7496cc71b4.

k8s-github-robot · 2016-06-02T01:51:11Z

@k8s-bot test this [submit-queue is verifying that this PR is safe to merge]

k8s-bot · 2016-06-02T02:44:12Z

GCE e2e build/test passed for commit 65e1fec.

k8s-github-robot · 2016-06-02T02:47:55Z

Automatic merge from submit-queue

ghost added priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. team/control-plane labels May 20, 2016

ghost added this to the v1.3 milestone May 20, 2016

ghost assigned madhusudancs May 20, 2016

googlebot added the cla: no label May 20, 2016

k8s-github-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. release-note-label-needed labels May 20, 2016

ghost mentioned this pull request May 21, 2016

[WIP]Fed service controller #25517

Closed

madhusudancs reviewed May 21, 2016
View reviewed changes

ghost added release-note-none Denotes a PR that doesn't merit a release note. cla: human-approved and removed release-note-label-needed labels May 27, 2016

k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 28, 2016

googlebot added the cla: yes label May 29, 2016

k8s-github-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels May 29, 2016

madhusudancs reviewed Jun 1, 2016
View reviewed changes

ghost mentioned this pull request Jun 1, 2016

AWS Route53 dnsprovider #26049

Merged

madhusudancs reviewed Jun 1, 2016
View reviewed changes

Integrate federated service DNS record management

65e1fec

ghost added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 1, 2016

k8s-github-robot merged commit 9f520cf into kubernetes:master Jun 2, 2016

ghost mentioned this pull request Nov 11, 2016

Integrate federated service DNS record management: Address remaining code review comments. #26669

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Integrate federated service DNS record management #25991

Integrate federated service DNS record management #25991

ghost commented May 20, 2016

googlebot commented May 20, 2016

ghost commented May 20, 2016

madhusudancs commented May 20, 2016

madhusudancs commented May 20, 2016

mfanjie commented May 21, 2016

madhusudancs May 21, 2016

ghost commented May 22, 2016

ghost commented May 22, 2016 •

edited by ghost

Loading

ghost commented May 23, 2016

googlebot commented May 29, 2016

ghost commented May 29, 2016 •

edited by ghost

Loading

ghost commented May 29, 2016

ghost commented Jun 1, 2016

madhusudancs commented Jun 1, 2016

ghost commented Jun 1, 2016

ghost commented Jun 1, 2016

madhusudancs commented Jun 1, 2016 •

edited

Loading

madhusudancs Jun 1, 2016

ghost Jun 1, 2016

ghost Jun 1, 2016

madhusudancs commented Jun 1, 2016

ghost commented Jun 1, 2016

ghost commented Jun 1, 2016

madhusudancs Jun 1, 2016

madhusudancs commented Jun 1, 2016

ghost commented Jun 1, 2016

k8s-bot commented Jun 2, 2016

k8s-github-robot commented Jun 2, 2016

k8s-bot commented Jun 2, 2016

k8s-github-robot commented Jun 2, 2016

Integrate federated service DNS record management #25991

Integrate federated service DNS record management #25991

Conversation

ghost commented May 20, 2016

googlebot commented May 20, 2016

ghost commented May 20, 2016

madhusudancs commented May 20, 2016

madhusudancs commented May 20, 2016

mfanjie commented May 21, 2016

madhusudancs May 21, 2016

Choose a reason for hiding this comment

ghost commented May 22, 2016

ghost commented May 22, 2016 • edited by ghost Loading

ghost commented May 23, 2016

googlebot commented May 29, 2016

ghost commented May 29, 2016 • edited by ghost Loading

ghost commented May 29, 2016

ghost commented Jun 1, 2016

madhusudancs commented Jun 1, 2016

ghost commented Jun 1, 2016

ghost commented Jun 1, 2016

madhusudancs commented Jun 1, 2016 • edited Loading

madhusudancs Jun 1, 2016

Choose a reason for hiding this comment

ghost Jun 1, 2016

Choose a reason for hiding this comment

ghost Jun 1, 2016

Choose a reason for hiding this comment

madhusudancs commented Jun 1, 2016

ghost commented Jun 1, 2016

ghost commented Jun 1, 2016

madhusudancs Jun 1, 2016

Choose a reason for hiding this comment

madhusudancs commented Jun 1, 2016

ghost commented Jun 1, 2016

k8s-bot commented Jun 2, 2016

k8s-github-robot commented Jun 2, 2016

k8s-bot commented Jun 2, 2016

k8s-github-robot commented Jun 2, 2016

ghost commented May 22, 2016 •

edited by ghost

Loading

ghost commented May 29, 2016 •

edited by ghost

Loading

madhusudancs commented Jun 1, 2016 •

edited

Loading