-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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 leader config for election events #1803
🐛 Use leader config for election events #1803
Conversation
|
Welcome @JustinKuli! |
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. |
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea! I've updated it to re-use the previous recorderProvider if options.LeaderElectionConfig
is nil.
84bd552
to
d905c2b
Compare
pkg/manager/manager.go
Outdated
leaderConfig = rest.CopyConfig(config) | ||
leaderRecorderProvider = recorderProvider | ||
} else { | ||
leaderConfig = options.LeaderElectionConfig |
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 |
Fixes: kubernetes-sigs#1798 Signed-off-by: Justin Kulikauskas <jkulikau@redhat.com>
d905c2b
to
b28fa4d
Compare
leaderConfig = rest.CopyConfig(config) | ||
leaderRecorderProvider = recorderProvider | ||
} else { | ||
leaderConfig = rest.CopyConfig(options.LeaderElectionConfig) |
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.
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
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.
thanks!
[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 |
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).