🐛 Use leader config for election events #1803
Conversation
|
|
k8s-ci-robot
added
the
cncf-cla: no
|
Welcome @JustinKuli! |
k8s-ci-robot
added
the
needs-ok-to-test
|
Hi @JustinKuli. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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. |
k8s-ci-robot
added
the
size/XS
k8s-ci-robot
added
cncf-cla: yes
pkg/manager/manager.go
Outdated
| @@ -339,7 +339,13 @@ func New(config *rest.Config, options Options) (Manager, error) { | |||
| if leaderConfig == nil { | |||
| leaderConfig = rest.CopyConfig(config) | |||
| } | |||
| resourceLock, err := options.newResourceLock(leaderConfig, recorderProvider, leaderelection.Options{ | |||
|
|
|||
| leaderRecorderProvider, err := options.newRecorderProvider(leaderConfig, cluster.GetScheme(), options.Logger.WithName("events"), options.makeBroadcaster) | |||
There was a problem hiding this comment.
Can we new this leaderRecorderProvider only if options.LeaderElectionConfig is not nil? Otherwise it should be redundant to recorderProvider.
There was a problem hiding this comment.
Good idea! I've updated it to re-use the previous recorderProvider if options.LeaderElectionConfig is nil.
JustinKuli
force-pushed
the
leader-election-events
branch
from
84bd552
to
d905c2b
Compare
k8s-ci-robot
added
size/S
pkg/manager/manager.go
Outdated
| leaderConfig = rest.CopyConfig(config) | ||
| leaderRecorderProvider = recorderProvider | ||
| } else { | ||
| leaderConfig = options.LeaderElectionConfig |
There was a problem hiding this comment.
Please also copy the config here, the leader election manipulates it down the line and we don't want that to get propagated anywhere else
There was a problem hiding this comment.
I'm not sure what you mean. If given, that config would only be used for the leader election, so I don't see where manipulating it would be an issue.
I've updated the PR to copy it, since that's also not a problem. I'm just curious if you can provide some more information to help me learn.
|
/ok-to-test |
k8s-ci-robot
added
ok-to-test
Fixes: kubernetes-sigs#1798 Signed-off-by: Justin Kulikauskas <jkulikau@redhat.com>
JustinKuli
force-pushed
the
leader-election-events
branch
from
d905c2b
to
b28fa4d
Compare
| leaderConfig = rest.CopyConfig(config) | ||
| leaderRecorderProvider = recorderProvider | ||
| } else { | ||
| leaderConfig = rest.CopyConfig(options.LeaderElectionConfig) |
There was a problem hiding this comment.
The config is a pointer to a *rest.Config. We set some fields down the line for User-Agent and similiar which means we manipulate the value behind the pointer, so it appears in the original config, i.E.:
leCfg := GetConfig()
mgr.New(cfg, manager.Options{LeaderElectionConfig: leCfg}
// If we don't do the CopyConfig, the User-Agent in the leCfg got changed
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: alvaroaleman, JustinKuli The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
approved
Fixes: #1798
Signed-off-by: Justin Kulikauskas jkulikau@redhat.com
This creates a new Recorder for the leader election events (using the leader config, if provided).