From 0960cf972c9e07ef4e6ed4b33f56a367407f155a Mon Sep 17 00:00:00 2001 From: UnfamousThomas Date: Tue, 13 May 2025 22:26:10 +0300 Subject: [PATCH] Add some tests for fleet oldest checking utilities --- operator/cmd/main.go | 7 +- .../internal/controller/fleet_controller.go | 7 +- .../controller/fleet_controller_test.go | 58 ++++--- operator/internal/utils/fleet_deletion.go | 82 +++++----- .../internal/utils/fleet_deletion_test.go | 152 ++++++++++++++++++ operator/internal/utils/suite_test.go | 24 +++ 6 files changed, 262 insertions(+), 68 deletions(-) create mode 100644 operator/internal/utils/fleet_deletion_test.go create mode 100644 operator/internal/utils/suite_test.go diff --git a/operator/cmd/main.go b/operator/cmd/main.go index 3175034..7552bc5 100644 --- a/operator/cmd/main.go +++ b/operator/cmd/main.go @@ -172,9 +172,10 @@ func main() { } } if err = (&controller.FleetReconciler{ - Client: mgr.GetClient(), - Recorder: mgr.GetEventRecorderFor("fleet"), - Scheme: mgr.GetScheme(), + Client: mgr.GetClient(), + Recorder: mgr.GetEventRecorderFor("fleet"), + DeletionChecker: prodChecker, + Scheme: mgr.GetScheme(), }).SetupWithManager(mgr); err != nil { setupLog.Error(err, "unable to create controller", "controller", "Fleet") os.Exit(1) diff --git a/operator/internal/controller/fleet_controller.go b/operator/internal/controller/fleet_controller.go index d25e8ca..0b55ce9 100644 --- a/operator/internal/controller/fleet_controller.go +++ b/operator/internal/controller/fleet_controller.go @@ -35,8 +35,9 @@ const FLEET_FINALIZER = "fleets.unfamousthomas.me/finalizer" // FleetReconciler reconciles a Fleet object type FleetReconciler struct { client.Client - Scheme *runtime.Scheme - Recorder record.EventRecorder + Scheme *runtime.Scheme + Recorder record.EventRecorder + DeletionChecker utils.FleetDeletionChecker } // +kubebuilder:rbac:groups=network.unfamousthomas.me,resources=fleets,verbs=get;list;watch;create;update;patch;delete @@ -128,7 +129,7 @@ func (r *FleetReconciler) scaleServerCount(ctx context.Context, fleet *networkv1 if err != nil { return err } - server, err := utils.FindDeleteServer(ctx, fleet, servers, r.Client) + server, err := utils.FindDeleteServer(ctx, fleet, servers, r.Client, r.DeletionChecker) if err != nil { return err } diff --git a/operator/internal/controller/fleet_controller_test.go b/operator/internal/controller/fleet_controller_test.go index 006ab8b..4b4e569 100644 --- a/operator/internal/controller/fleet_controller_test.go +++ b/operator/internal/controller/fleet_controller_test.go @@ -22,6 +22,7 @@ import ( . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" networkv1alpha1 "github.com/unfamousthomas/thesis-operator/api/v1alpha1" + "github.com/unfamousthomas/thesis-operator/internal/utils" "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/types" @@ -30,6 +31,7 @@ import ( "time" ) +var prodChecker = utils.ProdDeletionChecker{} var basicFleetSpec = networkv1alpha1.FleetSpec{ Scaling: networkv1alpha1.FleetScaling{ Replicas: 2, @@ -79,9 +81,10 @@ var _ = Describe("Fleet Controller", func() { Expect(k8sClient.Delete(ctx, fleet)).To(Succeed()) recorder := NewFakeRecorder() reconciler := &FleetReconciler{ - Client: k8sClient, - Scheme: k8sClient.Scheme(), - Recorder: recorder, + Client: k8sClient, + Scheme: k8sClient.Scheme(), + Recorder: recorder, + DeletionChecker: prodChecker, } _, err = reconciler.Reconcile(ctx, reconcile.Request{NamespacedName: namespacedName}) Expect(err).ToNot(HaveOccurred()) @@ -105,9 +108,10 @@ var _ = Describe("Fleet Controller", func() { By("Setting up reconciler") recorder := NewFakeRecorder() reconciler := &FleetReconciler{ - Client: k8sClient, - Scheme: k8sClient.Scheme(), - Recorder: recorder, + Client: k8sClient, + Scheme: k8sClient.Scheme(), + Recorder: recorder, + DeletionChecker: prodChecker, } By("Initial reconcile") @@ -161,9 +165,10 @@ var _ = Describe("Fleet Controller", func() { }) It("should delete all servers and remove the finalizer on fleet deletion", func() { reconciler := &FleetReconciler{ - Client: k8sClient, - Scheme: k8sClient.Scheme(), - Recorder: NewFakeRecorder(), + Client: k8sClient, + Scheme: k8sClient.Scheme(), + Recorder: NewFakeRecorder(), + DeletionChecker: prodChecker, } // Initial reconciles to create servers @@ -204,9 +209,10 @@ var _ = Describe("Fleet Controller", func() { It("should add a finalizer if not present", func() { reconciler := &FleetReconciler{ - Client: k8sClient, - Scheme: k8sClient.Scheme(), - Recorder: NewFakeRecorder(), + Client: k8sClient, + Scheme: k8sClient.Scheme(), + Recorder: NewFakeRecorder(), + DeletionChecker: prodChecker, } _, err := reconciler.Reconcile(ctx, reconcile.Request{NamespacedName: namespacedName}) @@ -219,9 +225,10 @@ var _ = Describe("Fleet Controller", func() { It("should scale up servers to match the desired replicas", func() { reconciler := &FleetReconciler{ - Client: k8sClient, - Scheme: k8sClient.Scheme(), - Recorder: NewFakeRecorder(), + Client: k8sClient, + Scheme: k8sClient.Scheme(), + Recorder: NewFakeRecorder(), + DeletionChecker: prodChecker, } _, err := reconciler.Reconcile(ctx, reconcile.Request{NamespacedName: namespacedName}) @@ -262,9 +269,10 @@ var _ = Describe("Fleet Controller", func() { It("should delete all servers when fleet is deleted", func() { reconciler := &FleetReconciler{ - Client: k8sClient, - Scheme: k8sClient.Scheme(), - Recorder: NewFakeRecorder(), + Client: k8sClient, + Scheme: k8sClient.Scheme(), + Recorder: NewFakeRecorder(), + DeletionChecker: prodChecker, } // Ensure the fleet is scaled first @@ -307,9 +315,10 @@ var _ = Describe("Fleet Controller", func() { FailPatch: false, } reconciler := &FleetReconciler{ - Client: fakeClient, - Scheme: k8sClient.Scheme(), - Recorder: NewFakeRecorder(), + Client: fakeClient, + Scheme: k8sClient.Scheme(), + Recorder: NewFakeRecorder(), + DeletionChecker: prodChecker, } By("Fail Get") @@ -347,9 +356,10 @@ var _ = Describe("Fleet Controller", func() { FailPatch: false, } reconciler := &FleetReconciler{ - Client: fakeClient, - Scheme: k8sClient.Scheme(), - Recorder: NewFakeRecorder(), + Client: fakeClient, + Scheme: k8sClient.Scheme(), + Recorder: NewFakeRecorder(), + DeletionChecker: prodChecker, } By("Fail Create") diff --git a/operator/internal/utils/fleet_deletion.go b/operator/internal/utils/fleet_deletion.go index d2c51ae..e7b23de 100644 --- a/operator/internal/utils/fleet_deletion.go +++ b/operator/internal/utils/fleet_deletion.go @@ -11,18 +11,22 @@ import ( "sigs.k8s.io/controller-runtime/pkg/client" ) +type FleetDeletionChecker interface { + isDeleteAllowed(ctx context.Context, server *networkv1alpha1.Server, c *client.Client) (bool, error) +} + // FindDeleteServer is used to find the server that should be deleted. // It is based on the specs agepriority field. -func FindDeleteServer(ctx context.Context, fleet *networkv1alpha1.Fleet, servers *networkv1alpha1.ServerList, client client.Client) (*networkv1alpha1.Server, error) { +func FindDeleteServer(ctx context.Context, fleet *networkv1alpha1.Fleet, servers *networkv1alpha1.ServerList, client client.Client, checker FleetDeletionChecker) (*networkv1alpha1.Server, error) { strategy := fleet.Spec.Scaling.AgePriority deleteFirst := fleet.Spec.Scaling.PrioritizeAllowed if strategy == networkv1alpha1.OldestFirst { - return getOldestServer(ctx, servers, deleteFirst, client) + return getOldestServer(ctx, servers, deleteFirst, &client, checker) } if strategy == networkv1alpha1.NewestFirst { - return getNewestServer(ctx, servers, deleteFirst, client) + return getNewestServer(ctx, servers, deleteFirst, &client, checker) } return nil, fmt.Errorf("invalid scaling strategy: %s", strategy) @@ -31,33 +35,34 @@ func FindDeleteServer(ctx context.Context, fleet *networkv1alpha1.Fleet, servers // getOldestServer gets the server of the fleet, that is the oldest // If deleteFirst is enabled, then it tries to get the server that can be deleted, but that is the oldest out of those. // If it cannot find any where deletion is allowed, it returns the oldest server. -func getOldestServer(ctx context.Context, servers *networkv1alpha1.ServerList, deleteFirst bool, client client.Client) (*networkv1alpha1.Server, error) { +func getOldestServer(ctx context.Context, servers *networkv1alpha1.ServerList, deleteFirst bool, client *client.Client, checker FleetDeletionChecker) (*networkv1alpha1.Server, error) { var oldestServer *networkv1alpha1.Server var oldestTime *metav1.Time var oldestAllowedServer *networkv1alpha1.Server var oldestAllowTime *metav1.Time //Go over all servers - for _, server := range servers.Items { + for i := range servers.Items { + server := &servers.Items[i] //If time is null, or this server was created before current oldest if oldestTime == nil || server.CreationTimestamp.Before(oldestTime) { //Set new oldest oldestTime = &server.CreationTimestamp - oldestServer = &server - // Check if we want to prioritize allowed - if deleteFirst { - //Check if this is allowed - allowed, err := isDeleteAllowed(ctx, &server, client) - if err != nil { - return nil, err - } - if allowed { - //If it is allowed, check current oldest allowed - if oldestAllowTime == nil || server.CreationTimestamp.Before(oldestAllowTime) { - //Update to new oldest allowed - oldestAllowTime = &server.CreationTimestamp - oldestAllowedServer = &server - } + oldestServer = server + } + // Check if we want to prioritize allowed + if deleteFirst { + //Check if this is allowed + allowed, err := checker.isDeleteAllowed(ctx, server, client) + if err != nil { + return nil, err + } + if allowed { + //If it is allowed, check current oldest allowed + if oldestAllowTime == nil || server.CreationTimestamp.Before(oldestAllowTime) { + //Update to new oldest allowed + oldestAllowTime = &server.CreationTimestamp + oldestAllowedServer = server } } } @@ -79,7 +84,7 @@ func getOldestServer(ctx context.Context, servers *networkv1alpha1.ServerList, d // getNewestServer gets the server of the fleet, that is the newest // If deleteFirst is enabled, then it tries to get the server that can be deleted, but that is the youngest out of those. // If it cannot find any where deletion is allowed, it returns the youngest server. -func getNewestServer(ctx context.Context, servers *networkv1alpha1.ServerList, deleteFirst bool, client client.Client) (*networkv1alpha1.Server, error) { +func getNewestServer(ctx context.Context, servers *networkv1alpha1.ServerList, deleteFirst bool, client *client.Client, checker FleetDeletionChecker) (*networkv1alpha1.Server, error) { var newestServer *networkv1alpha1.Server var newestTime *metav1.Time @@ -87,25 +92,26 @@ func getNewestServer(ctx context.Context, servers *networkv1alpha1.ServerList, d var newestAllowTime *metav1.Time // Go over all of the servers - for _, server := range servers.Items { + for i := range servers.Items { + server := &servers.Items[i] //If time is null, or this server was created after current youngest if newestTime == nil || server.CreationTimestamp.After(newestTime.Time) { // Update with new youngest newestTime = &server.CreationTimestamp - newestServer = &server - // Check if we want to prioritize allowed - if deleteFirst { - //Check if this is allowed - allowed, err := isDeleteAllowed(ctx, &server, client) - if err != nil { - return nil, err - } - if allowed { - // If it is, we want to check if there already is a prioritized time, and is it after the current iterations one - if newestAllowTime == nil || server.CreationTimestamp.After(newestAllowTime.Time) { - newestAllowTime = &server.CreationTimestamp - newestAllowedServer = &server - } + newestServer = server + } + // Check if we want to prioritize allowed + if deleteFirst { + //Check if this is allowed + allowed, err := checker.isDeleteAllowed(ctx, server, client) + if err != nil { + return nil, err + } + if allowed { + // If it is, we want to check if there already is a prioritized time, and is it after the current iterations one + if newestAllowTime == nil || server.CreationTimestamp.After(newestAllowTime.Time) { + newestAllowTime = &server.CreationTimestamp + newestAllowedServer = server } } } @@ -125,10 +131,10 @@ func getNewestServer(ctx context.Context, servers *networkv1alpha1.ServerList, d } // isDeleteAllowed is a utility for a server object, to communicate with the sidecar to see if deletion is allowed -func isDeleteAllowed(ctx context.Context, server *networkv1alpha1.Server, c client.Client) (bool, error) { +func (ProdDeletionChecker) isDeleteAllowed(ctx context.Context, server *networkv1alpha1.Server, c *client.Client) (bool, error) { podName := server.Name + "-pod" pod := &v1.Pod{} - err := c.Get(ctx, types.NamespacedName{Namespace: server.Namespace, Name: podName}, pod) + err := (*c).Get(ctx, types.NamespacedName{Namespace: server.Namespace, Name: podName}, pod) if err != nil { return false, err } diff --git a/operator/internal/utils/fleet_deletion_test.go b/operator/internal/utils/fleet_deletion_test.go new file mode 100644 index 0000000..025435f --- /dev/null +++ b/operator/internal/utils/fleet_deletion_test.go @@ -0,0 +1,152 @@ +package utils + +import ( + "context" + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" + networkv1alpha1 "github.com/unfamousthomas/thesis-operator/api/v1alpha1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "sigs.k8s.io/controller-runtime/pkg/client" + "time" +) + +type FakeFleetDeleteChecker struct { + DeletionState map[string]bool +} + +func (f FakeFleetDeleteChecker) isDeleteAllowed(ctx context.Context, server *networkv1alpha1.Server, c *client.Client) (bool, error) { + return f.DeletionState[server.Name], nil +} + +var _ = Describe("Fleet Utility Testing", func() { + Context("When finding the server to delete", func() { + ctx := context.Background() + It("Find oldest", func() { + By("Setup objects") + baseTime := time.Now() + fake := FakeFleetDeleteChecker{DeletionState: make(map[string]bool)} + servers := networkv1alpha1.ServerList{Items: []networkv1alpha1.Server{ + { + ObjectMeta: metav1.ObjectMeta{ + Name: "server1", + CreationTimestamp: metav1.Time{Time: baseTime}, + }, + }, + { + ObjectMeta: metav1.ObjectMeta{ + Name: "server2", + CreationTimestamp: metav1.Time{Time: baseTime.Add(time.Hour)}, + }, + }, + { + ObjectMeta: metav1.ObjectMeta{ + Name: "server3", + CreationTimestamp: metav1.Time{Time: baseTime.Add(time.Minute)}, + }, + }, + }} + By("Find the oldest server") + server, err := getOldestServer(ctx, &servers, false, nil, fake) + Expect(err).ToNot(HaveOccurred()) + Expect(servers.Items).To(HaveLen(3)) + Expect(server.Name).To(Equal("server1")) + }) + + It("Find oldest with delete allowed", func() { + By("Setup objects") + baseTime := time.Now() + fake := FakeFleetDeleteChecker{DeletionState: make(map[string]bool)} + servers := networkv1alpha1.ServerList{Items: []networkv1alpha1.Server{ + { + ObjectMeta: metav1.ObjectMeta{ + Name: "server1", + CreationTimestamp: metav1.Time{Time: baseTime}, // 0 + }, + }, + { + ObjectMeta: metav1.ObjectMeta{ + Name: "server2", + CreationTimestamp: metav1.Time{Time: baseTime.Add(time.Hour)}, // 2 + }, + }, + { + ObjectMeta: metav1.ObjectMeta{ + Name: "server3", + CreationTimestamp: metav1.Time{Time: baseTime.Add(time.Minute)}, // 1 + }, + }, + }} + By("Find oldest deletable server") + fake.DeletionState["server2"] = true + fake.DeletionState["server3"] = true + server, err := getOldestServer(ctx, &servers, true, nil, fake) + Expect(err).ToNot(HaveOccurred()) + Expect(servers.Items).To(HaveLen(3)) + Expect(server.Name).To(Equal("server3")) + }) + + It("Find youngest", func() { + By("Setup objects") + baseTime := time.Now() + fake := FakeFleetDeleteChecker{DeletionState: make(map[string]bool)} + servers := networkv1alpha1.ServerList{Items: []networkv1alpha1.Server{ + { + ObjectMeta: metav1.ObjectMeta{ + Name: "server1", + CreationTimestamp: metav1.Time{Time: baseTime}, // 0 + }, + }, + { + ObjectMeta: metav1.ObjectMeta{ + Name: "server2", + CreationTimestamp: metav1.Time{Time: baseTime.Add(time.Hour)}, // 2 + }, + }, + { + ObjectMeta: metav1.ObjectMeta{ + Name: "server3", + CreationTimestamp: metav1.Time{Time: baseTime.Add(time.Minute)}, // 1 + }, + }, + }} + By("Find the oldest server") + server, err := getNewestServer(ctx, &servers, false, nil, fake) + Expect(err).ToNot(HaveOccurred()) + Expect(servers.Items).To(HaveLen(3)) + Expect(server.Name).To(Equal("server2")) + }) + + It("Find oldest with delete allowed", func() { + By("Setup objects") + baseTime := time.Now() + fake := FakeFleetDeleteChecker{DeletionState: make(map[string]bool)} + servers := networkv1alpha1.ServerList{Items: []networkv1alpha1.Server{ + { + ObjectMeta: metav1.ObjectMeta{ + Name: "server1", + CreationTimestamp: metav1.Time{Time: baseTime}, + }, + }, + { + ObjectMeta: metav1.ObjectMeta{ + Name: "server2", + CreationTimestamp: metav1.Time{Time: baseTime.Add(time.Hour)}, + }, + }, + { + ObjectMeta: metav1.ObjectMeta{ + Name: "server3", + CreationTimestamp: metav1.Time{Time: baseTime.Add(time.Minute)}, + }, + }, + }} + By("Find youngest deletable server") + fake.DeletionState["server2"] = true + fake.DeletionState["server3"] = true + server, err := getOldestServer(ctx, &servers, true, nil, fake) + Expect(err).ToNot(HaveOccurred()) + Expect(servers.Items).To(HaveLen(3)) + Expect(server.Name).To(Equal("server3")) + }) + }) +}) diff --git a/operator/internal/utils/suite_test.go b/operator/internal/utils/suite_test.go new file mode 100644 index 0000000..5f301c1 --- /dev/null +++ b/operator/internal/utils/suite_test.go @@ -0,0 +1,24 @@ +package utils + +import ( + "testing" + + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" + + logf "sigs.k8s.io/controller-runtime/pkg/log" + "sigs.k8s.io/controller-runtime/pkg/log/zap" +) + +func TestControllers(t *testing.T) { + RegisterFailHandler(Fail) + + RunSpecs(t, "Controller Utils Suite") +} + +var _ = BeforeSuite(func() { + logf.SetLogger(zap.New(zap.WriteTo(GinkgoWriter), zap.UseDevMode(true))) +}) + +var _ = AfterSuite(func() { +})