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

[silenceshell]

What happened?

Opinion 1

Lock resource is created in tryAcquireOrRenew(), but not deleted when release().

If resource cannot be deleted in pkg, it shall not be created. Or, at lease a choice to ForbiddenCreation.

type Interface interface {
	// Get returns the LeaderElectionRecord
	Get(ctx context.Context) (*LeaderElectionRecord, []byte, error)

	// Create attempts to create a LeaderElectionRecord
	Create(ctx context.Context, ler LeaderElectionRecord) error

	// Update will update and existing LeaderElectionRecord
	Update(ctx context.Context, ler LeaderElectionRecord) error

	// RecordEvent is used to record events
	RecordEvent(string)

	// Identity will return the locks Identity
	Identity() string

	// Describe is used to convert details on current resource lock
	// into a string
	Describe() string
}

Opinion 2

Different lock resources react differently with update method.

Some resources can be created via update method, but some cannot. Shall we change update method to patch?

LockResource	AllowCreateOnUpdate
Configmaps	false
Leases	true
Endpoints	true

What did you expect to happen?

Add a new bool parameter ForbiddenCreation in LeaderElectionConfig struct.

type LeaderElectionConfig struct {
	// Lock is the resource that will be used for locking
	Lock rl.Interface

	// LeaseDuration is the duration that non-leader candidates will
	// wait to force acquire leadership. This is measured against time of
	// last observed ack.
	//
	// A client needs to wait a full LeaseDuration without observing a change to
	// the record before it can attempt to take over. When all clients are
	// shutdown and a new set of clients are started with different names against
	// the same leader record, they must wait the full LeaseDuration before
	// attempting to acquire the lease. Thus LeaseDuration should be as short as
	// possible (within your tolerance for clock skew rate) to avoid a possible
	// long waits in the scenario.
	//
	// Core clients default this value to 15 seconds.
	LeaseDuration time.Duration
	// RenewDeadline is the duration that the acting master will retry
	// refreshing leadership before giving up.
	//
	// Core clients default this value to 10 seconds.
	RenewDeadline time.Duration
	// RetryPeriod is the duration the LeaderElector clients should wait
	// between tries of actions.
	//
	// Core clients default this value to 2 seconds.
	RetryPeriod time.Duration

	// Callbacks are callbacks that are triggered during certain lifecycle
	// events of the LeaderElector
	Callbacks LeaderCallbacks

	// WatchDog is the associated health checker
	// WatchDog may be null if it's not needed/configured.
	WatchDog *HealthzAdaptor

	// ReleaseOnCancel should be set true if the lock should be released
	// when the run context is cancelled. If you set this to true, you must
	// ensure all code guarded by this lease has successfully completed
	// prior to cancelling the context, or you may have two processes
	// simultaneously acting on the critical path.
	ReleaseOnCancel bool

	// Name is the name of the resource lock for debugging
	Name string

	// ForbiddenCreation provides a choice to manage lock resource
        // lifecycle outside of leaderElection pkg. Note that if set to true,
        // you should create lock resource before run leaderElector.
	ForbiddenCreation bool
}

By default, ForbiddenCreation is false, which doesn't impact current users. It's a compatible change.

How can we reproduce it (as minimally and precisely as possible)?

Write a main() to take leaderelection into use.

package main

import (
	"context"
	"time"

	"k8s.io/apimachinery/pkg/apis/meta/v1"
	"k8s.io/client-go/kubernetes"
	"k8s.io/client-go/rest"
	"k8s.io/client-go/tools/leaderelection"
	"k8s.io/client-go/tools/leaderelection/resourcelock"
)

func main() {
	kc, _ := rest.InClusterConfig()
	clientSet, _ := kubernetes.NewForConfig(kc)
	elector, _ := leaderelection.NewLeaderElector(leaderelection.LeaderElectionConfig{
		Lock: &resourcelock.ConfigMapLock{
			ConfigMapMeta: v1.ObjectMeta{
				Name:      "myLocker",
				Namespace: "myNamespace",
			},
			Client: clientSet.CoreV1(),
			LockConfig: resourcelock.ResourceLockConfig{
				Identity: "myPod",
			},
		},
		ReleaseOnCancel: true,
		LeaseDuration:   11 * time.Second,
		RenewDeadline:   7 * time.Second,
		RetryPeriod:     2 * time.Second,
	})
	elector.Run(context.Background())
}

Anything else we need to know?

Code: https://github.com/kubernetes/client-go/tree/master/tools/leaderelection

Kubernetes version

$ kubectl version
Client Version: version.Info{Major:"1", Minor:"21", GitVersion:"v0.21.0-beta.1", GitCommit:"e1bfb408ac5421021cd2f8c676ca398c9430674a", GitTreeState:"clean", BuildDate:"2022-02-15T19:14:53Z", GoVersion:"go1.16.12", Compiler:"gc", Platform:"linux/amd64"}
Server Version: version.Info{Major:"1", Minor:"22", GitVersion:"v1.22.3+6b15b0f", GitCommit:"3a0f2c90b43e6cffd07f57b5b78dd9f083e47ee2", GitTreeState:"clean", BuildDate:"2022-02-23T20:12:54Z", GoVersion:"go1.16.6", Compiler:"gc", Platform:"linux/amd64"}

Cloud provider

N/A

OS version

N/A

Install tools

N/A

Container runtime (CRI) and version (if applicable)

N/A

Related plugins (CNI, CSI, ...) and versions (if applicable)

N/A

[aojea]

release() just remove the HolderIdentity, if you delete the object you have to create and delete it constantly vs updating the field

[neolit123]

/sig api-machinery

[silenceshell]

release() just remove the HolderIdentity, if you delete the object you have to create and delete it constantly vs updating the field

I can understand why leaderelection doesn't delete lock resource when release()... But create and delete is a pair action... If we can't delete lock resource, we'd better not to create it...

Leaderelection can rely on others to manage the lifecycle of lock resource, it just need to update it. If no lock resource available when run(), we can exit simply. But consider of compatibility, I propose a new ForbiddenCreation here.

[aojea]

are you proposing that the leaderElection logic doesn't create the resource and just depend on an external to create the resource used to determine the leader?

[silenceshell]

are you proposing that the leaderElection logic doesn't create the resource and just depend on an external to create the resource used to determine the leader?

Yes.

[leilajal]

/cc @roycaihw
/triage accepted

[aryan9600]

@aojea @silenceshell what about adding a DisableLockCreation bool variable to disable lock creation? I feel it's a clearer name than ForbiddenCreation.

[roycaihw]

I think an optional flag makes sense.

Users still need to decide how to manage the lifecycle of lock resources. One option is we have a garbage collector that deletes expired Lease objects (the idea was using Lease as heartbeat, but could be extended to clean up unused leaderelection locks)

[silenceshell]

@aojea @silenceshell what about adding a DisableLockCreation bool variable to disable lock creation? I feel it's a clearer name than ForbiddenCreation.

Your proposal name DisableLockCreation is better. Good to me.

[silenceshell]

I think an optional flag makes sense.

Users still need to decide how to manage the lifecycle of lock resources. One option is we have a garbage collector that deletes expired Lease objects (the idea was using Lease as heartbeat, but could be extended to clean up unused leaderelection locks)

The optional flag will help me much.

In my user case, helm chart is designed to manage the lifecycle of the lock resource. The issue I met before is that a lock resource was left after helm uninstall, but it was a pure new one created during helm uninstall. The process is that helm has already deleted my StatefulSet and my lock resource, but my Pod created by the Statefulset still had a chance to update the lock resource, then it found no lock exist and created a new lock. Although I bound a role to limit my pod with only get, watch, list, update lock resources, it seems depend on RBAC settings in Kubenetes cluster -- A loose clusterRole can be bound to whole serviceAccount group.

So if we can have a choice not to create lock resource in leaderelection, my issue can be solved.

[roycaihw]

SGTM. Would you like to send a PR?

cc @mikedanese for visibility.

[aryan9600]

@roycaihw i was hoping to send a PR for this (and explore the gc option for expired leases), if that's okay? 😅

[roycaihw]

yes, please!

[aryan9600]

/assign

[silenceshell]

@aryan Thanks for your PR! It works for those configmapLock, but other locks leaseLock and endpointLock can still be created by update method with DisableLockCreation=true, please refer my option2.

[aryan9600]

@silenceshell I don't see how that's a problem. Even if a Lease/Endpoint can be created with the Update method, since clients are expected to create the Lease/Endpoint themselves when they have DisableLockCreation=true. The leader election logic only calls the Update method if it wants to update the record or release the lock itself, both of which are not possible if the client doesn't create a lock in the first place. @roycaihw It'd be good to know your thoughts on this as well, thanks!

[roycaihw]

@silenceshell Could you point out where the update call is? I thought #109875 would short-circuit if DisableLockCreation=true-- no update call will be made.

[silenceshell]

@roycaihw @aryan9600 There is still a chance that lock still exists when getting lock, but it is deleted somehow after getting lock and before updating lock. For those Lease/Endpoint locks, they will be created by update method. In this special case, DisableLockCreation=true cannot work as expected...

[aryan9600]

@silenceshell I understand your point, but I don't think it falls within the scope of this. DisableLockCreation is a way to prevent the leader elector from creating a lock only when it's not found.
@roycaihw would you be for changing the Update method of locks to use Patch instead of update for modification? Alternatively, if DisableLockCreation is true, we could try fetching the lock object and skip up updating the lock object if it doesn't exist.

[silenceshell]

@aryan9600 OK, I got your point. Then here DisableLockCreation only prevents lock creation from create method, but not from update method. A little tricky but acceptable for me if DisableLockCreation definition is well written to warning this situation.

[silenceshell]

Any update for this issue? #109875 seems stop updating for a long time... Looking forwards to merge it.

[JackZxj]

I got the same issue that the lease lock would not be released sometimes, and I found that was caused by the Get method Incorrectly overriding the lock. Can you see if #112022 fixes your lock release issue? @silenceshell

[JackZxj]

I got the same issue that the lease lock would not be released sometimes, and I found that was caused by the Get method Incorrectly overriding the lock. Can you see if #112022 fixes your lock release issue? @silenceshell

I'm sorry. I've looked carefully again, and I think what we said may not be the same thing. I will open another issue to record my problem. @silenceshell

[k8s-triage-robot]

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

[vaibhav2107]

/remove-lifecycle stale

[k8s-triage-robot]

This issue has not been updated in over 1 year, and should be re-triaged.

You can:

• Confirm that this issue is still relevant with /triage accepted (org members only)
• Close this issue with /close

For more details on the triage process, see https://www.kubernetes.dev/docs/guide/issue-triage/

/remove-triage accepted

[seans3]

/triage accepted