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.

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

Jump to bottom

⚠️ Rename leader election config fields #2637

Closed
wants to merge 1 commit into from
Closed

⚠️ Rename leader election config fields #2637

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
34 changes: 17 additions & 17 deletions pkg/leaderelection/leader_election.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,17 +38,17 @@ type Options struct {
// starting the manager.
LeaderElection bool

// LeaderElectionResourceLock determines which resource lock to use for leader election,
// ResourceLock determines which resource lock to use for leader election,
// defaults to "leases".
LeaderElectionResourceLock string
ResourceLock string

// LeaderElectionNamespace determines the namespace in which the leader
// LockNamespace determines the namespace in which the leader
// election resource will be created.
LeaderElectionNamespace string
LockNamespace string

// LeaderElectionID determines the name of the resource that leader election
// LockName determines the name of the resource that leader election
// will use for holding the leader lock.
LeaderElectionID string
LockName string
}

// NewResourceLock creates a new resource lock for use in a leader election loop.
Expand All @@ -61,19 +61,19 @@ func NewResourceLock(config *rest.Config, recorderProvider recorder.Provider, op
// used to migrate from configmaps to leases. Since the default was "configmapsleases" for over a year, spanning
// five minor releases, any actively maintained operators are very likely to have a released version that uses
// "configmapsleases". Therefore defaulting to "leases" should be safe.
if options.LeaderElectionResourceLock == "" {
options.LeaderElectionResourceLock = resourcelock.LeasesResourceLock
if options.ResourceLock == "" {
options.ResourceLock = resourcelock.LeasesResourceLock
}

// LeaderElectionID must be provided to prevent clashes
if options.LeaderElectionID == "" {
return nil, errors.New("LeaderElectionID must be configured")
// LockName must be provided to prevent clashes
if options.LockName == "" {
return nil, errors.New("LockName must be configured")
}

// Default the namespace (if running in cluster)
if options.LeaderElectionNamespace == "" {
if options.LockNamespace == "" {
var err error
options.LeaderElectionNamespace, err = getInClusterNamespace()
options.LockNamespace, err = getInClusterNamespace()
if err != nil {
return nil, fmt.Errorf("unable to find leader election namespace: %w", err)
}
Expand All @@ -98,9 +98,9 @@ func NewResourceLock(config *rest.Config, recorderProvider recorder.Provider, op
return nil, err
}

return resourcelock.New(options.LeaderElectionResourceLock,
options.LeaderElectionNamespace,
options.LeaderElectionID,
return resourcelock.New(options.ResourceLock,
options.LockNamespace,
options.LockName,
corev1Client,
coordinationClient,
resourcelock.ResourceLockConfig{
Expand All @@ -113,7 +113,7 @@ func getInClusterNamespace() (string, error) {
// Check whether the namespace file exists.
// If not, we are not running in cluster so can't guess the namespace.
if _, err := os.Stat(inClusterNamespacePath); os.IsNotExist(err) {
return "", fmt.Errorf("not running in-cluster, please specify LeaderElectionNamespace")
return "", fmt.Errorf("not running in-cluster, please specify LeaderElectionLockNamespace")
} else if err != nil {
return "", fmt.Errorf("error checking namespace file: %w", err)
}
Expand Down
6 changes: 3 additions & 3 deletions pkg/manager/internal.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -129,9 +129,9 @@ type controllerManager struct {
// webhookServer if unset, and Add() it to controllerManager.
webhookServerOnce sync.Once

// leaderElectionID is the name of the resource that leader election
// leaderElectionLockName is the name of the resource that leader election
// will use for holding the leader lock.
leaderElectionID string
leaderElectionLockName string
// leaseDuration is the duration that non-leader candidates will
// wait to force acquire leadership.
leaseDuration time.Duration
Expand Down Expand Up @@ -585,7 +585,7 @@ func (cm *controllerManager) startLeaderElection(ctx context.Context) (err error
},
},
ReleaseOnCancel: cm.leaderElectionReleaseOnCancel,
Name: cm.leaderElectionID,
Name: cm.leaderElectionLockName,
})
if err != nil {
return err
Expand Down
30 changes: 15 additions & 15 deletions pkg/manager/manager.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -176,13 +176,13 @@ type Options struct {
// act on the same resources concurrently.
LeaderElectionResourceLock string

// LeaderElectionNamespace determines the namespace in which the leader
// election resource will be created.
LeaderElectionNamespace string
// LeaderElectionLockNamespace determines the namespace of the leader election lock
Copy link
Member

Choose a reason for hiding this comment

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

While I do agree that the new names are better, renaming these will result in a lot of churn for consumers, I am not sure if that is worth it

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah totally valid. It doesn't offer any net new improvement.

Copy link
Member

Choose a reason for hiding this comment

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

Agreed with @alvaroaleman.

If we think the name change really is worth it, we could introduce the new fields, deprecate the old ones, and then go a few minor releases before deleting the old ones. Still churn, but gives folks warning/time. That would bring its own complexity in the interim though (multiple fields that do the same thing, coding for mutual exclusivity, etc.)

Copy link
Member

Choose a reason for hiding this comment

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

Would leaving a comment in the docs for this be better? I don't see this getting merged being that the churn associated with this and the complexity @joelanford mentioned with trying to promote these fields and deprecating others would help either.

Copy link
Member

Choose a reason for hiding this comment

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

To echo other's comments, given that the field are self explanatory, maybe not 100% accurate I guess (for historical reasons these have changed the internal meaning); could we add more information on the comment, rather than renaming them?

// resource will be created in.
LeaderElectionLockNamespace string

// LeaderElectionID determines the name of the resource that leader election
// LeaderElectionLockName determines the name of the resource that leader election
// will use for holding the leader lock.
LeaderElectionID string
LeaderElectionLockName string

// LeaderElectionConfig can be specified to override the default configuration
// that is used to build the leader election client.
Expand All @@ -196,7 +196,7 @@ type Options struct {
LeaderElectionReleaseOnCancel bool

// LeaderElectionResourceLockInterface allows to provide a custom resourcelock.Interface that was created outside
// of the controller-runtime. If this value is set the options LeaderElectionID, LeaderElectionNamespace,
// of the controller-runtime. If this value is set the options LeaderElectionLockName, LeaderElectionLockNamespace,
// LeaderElectionResourceLock, LeaseDuration, RenewDeadline and RetryPeriod will be ignored. This can be useful if you
// want to use a locking mechanism that is currently not supported, like a MultiLock across two Kubernetes clusters.
LeaderElectionResourceLockInterface resourcelock.Interface
Expand Down Expand Up @@ -379,10 +379,10 @@ func New(config *rest.Config, options Options) (Manager, error) {
resourceLock = options.LeaderElectionResourceLockInterface
} else {
resourceLock, err = options.newResourceLock(leaderConfig, leaderRecorderProvider, leaderelection.Options{
LeaderElection: options.LeaderElection,
LeaderElectionResourceLock: options.LeaderElectionResourceLock,
LeaderElectionID: options.LeaderElectionID,
LeaderElectionNamespace: options.LeaderElectionNamespace,
LeaderElection: options.LeaderElection,
ResourceLock: options.LeaderElectionResourceLock,
LockName: options.LeaderElectionLockName,
LockNamespace: options.LeaderElectionLockNamespace,
})
if err != nil {
return nil, err
Expand Down Expand Up @@ -423,7 +423,7 @@ func New(config *rest.Config, options Options) (Manager, error) {
logger: options.Logger,
elected: make(chan struct{}),
webhookServer: options.WebhookServer,
leaderElectionID: options.LeaderElectionID,
leaderElectionLockName: options.LeaderElectionLockName,
leaseDuration: *options.LeaseDuration,
renewDeadline: *options.RenewDeadline,
retryPeriod: *options.RetryPeriod,
Expand Down Expand Up @@ -535,12 +535,12 @@ func (o Options) setLeaderElectionConfig(obj v1alpha1.ControllerManagerConfigura
o.LeaderElectionResourceLock = obj.LeaderElection.ResourceLock
}

if o.LeaderElectionNamespace == "" && obj.LeaderElection.ResourceNamespace != "" {
o.LeaderElectionNamespace = obj.LeaderElection.ResourceNamespace
if o.LeaderElectionLockNamespace == "" && obj.LeaderElection.ResourceNamespace != "" {
o.LeaderElectionLockNamespace = obj.LeaderElection.ResourceNamespace
}

if o.LeaderElectionID == "" && obj.LeaderElection.ResourceName != "" {
o.LeaderElectionID = obj.LeaderElection.ResourceName
if o.LeaderElectionLockName == "" && obj.LeaderElection.ResourceName != "" {
o.LeaderElectionLockName = obj.LeaderElection.ResourceName
}

if o.LeaseDuration == nil && !reflect.DeepEqual(obj.LeaderElection.LeaseDuration, metav1.Duration{}) {
Expand Down
92 changes: 46 additions & 46 deletions pkg/manager/manager_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -160,8 +160,8 @@ var _ = Describe("manger.Manager", func() {
Expect(*m.Cache.SyncPeriod).To(Equal(duration.Duration))
Expect(m.LeaderElection).To(Equal(leaderElect))
Expect(m.LeaderElectionResourceLock).To(Equal("leases"))
Expect(m.LeaderElectionNamespace).To(Equal("default"))
Expect(m.LeaderElectionID).To(Equal("ctrl-lease"))
Expect(m.LeaderElectionLockNamespace).To(Equal("default"))
Expect(m.LeaderElectionLockName).To(Equal("ctrl-lease"))
Expect(m.LeaseDuration.String()).To(Equal(duration.Duration.String()))
Expect(m.RenewDeadline.String()).To(Equal(duration.Duration.String()))
Expect(m.RetryPeriod.String()).To(Equal(duration.Duration.String()))
Expand Down Expand Up @@ -218,17 +218,17 @@ var _ = Describe("manger.Manager", func() {
SyncPeriod: &optDuration,
DefaultNamespaces: map[string]cache.Config{"ctrl": {}},
},
LeaderElection: true,
LeaderElectionResourceLock: "configmaps",
LeaderElectionNamespace: "ctrl",
LeaderElectionID: "ctrl-configmap",
LeaseDuration: &optDuration,
RenewDeadline: &optDuration,
RetryPeriod: &optDuration,
Metrics: metricsserver.Options{BindAddress: ":7000"},
HealthProbeBindAddress: "5000",
ReadinessEndpointName: "/readiness",
LivenessEndpointName: "/liveness",
LeaderElection: true,
LeaderElectionResourceLock: "configmaps",
LeaderElectionLockNamespace: "ctrl",
LeaderElectionLockName: "ctrl-configmap",
LeaseDuration: &optDuration,
RenewDeadline: &optDuration,
RetryPeriod: &optDuration,
Metrics: metricsserver.Options{BindAddress: ":7000"},
HealthProbeBindAddress: "5000",
ReadinessEndpointName: "/readiness",
LivenessEndpointName: "/liveness",
WebhookServer: webhook.NewServer(webhook.Options{
Port: 8080,
Host: "example.com",
Expand All @@ -241,8 +241,8 @@ var _ = Describe("manger.Manager", func() {
Expect(m.Cache.SyncPeriod.String()).To(Equal(optDuration.String()))
Expect(m.LeaderElection).To(BeTrue())
Expect(m.LeaderElectionResourceLock).To(Equal("configmaps"))
Expect(m.LeaderElectionNamespace).To(Equal("ctrl"))
Expect(m.LeaderElectionID).To(Equal("ctrl-configmap"))
Expect(m.LeaderElectionLockNamespace).To(Equal("ctrl"))
Expect(m.LeaderElectionLockName).To(Equal("ctrl-configmap"))
Expect(m.LeaseDuration.String()).To(Equal(optDuration.String()))
Expect(m.RenewDeadline.String()).To(Equal(optDuration.String()))
Expect(m.RetryPeriod.String()).To(Equal(optDuration.String()))
Expand Down Expand Up @@ -302,12 +302,12 @@ var _ = Describe("manger.Manager", func() {
Context("with leader election enabled", func() {
It("should only cancel the leader election after all runnables are done", func() {
m, err := New(cfg, Options{
LeaderElection: true,
LeaderElectionNamespace: "default",
LeaderElectionID: "test-leader-election-id-2",
HealthProbeBindAddress: "0",
Metrics: metricsserver.Options{BindAddress: "0"},
PprofBindAddress: "0",
LeaderElection: true,
LeaderElectionLockNamespace: "default",
LeaderElectionLockName: "test-leader-election-id-2",
HealthProbeBindAddress: "0",
Metrics: metricsserver.Options{BindAddress: "0"},
PprofBindAddress: "0",
})
Expect(err).ToNot(HaveOccurred())
gvkcorev1 := schema.GroupVersionKind{Group: corev1.SchemeGroupVersion.Group, Version: corev1.SchemeGroupVersion.Version, Kind: "ConfigMap"}
Expand Down Expand Up @@ -351,12 +351,12 @@ var _ = Describe("manger.Manager", func() {
})
It("should disable gracefulShutdown when stopping to lead", func() {
m, err := New(cfg, Options{
LeaderElection: true,
LeaderElectionNamespace: "default",
LeaderElectionID: "test-leader-election-id-3",
HealthProbeBindAddress: "0",
Metrics: metricsserver.Options{BindAddress: "0"},
PprofBindAddress: "0",
LeaderElection: true,
LeaderElectionLockNamespace: "default",
LeaderElectionLockName: "test-leader-election-id-3",
HealthProbeBindAddress: "0",
Metrics: metricsserver.Options{BindAddress: "0"},
PprofBindAddress: "0",
})
Expect(err).ToNot(HaveOccurred())

Expand All @@ -381,9 +381,9 @@ var _ = Describe("manger.Manager", func() {
It("should default ID to controller-runtime if ID is not set", func() {
var rl resourcelock.Interface
m1, err := New(cfg, Options{
LeaderElection: true,
LeaderElectionNamespace: "default",
LeaderElectionID: "test-leader-election-id",
LeaderElection: true,
LeaderElectionLockNamespace: "default",
LeaderElectionLockName: "test-leader-election-id",
newResourceLock: func(config *rest.Config, recorderProvider recorder.Provider, options leaderelection.Options) (resourcelock.Interface, error) {
var err error
rl, err = leaderelection.NewResourceLock(config, recorderProvider, options)
Expand All @@ -402,9 +402,9 @@ var _ = Describe("manger.Manager", func() {
m1cm.onStoppedLeading = func() {}

m2, err := New(cfg, Options{
LeaderElection: true,
LeaderElectionNamespace: "default",
LeaderElectionID: "test-leader-election-id",
LeaderElection: true,
LeaderElectionLockNamespace: "default",
LeaderElectionLockName: "test-leader-election-id",
newResourceLock: func(config *rest.Config, recorderProvider recorder.Provider, options leaderelection.Options) (resourcelock.Interface, error) {
var err error
rl, err = leaderelection.NewResourceLock(config, recorderProvider, options)
Expand Down Expand Up @@ -471,17 +471,17 @@ var _ = Describe("manger.Manager", func() {
})

It("should return an error if namespace not set and not running in cluster", func() {
m, err := New(cfg, Options{LeaderElection: true, LeaderElectionID: "controller-runtime"})
m, err := New(cfg, Options{LeaderElection: true, LeaderElectionLockName: "controller-runtime"})
Expect(m).To(BeNil())
Expect(err).To(HaveOccurred())
Expect(err.Error()).To(ContainSubstring("unable to find leader election namespace: not running in-cluster, please specify LeaderElectionNamespace"))
Expect(err.Error()).To(ContainSubstring("unable to find leader election namespace: not running in-cluster, please specify LeaderElectionLockNamespace"))
})

// We must keep this default until we are sure all controller-runtime users have upgraded from the original default
// ConfigMap lock to a controller-runtime version that has this new default. Many users of controller-runtime skip
// versions, so we should be extremely conservative here.
It("should default to LeasesResourceLock", func() {
m, err := New(cfg, Options{LeaderElection: true, LeaderElectionID: "controller-runtime", LeaderElectionNamespace: "my-ns"})
m, err := New(cfg, Options{LeaderElection: true, LeaderElectionLockName: "controller-runtime", LeaderElectionLockNamespace: "my-ns"})
Expect(m).ToNot(BeNil())
Expect(err).ToNot(HaveOccurred())
cm, ok := m.(*controllerManager)
Expand All @@ -492,10 +492,10 @@ var _ = Describe("manger.Manager", func() {
})
It("should use the specified ResourceLock", func() {
m, err := New(cfg, Options{
LeaderElection: true,
LeaderElectionResourceLock: resourcelock.LeasesResourceLock,
LeaderElectionID: "controller-runtime",
LeaderElectionNamespace: "my-ns",
LeaderElection: true,
LeaderElectionResourceLock: resourcelock.LeasesResourceLock,
LeaderElectionLockName: "controller-runtime",
LeaderElectionLockNamespace: "my-ns",
})
Expect(m).ToNot(BeNil())
Expect(err).ToNot(HaveOccurred())
Expand All @@ -509,8 +509,8 @@ var _ = Describe("manger.Manager", func() {
m, err := New(cfg, Options{
LeaderElection: true,
LeaderElectionResourceLock: resourcelock.LeasesResourceLock,
LeaderElectionID: "controller-runtime",
LeaderElectionNamespace: "my-ns",
LeaderElectionLockName: "controller-runtime",
LeaderElectionLockNamespace: "my-ns",
LeaderElectionReleaseOnCancel: true,
newResourceLock: func(config *rest.Config, recorderProvider recorder.Provider, options leaderelection.Options) (resourcelock.Interface, error) {
var err error
Expand Down Expand Up @@ -1216,10 +1216,10 @@ var _ = Describe("manger.Manager", func() {
Context("with leaderelection enabled", func() {
startSuite(
Options{
LeaderElection: true,
LeaderElectionID: "controller-runtime",
LeaderElectionNamespace: "default",
newResourceLock: fakeleaderelection.NewResourceLock,
LeaderElection: true,
LeaderElectionLockName: "controller-runtime",
LeaderElectionLockNamespace: "default",
newResourceLock: fakeleaderelection.NewResourceLock,
},
func(m Manager) {
cm, ok := m.(*controllerManager)
Expand Down