Drop event recorder from shard lease lock

Author: timebertt

cmd/shard/main.go

@@ -126,7 +126,7 @@ func (o *options) run(ctx context.Context) error {
 	}
 
 	log.Info("Setting up shard lease")
-	shardLease, err := shardlease.NewResourceLock(restConfig, nil, shardlease.Options{
+	shardLease, err := shardlease.NewResourceLock(restConfig, shardlease.Options{
 		ControllerRingName: o.controllerRingName,
 		LeaseNamespace:     o.leaseNamespace, // optional, can be empty
 		ShardName:          o.shardName,      // optional, can be empty

docs/implement-sharding.md

@@ -135,7 +135,7 @@ import (
 func run() error {
 	restConfig := config.GetConfigOrDie()
 
-	shardLease, err := shardlease.NewResourceLock(restConfig, nil, shardlease.Options{
+	shardLease, err := shardlease.NewResourceLock(restConfig, shardlease.Options{
 		ControllerRingName: "my-controllerring",
 	})
 	if err != nil {

pkg/shard/lease/lease.go

@@ -25,7 +25,6 @@ import (
 	"os"
 
 	coordinationv1 "k8s.io/api/coordination/v1"
-	corev1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	coordinationv1client "k8s.io/client-go/kubernetes/typed/coordination/v1"
 	"k8s.io/client-go/rest"
@@ -50,7 +49,7 @@ type Options struct {
 // NewResourceLock returns a new resource lock that implements the shard lease.
 // Pass this to the leader elector, e.g., leaderelection.LeaderElectionConfig.Lock (if using plain client-go) or
 // manager.Options.LeaderElectionResourceLockInterface (if using controller-runtime).
-func NewResourceLock(config *rest.Config, eventRecorder resourcelock.EventRecorder, options Options) (resourcelock.Interface, error) {
+func NewResourceLock(config *rest.Config, options Options) (resourcelock.Interface, error) {
 	// Construct client for leader election
 	rest.AddUserAgent(config, "shard-lease")
 
@@ -89,8 +88,6 @@ func NewResourceLock(config *rest.Config, eventRecorder resourcelock.EventRecord
 		Client: coordinationClient,
 		LockConfig: resourcelock.ResourceLockConfig{
 			Identity: options.ShardName,
-			// eventRecorder is optional
-			EventRecorder: eventRecorder,
 		},
 		Labels: map[string]string{
 			shardingv1alpha1.LabelControllerRing: options.ControllerRingName,
@@ -158,18 +155,8 @@ func (ll *LeaseLock) Update(ctx context.Context, ler resourcelock.LeaderElection
 	return nil
 }
 
-// RecordEvent in leader election while adding meta-data
-func (ll *LeaseLock) RecordEvent(s string) {
-	if ll.LockConfig.EventRecorder == nil {
-		return
-	}
-	events := fmt.Sprintf("%v %v", ll.LockConfig.Identity, s)
-	subject := &coordinationv1.Lease{ObjectMeta: ll.lease.ObjectMeta}
-	// Populate the type meta, so we don't have to get it from the schema
-	subject.Kind = "Lease"
-	subject.APIVersion = coordinationv1.SchemeGroupVersion.String()
-	ll.LockConfig.EventRecorder.Eventf(subject, corev1.EventTypeNormal, "LeaderElection", events)
-}
+// RecordEvent does nothing, as recording events on shard leases is not meaningful.
+func (ll *LeaseLock) RecordEvent(string) {}
 
 // Describe is used to convert details on current resource lock
 // into a string

pkg/shard/lease/lease_test.go

@@ -29,7 +29,6 @@ import (
 	coordinationv1client "k8s.io/client-go/kubernetes/typed/coordination/v1"
 	"k8s.io/client-go/rest"
 	"k8s.io/client-go/tools/leaderelection/resourcelock"
-	"k8s.io/client-go/tools/record"
 	"k8s.io/utils/ptr"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 
@@ -75,11 +74,11 @@ var _ = Describe("LeaseLock", func() {
 		It("should fail if ControllerRingName is empty", func() {
 			options.ControllerRingName = ""
 
-			Expect(NewResourceLock(restConfig, nil, options)).Error().To(MatchError("ControllerRingName is required"))
+			Expect(NewResourceLock(restConfig, options)).Error().To(MatchError("ControllerRingName is required"))
 		})
 
 		It("should use the configured namespace and name", func() {
-			resourceLock, err := NewResourceLock(restConfig, nil, options)
+			resourceLock, err := NewResourceLock(restConfig, options)
 			Expect(err).NotTo(HaveOccurred())
 
 			leaseLock := resourceLock.(*LeaseLock)
@@ -93,7 +92,7 @@ var _ = Describe("LeaseLock", func() {
 			hostname, err := os.Hostname()
 			Expect(err).NotTo(HaveOccurred())
 
-			resourceLock, err := NewResourceLock(restConfig, nil, options)
+			resourceLock, err := NewResourceLock(restConfig, options)
 			Expect(err).NotTo(HaveOccurred())
 
 			leaseLock := resourceLock.(*LeaseLock)
@@ -104,7 +103,7 @@ var _ = Describe("LeaseLock", func() {
 		It("should default the namespace to the in-cluster namespace", func() {
 			options.LeaseNamespace = ""
 
-			resourceLock, err := NewResourceLock(restConfig, nil, options)
+			resourceLock, err := NewResourceLock(restConfig, options)
 			Expect(err).NotTo(HaveOccurred())
 
 			leaseLock := resourceLock.(*LeaseLock)
@@ -115,7 +114,7 @@ var _ = Describe("LeaseLock", func() {
 			options.LeaseNamespace = ""
 			fsys = fstest.MapFS{}
 
-			Expect(NewResourceLock(restConfig, nil, options)).Error().To(MatchError(And(
+			Expect(NewResourceLock(restConfig, options)).Error().To(MatchError(And(
 				ContainSubstring("not running in cluster"),
 				ContainSubstring("please specify LeaseNamespace"),
 			)))
@@ -132,7 +131,7 @@ var _ = Describe("LeaseLock", func() {
 
 		BeforeEach(func() {
 			var err error
-			lock, err = NewResourceLock(&rest.Config{}, nil, Options{
+			lock, err = NewResourceLock(&rest.Config{}, Options{
 				ControllerRingName: controllerRingName,
 				LeaseNamespace:     namespace,
 				ShardName:          shardName,
@@ -235,29 +234,8 @@ var _ = Describe("LeaseLock", func() {
 		})
 
 		Describe("#RecordEvent", func() {
-			Context("no EventRecorder configured", func() {
-				It("should do nothing", func() {
-					lock.RecordEvent("foo")
-				})
-			})
-
-			Context("EventRecorder configured", func() {
-				var recorder *record.FakeRecorder
-
-				BeforeEach(func() {
-					recorder = record.NewFakeRecorder(1)
-					lock.(*LeaseLock).LockConfig.EventRecorder = recorder
-				})
-
-				It("should send the event", func() {
-					Expect(lock.Get(ctx)).Error().To(Succeed())
-
-					lock.RecordEvent("foo")
-
-					Eventually(recorder.Events).Should(Receive(
-						Equal("Normal LeaderElection " + shardName + " foo"),
-					))
-				})
+			It("should do nothing", func() {
+				lock.RecordEvent("foo")
 			})
 		})
 

test/integration/shard/lease/lease_test.go

@@ -68,7 +68,7 @@ var _ = Describe("Shard lease", func() {
 	}, OncePerOrdered)
 
 	JustBeforeEach(func() {
-		shardLease, err := shardlease.NewResourceLock(restConfig, nil, leaseOptions)
+		shardLease, err := shardlease.NewResourceLock(restConfig, leaseOptions)
 		Expect(err).NotTo(HaveOccurred())
 		mgrOptions.LeaderElectionResourceLockInterface = shardLease
 

webhosting-operator/cmd/webhosting-operator/main.go

@@ -273,7 +273,7 @@ func (o *options) applyOptionsOverrides() error {
 
 		// SHARD LEASE
 		o.controllerRingName = "webhosting-operator"
-		shardLease, err := shardlease.NewResourceLock(o.restConfig, nil, shardlease.Options{
+		shardLease, err := shardlease.NewResourceLock(o.restConfig, shardlease.Options{
 			ControllerRingName: o.controllerRingName,
 		})
 		if err != nil {