-
Notifications
You must be signed in to change notification settings - Fork 38.7k
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
Use ObjectGetter Interface instead of clientset.Interface for leaderelection pkg #44338
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. might be out of scope, but why don't we use client-go here? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yes @luxas, it requires big "switch-all" to client-go and is out of scope for this pr. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It will be done in #39173. |
||
) | ||
|
||
type EndpointsLock struct { | ||
// EndpointsMeta should contain a Name and a Namespace of an | ||
// Endpoints object that the LeaderElector will attempt to lead. | ||
EndpointsMeta metav1.ObjectMeta | ||
Client clientset.Interface | ||
Client corev1client.EndpointsGetter | ||
LockConfig ResourceLockConfig | ||
e *v1.Endpoints | ||
} | ||
|
@@ -39,7 +39,7 @@ type EndpointsLock struct { | |
func (el *EndpointsLock) Get() (*LeaderElectionRecord, error) { | ||
var record LeaderElectionRecord | ||
var err error | ||
el.e, err = el.Client.Core().Endpoints(el.EndpointsMeta.Namespace).Get(el.EndpointsMeta.Name, metav1.GetOptions{}) | ||
el.e, err = el.Client.Endpoints(el.EndpointsMeta.Namespace).Get(el.EndpointsMeta.Name, metav1.GetOptions{}) | ||
if err != nil { | ||
return nil, err | ||
} | ||
|
@@ -60,7 +60,7 @@ 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{ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is Endpoints missed in federation clientset? You can generate it by adding |
||
el.e, err = el.Client.Endpoints(el.EndpointsMeta.Namespace).Create(&v1.Endpoints{ | ||
ObjectMeta: metav1.ObjectMeta{ | ||
Name: el.EndpointsMeta.Name, | ||
Namespace: el.EndpointsMeta.Namespace, | ||
|
@@ -82,7 +82,7 @@ func (el *EndpointsLock) Update(ler LeaderElectionRecord) error { | |
return err | ||
} | ||
el.e.Annotations[LeaderElectionRecordAnnotationKey] = string(recordBytes) | ||
el.e, err = el.Client.Core().Endpoints(el.EndpointsMeta.Namespace).Update(el.e) | ||
el.e, err = el.Client.Endpoints(el.EndpointsMeta.Namespace).Update(el.e) | ||
return err | ||
} | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
we should start using client-go, then these types will be unique
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okie, i will try. thanks for suggestion.