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", + }, }, }