Skip to content

Allow the leader election resource lock type to be configurable #1448

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.

Sign up for GitHub

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

Closed
mprahl wants to merge 1 commit into from
Closed

Allow the leader election resource lock type to be configurable #1448

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
16 changes: 14 additions & 2 deletions pkg/config/leaderelection/leaderelection.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,8 +29,20 @@ import (
// See https://github.com/kubernetes/kubernetes/issues/107454 for
// details on how to migrate to "leases" leader election.
// Don't forget the callbacks!
// TODO: In the next version we should switch to using "leases"
Copy link
Member

@dgrisonnet dgrisonnet Feb 6, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

@tkashem did you intend to switch the default to leases instead of configmapleases? If so we might want to keep this TODO until the default resource is updated.

Copy link
Author

@mprahl mprahl Feb 13, 2023

Choose a reason for hiding this comment

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

@dgrisonnet and @tkashem I moved the TODO to where the logic for the default resource lock is made. Is this okay with you?

Copy link
Contributor

@tkashem tkashem Feb 16, 2023

Choose a reason for hiding this comment

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

Yes, actually, that was the goal

we want the following:

  • A: a brand new operator being written should us lease (preferably by default)
  • B: existing operators who are currently using configmapsleases should use lease (preferably by default)
  • C: existing operators who have not yet upgraded libray-go for a while (still using configmaps) should start using configmapsleases as soon as they upgrade to the latest library-go (we can't have these operators use lease by default, it will break their upgrade flow)

I am concerned about C - if we don't have any existing operators in C category today then we can safely rename ToLeaderElectionWithConfigmapLease to ToLeaderElectionWithLease and have lease be the default option.
So I would like to know if C is empty today - it can be possible today to count C, maybe by having a test in CI?

or is there a way we can break (static, compile time) these operators in C when they upgrade library-go so they can opt-in? I would like us to be able to avoid these changes if possible, especially if C is empty.

Copy link
Contributor

@deads2k deads2k Feb 17, 2023

Choose a reason for hiding this comment

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

Leases are not 100% reliable today. It's been a while since the switch I think, I'd simply switch.

Copy link
Contributor

@tkashem tkashem Feb 17, 2023

Choose a reason for hiding this comment

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

@mprahl can you revise the pr to switch to lease? (disregard C, that means no need to make anything configurable)

Copy link
Author

@mprahl mprahl Feb 17, 2023

Choose a reason for hiding this comment

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

@tkashem in ACM we would like to still have the option for configmapsleases if that's okay. I'd make lease the default though.

Copy link
Contributor

@tkashem tkashem Feb 20, 2023

Choose a reason for hiding this comment

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

@mprahl yeah, sure, go ahead. (just curious, you have an operator that is currently using configmaps?)

func ToLeaderElectionWithConfigmapLease(clientConfig *rest.Config, config configv1.LeaderElection, component, identity string) (leaderelection.LeaderElectionConfig, error) {
return toLeaderElection(clientConfig, config, component, identity, resourcelock.ConfigMapsLeasesResourceLock)
}

// ToLeaderElectionWithLease returns a "leases" based leader
// election config that you just need to fill in the Callback for.
// Don't forget the callbacks!
func ToLeaderElectionWithLease(clientConfig *rest.Config, config configv1.LeaderElection, component, identity string) (leaderelection.LeaderElectionConfig, error) {
Copy link
Contributor

@tkashem tkashem Feb 16, 2023

Choose a reason for hiding this comment

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

we can remove ToLeaderElectionWithLease and restore toLeaderElection to ToLeaderElection

return toLeaderElection(clientConfig, config, component, identity, resourcelock.LeasesResourceLock)
}

// toLeaderElection returns a leader election config that you just need to fill in the Callback for. The input
// resourceLock must be a value from the k8s.io/client-go/tools/leaderelection/resourcelock package.
func toLeaderElection(clientConfig *rest.Config, config configv1.LeaderElection, component, identity, resourceLock string) (leaderelection.LeaderElectionConfig, error) {
kubeClient, err := kubernetes.NewForConfig(clientConfig)
if err != nil {
return leaderelection.LeaderElectionConfig{}, err
Expand All @@ -57,7 +69,7 @@ func ToLeaderElectionWithConfigmapLease(clientConfig *rest.Config, config config
eventBroadcaster.StartRecordingToSink(&v1core.EventSinkImpl{Interface: v1core.New(kubeClient.CoreV1().RESTClient()).Events("")})
eventRecorder := eventBroadcaster.NewRecorder(clientgoscheme.Scheme, corev1.EventSource{Component: component})
rl, err := resourcelock.New(
resourcelock.ConfigMapsLeasesResourceLock,
resourceLock,
config.Namespace,
config.Name,
kubeClient.CoreV1(),
Expand Down
47 changes: 39 additions & 8 deletions pkg/controller/controllercmd/builder.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package controllercmd

import (
"context"
"errors"
"fmt"
"io/ioutil"
"os"
Expand Down Expand Up @@ -29,6 +30,7 @@ import (
"k8s.io/client-go/kubernetes"
"k8s.io/client-go/rest"
"k8s.io/client-go/tools/leaderelection"
"k8s.io/client-go/tools/leaderelection/resourcelock"
"k8s.io/client-go/tools/record"
"k8s.io/component-base/metrics"
"k8s.io/component-base/metrics/legacyregistry"
Expand Down Expand Up @@ -59,19 +61,29 @@ type ControllerContext struct {
OperatorNamespace string
}

type leaderElectionResourceLock string

const (
LeasesResourceLock leaderElectionResourceLock = resourcelock.LeasesResourceLock
ConfigMapsLeasesResourceLock leaderElectionResourceLock = resourcelock.ConfigMapsLeasesResourceLock
)

var ErrInvalidResourceLock = errors.New("the leader election resource lock is not supported")

// defaultObserverInterval specifies the default interval that file observer will do rehash the files it watches and react to any changes
// in those files.
var defaultObserverInterval = 5 * time.Second

// ControllerBuilder allows the construction of an controller in optional pieces.
type ControllerBuilder struct {
kubeAPIServerConfigFile *string
clientOverrides *client.ClientConnectionOverrides
leaderElection *configv1.LeaderElection
fileObserver fileobserver.Observer
fileObserverReactorFn func(file string, action fileobserver.ActionType) error
eventRecorderOptions record.CorrelatorOptions
componentOwnerReference *corev1.ObjectReference
kubeAPIServerConfigFile *string
clientOverrides *client.ClientConnectionOverrides
leaderElection *configv1.LeaderElection
leaderElectionResourceLock leaderElectionResourceLock
fileObserver fileobserver.Observer
fileObserverReactorFn func(file string, action fileobserver.ActionType) error
eventRecorderOptions record.CorrelatorOptions
componentOwnerReference *corev1.ObjectReference

startFunc StartFunc
componentName string
Expand Down Expand Up @@ -157,6 +169,14 @@ func (b *ControllerBuilder) WithLeaderElection(leaderElection configv1.LeaderEle
return b
}

// WithLeaderElectionResourceLock sets the resource lock. If not set, controllercmd.ConfigMapsLeasesResourceLock will
// be used for backwards compatibility.
func (b *ControllerBuilder) WithLeaderElectionResourceLock(resourceLock leaderElectionResourceLock) *ControllerBuilder {
Copy link
Contributor

@tkashem tkashem Feb 16, 2023

Choose a reason for hiding this comment

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

instead of using the lock as a parameter, can we have two methods?

WithLeaderElectionLeaseResourceLock() *ControllerBuilder
WithLeaderElectionConfigmapLeaseResourceLock() *ControllerBuilder

this way we don't have to have validation around resource lock names and in future we can simply remove WithLeaderElectionConfigmapLeaseResourceLock

b.leaderElectionResourceLock = resourceLock

return b
}

// WithVersion accepts a getting that provide binary version information that is used to report build_info information to prometheus
func (b *ControllerBuilder) WithVersion(info version.Info) *ControllerBuilder {
b.versionInfo = &info
Expand Down Expand Up @@ -328,8 +348,19 @@ func (b *ControllerBuilder) Run(ctx context.Context, config *unstructured.Unstru
// ensure blocking TCP connections don't block the leader election
leaderConfig := rest.CopyConfig(protoConfig)
leaderConfig.Timeout = b.leaderElection.RenewDeadline.Duration
var leaderElection leaderelection.LeaderElectionConfig

switch b.leaderElectionResourceLock {
// default to configmapsleases for leader election
// TODO: In the next version we should switch to using "leases" by default
case "", ConfigMapsLeasesResourceLock:
leaderElection, err = leaderelectionconverter.ToLeaderElectionWithConfigmapLease(leaderConfig, *b.leaderElection, b.componentName, b.instanceIdentity)
case LeasesResourceLock:
leaderElection, err = leaderelectionconverter.ToLeaderElectionWithLease(leaderConfig, *b.leaderElection, b.componentName, b.instanceIdentity)
default:
return fmt.Errorf("%w: %s", ErrInvalidResourceLock, b.leaderElectionResourceLock)
}

leaderElection, err := leaderelectionconverter.ToLeaderElectionWithConfigmapLease(leaderConfig, *b.leaderElection, b.componentName, b.instanceIdentity)
Copy link
Contributor

@tkashem tkashem Feb 16, 2023

Choose a reason for hiding this comment

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

use proposed eaderelectionconverter.ToLeaderElection(leaderConfig, *b.leaderElection, b.componentName, b.instanceIdentity, b.leaderElectionResourceLock

we can get rid of the switch here

if err != nil {
return err
}
Expand Down
11 changes: 9 additions & 2 deletions pkg/controller/controllercmd/cmd.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ import (
"k8s.io/apimachinery/pkg/util/sets"
"k8s.io/apimachinery/pkg/version"
"k8s.io/apiserver/pkg/server"
"k8s.io/client-go/tools/leaderelection/resourcelock"
"k8s.io/component-base/logs"
"k8s.io/klog/v2"

Expand All @@ -43,6 +44,10 @@ type ControllerCommandConfig struct {
// DisableServing disables serving metrics, debug and health checks and so on.
DisableServing bool

// LeaderElectionResourceLock sets the resource lock. If not set, controllercmd.ConfigMapsLeasesResourceLock will
// be used for backwards compatibility.
LeaderElectionResourceLock leaderElectionResourceLock
Copy link
Contributor

@tkashem tkashem Feb 16, 2023

Choose a reason for hiding this comment

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

we need to know whether it's lease or configmapsleases, so can we make it a boolean

// DEPRECATED: at some point everything should uses lease by default
OverrideConfigmapsLeasesResourceLock bool

This will allow new operators to opt in for lease


// DisableLeaderElection allows leader election to be suspended
DisableLeaderElection bool

Expand All @@ -59,8 +64,9 @@ func NewControllerCommandConfig(componentName string, version version.Info, star

basicFlags: NewControllerFlags(),

DisableServing: false,
DisableLeaderElection: false,
LeaderElectionResourceLock: resourcelock.ConfigMapsLeasesResourceLock,
DisableServing: false,
DisableLeaderElection: false,
}
}

Expand Down Expand Up @@ -282,6 +288,7 @@ func (c *ControllerCommandConfig) StartController(ctx context.Context) error {
WithKubeConfigFile(c.basicFlags.KubeConfigFile, nil).
WithComponentNamespace(c.basicFlags.Namespace).
WithLeaderElection(config.LeaderElection, c.basicFlags.Namespace, c.componentName+"-lock").
WithLeaderElectionResourceLock(c.LeaderElectionResourceLock).
Copy link
Contributor

@tkashem tkashem Feb 16, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

replace it with

builder = builder. WithLeaderElectionConfigmapLeaseResourceLock() 
if c.OverrideConfigmapsLeasesResourceLock {
     builder = builder. WithLeaderElectionLeaseResourceLock() 
}

WithVersion(c.version).
WithEventRecorderOptions(events.RecommendedClusterSingletonCorrelatorOptions()).
WithRestartOnChange(exitOnChangeReactorCh, startingFileContent, observedFiles...).
Expand Down