Migrate to client-go #539

aledbf · 2017-04-01T14:52:52Z

replaces #463

k8s-reviewable · 2017-04-01T14:53:58Z

This change is

coveralls · 2017-04-01T15:17:35Z

Coverage increased (+0.3%) to 44.084% when pulling 19c727f on aledbf:migrate-client-go into 638ea2b on kubernetes:master.

nicksardo · 2017-04-03T22:37:00Z

@aledbf There are a lot of gaps in my k8s knowledge outside of the GLBC and services, so please be patient with me on this. We can also try pinging @bprashanth if he has any spare time to help overlook this. (Not a problem if he doesn't)

First, have you tried testing a simple gce use case with this branch?
Second, I see you're vendoring client-go version 2.0. Did you make any changes that would break compatibility with k8s 1.4? If not, can you spin up a 1.4 cluster and test?
Third, I see the leaderelection package has been moved locally. What's the reasoning for this? Was it dropped from all other repos?

Thanks

aledbf · 2017-04-03T22:57:09Z

First, have you tried testing a simple gce use case with this branch?

Yes but I don't know if there is an additional e2e test for the gce ingress controller outside this repo.

Second, I see you're vendoring client-go version 2.0. Did you make any changes that would break compatibility with k8s 1.4? If not, can you spin up a 1.4 cluster and test?

From the compatibility matrix this should not be an issue

Third, I see the leaderelection package has been moved locally. What's the reasoning for this? Was it dropped from all other repos?

The issue here is that the leader election code is not (yet) present in client-go and we rely on this feature for the ingress status update (the gce ingress controller does not uses this code)

aledbf · 2017-04-03T22:57:53Z

@bprashanth can you help to review this PR? (the gce part)

nicksardo · 2017-04-03T23:02:32Z

@bprashanth can you help to review this PR? (the gce part)

He can look at the GCE changes, but I was actually concerned about core/pkg/... as I'm not familiar with it.

From the compatibility matrix this should not be an issue

The compatibility matrix says client-go contains items that may not exist in the cluster. I was asking if you were only using things in common.

nicksardo · 2017-04-03T23:05:49Z

Also, being able to cherrypick kubernetes/kubernetes#43644 into 1.6.x is looking less likely and 1.7 is months out. Although not ideal, I feel it would be innocuous to vendor from commit 289ef62442b2e74e533e2b0f1e309c70750d7423. Pinging @thockin to provide his input on this.

nicksardo · 2017-04-03T23:36:54Z

controllers/gce/controller/controller_test.go

@@ -52,8 +52,9 @@ func defaultBackendName(clusterName string) string {

 // newLoadBalancerController create a loadbalancer controller.
 func newLoadBalancerController(t *testing.T, cm *fakeClusterManager, masterURL string) *LoadBalancerController {


masterURL is declared but not used.

nicksardo · 2017-04-03T23:37:14Z

controllers/gce/controller/controller_test.go

-	client := client.NewForConfigOrDie(&restclient.Config{Host: masterURL, ContentConfig: restclient.ContentConfig{GroupVersion: testapi.Default.GroupVersion()}})
-	lb, err := NewLoadBalancerController(client, cm.ClusterManager, 1*time.Second, api.NamespaceAll)
+	kubeClient := fake.NewSimpleClientset()
+	//ContentConfig: restclient.ContentConfig{GroupVersion: testapi_v1.Default.GroupVersion()}


Was this kept for a reason?

thockin · 2017-04-04T00:03:15Z

Yeah, it seems untenable to have ingress depend on kubernetes but on vendor stable release versions - doing anything that moves them forward in tandem is problematic. In the end, cloud provider stuff will be moved out of kubernetes tree anyway..

…

On Mon, Apr 3, 2017 at 4:05 PM, Nick Sardo ***@***.***> wrote: Also, being able to cherrypick kubernetes/kubernetes#43644 <kubernetes/kubernetes#43644> into 1.6.1 is looking less likely and 1.7 is months out. Although not ideal, I feel it would be innocuous to vendor from commit 289ef62442b2e74e533e2b0f1e309c70750d7423. Pinging @thockin <https://github.com/thockin> to provide his input on this. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#539 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AFVgVL37hzGmrzx4xc2JyPfE3vTUSXNdks5rsXtPgaJpZM4Mwgq8> .

coveralls · 2017-04-04T15:20:27Z

Coverage increased (+0.3%) to 44.341% when pulling 533c10dcbacdca90d6b0d56bcb6ef8569ea6c625 on aledbf:migrate-client-go into 22c3226 on kubernetes:master.

aledbf · 2017-04-04T15:22:59Z

@thockin which version of client-go should be used?

thockin · 2017-04-04T16:11:48Z

I think client-go can be more stable - it changes less often, and generally major changes in client are related to major changes in server..

…

On Tue, Apr 4, 2017 at 8:23 AM, Manuel Alejandro de Brito Fontes < ***@***.***> wrote: @thockin <https://github.com/thockin> which version of client-go should be used? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#539 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AFVgVD12-ydYjWxextxsA6r-71nEk8pfks5rsmBUgaJpZM4Mwgq8> .

aledbf · 2017-04-04T16:15:29Z

@thockin sorry, I meant which branch of the repository kubernetes/client-go should be used (this PR is now using the master branch)

thockin · 2017-04-04T16:17:47Z

We should use whichever branch is stable - as of a few weeks ago there was a proposal to make `master` be stable and do dev in another branch. Unless there's a reason to pull a non-stable version of that?

…

On Tue, Apr 4, 2017 at 9:15 AM, Manuel Alejandro de Brito Fontes < ***@***.***> wrote: @thockin <https://github.com/thockin> sorry, I meant which branch of the repository kubernetes/client-go should be used (this PR is now using the master branch) — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#539 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AFVgVI4fkmBMAA-emiaAbupkFxj8ICcxks5rsmyjgaJpZM4Mwgq8> .

aledbf · 2017-04-04T19:13:20Z

Although not ideal, I feel it would be innocuous to vendor from commit 289ef62442b2e74e533e2b0f1e309c70750d7423

@nicksardo done

coveralls · 2017-04-04T19:36:50Z

Coverage increased (+0.3%) to 44.383% when pulling 88a2751 on aledbf:migrate-client-go into 0fe0d6f on kubernetes:master.

nicksardo · 2017-04-04T21:37:13Z

@aledbf Is there a required configuration change to the manifest file in k8s? The controller crashes at start in an e2e cluster.

F0404 20:56:41.281794       5 main.go:201] error creating client configuration: invalid configuration: default cluster has no server defined

nicksardo · 2017-04-04T22:43:08Z

controllers/gce/main.go

-		config, err = clientConfig.ClientConfig()
+		config, err = clientcmd.NewNonInteractiveDeferredLoadingClientConfig(
+			&clientcmd.ClientConfigLoadingRules{},
+			&clientcmd.ConfigOverrides{}).ClientConfig()


Now that clientcmd.ClusterDefault is being deprecated, I assume we should either specify the master address via (new) flag or assume in-cluster (similar to nginx controller). I think we can make this change - will need to remember this when bumping the manifest file.

coveralls · 2017-04-05T13:36:01Z

Coverage increased (+0.3%) to 44.383% when pulling e492a4b on aledbf:migrate-client-go into 0fe0d6f on kubernetes:master.

nicksardo · 2017-04-05T20:11:59Z

Changes in core/pkg/... look good.

I've ran one successful test on a 1.6 cluster with the new flag. After doing a test on a 1.4 cluster, I'll merge this.

I'll update controller docs regarding the new flag(s) in a separate PR.

nicksardo · 2017-04-05T20:50:07Z

No problems with 1.4
/lgtm

nicksardo · 2017-04-05T20:50:33Z

Thanks for updating the vendored k8s!

aledbf · 2017-04-05T20:51:57Z

@nicksardo thank you for merging :)

aledbf added backend/gce nginx labels Apr 1, 2017

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Apr 1, 2017

aledbf assigned nicksardo Apr 3, 2017

aledbf requested a review from bprashanth April 3, 2017 22:57

nicksardo reviewed Apr 3, 2017

View reviewed changes

aledbf added 2 commits April 4, 2017 11:51

Update gce controller

c7c2a56

Update nginx and generic controller

e0561dd

aledbf force-pushed the migrate-client-go branch from 19c727f to 533c10d Compare April 4, 2017 14:53

Update go dependencies

88a2751

aledbf force-pushed the migrate-client-go branch from 533c10d to 88a2751 Compare April 4, 2017 19:13

nicksardo reviewed Apr 4, 2017

View reviewed changes

Add flag to specify the api server url

e492a4b

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 5, 2017

nicksardo merged commit 12a0373 into kubernetes:master Apr 5, 2017

aledbf deleted the migrate-client-go branch April 6, 2017 01:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate to client-go #539

Migrate to client-go #539

aledbf commented Apr 1, 2017 •

edited

Loading

k8s-reviewable commented Apr 1, 2017

coveralls commented Apr 1, 2017

nicksardo commented Apr 3, 2017 •

edited

Loading

aledbf commented Apr 3, 2017

aledbf commented Apr 3, 2017

nicksardo commented Apr 3, 2017 •

edited

Loading

nicksardo commented Apr 3, 2017 •

edited

Loading

nicksardo Apr 3, 2017

nicksardo Apr 3, 2017

thockin commented Apr 4, 2017 via email

coveralls commented Apr 4, 2017

aledbf commented Apr 4, 2017

thockin commented Apr 4, 2017 via email

aledbf commented Apr 4, 2017

thockin commented Apr 4, 2017 via email

aledbf commented Apr 4, 2017 •

edited

Loading

coveralls commented Apr 4, 2017

nicksardo commented Apr 4, 2017

nicksardo Apr 4, 2017 •

edited

Loading

aledbf Apr 5, 2017

coveralls commented Apr 5, 2017

nicksardo commented Apr 5, 2017

nicksardo commented Apr 5, 2017

nicksardo commented Apr 5, 2017

aledbf commented Apr 5, 2017

		@@ -52,8 +52,9 @@ func defaultBackendName(clusterName string) string {

		// newLoadBalancerController create a loadbalancer controller.
		func newLoadBalancerController(t testing.T, cm fakeClusterManager, masterURL string) *LoadBalancerController {

Migrate to client-go #539

Migrate to client-go #539

Conversation

aledbf commented Apr 1, 2017 • edited Loading

k8s-reviewable commented Apr 1, 2017

coveralls commented Apr 1, 2017

nicksardo commented Apr 3, 2017 • edited Loading

aledbf commented Apr 3, 2017

aledbf commented Apr 3, 2017

nicksardo commented Apr 3, 2017 • edited Loading

nicksardo commented Apr 3, 2017 • edited Loading

nicksardo Apr 3, 2017

Choose a reason for hiding this comment

nicksardo Apr 3, 2017

Choose a reason for hiding this comment

thockin commented Apr 4, 2017 via email

coveralls commented Apr 4, 2017

aledbf commented Apr 4, 2017

thockin commented Apr 4, 2017 via email

aledbf commented Apr 4, 2017

thockin commented Apr 4, 2017 via email

aledbf commented Apr 4, 2017 • edited Loading

coveralls commented Apr 4, 2017

nicksardo commented Apr 4, 2017

nicksardo Apr 4, 2017 • edited Loading

Choose a reason for hiding this comment

aledbf Apr 5, 2017

Choose a reason for hiding this comment

coveralls commented Apr 5, 2017

nicksardo commented Apr 5, 2017

nicksardo commented Apr 5, 2017

nicksardo commented Apr 5, 2017

aledbf commented Apr 5, 2017

aledbf commented Apr 1, 2017 •

edited

Loading

nicksardo commented Apr 3, 2017 •

edited

Loading

nicksardo commented Apr 3, 2017 •

edited

Loading

nicksardo commented Apr 3, 2017 •

edited

Loading

aledbf commented Apr 4, 2017 •

edited

Loading

nicksardo Apr 4, 2017 •

edited

Loading