-
Notifications
You must be signed in to change notification settings - Fork 39k
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
Set leader-elect for kube-scheduler to true / Adds new configuration types and rework how we set defaults #59732
Set leader-elect for kube-scheduler to true / Adds new configuration types and rework how we set defaults #59732
Conversation
/assign @timothysc |
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.
/lgtm
/approve
cmd/kube-scheduler/app/server.go
Outdated
@@ -150,6 +150,7 @@ func NewOptions() (*Options, error) { | |||
return nil, err | |||
} | |||
|
|||
o.config.LeaderElection.LeaderElect = true |
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.
seems like defaults should be applied in defaults.go, not here (and options round-tripped through those defaults)
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 was trying to match what is done in kube-controller ... will fix https://github.com/kubernetes/kubernetes/blob/master/cmd/cloud-controller-manager/app/options/options.go#L52
/hold |
why is this default different than all the other defaults for the scheduler config, applied in the api defaulting path? the setting here won't get applied when reading from file |
e1f6935
to
d85f8df
Compare
@liggitt looks like we need to do both ... cmd/kube-scheduler/app/server.go and pkg/apis/componentconfig/v1alpha1/defaults.go if i don't patch up server.go, then the command line default is still false. |
d85f8df
to
0f2280a
Compare
that means the internal config is not being round tripped and defaulted... that seems like it should be fixed as well |
@liggitt can you point me to an existing pattern/files where this is done please? |
for kubeproxy: kubernetes/cmd/kube-proxy/app/server.go Lines 303 to 343 in fdeaa8c
for kubelet: kubernetes/cmd/kubelet/app/options/options.go Lines 255 to 268 in fdeaa8c
|
0f2280a
to
630f6e1
Compare
Thanks @liggitt looks like it was already setup. but there was a mismatch in |
/milestone clear |
we merged a small change and pushing this PR off to v1.11 |
a86e3f7
to
b68dbf8
Compare
[MILESTONENOTIFIER] Milestone Removed From Pull Request @deads2k @dims @mtaufen @timothysc Important: This pull request was missing labels required for the v1.11 milestone for more than 3 days: kind: Must specify exactly one of |
@liggitt now that master is open and it's early in the cycle, let's please get this in. |
// LeaderElectionConfiguration defines the configuration of leader election | ||
// clients for components that can run with leader election enabled. | ||
type LeaderElectionConfiguration struct { | ||
metav1.TypeMeta `json:",inline"` |
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.
Why does this need type info? It's not a top level type, right?
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.
@liggitt some of the tests fail without it
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.
unless this is serialized in standalone way (which I'm not aware of), this should not have type info. tests that fail without that should be fixed.
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.
ack removing it
b68dbf8
to
512b3b8
Compare
Ready and all green now @liggitt . thanks! |
|
||
// +k8s:deepcopy-gen:interfaces=k8s.io/apimachinery/pkg/runtime.Object | ||
|
||
type KubeControllerManagerConfiguration struct { |
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.
these should probably end up in controllermanager.config.k8s.io
under pkg/controller/apis
, similar to kubelet. can you move them in a follow up?
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.
yes i can
512b3b8
to
ba2778b
Compare
Thanks to some great sleuthing by ikruglov! kube-controller-manager defaults --leader-elect to true. We should do the same for kube-scheduler. kube-scheduler used to have this set to true, but it got lost during refactoring in: efb2bb7
thanks |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: deads2k, dims, liggitt, timothysc, wlan0 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 |
/test all [submit-queue is verifying that this PR is safe to merge] |
Could you please update the title of this PR and state that it adds new configuration types? |
Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions here. |
What this PR does / why we need it:
Thanks to some great sleuthing by ikruglov!
kube-controller-manager defaults --leader-elect to true. We should
do the same for kube-scheduler. kube-scheduler used to have this
set to true, but it got lost during refactoring in:
efb2bb7
Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)
format, will close the issue(s) when PR gets merged):Fixes #59729
Special notes for your reviewer:
Release note: