From d192823c0778d1ae2ed466bbed6a2692326478a4 Mon Sep 17 00:00:00 2001 From: Jagpreet Singh Tamber Date: Thu, 28 Mar 2024 09:54:55 -0400 Subject: [PATCH 1/3] Release leader for life lock in case pod is preempted. Signed-off-by: Jagpreet Singh Tamber --- leader/leader.go | 10 ++++++++-- leader/leader_test.go | 25 +++++++++++++++++++++++++ 2 files changed, 33 insertions(+), 2 deletions(-) diff --git a/leader/leader.go b/leader/leader.go index 16d497c..ecb2b4c 100644 --- a/leader/leader.go +++ b/leader/leader.go @@ -179,8 +179,8 @@ func Become(ctx context.Context, lockName string, opts ...Option) error { log.Info("Leader pod has been deleted, waiting for garbage collection to remove the lock.") case err != nil: return err - case isPodEvicted(*leaderPod) && leaderPod.GetDeletionTimestamp() == nil: - log.Info("Operator pod with leader lock has been evicted.", "leader", leaderPod.Name) + case (isPodEvicted(*leaderPod) || isPodPreempted(*leaderPod)) && leaderPod.GetDeletionTimestamp() == nil: + log.Info("Operator pod with leader lock has been evicted or preempted.", "leader", leaderPod.Name) log.Info("Deleting evicted leader.") // Pod may not delete immediately, continue with backoff err := config.Client.Delete(ctx, leaderPod) @@ -241,6 +241,12 @@ func isPodEvicted(pod corev1.Pod) bool { return podFailed && podEvicted } +func isPodPreempted(pod corev1.Pod) bool { + podFailed := pod.Status.Phase == corev1.PodFailed + podPreempted := pod.Status.Reason == "Preempting" + return podFailed && podPreempted +} + // getPod returns a Pod object that corresponds to the pod in which the code // is currently running. // It expects the environment variable POD_NAME to be set by the downwards API. diff --git a/leader/leader_test.go b/leader/leader_test.go index 4798795..9fd812c 100644 --- a/leader/leader_test.go +++ b/leader/leader_test.go @@ -108,6 +108,31 @@ var _ = Describe("Leader election", func() { Expect(isPodEvicted(*leaderPod)).To(BeTrue()) }) }) + Describe("isPodPreempted", func() { + var ( + leaderPod *corev1.Pod + ) + BeforeEach(func() { + leaderPod = &corev1.Pod{} + }) + It("should return false with an empty status", func() { + Expect(isPodPreempted(*leaderPod)).To(BeFalse()) + }) + It("should return false if reason is incorrect", func() { + leaderPod.Status.Phase = corev1.PodFailed + leaderPod.Status.Reason = "invalid" + Expect(isPodPreempted(*leaderPod)).To(BeFalse()) + }) + It("should return false if pod is in the wrong phase", func() { + leaderPod.Status.Phase = corev1.PodRunning + Expect(isPodPreempted(*leaderPod)).To(BeFalse()) + }) + It("should return true when Phase and Reason are set", func() { + leaderPod.Status.Phase = corev1.PodFailed + leaderPod.Status.Reason = "Preempting" + Expect(isPodPreempted(*leaderPod)).To(BeTrue()) + }) + }) Describe("myOwnerRef", func() { var ( client crclient.Client From f4b850e313ed3f51e9cdf3841aa78636fe17333a Mon Sep 17 00:00:00 2001 From: Jagpreet Singh Tamber Date: Thu, 28 Mar 2024 15:13:32 -0400 Subject: [PATCH 2/3] Review comments Signed-off-by: Jagpreet Singh Tamber --- leader/leader.go | 12 +++- leader/leader_test.go | 144 ++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 154 insertions(+), 2 deletions(-) diff --git a/leader/leader.go b/leader/leader.go index ecb2b4c..7623fe2 100644 --- a/leader/leader.go +++ b/leader/leader.go @@ -179,14 +179,22 @@ func Become(ctx context.Context, lockName string, opts ...Option) error { log.Info("Leader pod has been deleted, waiting for garbage collection to remove the lock.") case err != nil: return err - case (isPodEvicted(*leaderPod) || isPodPreempted(*leaderPod)) && leaderPod.GetDeletionTimestamp() == nil: - log.Info("Operator pod with leader lock has been evicted or preempted.", "leader", leaderPod.Name) + case isPodEvicted(*leaderPod) && leaderPod.GetDeletionTimestamp() == nil: + log.Info("Operator pod with leader lock has been evicted.", "leader", leaderPod.Name) log.Info("Deleting evicted leader.") // Pod may not delete immediately, continue with backoff err := config.Client.Delete(ctx, leaderPod) if err != nil { log.Error(err, "Leader pod could not be deleted.") } + case isPodPreempted(*leaderPod) && leaderPod.GetDeletionTimestamp() == nil: + log.Info("Operator pod with leader lock has been preempted.", "leader", leaderPod.Name) + log.Info("Deleting preempted leader.") + // Pod may not delete immediately, continue with backoff + err := config.Client.Delete(ctx, leaderPod) + if err != nil { + log.Error(err, "Leader pod could not be deleted.") + } case isNotReadyNode(ctx, config.Client, leaderPod.Spec.NodeName): log.Info("the status of the node where operator pod with leader lock was running has been 'notReady'") log.Info("Deleting the leader.") diff --git a/leader/leader_test.go b/leader/leader_test.go index 9fd812c..35d49df 100644 --- a/leader/leader_test.go +++ b/leader/leader_test.go @@ -21,9 +21,11 @@ import ( . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" corev1 "k8s.io/api/core/v1" + v1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" crclient "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/client/fake" + "sigs.k8s.io/controller-runtime/pkg/client/interceptor" ) var _ = Describe("Leader election", func() { @@ -82,6 +84,148 @@ var _ = Describe("Leader election", func() { Expect(Become(context.TODO(), "leader-test", WithClient(client))).To(Succeed()) }) + It("should become leader when pod is evicted and rescheduled", func() { + + evictedPodStatusClient := fake.NewClientBuilder().WithObjects( + &corev1.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Name: "leader-test-new", + Namespace: "testns", + OwnerReferences: []metav1.OwnerReference{ + { + APIVersion: "v1", + Kind: "Pod", + Name: "leader-test-new", + }, + }, + }, + }, + &corev1.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Name: "leader-test", + Namespace: "testns", + OwnerReferences: []metav1.OwnerReference{ + { + APIVersion: "v1", + Kind: "Pod", + Name: "leader-test", + }, + }, + }, + Status: v1.PodStatus{ + Phase: v1.PodFailed, + Reason: "Evicted", + }, + }, + &corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{ + Name: "leader-test", + Namespace: "testns", + OwnerReferences: []metav1.OwnerReference{ + { + APIVersion: "v1", + Kind: "Pod", + Name: "leader-test", + }, + }, + }, + }, + ).WithInterceptorFuncs( + interceptor.Funcs{ + // Mock garbage collection of the ConfigMap when the Pod is deleted. + Delete: func(ctx context.Context, client crclient.WithWatch, obj crclient.Object, opts ...crclient.DeleteOption) error { + if obj.GetObjectKind() != nil && obj.GetObjectKind().GroupVersionKind().Kind == "Pod" && obj.GetName() == "leader-test" { + cm := &corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{ + Name: "leader-test", + Namespace: "testns", + }, + } + client.Delete(ctx, cm) + } + return nil + }, + }, + ).Build() + + os.Setenv("POD_NAME", "leader-test-new") + readNamespace = func() (string, error) { + return "testns", nil + } + + Expect(Become(context.TODO(), "leader-test", WithClient(evictedPodStatusClient))).To(Succeed()) + }) + It("should become leader when pod is preempted and rescheduled", func() { + + preemptedPodStatusClient := fake.NewClientBuilder().WithObjects( + &corev1.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Name: "leader-test-new", + Namespace: "testns", + OwnerReferences: []metav1.OwnerReference{ + { + APIVersion: "v1", + Kind: "Pod", + Name: "leader-test-new", + }, + }, + }, + }, + &corev1.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Name: "leader-test", + Namespace: "testns", + OwnerReferences: []metav1.OwnerReference{ + { + APIVersion: "v1", + Kind: "Pod", + Name: "leader-test", + }, + }, + }, + Status: v1.PodStatus{ + Phase: v1.PodFailed, + Reason: "Preempting", + }, + }, + &corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{ + Name: "leader-test", + Namespace: "testns", + OwnerReferences: []metav1.OwnerReference{ + { + APIVersion: "v1", + Kind: "Pod", + Name: "leader-test", + }, + }, + }, + }, + ).WithInterceptorFuncs( + interceptor.Funcs{ + // Mock garbage collection of the ConfigMap when the Pod is deleted. + Delete: func(ctx context.Context, client crclient.WithWatch, obj crclient.Object, opts ...crclient.DeleteOption) error { + if obj.GetObjectKind() != nil && obj.GetObjectKind().GroupVersionKind().Kind == "Pod" && obj.GetName() == "leader-test" { + cm := &corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{ + Name: "leader-test", + Namespace: "testns", + }, + } + client.Delete(ctx, cm) + } + return nil + }, + }, + ).Build() + + os.Setenv("POD_NAME", "leader-test-new") + readNamespace = func() (string, error) { + return "testns", nil + } + + Expect(Become(context.TODO(), "leader-test", WithClient(preemptedPodStatusClient))).To(Succeed()) + }) }) Describe("isPodEvicted", func() { var ( From 719af6b285343800bc21824f0f1df943e3103224 Mon Sep 17 00:00:00 2001 From: Jagpreet Singh Tamber Date: Thu, 28 Mar 2024 16:12:16 -0400 Subject: [PATCH 3/3] Fix linting errors. Signed-off-by: Jagpreet Singh Tamber --- leader/leader_test.go | 21 ++++++++++++++------- 1 file changed, 14 insertions(+), 7 deletions(-) diff --git a/leader/leader_test.go b/leader/leader_test.go index 35d49df..5817211 100644 --- a/leader/leader_test.go +++ b/leader/leader_test.go @@ -21,7 +21,6 @@ import ( . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" corev1 "k8s.io/api/core/v1" - v1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" crclient "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/client/fake" @@ -112,8 +111,8 @@ var _ = Describe("Leader election", func() { }, }, }, - Status: v1.PodStatus{ - Phase: v1.PodFailed, + Status: corev1.PodStatus{ + Phase: corev1.PodFailed, Reason: "Evicted", }, }, @@ -141,7 +140,11 @@ var _ = Describe("Leader election", func() { Namespace: "testns", }, } - client.Delete(ctx, cm) + + err := client.Delete(ctx, cm) + if err != nil { + return err + } } return nil }, @@ -183,8 +186,8 @@ var _ = Describe("Leader election", func() { }, }, }, - Status: v1.PodStatus{ - Phase: v1.PodFailed, + Status: corev1.PodStatus{ + Phase: corev1.PodFailed, Reason: "Preempting", }, }, @@ -212,7 +215,11 @@ var _ = Describe("Leader election", func() { Namespace: "testns", }, } - client.Delete(ctx, cm) + + err := client.Delete(ctx, cm) + if err != nil { + return err + } } return nil },