From 35f81997aff31544c39446c765e1030716179e3c Mon Sep 17 00:00:00 2001 From: Miguel Angel Ajo Pelayo Date: Fri, 3 Oct 2025 11:45:51 +0000 Subject: [PATCH] Offline exporters should be Pending and not Unsatisfiable There are situations where an exporter is the only existing one, but the service could be restarting, or the exporter could be rebooting due to OS updates. We don't want to make it unsatisfiable, but instead keep trying until the node comes back. We leave the decission of canceling the lease to the client, which will be informed of the reason for which the lease is pending "Offline" --- internal/controller/lease_controller.go | 56 ++++---- internal/controller/lease_controller_test.go | 133 ++++++++++++++++++- internal/controller/suite_test.go | 3 + 3 files changed, 165 insertions(+), 27 deletions(-) diff --git a/internal/controller/lease_controller.go b/internal/controller/lease_controller.go index bcb548fc..3b2b68ea 100644 --- a/internal/controller/lease_controller.go +++ b/internal/controller/lease_controller.go @@ -204,19 +204,7 @@ func (r *LeaseReconciler) reconcileStatusExporterRef( return fmt.Errorf("reconcileStatusExporterRef: failed to list matching exporters: %w", err) } - // Filter out offline exporters - onlineExporters := filterOutOfflineExporters(matchingExporters.Items) - - // No matching exporter online, lease unsatisfiable - if len(onlineExporters) == 0 { - lease.SetStatusUnsatisfiable( - "NoExporter", - "There are no online exporter matching the selector, but there are %d matching offline exporters", - len(matchingExporters.Items)) - return nil - } - - approvedExporters, err := r.attachMatchingPolicies(ctx, lease, onlineExporters) + approvedExporters, err := r.attachMatchingPolicies(ctx, lease, matchingExporters.Items) if err != nil { return fmt.Errorf("reconcileStatusExporterRef: failed to handle policy approval: %w", err) } @@ -224,18 +212,32 @@ func (r *LeaseReconciler) reconcileStatusExporterRef( if len(approvedExporters) == 0 { lease.SetStatusUnsatisfiable( "NoAccess", - "While there are %d online exporters matching the selector, none of them are approved by any policy for your client", - len(onlineExporters)) + "While there are %d exporters matching the selector, none of them are approved by any policy for your client", + len(matchingExporters.Items), + ) return nil } + + onlineApprovedExporters := filterOutOfflineExporters(approvedExporters) + if len(onlineApprovedExporters) == 0 { + lease.SetStatusPending( + "Offline", + "While there are %d available exporters (i.e. %s), none of them are online", + len(approvedExporters), + approvedExporters[0].Exporter.Name, + ) + result.RequeueAfter = time.Second + return nil + } + // Filter out exporters that are already leased activeLeases, err := r.ListActiveLeases(ctx, lease.Namespace) if err != nil { return fmt.Errorf("reconcileStatusExporterRef: failed to list active leases: %w", err) } - approvedExporters = attachExistingLeases(approvedExporters, activeLeases.Items) - orderedExporters := orderApprovedExporters(approvedExporters) + onlineApprovedExporters = attachExistingLeases(onlineApprovedExporters, activeLeases.Items) + orderedExporters := orderApprovedExporters(onlineApprovedExporters) if len(orderedExporters) > 0 && orderedExporters[0].Policy.SpotAccess { lease.SetStatusUnsatisfiable("SpotAccess", @@ -244,11 +246,13 @@ func (r *LeaseReconciler) reconcileStatusExporterRef( return nil } - availableExporters := filterOutLeasedExporters(orderedExporters) + availableExporters := filterOutLeasedExporters(onlineApprovedExporters) if len(availableExporters) == 0 { lease.SetStatusPending("NotAvailable", - "There are %d approved exporters, but all of them are already leased", - len(approvedExporters)) + "There are %d approved exporters, (i.e. %s) but all of them are already leased", + len(onlineApprovedExporters), + onlineApprovedExporters[0].Exporter.Name, + ) result.RequeueAfter = time.Second return nil } @@ -461,15 +465,15 @@ func filterOutLeasedExporters(exporters []ApprovedExporter) []ApprovedExporter { } // filterOutOfflineExporters filters out the exporters that are not online -func filterOutOfflineExporters(matchingExporters []jumpstarterdevv1alpha1.Exporter) []jumpstarterdevv1alpha1.Exporter { +func filterOutOfflineExporters(approvedExporters []ApprovedExporter) []ApprovedExporter { onlineExporters := slices.DeleteFunc( - matchingExporters, - func(exporter jumpstarterdevv1alpha1.Exporter) bool { - return !true || !meta.IsStatusConditionTrue( - exporter.Status.Conditions, + approvedExporters, + func(approvedExporter ApprovedExporter) bool { + return !meta.IsStatusConditionTrue( + approvedExporter.Exporter.Status.Conditions, string(jumpstarterdevv1alpha1.ExporterConditionTypeRegistered), ) || !meta.IsStatusConditionTrue( - exporter.Status.Conditions, + approvedExporter.Exporter.Status.Conditions, string(jumpstarterdevv1alpha1.ExporterConditionTypeOnline), ) }, diff --git a/internal/controller/lease_controller_test.go b/internal/controller/lease_controller_test.go index 9421fcbe..281f0c99 100644 --- a/internal/controller/lease_controller_test.go +++ b/internal/controller/lease_controller_test.go @@ -150,11 +150,69 @@ var _ = Describe("Lease Controller", func() { }) When("trying to lease an offline exporter", func() { - It("should fail right away", func() { + It("should set status to pending with offline reason", func() { + lease := leaseDutA2Sec.DeepCopy() + + ctx := context.Background() + + setExporterOnlineConditions(ctx, testExporter1DutA.Name, metav1.ConditionFalse) + setExporterOnlineConditions(ctx, testExporter2DutA.Name, metav1.ConditionFalse) + + Expect(k8sClient.Create(ctx, lease)).To(Succeed()) + _ = reconcileLease(ctx, lease) + + updatedLease := getLease(ctx, lease.Name) + Expect(updatedLease.Status.ExporterRef).To(BeNil()) + + Expect(meta.IsStatusConditionTrue( + updatedLease.Status.Conditions, + string(jumpstarterdevv1alpha1.LeaseConditionTypePending), + )).To(BeTrue()) + + // Check that the condition has the correct reason + condition := meta.FindStatusCondition(updatedLease.Status.Conditions, string(jumpstarterdevv1alpha1.LeaseConditionTypePending)) + Expect(condition).NotTo(BeNil()) + Expect(condition.Reason).To(Equal("Offline")) + }) + }) + + When("trying to lease approved exporters that are offline", func() { + It("should set status to pending with offline reason", func() { lease := leaseDutA2Sec.DeepCopy() ctx := context.Background() + // Create a policy that approves the exporters + policy := &jumpstarterdevv1alpha1.ExporterAccessPolicy{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-policy", + Namespace: "default", + }, + Spec: jumpstarterdevv1alpha1.ExporterAccessPolicySpec{ + ExporterSelector: metav1.LabelSelector{ + MatchLabels: map[string]string{ + "dut": "a", + }, + }, + Policies: []jumpstarterdevv1alpha1.Policy{ + { + Priority: 0, + From: []jumpstarterdevv1alpha1.From{ + { + ClientSelector: metav1.LabelSelector{ + MatchLabels: map[string]string{ + "name": "client", + }, + }, + }, + }, + }, + }, + }, + } + Expect(k8sClient.Create(ctx, policy)).To(Succeed()) + + // Set exporters offline while they are approved by policy setExporterOnlineConditions(ctx, testExporter1DutA.Name, metav1.ConditionFalse) setExporterOnlineConditions(ctx, testExporter2DutA.Name, metav1.ConditionFalse) @@ -164,10 +222,77 @@ var _ = Describe("Lease Controller", func() { updatedLease := getLease(ctx, lease.Name) Expect(updatedLease.Status.ExporterRef).To(BeNil()) + Expect(meta.IsStatusConditionTrue( + updatedLease.Status.Conditions, + string(jumpstarterdevv1alpha1.LeaseConditionTypePending), + )).To(BeTrue()) + + // Check that the condition has the correct reason + condition := meta.FindStatusCondition(updatedLease.Status.Conditions, string(jumpstarterdevv1alpha1.LeaseConditionTypePending)) + Expect(condition).NotTo(BeNil()) + Expect(condition.Reason).To(Equal("Offline")) + Expect(condition.Message).To(ContainSubstring("none of them are online")) + + // Clean up + Expect(k8sClient.Delete(ctx, policy)).To(Succeed()) + }) + }) + + When("trying to lease exporters that match selector but are not approved by any policy", func() { + It("should set status to unsatisfiable with NoAccess reason", func() { + lease := leaseDutA2Sec.DeepCopy() + + ctx := context.Background() + + // Create a policy that does NOT approve the exporters (different client selector) + policy := &jumpstarterdevv1alpha1.ExporterAccessPolicy{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-policy", + Namespace: "default", + }, + Spec: jumpstarterdevv1alpha1.ExporterAccessPolicySpec{ + ExporterSelector: metav1.LabelSelector{ + MatchLabels: map[string]string{ + "dut": "a", + }, + }, + Policies: []jumpstarterdevv1alpha1.Policy{ + { + Priority: 0, + From: []jumpstarterdevv1alpha1.From{ + { + ClientSelector: metav1.LabelSelector{ + MatchLabels: map[string]string{ + "name": "different-client", // Different from testClient + }, + }, + }, + }, + }, + }, + }, + } + Expect(k8sClient.Create(ctx, policy)).To(Succeed()) + + Expect(k8sClient.Create(ctx, lease)).To(Succeed()) + _ = reconcileLease(ctx, lease) + + updatedLease := getLease(ctx, lease.Name) + Expect(updatedLease.Status.ExporterRef).To(BeNil()) + Expect(meta.IsStatusConditionTrue( updatedLease.Status.Conditions, string(jumpstarterdevv1alpha1.LeaseConditionTypeUnsatisfiable), )).To(BeTrue()) + + // Check that the condition has the correct reason + condition := meta.FindStatusCondition(updatedLease.Status.Conditions, string(jumpstarterdevv1alpha1.LeaseConditionTypeUnsatisfiable)) + Expect(condition).NotTo(BeNil()) + Expect(condition.Reason).To(Equal("NoAccess")) + Expect(condition.Message).To(ContainSubstring("none of them are approved by any policy")) + + // Clean up + Expect(k8sClient.Delete(ctx, policy)).To(Succeed()) }) }) @@ -225,6 +350,12 @@ var _ = Describe("Lease Controller", func() { updatedLease.Status.Conditions, string(jumpstarterdevv1alpha1.LeaseConditionTypePending), )).To(BeTrue()) + + // Check that the condition has the correct reason and message format + condition := meta.FindStatusCondition(updatedLease.Status.Conditions, string(jumpstarterdevv1alpha1.LeaseConditionTypePending)) + Expect(condition).NotTo(BeNil()) + Expect(condition.Reason).To(Equal("NotAvailable")) + Expect(condition.Message).To(ContainSubstring("but all of them are already leased")) }) It("should be acquired when a valid exporter lease times out", func() { diff --git a/internal/controller/suite_test.go b/internal/controller/suite_test.go index 0613e465..ce79acd6 100644 --- a/internal/controller/suite_test.go +++ b/internal/controller/suite_test.go @@ -104,6 +104,9 @@ var testClient = &jumpstarterdevv1alpha1.Client{ ObjectMeta: metav1.ObjectMeta{ Name: "client", Namespace: "default", + Labels: map[string]string{ + "name": "client", + }, }, }