Use ObjectGetter Interface instead of clientset.Interface for leaderelection pkg #44338

shashidharatd · 2017-04-11T13:34:16Z

What this PR does / why we need it:
We plan to reuse leaderelection pkg to add leader election function to federation controller manager, but the current implementation uses kubernetes clientset.Interface and federation clientset does not satisfy all the interface methods. It would be better if the leaderelection package use rest.Interface which is also supported by federation clientset.
This pr is to refactor leaderelection pkg to use rest.Interface instead of clientset.Interface

Special notes for your reviewer:
This is a sub-task of bigger work to add leader election to federation controller manager as documented in #44283

Release note:

NONE

k8s-reviewable · 2017-04-11T13:34:24Z

This change is

mikedanese · 2017-04-12T15:38:44Z

These seems wrong. We are subverting all the query param and path versioning built into the client with this change. Why aren't you using a typed client? cc @deads2k @caesarxuchao what do you guys think?

deads2k · 2017-04-12T16:21:58Z

cc @deads2k @caesarxuchao what do you guys think?

I would expect a typed client. Maybe you just want a configmap getter?

caesarxuchao · 2017-04-12T16:38:59Z

pkg/client/leaderelection/resourcelock/endpointslock.go

@@ -60,15 +68,21 @@ func (el *EndpointsLock) Create(ler LeaderElectionRecord) error {
 	if err != nil {
 		return err
 	}
-	el.e, err = el.Client.Core().Endpoints(el.EndpointsMeta.Namespace).Create(&v1.Endpoints{


Is Endpoints missed in federation clientset? You can generate it by adding endpoints at this line. I'm not sure if the generated client will actually work, you might need to register it to the federation's scheme as well, but it's worth a try.

shashidharatd · 2017-04-12T16:50:24Z

Thanks for the review. i would get back on the suggestions.
meanwhile i also request @nikhiljindal, who is an expert on federation api's and the clientset to comment on this.

shashidharatd · 2017-04-13T11:07:36Z

cc @kubernetes/sig-federation-api-reviews

zhouhaibing089 · 2017-04-14T08:38:01Z

@deads2k agree with your point ConfigmapGetter(though I believe not only getter)

shashidharatd · 2017-04-14T10:53:19Z

Thank you all for suggestions. Now i used ConfigMapsGetter for ConfigMapLock and EndpointsGetter for EndpointsLock. it worked out pretty well. @mikedanese, @deads2k PTAL

However we do have a situation in federation wherein i am not able to use the ConfigMapGetter from federation clientset. I am getting below error:

# k8s.io/kubernetes/federation/cmd/federation-controller-manager/app
federation/cmd/federation-controller-manager/app/controllermanager.go:163: cannot use leaderElectionClient.CoreV1() (type "k8s.io/kubernetes/federation/client/clientset_generated/federation_clientset/typed/core/v1".CoreV1Interface) as type "k8s.io/kubernetes/pkg/client/clientset_generated/clientset/typed/core/v1".ConfigMapsGetter in field value:
	"k8s.io/kubernetes/federation/client/clientset_generated/federation_clientset/typed/core/v1".CoreV1Interface does not implement "k8s.io/kubernetes/pkg/client/clientset_generated/clientset/typed/core/v1".ConfigMapsGetter (wrong type for ConfigMaps method)
		have ConfigMaps(string) "k8s.io/kubernetes/federation/client/clientset_generated/federation_clientset/typed/core/v1".ConfigMapInterface
		want ConfigMaps(string) "k8s.io/kubernetes/pkg/client/clientset_generated/clientset/typed/core/v1".ConfigMapInterface

I would open a new issue to fix the compatibility issue in federation clientset.
@nikhiljindal @caesarxuchao, do you have any suggestions

shashidharatd · 2017-04-14T10:59:18Z

The issue i mentioned above might get solved if we start using "k8s.io/client-go/kubernetes/typed/core/v1" pkg for corev1 client.

luxas · 2017-04-16T15:36:14Z

pkg/client/leaderelection/resourcelock/endpointslock.go

@@ -23,14 +23,14 @@ import (

 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/kubernetes/pkg/api/v1"
-	"k8s.io/kubernetes/pkg/client/clientset_generated/clientset"
+	corev1client "k8s.io/kubernetes/pkg/client/clientset_generated/clientset/typed/core/v1"


might be out of scope, but why don't we use client-go here?
Would that require a big "switch-all" to client-go?

yes @luxas, it requires big "switch-all" to client-go and is out of scope for this pr.

It will be done in #39173.

mikedanese · 2017-04-17T18:26:19Z

Thanks, lgtm. Please squash.

caesarxuchao · 2017-04-17T18:46:58Z

I'm not familiar with federation. Are the non-federation resources available on a federation apiserver? If so, we can plumb a clientset to the federation control plane. @nikhiljindal wdyt?

nikhiljindal · 2017-04-17T19:56:57Z

Are the non-federation resources available on a federation apiserver

Not today. We want to support that #33622.

caesarxuchao · 2017-04-17T20:22:37Z

pkg/client/leaderelection/resourcelock/configmaplock.go

@@ -35,7 +35,7 @@ type ConfigMapLock struct {
 	// ConfigMapMeta should contain a Name and a Namespace of an
 	// ConfigMapMeta object that the Leadercmlector will attempt to lead.
 	ConfigMapMeta metav1.ObjectMeta
-	Client        clientset.Interface
+	Client        corev1client.ConfigMapsGetter


Can we pass corev1client.ConfigMapInterface around, i.e., passing Client.ConfigMaps(ConfigMapMeta.Namespace) around? It looks like federation.ConfigMapInterface has the same methods as corev1client.ConfigMapInterface, so you can reuse the leaderelection code for federation.

@caesarxuchao, even though ConfigMapInterface is the same, as they are generated. they are 2 different types today as is evident from below error i got when i passed federation corev1 client.

# k8s.io/kubernetes/federation/cmd/federation-controller-manager/app federation/cmd/federation-controller-manager/app/controllermanager.go:163: cannot use leaderElectionClient.CoreV1() (type "k8s.io/kubernetes/federation/client/clientset_generated/federation_clientset/typed/core/v1".CoreV1Interface) as type "k8s.io/kubernetes/pkg/client/clientset_generated/clientset/typed/core/v1".ConfigMapsGetter in field value: "k8s.io/kubernetes/federation/client/clientset_generated/federation_clientset/typed/core/v1".CoreV1Interface does not implement "k8s.io/kubernetes/pkg/client/clientset_generated/clientset/typed/core/v1".ConfigMapsGetter (wrong type for ConfigMaps method) have ConfigMaps(string) "k8s.io/kubernetes/federation/client/clientset_generated/federation_clientset/typed/core/v1".ConfigMapInterface want ConfigMaps(string) "k8s.io/kubernetes/pkg/client/clientset_generated/clientset/typed/core/v1".ConfigMapInterface

we should start using client-go, then these types will be unique

we should start using client-go, then these types will be unique

I don't think so, client-go doesn't include any federation code.

I saw this error message, it's saying federation corev1 cannot be used as clientset's ConfigMapsGetter, because although the two definitions of ConfigMapInterface are the same, to the compiler, they are two different types.

However, if you pass federation.ConfigMapInterface directly, compiler should accept it as a clientset.ConfigMapInterface, because they have the same methods defined.

I didn't try it though, so i could be wrong :)

Okie, i will try. thanks for suggestion.

shashidharatd · 2017-04-17T23:52:06Z

@mikedanese, commits squashed. PTAL

shashidharatd · 2017-04-18T00:11:31Z

@k8s-bot bazel test this

mikedanese · 2017-04-18T16:22:47Z

@k8s-bot bazel test this

mikedanese · 2017-04-18T16:24:41Z

/lgtm

shashidharatd · 2017-04-18T18:01:49Z

@davidopp, can you approve if it looks good please

…lection pkg

shashidharatd · 2017-04-19T14:40:50Z

@k8s-bot cvm gce e2e test this

shashidharatd · 2017-04-19T15:52:51Z

@mikedanese, realized that the consumers of leaderelection can also pass clientset interface for Client which is a superset of corev1 client interface. So updated this pr to just contain the changes to leaderelection package. PTAL and request a LGTM once again.

shashidharatd · 2017-04-24T12:28:24Z

@mikedanese, gentle reminder. This PR is smaller now :) request an lgtm again please.
Thanks !

mikedanese · 2017-05-02T17:42:30Z

Sorry I missed this!

/lgtm

k8s-github-robot · 2017-05-02T17:47:49Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mikedanese, shashidharatd

Needs approval from an approver in each of these OWNERS Files:

~~pkg/client/leaderelection/OWNERS~~ [mikedanese]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

k8s-github-robot · 2017-05-02T19:45:56Z

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

k8s-github-robot · 2017-05-02T20:03:57Z

Automatic merge from submit-queue (batch tested with PRs 40544, 44338, 45225)

k8s-ci-robot · 2017-05-02T20:52:53Z

@shashidharatd: The following test(s) failed:

Test name	Commit	Details	Rerun command
Jenkins GCE etcd3 e2e	`e23f1d5`	link	`@k8s-bot gce etcd3 e2e test this`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

shashidharatd · 2017-05-02T21:10:22Z

Thanks @mikedanese

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

k8s-github-robot assigned ingvagabund and luxas Apr 11, 2017

k8s-github-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. release-note-none Denotes a PR that doesn't merit a release note. labels Apr 11, 2017

shashidharatd mentioned this pull request Apr 11, 2017

[Federation] Support leader election for controller manager #44283

Closed

4 tasks

shashidharatd force-pushed the leader-election branch from 2c8455d to f86570f Compare April 11, 2017 14:35

caesarxuchao reviewed Apr 12, 2017

View reviewed changes

shashidharatd force-pushed the leader-election branch from f86570f to dc7f454 Compare April 14, 2017 10:45

k8s-github-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Apr 14, 2017

shashidharatd changed the title ~~Use rest.Interface instead of clientset.Interface for leaderelection pkg~~ Use ObjectGetter Interface instead of clientset.Interface for leaderelection pkg Apr 14, 2017

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

luxas reviewed Apr 16, 2017

View reviewed changes

shashidharatd force-pushed the leader-election branch from dc7f454 to 616594b Compare April 17, 2017 03:43

k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 17, 2017

caesarxuchao reviewed Apr 17, 2017

View reviewed changes

shashidharatd force-pushed the leader-election branch from 616594b to ef13a4c Compare April 17, 2017 23:51

shashidharatd mentioned this pull request Apr 18, 2017

Bazel test failure - Server terminated abruptly #44580

Closed

k8s-ci-robot assigned mikedanese Apr 18, 2017

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

Use ObjectGetter Interface instead of clientset.Interface for leadere…

e23f1d5

…lection pkg

shashidharatd force-pushed the leader-election branch from ef13a4c to e23f1d5 Compare April 19, 2017 13:09

luxas removed their assignment Apr 20, 2017

deads2k mentioned this pull request Apr 24, 2017

controller: enable leader election kubernetes-retired/service-catalog#647

Merged

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

k8s-github-robot merged commit 932dabd into kubernetes:master May 2, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use ObjectGetter Interface instead of clientset.Interface for leaderelection pkg #44338

Use ObjectGetter Interface instead of clientset.Interface for leaderelection pkg #44338

shashidharatd commented Apr 11, 2017

k8s-reviewable commented Apr 11, 2017

mikedanese commented Apr 12, 2017

deads2k commented Apr 12, 2017

caesarxuchao Apr 12, 2017

shashidharatd commented Apr 12, 2017

shashidharatd commented Apr 13, 2017

zhouhaibing089 commented Apr 14, 2017

shashidharatd commented Apr 14, 2017

shashidharatd commented Apr 14, 2017

luxas Apr 16, 2017

shashidharatd Apr 17, 2017

caesarxuchao Apr 17, 2017 •

edited

mikedanese commented Apr 17, 2017

caesarxuchao commented Apr 17, 2017

nikhiljindal commented Apr 17, 2017

caesarxuchao Apr 17, 2017

shashidharatd Apr 17, 2017

caesarxuchao Apr 17, 2017

shashidharatd Apr 17, 2017

shashidharatd commented Apr 17, 2017

shashidharatd commented Apr 18, 2017

mikedanese commented Apr 18, 2017

mikedanese commented Apr 18, 2017

shashidharatd commented Apr 18, 2017

shashidharatd commented Apr 19, 2017

shashidharatd commented Apr 19, 2017

shashidharatd commented Apr 24, 2017

mikedanese commented May 2, 2017

k8s-github-robot commented May 2, 2017

k8s-github-robot commented May 2, 2017

k8s-github-robot commented May 2, 2017

k8s-ci-robot commented May 2, 2017

shashidharatd commented May 2, 2017

Use ObjectGetter Interface instead of clientset.Interface for leaderelection pkg #44338

Use ObjectGetter Interface instead of clientset.Interface for leaderelection pkg #44338

Conversation

shashidharatd commented Apr 11, 2017

k8s-reviewable commented Apr 11, 2017

mikedanese commented Apr 12, 2017

deads2k commented Apr 12, 2017

caesarxuchao Apr 12, 2017

Choose a reason for hiding this comment

shashidharatd commented Apr 12, 2017

shashidharatd commented Apr 13, 2017

zhouhaibing089 commented Apr 14, 2017

shashidharatd commented Apr 14, 2017

shashidharatd commented Apr 14, 2017

luxas Apr 16, 2017

Choose a reason for hiding this comment

shashidharatd Apr 17, 2017

Choose a reason for hiding this comment

caesarxuchao Apr 17, 2017 • edited

Choose a reason for hiding this comment

mikedanese commented Apr 17, 2017

caesarxuchao commented Apr 17, 2017

nikhiljindal commented Apr 17, 2017

caesarxuchao Apr 17, 2017

Choose a reason for hiding this comment

shashidharatd Apr 17, 2017

Choose a reason for hiding this comment

caesarxuchao Apr 17, 2017

Choose a reason for hiding this comment

shashidharatd Apr 17, 2017

Choose a reason for hiding this comment

shashidharatd commented Apr 17, 2017

shashidharatd commented Apr 18, 2017

mikedanese commented Apr 18, 2017

mikedanese commented Apr 18, 2017

shashidharatd commented Apr 18, 2017

shashidharatd commented Apr 19, 2017

shashidharatd commented Apr 19, 2017

shashidharatd commented Apr 24, 2017

mikedanese commented May 2, 2017

k8s-github-robot commented May 2, 2017

k8s-github-robot commented May 2, 2017

k8s-github-robot commented May 2, 2017

k8s-ci-robot commented May 2, 2017

shashidharatd commented May 2, 2017

caesarxuchao Apr 17, 2017 •

edited