Skip to content

Drop event recorder from shard lease lock #508

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

Merged
merged 1 commit into from
Mar 16, 2025
Merged
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
2 changes: 1 addition & 1 deletion cmd/checksum-controller/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -125,7 +125,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
Expand Down
2 changes: 1 addition & 1 deletion docs/implement-sharding.md
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
19 changes: 3 additions & 16 deletions pkg/shard/lease/lease.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand All @@ -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")

Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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
Expand Down
38 changes: 8 additions & 30 deletions pkg/shard/lease/lease_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"

Expand Down Expand Up @@ -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)
Expand All @@ -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)
Expand All @@ -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)
Expand All @@ -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"),
)))
Expand All @@ -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,
Expand Down Expand Up @@ -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")
})
})

Expand Down
2 changes: 1 addition & 1 deletion test/integration/shard/lease/lease_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
2 changes: 1 addition & 1 deletion webhosting-operator/cmd/webhosting-operator/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
Loading