Set default leader election configuration #1598

astefanutti · 2024-01-17T13:43:53Z

What type of PR is this?

/kind feature

What this PR does / why we need it:

The leader election section of the Kueue configuration is not defaulted.

Kubernetes components, like the scheduler, usually delegate to component-base to set sensible defaults:

https://github.com/kubernetes/component-base/blob/d5ddea0e47af10468dcd90e09768b2661caf23f7/config/v1alpha1/defaults.go#L35

Implementing that recommended defaulting has been suggested in the following discussion: #1554 (comment).

Does this PR introduce a user-facing change?

The leaderElection field in the Configuration API is now defaulted.
Leader election is now enabled by default.

netlify · 2024-01-17T13:43:59Z

✅ Deploy Preview for kubernetes-sigs-kueue canceled.

Name	Link
🔨 Latest commit	`21a2512`
🔍 Latest deploy log	https://app.netlify.com/sites/kubernetes-sigs-kueue/deploys/65a8f09e35ed320008d7e4a9

k8s-ci-robot · 2024-01-17T13:44:02Z

Hi @astefanutti. 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.

tenzen-y · 2024-01-17T17:33:11Z

/ok-to-test

mimowo

LGTM overall

apis/config/v1beta1/groupversion_info.go

mimowo · 2024-01-18T08:43:10Z

pkg/config/config_test.go

+				LeaderElection:             true,
+				LeaderElectionID:           configapi.DefaultLeaderElectionID,
+				LeaderElectionResourceLock: resourcelock.LeasesResourceLock,
+				LeaseDuration:              ptr.To(15 * time.Second),


Maybe instead of the magic values refer to values in defaultControlOption, so that is easier to identify which fields deviate from defaults?

Good idea. Updated.

mimowo · 2024-01-18T08:47:22Z

apis/config/v1beta1/defaults.go

+	if len(cfg.LeaderElection.ResourceLock) == 0 {
+		// Default to Lease as component-base still defaults to endpoint resources
+		// until core components migrate to using Leases. See k/k #80289 for more details.
+		cfg.LeaderElection.ResourceLock = resourcelock.LeasesResourceLock


Is this a change compared to what is happening on the main branch? I'm wondering if this deserves a release note explaining what are the user-facing differences when using defaults.

No, it doesn't change the default value compared to main. It's necessary so we can delegate to RecommendedDefaultLeaderElectionConfiguration, but that still defaults to endpoints as resource lock, because some Kubernetes components haven't migrated yet, and maintain the current default value for Kueue.
This can be removed once kubernetes/kubernetes#80289 is fixed.

mimowo · 2024-01-18T08:47:46Z

apis/config/v1beta1/defaults.go

+		cfg.LeaderElection.ResourceLock = resourcelock.LeasesResourceLock
+	}
+	// Use the default LeaderElectionConfiguration options
+	configv1alpha1.RecommendedDefaultLeaderElectionConfiguration(cfg.LeaderElection)


As before, question about user-facing consequences of setting the defaults.

With the values already set in the default Kueue configuration, i.e.:

kueue/config/components/manager/controller_manager_config.yaml

Lines 10 to 12 in d4d3f3b

leaderElection:

leaderElect: true

resourceName: c1f6bfd2.kueue.x-k8s.io

That PR is "idem-potent".

The only change would be if an existing user would use a configuration with .leaderElection or .leaderElection.leaderElect not defined, then the new default would be true, instead of false.

But I'd be inclined to think enabling leader election by default is desirable.

How about the values of LeaseDuration, RenewDeadline, RetryPeriod, as the values are now generated, are they ignored, or their values are as implicit defaults used before?

Yes, the latter, their values are equal to the implicit defaults used before.

mimowo · 2024-01-18T08:48:22Z

apis/config/v1beta1/defaults.go

@@ -43,13 +44,6 @@ const (
 	DefaultClusterQueuesMaxCount                int32   = 10
 )

-func addDefaultingFuncs(scheme *runtime.Scheme) error {


mimowo

/lgtm
/assign @tenzen-y @alculquicondor

k8s-ci-robot · 2024-01-18T10:01:49Z

LGTM label has been added.

Git tree hash: d17270b277bc93330482b2f483ade14d255eea37

alculquicondor · 2024-01-18T16:41:06Z

/release-note-edit

The leaderElection field in the Configuration API is now defaulted.
Leader election is now enabled by default.

alculquicondor · 2024-01-18T16:41:11Z

/approve

k8s-ci-robot · 2024-01-18T16:41:19Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alculquicondor, astefanutti

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:

~~OWNERS~~ [alculquicondor]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

alculquicondor · 2024-02-13T20:13:11Z

/kind api-change

k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. kind/feature Categorizes issue or PR as related to a new feature. labels Jan 17, 2024

k8s-ci-robot requested review from denkensk and mimowo January 17, 2024 13:43

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jan 17, 2024

k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jan 17, 2024

astefanutti mentioned this pull request Jan 17, 2024

Add HA support for the visibility API #1554

Merged

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jan 17, 2024

astefanutti force-pushed the pr-08 branch from 3c9ae17 to eb95955 Compare January 18, 2024 08:38

mimowo reviewed Jan 18, 2024

View reviewed changes

astefanutti force-pushed the pr-08 branch from eb95955 to 596d57d Compare January 18, 2024 09:21

Set default leader election configuration

21a2512

astefanutti force-pushed the pr-08 branch from 596d57d to 21a2512 Compare January 18, 2024 09:34

mimowo reviewed Jan 18, 2024

View reviewed changes

k8s-ci-robot assigned alculquicondor, tenzen-y and mimowo Jan 18, 2024

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 18, 2024

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 18, 2024

k8s-ci-robot merged commit 94ec795 into kubernetes-sigs:main Jan 18, 2024
14 checks passed

k8s-ci-robot added this to the v0.6 milestone Jan 18, 2024

astefanutti deleted the pr-08 branch January 18, 2024 17:06

astefanutti mentioned this pull request Jan 26, 2024

Use leader election defaulting to set up controllers HA #1652

Merged

k8s-ci-robot added the kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API label Feb 13, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Set default leader election configuration #1598

Set default leader election configuration #1598

astefanutti commented Jan 17, 2024 •

edited by k8s-ci-robot

Loading

netlify bot commented Jan 17, 2024 •

edited

Loading

k8s-ci-robot commented Jan 17, 2024

tenzen-y commented Jan 17, 2024

mimowo left a comment

mimowo Jan 18, 2024

astefanutti Jan 18, 2024

mimowo Jan 18, 2024

astefanutti Jan 18, 2024

mimowo Jan 18, 2024

astefanutti Jan 18, 2024

mimowo Jan 18, 2024

astefanutti Jan 18, 2024

mimowo Jan 18, 2024

mimowo left a comment

k8s-ci-robot commented Jan 18, 2024

alculquicondor commented Jan 18, 2024

alculquicondor commented Jan 18, 2024

k8s-ci-robot commented Jan 18, 2024

alculquicondor commented Feb 13, 2024

	leaderElection:
	leaderElect: true
	resourceName: c1f6bfd2.kueue.x-k8s.io

Set default leader election configuration #1598

Set default leader election configuration #1598

Conversation

astefanutti commented Jan 17, 2024 • edited by k8s-ci-robot Loading

What type of PR is this?

What this PR does / why we need it:

Does this PR introduce a user-facing change?

netlify bot commented Jan 17, 2024 • edited Loading

✅ Deploy Preview for kubernetes-sigs-kueue canceled.

k8s-ci-robot commented Jan 17, 2024

tenzen-y commented Jan 17, 2024

mimowo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mimowo left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Jan 18, 2024

alculquicondor commented Jan 18, 2024

alculquicondor commented Jan 18, 2024

k8s-ci-robot commented Jan 18, 2024

alculquicondor commented Feb 13, 2024

astefanutti commented Jan 17, 2024 •

edited by k8s-ci-robot

Loading

netlify bot commented Jan 17, 2024 •

edited

Loading