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

Federation service controller #26034

Merged
1 commit merged into from
May 28, 2016

Conversation

mfanjie
Copy link

@mfanjie mfanjie commented May 22, 2016

Federation service controller is one key component of federation controller manager, it watches federation service, creates/updates services to all registered clusters, and update DNS records to global DNS server.

Analytics

@k8s-bot
Copy link

k8s-bot commented May 22, 2016

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.

2 similar comments
@k8s-bot
Copy link

k8s-bot commented May 22, 2016

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
Copy link

k8s-bot commented May 22, 2016

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
Copy link
Author

mfanjie commented May 22, 2016

@quinton-hoole I created this PR based on #26020, when you finish code review on the wip #26034, I will update all changes on both branches, so we can be ready for code merge.

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

ghost commented May 22, 2016

ok to test

@ghost ghost added this to the v1.3 milestone May 22, 2016
@ghost ghost self-assigned this May 22, 2016
@ghost ghost added the priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. label May 22, 2016
@ghost
Copy link

ghost commented May 22, 2016

Thanks @mfanjie !

@ghost
Copy link

ghost commented May 22, 2016

Fixes #23848

@ghost
Copy link

ghost commented May 22, 2016

Still a few minor govet and flags issues to resolve, then LGTM.

FAILED   ./hack/../hack/verify-gofmt.sh 9s
Verifying ./hack/../hack/verify-govet.sh
federation/pkg/federation-controller/service/endpoint_helper.go:38: range variable clusterCache captured by func literal
federation/pkg/federation-controller/service/endpoint_helper.go:43: range variable clusterCache captured by func literal
federation/pkg/federation-controller/service/endpoint_helper.go:44: range variable clusterName captured by func literal
federation/pkg/federation-controller/service/endpoint_helper.go:44: range variable clusterCache captured by func literal
federation/pkg/federation-controller/service/service_helper.go:41: range variable clusterCache captured by func literal
federation/pkg/federation-controller/service/service_helper.go:46: range variable clusterCache captured by func literal
federation/pkg/federation-controller/service/service_helper.go:47: range variable clusterName captured by func literal
federation/pkg/federation-controller/service/service_helper.go:47: range variable clusterCache captured by func literal
federation/pkg/federation-controller/service/servicecontroller.go:291: range variable clusterName captured by func literal
federation/pkg/federation-controller/service/servicecontroller.go:291: range variable cluster captured by func literal
federation/pkg/federation-controller/service/endpoint_helper_test.go:32: k8s.io/kubernetes/pkg/api/v1.EndpointAddress composite literal uses unkeyed fields
federation/pkg/federation-controller/service/service_helper_test.go:32: k8s.io/kubernetes/pkg/api/v1.LoadBalancerIngress composite literal uses unkeyed fields
federation/pkg/federation-controller/service/service_helper_test.go:56: k8s.io/kubernetes/pkg/api/v1.ServiceStatus composite literal uses unkeyed fields
federation/pkg/federation-controller/service/service_helper_test.go:64: k8s.io/kubernetes/pkg/api/v1.ServiceStatus composite literal uses unkeyed fields
federation/pkg/federation-controller/service/service_helper_test.go:66: k8s.io/kubernetes/pkg/api/v1.LoadBalancerIngress composite literal uses unkeyed fields
federation/pkg/federation-controller/service/service_helper_test.go:69: k8s.io/kubernetes/pkg/api/v1.ServiceStatus composite literal uses unkeyed fields
federation/pkg/federation-controller/service/service_helper_test.go:81: k8s.io/kubernetes/pkg/api/v1.LoadBalancerIngress composite literal uses unkeyed fields
federation/pkg/federation-controller/service/service_helper_test.go:84: k8s.io/kubernetes/pkg/api/v1.ServiceStatus composite literal uses unkeyed fields
federation/pkg/federation-controller/service/service_helper_test.go:92: k8s.io/kubernetes/pkg/api/v1.ServiceStatus composite literal uses unkeyed fields
federation/pkg/federation-controller/service/service_helper_test.go:97: k8s.io/kubernetes/pkg/api/v1.ServiceStatus composite literal uses unkeyed fields
federation/pkg/federation-controller/service/service_helper_test.go:129: k8s.io/kubernetes/pkg/api/v1.ServiceStatus composite literal uses unkeyed fields
federation/pkg/federation-controller/service/service_helper_test.go:131: k8s.io/kubernetes/pkg/api/v1.LoadBalancerIngress composite literal uses unkeyed fields
federation/pkg/federation-controller/service/service_helper_test.go:134: k8s.io/kubernetes/pkg/api/v1.ServiceStatus composite literal uses unkeyed fields
federation/pkg/federation-controller/service/service_helper_test.go:142: k8s.io/kubernetes/pkg/api/v1.ServiceStatus composite literal uses unkeyed fields
federation/pkg/federation-controller/service/service_helper_test.go:148: k8s.io/kubernetes/pkg/api/v1.ServiceStatus composite literal uses unkeyed fields
FAILED   ./hack/../hack/verify-govet.sh 89s
Verifying ./hack/../hack/verify-flags-underscore.py
Found flags in golang files not in the list of known flags. Please add these to hack/verify-flags/known-flags.txt
concurrent-service-syncs
FAILED   ./hack/../hack/verify-flags-underscore.py  5s

@ghost ghost added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-label-needed labels May 22, 2016
@mfanjie mfanjie force-pushed the federation-service-controller branch from 5f44379 to 6b589e3 Compare May 23, 2016 04:11
@mfanjie
Copy link
Author

mfanjie commented May 23, 2016

resolved above problems and squashed commit.

@mfanjie mfanjie force-pushed the federation-service-controller branch from 6b589e3 to 032e0ab Compare May 23, 2016 04:17
@nikhiljindal
Copy link
Contributor

verify-govet is still failing. From logs:

Verifying ./hack/../hack/verify-govet.sh
federation/pkg/federation-controller/service/endpoint_helper_test.go:32: k8s.io/kubernetes/pkg/api/v1.EndpointAddress composite literal uses unkeyed fields
federation/pkg/federation-controller/service/service_helper_test.go:31: k8s.io/kubernetes/pkg/api/v1.LoadBalancerIngress composite literal uses unkeyed fields
FAILED   ./hack/../hack/verify-govet.sh

@ghost
Copy link

ghost commented May 23, 2016

@mfanjie govet is still not quite happy

Verifying ./hack/../hack/verify-govet.sh
federation/pkg/federation-controller/service/endpoint_helper_test.go:32: k8s.io/kubernetes/pkg/api/v1.EndpointAddress composite literal uses unkeyed fields
federation/pkg/federation-controller/service/service_helper_test.go:31: k8s.io/kubernetes/pkg/api/v1.LoadBalancerIngress composite literal uses unkeyed fields
FAILED   ./hack/../hack/verify-govet.sh 101s

@ghost
Copy link

ghost commented May 23, 2016

Snap :-)

@mfanjie
Copy link
Author

mfanjie commented May 23, 2016

acked, it is strange that I can not get these two errors when I ran govet.sh on my laptop...
@quinton-hoole One thing I am not sure about it that should we use internalclient instead of release_1_3 as you discussed in the wip PR.
I am trying to make that change and seems I got no error with internalclient, I will leave it as is now.

@mfanjie mfanjie force-pushed the federation-service-controller branch from 032e0ab to 25f292d Compare May 23, 2016 05:20
@mfanjie
Copy link
Author

mfanjie commented May 23, 2016

pushed again, I will monitor the e2e result.

s.queue.Add(key)
}

// Run starts a background goroutine that watches for changes to Federated services
Copy link
Contributor

Choose a reason for hiding this comment

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

federation services

@nikhiljindal
Copy link
Contributor

Still trying to understand the code. Have added a few comments.

@nikhiljindal
Copy link
Contributor

cc @kubernetes/sig-cluster-federation

@mfanjie mfanjie force-pushed the federation-service-controller branch from f535db0 to 9d67f9d Compare May 27, 2016 02:32
@mfanjie
Copy link
Author

mfanjie commented May 27, 2016

@nikhiljindal all your comments addressed.

@ghost
Copy link

ghost commented May 27, 2016

LGTM! Needs to rebase against upstream/maser once #26020 merges (hopefully within the next few hours at most). Then ready for merge, once checks all pass.

@ghost ghost added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 27, 2016
@ghost
Copy link

ghost commented May 28, 2016

@mfanjie If you rebase this I should be able to merge it now.

@mfanjie mfanjie force-pushed the federation-service-controller branch from 9d67f9d to 15afaa7 Compare May 28, 2016 15:30
@mfanjie
Copy link
Author

mfanjie commented May 28, 2016

@quinton-hoole rebased and pushed

@k8s-github-robot k8s-github-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 28, 2016
@mfanjie
Copy link
Author

mfanjie commented May 28, 2016

failed for one checking, working on it.

In addition, I found some error on clustercontroller.go, even it is not related to this PR but it impacts the cluster status, so we have to manually update the cluster status to make service controller working properly, opened issue #26480 .

@mfanjie mfanjie force-pushed the federation-service-controller branch from 089325a to 6133db3 Compare May 28, 2016 16:25
@ghost
Copy link

ghost commented May 28, 2016

Thanks @mfanjie. As soon as the checks pass, I'll merge this.

@k8s-bot
Copy link

k8s-bot commented May 28, 2016

GCE e2e build/test passed for commit 089325a529d5888e119de6cd0f5f8c3aeca0cf90.

@ghost ghost added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 28, 2016
@ghost ghost added the ok-to-merge label May 28, 2016
@ghost
Copy link

ghost commented May 28, 2016

All checks have passed. Merge queue is blocked, so manually merging, as this is a v1.3 blocker, with several other blocker PR's waiting for it to merge.

@ghost ghost merged commit eff728a into kubernetes:master May 28, 2016
@mfanjie
Copy link
Author

mfanjie commented Jun 8, 2016

this is a sub task of #23653 , add linkage.

// dnsProvider is the provider for dns services.
DnsProvider string `json:"dnsProvider"`
// dnsConfigFile is the path to the dns provider configuration file.
DnsConfigFile string `json:"ndsConfigFile"`
Copy link

Choose a reason for hiding this comment

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

@quinton-hoole @mfanjie ndsConfigFile probably should be dnsConfigFile

(still exists on master)

Copy link
Contributor

Choose a reason for hiding this comment

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

You are right.
Being fixed in #27158

Though the json field name is not useful yet.

This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lgtm "Looks good to me", indicates that a PR is ready to be merged. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants