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.

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

Add option to disable lock creation by default #109875

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -153,6 +153,13 @@ type LeaderElectionConfig struct {
// simultaneously acting on the critical path.
ReleaseOnCancel bool

// DisableLockCreation should be set to true if the lock should not be created,
Copy link
Member

Choose a reason for hiding this comment

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

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.

// when it doesn't exist. By default, DisableLockCreation is false and leader elector
// creates a new lock 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 create a
// lock object and consequently can't become the leader.
DisableLockCreation bool

// Name is the name of the resource lock for debugging
Name string
}
Expand Down Expand Up @@ -330,6 +337,11 @@ func (le *LeaderElector) tryAcquireOrRenew(ctx context.Context) bool {
klog.Errorf("error retrieving resource lock %v: %v", le.config.Lock.Describe(), err)
return false
}
// don't create a lock and exit early, if lock creation is disabled.
if le.config.DisableLockCreation {
klog.Warningf("skipping lock creation since lock creation is disabled")
return false
}
if err = le.config.Lock.Create(ctx, leaderElectionRecord); err != nil {
klog.Errorf("error initially creating leader election record: %v", err)
return false
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -82,9 +82,10 @@ func testTryAcquireOrRenew(t *testing.T, objectType string) {
observedTime time.Time
reactors []Reactor

expectSuccess bool
transitionLeader bool
outHolder string
expectSuccess bool
transitionLeader bool
outHolder string
disableLockCreation bool
}{
{
name: "acquire from no object",
Expand Down Expand Up @@ -206,6 +207,19 @@ func testTryAcquireOrRenew(t *testing.T, objectType string) {
expectSuccess: false,
outHolder: "bing",
},
{
name: "don't create lock object if DisableLockCreation is true",
reactors: []Reactor{
{
verb: "get",
reaction: func(action fakeclient.Action) (handled bool, ret runtime.Object, err error) {
return true, nil, errors.NewNotFound(action.(fakeclient.GetAction).GetResource().GroupResource(), action.(fakeclient.GetAction).GetName())
},
},
},
expectSuccess: false,
disableLockCreation: true,
},
{
name: "renew already acquired object",
reactors: []Reactor{
Expand Down Expand Up @@ -273,6 +287,7 @@ func testTryAcquireOrRenew(t *testing.T, objectType string) {
reportedLeader = l
},
},
DisableLockCreation: test.disableLockCreation,
}
observedRawRecord := GetRawRecordOrDie(t, objectType, test.observedRecord)
le := &LeaderElector{
Expand Down Expand Up @@ -302,6 +317,12 @@ func testTryAcquireOrRenew(t *testing.T, objectType string) {
}

le.maybeReportTransition()
if test.disableLockCreation {
// This sleep is to make sure that there is no async leader transition,
// when lock creation is disabled.
time.Sleep(time.Second * 10)
wg.Done()
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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) ?

Copy link
Member

Choose a reason for hiding this comment

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

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

}
wg.Wait()
if reportedLeader != test.outHolder {
t.Errorf("reported leader was not the new leader. expected %q, got %q", test.outHolder, reportedLeader)
Expand Down