Skip to content
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.

Sign up for GitHub

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

Jump to bottom

🐛Add corev1, coordinationv1 scheme for leader election when LeaderElection manager option is true #2461

Merged
merged 1 commit into from Aug 31, 2023
Merged

🐛Add corev1, coordinationv1 scheme for leader election when LeaderElection manager option is true #2461

merged 1 commit into from Aug 31, 2023

Conversation

troy0820
Copy link
Member

  • Add corev1 coordinationv1 scheme to cluster scheme to manager when LeaderElection manager option is true.
  • Adjust unit test to see if the scheme recognizes the GroupVersionKind of the resources that were causing noisy logs

Resolves #2378

None

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Aug 30, 2023
@sbueringer
Copy link
Member

/ok-to-test

@k8s-ci-robot k8s-ci-robot added the ok-to-test Indicates a non-member PR verified by an org member that is safe to test. label Aug 30, 2023
@k8s-ci-robot
Copy link
Contributor

Hi @troy0820. 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 /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

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 k8s-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Aug 30, 2023
alvaroaleman
alvaroaleman reviewed Aug 30, 2023
View reviewed changes
pkg/manager/manager.go Outdated
@@ -353,7 +356,10 @@ func New(config *rest.Config, options Options) (Manager, error) {
leaderRecorderProvider = recorderProvider
} else {
leaderConfig = rest.CopyConfig(options.LeaderElectionConfig)
leaderRecorderProvider, err = options.newRecorderProvider(leaderConfig, cluster.GetHTTPClient(), cluster.GetScheme(), options.Logger.WithName("events"), options.makeBroadcaster)
scheme := cluster.GetScheme()
utilruntime.Must(corev1.AddToScheme(scheme))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lets please return an error if this fails rather than using the Must funcs that will panic on error

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Returned error instead of utilruntime.Must which panics on failure.

@troy0820
add corev1, coordinationv1 scheme for leader election when LeaderElec…
e92e5ee 
…tion option is true

Signed-off-by: Troy Connor <troy0820@users.noreply.github.com>
@troy0820 troy0820 force-pushed the troy0820/add-types-to-leader-election branch from b07acc7 to e92e5ee Compare August 30, 2023 22:43
@troy0820
Copy link
Member Author

/retest

alvaroaleman
alvaroaleman approved these changes Aug 31, 2023
View reviewed changes
Copy link
Member

@alvaroaleman alvaroaleman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 31, 2023
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alvaroaleman, troy0820

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 31, 2023
@k8s-ci-robot k8s-ci-robot merged commit c1d71fc into kubernetes-sigs:main Aug 31, 2023
7 checks passed
@sbueringer
Copy link
Member

/cherry-pick release-0.16

@k8s-infra-cherrypick-robot
Copy link

@sbueringer: #2461 failed to apply on top of branch "release-0.16":

Applying: add corev1, coordinationv1 scheme for leader election when LeaderElection option is true
Using index info to reconstruct a base tree...
M	pkg/manager/manager_test.go
Falling back to patching base and 3-way merge...
Auto-merging pkg/manager/manager_test.go
CONFLICT (content): Merge conflict in pkg/manager/manager_test.go
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 add corev1, coordinationv1 scheme for leader election when LeaderElection option is true
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

In response to this:

/cherry-pick release-0.16

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.

@sbueringer
Copy link
Member

Feel free to backport if you want. I'll go ahead with v0.16.1 for now to get the otel dependency fix out

@troy0820
Copy link
Member Author

No problem, I'll make the PR right now. @sbueringer

@troy0820 troy0820 mentioned this pull request Aug 31, 2023
Merged
2 tasks
@troy0820
Copy link
Member Author

@sbueringer Opened #2466

@sbueringer
Copy link
Member

Thx!

@kcp-ci-bot kcp-ci-bot mentioned this pull request Feb 29, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alvaroaleman alvaroaleman alvaroaleman approved these changes

@inteon inteon Awaiting requested review from inteon

@varshaprasad96 varshaprasad96 Awaiting requested review from varshaprasad96

Assignees

@alvaroaleman alvaroaleman

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Noisy errors in leader election lease type registration in scheme
5 participants
@troy0820 @sbueringer @k8s-ci-robot @k8s-infra-cherrypick-robot @alvaroaleman