-
Notifications
You must be signed in to change notification settings - Fork 532
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
Delete Useless options in controller #569
Conversation
Hi @czybjtu. 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. |
hi @czybjtu, thanks for the PR, but maybe we should implement those options instead of deleting them WDYT @Huang-Wei ? |
} | ||
schedInformerFactory.Start(stopCh) | ||
coreInformerFactory.Start(stopCh) | ||
if !s.EnableLeaderElection { |
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.
this seems to be totally disabling leader election?
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.
No. No need to implement this manually, the ctrl runtime can handle this instead. We have enabled leader election when create ctrl.NewManager
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.
scheduler-plugins/cmd/controller/app/server.go
Lines 93 to 100 in aeb7ff2
mgr, err := ctrl.NewManager(ctrl.GetConfigOrDie(), ctrl.Options{ | |
Scheme: scheme, | |
MetricsBindAddress: s.MetricsAddr, | |
Port: 9443, | |
HealthProbeBindAddress: s.ProbeAddr, | |
LeaderElection: s.EnableLeaderElection, | |
LeaderElectionID: "sched-plugins-controllers", | |
LeaderElectionNamespace: "kube-system", |
Yes, we need to bring the functionality back, which is part of the migration; otherwise, it's a breaking change as params like workers, leader elections are not supported. @czybjtu are you going to remove and add the param/function back, via 2 PRs? If so, I'm leaning towards merging into one PR, so reviewers only need to review function changes while keep parameters unchanged (or with minimum changes). |
AFAIK, after we migrate controllers to controller-runtime, there is no need to implement |
hi @czybjtu, sorry for the late reply again, I was kinda busy recently. my concern is that:
ps, I only glanced at the PR, will try to find some free cycles to check it deeply. |
/ok-to-test I'm not concerned with the features that didn't exist before, we can keep the behavior as-is as long as we can confirm some parameters didn't work (like masters). For kubeconfig -> kubeConfig and changes alike, I'm less concerned as well. Let's just stick to ctrl-runtime's naming conversion and write a release note about that. |
/test pull-scheduler-plugins-integration-test-master |
@@ -173,5 +175,6 @@ func (r *ElasticQuotaReconciler) SetupWithManager(mgr ctrl.Manager) error { | |||
return ctrl.NewControllerManagedBy(mgr). | |||
Watches(&source.Kind{Type: &v1.Pod{}}, &handler.EnqueueRequestForObject{}). | |||
For(&schedv1alpha1.ElasticQuota{}). | |||
WithOptions(controller.Options{MaxConcurrentReconciles: r.Workers}). |
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 for adding this.
/approve Leaving /lgtm to @zwpaper . |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: czybjtu, Huang-Wei 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 |
/lgtm thanks for the contribution @czybjtu! |
What type of PR is this?
/kind cleanup
What this PR does / why we need it:
Which issue(s) this PR fixes:
Fixes #567
Special notes for your reviewer:
Does this PR introduce a user-facing change?