Add option to disable lock creation by default #109875

aryan9600 · 2022-05-06T20:46:42Z

Signed-off-by: Sanskar Jaiswal jaiswalsanskar078@gmail.com

What type of PR is this?

/kind feature

What this PR does / why we need it:

Adds an option DisableLockCreation in LeaderElectionConfig to disable lock creation tryAcquireOrRenew(), if set to true. This lets users manage the lifecycle of the lock, since the lock is never explicitly deleted elsewhere.

Which issue(s) this PR fixes:

Fixes #109554

Special notes for your reviewer:

Does this PR introduce a user-facing change?

NONE

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

k8s-ci-robot · 2022-05-06T20:46:52Z

Hi @aryan9600. Thanks for your PR.

I'm waiting for a kubernetes 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.

fedebongio · 2022-05-10T20:07:09Z

/assign @roycaihw @jiahuif
/triage accepted

roycaihw · 2022-05-11T01:09:18Z

staging/src/k8s.io/client-go/tools/leaderelection/leaderelection.go

-			return false
-		}
+		// don't create a lock, if lock creation is disabled.
+		if !le.config.DisableLockCreation {


Please use

if le.config.DisableLockCreation { return false }

to avoid the nesting

Let's also add a klog.Errorf message. No process can become the leader if the lock doesn't exist. If a user forgets to create the lock when DisableLockCreation is true, it's their responsibility, but let's at least make the issue more obvious to the user.

I think a klog.Warningf could be a better log level for this, since there's technically no error, it's improper use of the api.

roycaihw · 2022-05-11T01:26:14Z

staging/src/k8s.io/client-go/tools/leaderelection/leaderelection_test.go

@@ -302,6 +317,9 @@ func testTryAcquireOrRenew(t *testing.T, objectType string) {
 			}

 			le.maybeReportTransition()
+			if test.disableLockCreation {
+				wg.Done()


Maybe also add a 30s sleep? We want to make sure no one becomes the leader asynchronously. We can sleep and verify reportedLeader is still emtpy.

I don't think that anyone can become the leader asynchronously if disableLockCreation is true. This can be verified by the fact that if we remove this if block, the test will hang since the OnNewLeader callback which calls wg.Done() will never be called. Is it necessary to have a fairly large sleep period to test something which is redundant(imo, please correct me if i'm wrong here) ?

It's to prevent someone from accidentally change the behavior and break the assumption in the future. But I agree a 30s sleep is probably too much for a unit test. I don't feel strongly

roycaihw · 2022-05-11T01:31:28Z

staging/src/k8s.io/client-go/tools/leaderelection/leaderelection.go

@@ -153,6 +153,10 @@ type LeaderElectionConfig struct {
 	// simultaneously acting on the critical path.
 	ReleaseOnCancel bool

+	// DisableLockCreation should be set to true if the lock should not be created,


nit: some clarifications:

"the lock should not be created when it doesn't exist"
By default (when DisableLockCreation is false), leader elector creates a new lock object when trying to acquire a non-existing lock object.
If DisableLockCreation is true and the lock object doesn't exist, the leader elector won't be able to become the leader.

roycaihw · 2022-05-11T16:41:19Z

/ok-to-test

roycaihw · 2022-05-11T23:14:37Z

/lgtm
/approve
Please squash the commits.

/assign @mikedanese

Signed-off-by: Sanskar Jaiswal <jaiswalsanskar078@gmail.com>

aryan9600 · 2022-05-12T05:39:21Z

/retest-required

k8s-triage-robot · 2022-08-10T07:01:34Z

The Kubernetes project currently lacks enough contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

After 90d of inactivity, lifecycle/stale is applied
After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

Mark this issue or PR as fresh with /remove-lifecycle stale
Mark this issue or PR as rotten with /lifecycle rotten
Close this issue or PR with /close
Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

aryan9600 · 2022-08-10T07:03:09Z

/remove-lifecycle stale
cc: @roycaihw @mikedanese

roycaihw · 2022-08-15T21:52:01Z

/lgtm
/approve

k8s-ci-robot · 2022-08-15T21:52:19Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: aryan9600, roycaihw
Once this PR has been reviewed and has the lgtm label, please ask for approval from mikedanese by writing /assign @mikedanese in a comment. For more information see:The Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

staging/src/k8s.io/client-go/tools/leaderelection/OWNERS

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

k8s-triage-robot · 2022-11-13T22:18:44Z

The Kubernetes project currently lacks enough contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

After 90d of inactivity, lifecycle/stale is applied
After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

Mark this issue or PR as fresh with /remove-lifecycle stale
Mark this issue or PR as rotten with /lifecycle rotten
Close this issue or PR with /close
Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

k8s-triage-robot · 2022-12-13T22:43:16Z

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

After 90d of inactivity, lifecycle/stale is applied
After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

Mark this issue or PR as fresh with /remove-lifecycle rotten
Close this issue or PR with /close
Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle rotten

k8s-triage-robot · 2023-01-12T23:40:37Z

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages PRs according to the following rules:

After 90d of inactivity, lifecycle/stale is applied
After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

Reopen this PR with /reopen
Mark this PR as fresh with /remove-lifecycle rotten
Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/close

k8s-ci-robot · 2023-01-12T23:40:43Z

@k8s-triage-robot: Closed this PR.

In response to this:

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages PRs according to the following rules:

After 90d of inactivity, lifecycle/stale is applied

After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied

After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

Reopen this PR with /reopen

Mark this PR as fresh with /remove-lifecycle rotten

Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/close

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 requested review from deads2k and mikedanese May 6, 2022 20:47

k8s-ci-robot added sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. and removed do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels May 6, 2022

aryan9600 force-pushed the disable-lock-creation branch from f4ac2b3 to 4700ecf Compare May 6, 2022 20:48

k8s-ci-robot assigned jiahuif and roycaihw May 10, 2022

k8s-ci-robot added triage/accepted Indicates an issue or PR is ready to be actively worked on. and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels May 10, 2022

roycaihw mentioned this pull request May 11, 2022

[client-go] Resource leak in kubernetes/client-go/tools/leaderelection #109554

Open

roycaihw reviewed May 11, 2022

View reviewed changes

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 May 11, 2022

k8s-ci-robot assigned mikedanese May 11, 2022

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 11, 2022

Add option to disable lock creation by default

54dadcd

Signed-off-by: Sanskar Jaiswal <jaiswalsanskar078@gmail.com>

aryan9600 force-pushed the disable-lock-creation branch from 4228490 to 54dadcd Compare May 12, 2022 05:07

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 12, 2022

k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Aug 10, 2022

k8s-ci-robot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Aug 10, 2022

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 15, 2022

aryan9600 mentioned this pull request Sep 7, 2022

REQUEST: New membership for aryan9600 kubernetes/org#3664

Closed

9 tasks

k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Nov 13, 2022

k8s-ci-robot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Dec 13, 2022

k8s-ci-robot closed this Jan 12, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add option to disable lock creation by default #109875

Add option to disable lock creation by default #109875

aryan9600 commented May 6, 2022 •

edited

k8s-ci-robot commented May 6, 2022

fedebongio commented May 10, 2022

roycaihw May 11, 2022

roycaihw May 11, 2022

aryan9600 May 11, 2022

roycaihw May 11, 2022

roycaihw May 11, 2022

aryan9600 May 11, 2022

roycaihw May 11, 2022

roycaihw May 11, 2022

roycaihw commented May 11, 2022

roycaihw commented May 11, 2022

aryan9600 commented May 12, 2022

k8s-triage-robot commented Aug 10, 2022

aryan9600 commented Aug 10, 2022

roycaihw commented Aug 15, 2022

k8s-ci-robot commented Aug 15, 2022

k8s-triage-robot commented Nov 13, 2022

k8s-triage-robot commented Dec 13, 2022

k8s-triage-robot commented Jan 12, 2023

k8s-ci-robot commented Jan 12, 2023

Add option to disable lock creation by default #109875

Add option to disable lock creation by default #109875

Conversation

aryan9600 commented May 6, 2022 • edited

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

k8s-ci-robot commented May 6, 2022

fedebongio commented May 10, 2022

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

roycaihw commented May 11, 2022

roycaihw commented May 11, 2022

aryan9600 commented May 12, 2022

k8s-triage-robot commented Aug 10, 2022

aryan9600 commented Aug 10, 2022

roycaihw commented Aug 15, 2022

k8s-ci-robot commented Aug 15, 2022

k8s-triage-robot commented Nov 13, 2022

k8s-triage-robot commented Dec 13, 2022

k8s-triage-robot commented Jan 12, 2023

k8s-ci-robot commented Jan 12, 2023

aryan9600 commented May 6, 2022 •

edited