federation: fix dns provider initialization issues #27252

mfanjie · 2016-06-12T09:54:43Z

This PR is based on the integration test with Google DNS API. This is the first time of full integration test.
So multiple issues was found and I combined all of them in this single PR

add dns provider initialization and add ensureDns call when removing federation service.
add new flags federation-name and zone-name to controller manager, both are used as part of the dns record name
fix assertion failure at rrsets.go#L61, which will cause panic
change getFederationDNSZoneName to get zoneName from config instead of hard code
change logic of ensureDnsRrsets, only add new dns record when endpointReachable(set to true when ready address is catched) is true
fix bug in processEndpointUpdate, only call ensuredns when ready address is caught
change behavior of syncService, there is cases that endpoint is created before ingress IP assignment, so before there is defect for this case, ensureDns was not called when service being updated, so if Ingress IP is assigned after endpoint ready address is caught, the corresponding A records can not be created
add a checking before update federation service

@nikhiljindal , can you help to add 1.3 milestone when @quinton-hoole is on leave?
Thanks.

k8s-bot · 2016-06-12T09:55:11Z

Can one of the admins verify that this patch is reasonable to test? If so, please reply "ok to test".
(Note: "add to whitelist" is no longer supported. Please update configurations in kubernetes/test-infra/jenkins/job-configs/kubernetes-jenkins-pull instead.)

This message may repeat a few times in short succession due to jenkinsci/ghprb-plugin#292. Sorry.

Otherwise, if this message is too spammy, please complain to ixdy.

mfanjie · 2016-06-12T09:55:14Z

@quinton-hoole @nikhiljindal

k8s-bot · 2016-06-12T09:56:11Z

Can one of the admins verify that this patch is reasonable to test? If so, please reply "ok to test".
(Note: "add to whitelist" is no longer supported. Please update configurations in kubernetes/test-infra/jenkins/job-configs/kubernetes-jenkins-pull instead.)

This message may repeat a few times in short succession due to jenkinsci/ghprb-plugin#292. Sorry.

Otherwise, if this message is too spammy, please complain to ixdy.

k8s-bot · 2016-06-12T09:56:30Z

Can one of the admins verify that this patch is reasonable to test? If so, please reply "ok to test".
(Note: "add to whitelist" is no longer supported. Please update configurations in kubernetes/test-infra/jenkins/job-configs/kubernetes-jenkins-pull instead.)

This message may repeat a few times in short succession due to jenkinsci/ghprb-plugin#292. Sorry.

Otherwise, if this message is too spammy, please complain to ixdy.

mfanjie · 2016-06-13T09:22:04Z

Updated test matrix, the expected result part.

ghost · 2016-06-13T18:12:33Z

Reviewing now...

nikhiljindal · 2016-06-13T18:31:32Z

federation/cmd/federation-controller-manager/app/options/options.go

+	// federation name.
+	FederationName string `json:"federationName"`
+	// zone name, like example.com.
+	ZoneName string `json:"zoneName"`


Which zone is this? The zone in which federation control plane is running?
k8s clusters could be in multiple zones.

nit: at other places we just use Zone (or Zones) rather than ZoneName

@nikhil this is the DNS zone name of the federation (see comment directly above). We could perhaps prefix it with "dns" to make that clearer. Zone or Zones usually refer to structs or interfaces, not plain names. Make sense?

nikhiljindal · 2016-06-13T18:40:10Z

@mfanjie Thanks for that great test matrix.
Is there a spreadsheet for it?

We can add more columns noting who is working on adding a test for each of them (@madhusudancs is adding a few in #26636)

ghost · 2016-06-13T19:15:06Z

federation/cmd/federation-controller-manager/app/options/options.go

@@ -90,6 +94,8 @@ func NewCMServer() *CMServer {
 func (s *CMServer) AddFlags(fs *pflag.FlagSet) {
 	fs.IntVar(&s.Port, "port", s.Port, "The port that the controller-manager's http service runs on")
 	fs.Var(componentconfig.IPVar{Val: &s.Address}, "address", "The IP address to serve on (set to 0.0.0.0 for all interfaces)")
+	fs.StringVar(&s.FederationName, "federation-name", s.FederationName, "Federation name.")
+	fs.StringVar(&s.ZoneName, "zone-name", s.ZoneName, "Zone name, like example.com.")


nit: I think that we should call this "DNSZoneName", "dns-zone-name" etc, to disambiguate it from an availability zone name. Comment/doc field should similarly include "DNS" to make it clearer.

ghost · 2016-06-13T20:03:27Z

@mfanjie Thanks! A few nits that can be addressed in a followup PR. I assume that you ran all of the tests in the attached test matrix manually and they passed? Adding LGTM to get this merged, as it's strictly an improvement over current HEAD, even if all tests are not passing consistently yet.

ghost · 2016-06-13T20:04:19Z

ok to test

nikhiljindal · 2016-06-13T22:32:56Z

Fixed the release note label (and the title) and added back LGTM

nikhiljindal · 2016-06-13T22:33:39Z

unit test seems to have failed to come up
@k8s-bot unit test this issue: #IGNORE

nikhiljindal · 2016-06-14T01:54:02Z

@k8s-bot node e2e test this issue: #IGNORE

(Logs say all tests passed)

mfanjie · 2016-06-14T02:14:17Z

I saw UT failure, fixing

mfanjie · 2016-06-14T05:10:05Z

fix two more issues:

the original fix solution for panic in rrsets.go#L61 will cause UT panic, by reviewing and testing more, the right solution is to change the List() function.
when zone info was added to cluster controller, and some changes were added to service controller, but the test case was not properly updated, this cause the ensureDNS function failed and kept retry, it took over 2.5 minutes to finish the UT. setup the UT correctly so that it can finish in ms level.

nikhiljindal · 2016-06-14T05:20:38Z

Thanks adding back LGTM

fejta · 2016-06-14T05:25:32Z

@k8s-bot node e2e test this issue #24479

mfanjie · 2016-06-14T06:05:04Z

I think @quinton-hoole has helped to answer most of the questions.
I have tested the matrix and all cases passed, and I have shared the spreadsheet by email.
The remaining comments are not function related from my perspective, so they can be addressed in later time.

nikhiljindal · 2016-06-14T07:08:11Z

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

@@ -242,6 +248,24 @@ func (s *ServiceController) Run(workers int, stopCh <-chan struct{}) error {
 	return nil
 }

+func (s *ServiceController) init() error {
+	if s.federationName == "" {
+		return fmt.Errorf("ServiceController should not be run without federationName.")


You will need to add these flags to the controller-manager YAML: https://github.com/kubernetes/kubernetes/blob/master/federation/manifests/federation-controller-manager-deployment.yaml#L22. Otherwise, the e2e tests will start failing as soon as this PR is merged.

You can use the FEDERATION_NAME env var for federation name:

kubernetes/federation/cluster/common.sh

Line 96 in 6e5faf5

export FEDERATION_NAME="${FEDERATION_NAME:-federation}"

You will have to add a similar env var for ZoneName

…creation 2. ensure dns record removal when service being removed

mfanjie · 2016-06-14T07:42:39Z

@nikhiljindal thanks for reminding, cloud you please help to review the last commit in this PR.

nikhiljindal · 2016-06-14T08:02:29Z

lgtm, thx

k8s-github-robot · 2016-06-14T09:13:53Z

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

k8s-bot · 2016-06-14T10:10:58Z

GCE e2e build/test passed for commit 8e26283.

k8s-github-robot · 2016-06-14T10:20:12Z

Automatic merge from submit-queue

googlebot added the cla: yes label Jun 12, 2016

k8s-github-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. release-note-label-needed labels Jun 12, 2016

k8s-github-robot assigned ghost Jun 12, 2016

mfanjie force-pushed the fix-ensuredns branch 2 times, most recently from 5a8a751 to e855199 Compare June 13, 2016 09:01

madhusudancs added this to the v1.3 milestone Jun 13, 2016

ghost added area/cluster-federation priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. labels Jun 13, 2016

nikhiljindal reviewed Jun 13, 2016
View reviewed changes

ghost reviewed Jun 13, 2016
View reviewed changes

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

k8s-github-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 13, 2016

mfanjie force-pushed the fix-ensuredns branch from bcfed44 to d88b8bd Compare June 14, 2016 01:19

k8s-github-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 14, 2016

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

mfanjie force-pushed the fix-ensuredns branch from d88b8bd to ca6367c Compare June 14, 2016 05:05

k8s-github-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 14, 2016

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

nikhiljindal reviewed Jun 14, 2016
View reviewed changes

mfanjie added 5 commits June 14, 2016 15:36

add dns provider initialzation

29cc7c0

add federation-name and zone-name as controller manager flags

318f37c

only update dns records when ready address available for endpoints

72a0806

1. ensure dns record when ingress ip is assigned after ready address …

dd78dd8

…creation 2. ensure dns record removal when service being removed

add federation-name and zone-name flags to e2e test

8e26283

mfanjie force-pushed the fix-ensuredns branch from ca6367c to 8e26283 Compare June 14, 2016 07:40

k8s-github-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 14, 2016

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

k8s-github-robot merged commit c7e3b2f into kubernetes:master Jun 14, 2016

mml mentioned this pull request Jun 14, 2016

Ubernetes e2e flakes #26762

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

federation: fix dns provider initialization issues #27252

federation: fix dns provider initialization issues #27252

mfanjie commented Jun 12, 2016 •

edited

Loading

k8s-bot commented Jun 12, 2016

mfanjie commented Jun 12, 2016

k8s-bot commented Jun 12, 2016

k8s-bot commented Jun 12, 2016

mfanjie commented Jun 13, 2016

ghost commented Jun 13, 2016

nikhiljindal Jun 13, 2016

nikhiljindal Jun 13, 2016

ghost Jun 13, 2016

nikhiljindal commented Jun 13, 2016

ghost Jun 13, 2016

ghost commented Jun 13, 2016

ghost commented Jun 13, 2016

nikhiljindal commented Jun 13, 2016

nikhiljindal commented Jun 13, 2016

nikhiljindal commented Jun 14, 2016 •

edited

Loading

mfanjie commented Jun 14, 2016

mfanjie commented Jun 14, 2016 •

edited

Loading

nikhiljindal commented Jun 14, 2016

fejta commented Jun 14, 2016

mfanjie commented Jun 14, 2016

nikhiljindal Jun 14, 2016

mfanjie commented Jun 14, 2016

nikhiljindal commented Jun 14, 2016

k8s-github-robot commented Jun 14, 2016

k8s-bot commented Jun 14, 2016

k8s-github-robot commented Jun 14, 2016

federation: fix dns provider initialization issues #27252

federation: fix dns provider initialization issues #27252

Conversation

mfanjie commented Jun 12, 2016 • edited Loading

k8s-bot commented Jun 12, 2016

mfanjie commented Jun 12, 2016

k8s-bot commented Jun 12, 2016

k8s-bot commented Jun 12, 2016

mfanjie commented Jun 13, 2016

ghost commented Jun 13, 2016

nikhiljindal Jun 13, 2016

Choose a reason for hiding this comment

nikhiljindal Jun 13, 2016

Choose a reason for hiding this comment

ghost Jun 13, 2016

Choose a reason for hiding this comment

nikhiljindal commented Jun 13, 2016

ghost Jun 13, 2016

Choose a reason for hiding this comment

ghost commented Jun 13, 2016

ghost commented Jun 13, 2016

nikhiljindal commented Jun 13, 2016

nikhiljindal commented Jun 13, 2016

nikhiljindal commented Jun 14, 2016 • edited Loading

mfanjie commented Jun 14, 2016

mfanjie commented Jun 14, 2016 • edited Loading

nikhiljindal commented Jun 14, 2016

fejta commented Jun 14, 2016

mfanjie commented Jun 14, 2016

nikhiljindal Jun 14, 2016

Choose a reason for hiding this comment

mfanjie commented Jun 14, 2016

nikhiljindal commented Jun 14, 2016

k8s-github-robot commented Jun 14, 2016

k8s-bot commented Jun 14, 2016

k8s-github-robot commented Jun 14, 2016

mfanjie commented Jun 12, 2016 •

edited

Loading

nikhiljindal commented Jun 14, 2016 •

edited

Loading

mfanjie commented Jun 14, 2016 •

edited

Loading