From e32c49556ffc3d8cc03b8df21cc8d41dbd7c5594 Mon Sep 17 00:00:00 2001 From: deefreak Date: Mon, 6 Nov 2023 11:52:13 +0530 Subject: [PATCH 1/7] added finalizer and handled delete/update operations for policies --- cmd/main.go | 3 +- pkg/controller/policy_controller.go | 131 +++++++++++++++++++++++++++- pkg/controller/suite_test.go | 3 + pkg/policy/store.go | 20 +++-- 4 files changed, 149 insertions(+), 8 deletions(-) diff --git a/cmd/main.go b/cmd/main.go index 571837b..3c310d6 100644 --- a/cmd/main.go +++ b/cmd/main.go @@ -331,7 +331,8 @@ func main() { if err = controller.NewPolicyWatcher(mgr.GetClient(), mgr.GetScheme(), - triggerHandler.QueueAllForExecution).SetupWithManager(mgr); err != nil { + triggerHandler.QueueAllForExecution, + triggerHandler.QueueForExecution).SetupWithManager(mgr); err != nil { setupLog.Error(err, "unable to create controller", "controller", "Policy") os.Exit(1) } diff --git a/pkg/controller/policy_controller.go b/pkg/controller/policy_controller.go index dec728e..9ab6fc3 100644 --- a/pkg/controller/policy_controller.go +++ b/pkg/controller/policy_controller.go @@ -18,9 +18,19 @@ package controller import ( "context" + "reflect" + ottoscaleriov1alpha1 "github.com/flipkart-incubator/ottoscalr/api/v1alpha1" + "github.com/go-logr/logr" "k8s.io/apimachinery/pkg/api/errors" + "k8s.io/apimachinery/pkg/fields" + "k8s.io/apimachinery/pkg/types" + "sigs.k8s.io/controller-runtime/pkg/builder" + "sigs.k8s.io/controller-runtime/pkg/event" + "sigs.k8s.io/controller-runtime/pkg/handler" "sigs.k8s.io/controller-runtime/pkg/log" + "sigs.k8s.io/controller-runtime/pkg/predicate" + "sigs.k8s.io/controller-runtime/pkg/source" "k8s.io/apimachinery/pkg/runtime" ctrl "sigs.k8s.io/controller-runtime" @@ -28,21 +38,27 @@ import ( ) const PolicyWatcherCtrl = "PolicyWatcher" +const policyFinalizerName = "finalizer.ottoscaler.io" + +var policyRefKey = ".spec.policy" // PolicyWatcher reconciles a Policy object type PolicyWatcher struct { Client client.Client Scheme *runtime.Scheme requeueAllFunc func() + requeueOneFunc func(types.NamespacedName) } func NewPolicyWatcher(client client.Client, scheme *runtime.Scheme, requeueAllFunc func(), + requeueOneFunc func(types.NamespacedName), ) *PolicyWatcher { return &PolicyWatcher{Client: client, Scheme: scheme, requeueAllFunc: requeueAllFunc, + requeueOneFunc: requeueOneFunc, } } @@ -63,8 +79,39 @@ func (r *PolicyWatcher) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.R return ctrl.Result{}, err } + // Add finalizer to the policy if it doesn't have one + policy, err := r.addFinalizer(ctx, policy) + + if err != nil { + logger.Error(err, "Error adding finalizer to policy") + return ctrl.Result{}, err + } + + //Handle Reconcile + //If it is a delete event or update in the spec + //Requeue all policyRecommendations having the request Policy object as a reference + err = r.handleReconcilation(ctx, policy, logger) + + if err != nil { + logger.Error(err, "Error handling reconcilation of policy") + return ctrl.Result{}, err + } + + // If the policy is deleted + if !policy.ObjectMeta.DeletionTimestamp.IsZero() { + + // Remove finalizer from the policy + policy.ObjectMeta.Finalizers = removeString(policy.ObjectMeta.Finalizers, policyFinalizerName) + if err := r.Client.Update(ctx, &policy); err != nil { + return ctrl.Result{}, err + } + + return ctrl.Result{}, nil + + } + + // If the policy is marked as default, ensure no other policy is marked as default if policy.Spec.IsDefault { - // If the policy is marked as default, ensure no other policy is marked as default var allPolicies ottoscaleriov1alpha1.PolicyList if err := r.Client.List(ctx, &allPolicies); err != nil { logger.Error(err, "Error getting allPolicies") @@ -95,8 +142,88 @@ func (r *PolicyWatcher) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.R // SetupWithManager sets up the controller with the Manager. func (r *PolicyWatcher) SetupWithManager(mgr ctrl.Manager) error { + if err := mgr.GetFieldIndexer().IndexField(context.Background(), &ottoscaleriov1alpha1.PolicyRecommendation{}, policyRefKey, func(rawObj client.Object) []string { + policyRecommendation := rawObj.(*ottoscaleriov1alpha1.PolicyRecommendation) + if policyRecommendation.Spec.Policy == "" { + return nil + } + return []string{policyRecommendation.Spec.Policy} + }); err != nil { + return err + } + + reconcilePredicate := predicate.Funcs{ + CreateFunc: func(e event.CreateEvent) bool { + return true + }, + DeleteFunc: func(e event.DeleteEvent) bool { + return true + }, + UpdateFunc: func(e event.UpdateEvent) bool { + newPolicy := e.ObjectNew.(*ottoscaleriov1alpha1.Policy) + oldPolicy := e.ObjectOld.(*ottoscaleriov1alpha1.Policy) + + return !reflect.DeepEqual(newPolicy.Spec, oldPolicy.Spec) + }, + GenericFunc: func(e event.GenericEvent) bool { + return false + }, + } + return ctrl.NewControllerManagedBy(mgr). - For(&ottoscaleriov1alpha1.Policy{}). + Watches(&source.Kind{Type: &ottoscaleriov1alpha1.Policy{}}, + &handler.EnqueueRequestForObject{}, + builder.WithPredicates(reconcilePredicate)). Named(PolicyWatcherCtrl). Complete(r) } + +func (r *PolicyWatcher) addFinalizer(ctx context.Context, policy ottoscaleriov1alpha1.Policy) (ottoscaleriov1alpha1.Policy, error) { + // Add finalizer to the policy if it doesn't have one + if !containsString(policy.ObjectMeta.Finalizers, policyFinalizerName) { + policy.ObjectMeta.Finalizers = append(policy.ObjectMeta.Finalizers, policyFinalizerName) + if err := r.Client.Update(ctx, &policy); err != nil { + return ottoscaleriov1alpha1.Policy{}, err + } + } + + return policy, nil +} + +func (r *PolicyWatcher) handleReconcilation(ctx context.Context, policy ottoscaleriov1alpha1.Policy, logger logr.Logger) error { + // Get all PolicyRecommendation objects that reference the Policy object + var policyRecommendations ottoscaleriov1alpha1.PolicyRecommendationList + if err := r.Client.List(ctx, &policyRecommendations, &client.ListOptions{ + FieldSelector: fields.OneTermEqualSelector(policyRefKey, policy.Name)}); err != nil { + return err + } + + // Requeue all PolicyRecommendation objects having the Policy object as a reference + for _, policyRecommendation := range policyRecommendations.Items { + r.requeueOneFunc(types.NamespacedName{Namespace: policyRecommendation.Namespace, + Name: policyRecommendation.Name}) + } + + return nil +} + +func containsString(slice []string, str string) bool { + for _, s := range slice { + if s == str { + return true + } + } + + return false +} + +func removeString(slice []string, str string) []string { + var result []string + for _, s := range slice { + if s != str { + result = append(result, s) + } + } + + return result +} diff --git a/pkg/controller/suite_test.go b/pkg/controller/suite_test.go index b44b9f5..6beb7f9 100644 --- a/pkg/controller/suite_test.go +++ b/pkg/controller/suite_test.go @@ -137,6 +137,9 @@ var _ = BeforeSuite(func() { requeueAllFunc: func() { queuedAllRecos = true }, + requeueOneFunc: func(namespacedName types.NamespacedName) { + queuedAllRecos = true + }, }).SetupWithManager(k8sManager) Expect(err).ToNot(HaveOccurred()) diff --git a/pkg/policy/store.go b/pkg/policy/store.go index 6f84762..c66c277 100644 --- a/pkg/policy/store.go +++ b/pkg/policy/store.go @@ -4,11 +4,12 @@ import ( "context" "errors" "fmt" + "log" + "sort" + "github.com/flipkart-incubator/ottoscalr/api/v1alpha1" k8serrors "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/types" - "log" - "sort" "sigs.k8s.io/controller-runtime/pkg/client" ) @@ -110,10 +111,19 @@ func (ps *PolicyStore) GetSortedPolicies() (*v1alpha1.PolicyList, error) { return nil, err2 } - sort.Slice(policies.Items, func(i, j int) bool { - return policies.Items[i].Spec.RiskIndex < policies.Items[j].Spec.RiskIndex + //Get only policies having deletion timestamp as zero + filteredPolicies := policies.DeepCopy() + filteredPolicies.Items = nil + for _, policy := range policies.Items { + if policy.ObjectMeta.DeletionTimestamp.IsZero() { + filteredPolicies.Items = append(filteredPolicies.Items, policy) + } + } + + sort.Slice(filteredPolicies.Items, func(i, j int) bool { + return filteredPolicies.Items[i].Spec.RiskIndex < filteredPolicies.Items[j].Spec.RiskIndex }) - return policies, nil + return filteredPolicies, nil } func (ps *PolicyStore) GetPolicyByName(name string) (*v1alpha1.Policy, error) { From 4a267b82fe5d40fc12a09fba93889fc8d260a173 Mon Sep 17 00:00:00 2001 From: deefreak Date: Mon, 6 Nov 2023 12:37:23 +0530 Subject: [PATCH 2/7] added finalizer and handled delete/update operations for policies --- .../policyrecommendation_controller_test.go | 47 ++++++++++++++++++- 1 file changed, 45 insertions(+), 2 deletions(-) diff --git a/pkg/controller/policyrecommendation_controller_test.go b/pkg/controller/policyrecommendation_controller_test.go index 1b64c10..cc228bd 100644 --- a/pkg/controller/policyrecommendation_controller_test.go +++ b/pkg/controller/policyrecommendation_controller_test.go @@ -92,10 +92,20 @@ var _ = Describe("PolicyrecommendationController", func() { updatedPolicy = &v1alpha1.PolicyRecommendation{} }) AfterEach(func() { + createdPolicy.Finalizers = nil + Expect(k8sClient.Update(ctx, createdPolicy)).Should(Succeed()) Expect(k8sClient.Delete(ctx, createdPolicy)).Should(Succeed()) + safestPolicy.Finalizers = nil + Expect(k8sClient.Update(ctx, &safestPolicy)).Should(Succeed()) Expect(k8sClient.Delete(ctx, &safestPolicy)).Should(Succeed()) + policy1.Finalizers = nil + Expect(k8sClient.Update(ctx, &policy1)).Should(Succeed()) Expect(k8sClient.Delete(ctx, &policy1)).Should(Succeed()) + policy2.Finalizers = nil + Expect(k8sClient.Update(ctx, &policy2)).Should(Succeed()) Expect(k8sClient.Delete(ctx, &policy2)).Should(Succeed()) + policy3.Finalizers = nil + Expect(k8sClient.Update(ctx, &policy3)).Should(Succeed()) Expect(k8sClient.Delete(ctx, &policy3)).Should(Succeed()) }) @@ -256,9 +266,17 @@ var _ = Describe("PolicyrecommendationController", func() { updatedPolicy = &v1alpha1.PolicyRecommendation{} }) AfterEach(func() { + createdPolicy.Finalizers = nil + Expect(k8sClient.Update(ctx, createdPolicy)).Should(Succeed()) Expect(k8sClient.Delete(ctx, createdPolicy)).Should(Succeed()) + safestPolicy.Finalizers = nil + Expect(k8sClient.Update(ctx, &safestPolicy)).Should(Succeed()) Expect(k8sClient.Delete(ctx, &safestPolicy)).Should(Succeed()) + policy1.Finalizers = nil + Expect(k8sClient.Update(ctx, &policy1)).Should(Succeed()) Expect(k8sClient.Delete(ctx, &policy1)).Should(Succeed()) + policy2.Finalizers = nil + Expect(k8sClient.Update(ctx, &policy2)).Should(Succeed()) Expect(k8sClient.Delete(ctx, &policy2)).Should(Succeed()) }) @@ -535,8 +553,14 @@ var _ = Describe("PolicyrecommendationController", func() { updatedPolicy = &v1alpha1.PolicyRecommendation{} }) AfterEach(func() { + createdPolicy.Finalizers = nil + Expect(k8sClient.Update(ctx, createdPolicy)).Should(Succeed()) Expect(k8sClient.Delete(ctx, createdPolicy)).Should(Succeed()) + safestPolicy.Finalizers = nil + Expect(k8sClient.Update(ctx, &safestPolicy)).Should(Succeed()) Expect(k8sClient.Delete(ctx, &safestPolicy)).Should(Succeed()) + policy1.Finalizers = nil + Expect(k8sClient.Update(ctx, &policy1)).Should(Succeed()) Expect(k8sClient.Delete(ctx, &policy1)).Should(Succeed()) }) @@ -667,6 +691,8 @@ var _ = Describe("PolicyrecommendationController", func() { }) AfterEach(func() { + safestPolicy.Finalizers = nil + Expect(k8sClient.Update(ctx, safestPolicy)).Should(Succeed()) Expect(k8sClient.Delete(ctx, safestPolicy)).Should(Succeed()) }) @@ -834,10 +860,19 @@ var _ = Describe("PolicyrecommendationController", func() { }) AfterEach(func() { - Expect(k8sClient.Delete(ctx, deployment)).Should(Succeed()) + createdPolicy.Finalizers = nil + Expect(k8sClient.Update(ctx, createdPolicy)).Should(Succeed()) + Expect(k8sClient.Delete(ctx, createdPolicy)).Should(Succeed()) + safestPolicy.Finalizers = nil + Expect(k8sClient.Update(ctx, &safestPolicy)).Should(Succeed()) Expect(k8sClient.Delete(ctx, &safestPolicy)).Should(Succeed()) + policy1.Finalizers = nil + Expect(k8sClient.Update(ctx, &policy1)).Should(Succeed()) Expect(k8sClient.Delete(ctx, &policy1)).Should(Succeed()) + policy2.Finalizers = nil + Expect(k8sClient.Update(ctx, &policy2)).Should(Succeed()) Expect(k8sClient.Delete(ctx, &policy2)).Should(Succeed()) + }) It("Should Create a new PolicyRecommendation with Initialized,RecoTaskQueued,Recommendation Generated and Target Reco Achieved Status Conditions", func() { By("By creating a new Deployment") @@ -955,9 +990,17 @@ var _ = Describe("PolicyrecommendationController", func() { }) AfterEach(func() { - Expect(k8sClient.Delete(ctx, deployment)).Should(Succeed()) + createdPolicy.Finalizers = nil + Expect(k8sClient.Update(ctx, createdPolicy)).Should(Succeed()) + Expect(k8sClient.Delete(ctx, createdPolicy)).Should(Succeed()) + safestPolicy.Finalizers = nil + Expect(k8sClient.Update(ctx, &safestPolicy)).Should(Succeed()) Expect(k8sClient.Delete(ctx, &safestPolicy)).Should(Succeed()) + policy1.Finalizers = nil + Expect(k8sClient.Update(ctx, &policy1)).Should(Succeed()) Expect(k8sClient.Delete(ctx, &policy1)).Should(Succeed()) + policy2.Finalizers = nil + Expect(k8sClient.Update(ctx, &policy2)).Should(Succeed()) Expect(k8sClient.Delete(ctx, &policy2)).Should(Succeed()) }) It("Should Create a new PolicyRecommendation with Initialized,RecoTaskQueued,Recommendation Generated and Target Reco Achieved Status Conditions", func() { From 95200029b537c28b640f7ba8ddc503e1dfba8692 Mon Sep 17 00:00:00 2001 From: deefreak Date: Mon, 6 Nov 2023 12:58:11 +0530 Subject: [PATCH 3/7] added finalizer and handled delete/update operations for policies --- pkg/controller/policyrecommendation_controller_test.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/pkg/controller/policyrecommendation_controller_test.go b/pkg/controller/policyrecommendation_controller_test.go index cc228bd..f4d44d2 100644 --- a/pkg/controller/policyrecommendation_controller_test.go +++ b/pkg/controller/policyrecommendation_controller_test.go @@ -97,6 +97,8 @@ var _ = Describe("PolicyrecommendationController", func() { Expect(k8sClient.Delete(ctx, createdPolicy)).Should(Succeed()) safestPolicy.Finalizers = nil Expect(k8sClient.Update(ctx, &safestPolicy)).Should(Succeed()) + safestPolicy = v1alpha1.Policy{} + Expect(k8sClient.Get(ctx, types.NamespacedName{Name: safestPolicy.Name, Namespace: safestPolicy.Namespace}, &safestPolicy)).Should(Succeed()) Expect(k8sClient.Delete(ctx, &safestPolicy)).Should(Succeed()) policy1.Finalizers = nil Expect(k8sClient.Update(ctx, &policy1)).Should(Succeed()) From 202817f21c88affca21538f4ae8eb4329e8b81b0 Mon Sep 17 00:00:00 2001 From: deefreak Date: Mon, 6 Nov 2023 16:25:12 +0530 Subject: [PATCH 4/7] added finalizer and handled delete/update operations for policies --- pkg/controller/policy_controller_test.go | 26 +++- .../policyrecommendation_controller_test.go | 114 ++++++++++++------ .../policyrecommendation_registrar_test.go | 6 +- 3 files changed, 109 insertions(+), 37 deletions(-) diff --git a/pkg/controller/policy_controller_test.go b/pkg/controller/policy_controller_test.go index 57a188e..7579421 100644 --- a/pkg/controller/policy_controller_test.go +++ b/pkg/controller/policy_controller_test.go @@ -2,12 +2,14 @@ package controller import ( "context" + "fmt" + "time" + ottoscaleriov1alpha1 "github.com/flipkart-incubator/ottoscalr/api/v1alpha1" . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/types" - "time" ) var _ = Describe("PolicyWatcher controller", func() { @@ -25,6 +27,25 @@ var _ = Describe("PolicyWatcher controller", func() { Expect(k8sClient.Delete(ctx, &policy1)).Should(Succeed()) Expect(k8sClient.Delete(ctx, &policy2)).Should(Succeed()) Expect(k8sClient.Delete(ctx, &policy3)).Should(Succeed()) + policy1 = ottoscaleriov1alpha1.Policy{} + Expect(k8sClient.Get(ctx, types.NamespacedName{Name: "policy1"}, &policy1)).Should(Succeed()) + Expect(policy1.DeletionTimestamp).ShouldNot(BeNil()) + policy1.Finalizers = nil + Expect(k8sClient.Update(ctx, &policy1)).Should(Succeed()) + time.Sleep(1 * time.Second) + + policy2 = ottoscaleriov1alpha1.Policy{} + Expect(k8sClient.Get(ctx, types.NamespacedName{Name: "policy2"}, &policy2)).Should(Succeed()) + Expect(policy2.DeletionTimestamp).ShouldNot(BeNil()) + policy2.Finalizers = nil + Expect(k8sClient.Update(ctx, &policy2)).Should(Succeed()) + time.Sleep(1 * time.Second) + policy3 = ottoscaleriov1alpha1.Policy{} + Expect(k8sClient.Get(ctx, types.NamespacedName{Name: "policy3"}, &policy3)).Should(Succeed()) + Expect(policy3.DeletionTimestamp).ShouldNot(BeNil()) + policy3.Finalizers = nil + Expect(k8sClient.Update(ctx, &policy3)).Should(Succeed()) + time.Sleep(1 * time.Second) }) It("Should mark other policies as non-default and requeue all policy recommendations ", func() { By("Seeding all policies") @@ -62,6 +83,9 @@ var _ = Describe("PolicyWatcher controller", func() { Expect(k8sClient.Create(ctx, &policy2)).Should(Succeed()) Expect(k8sClient.Create(ctx, &policy3)).Should(Succeed()) + time.Sleep(2 * time.Second) + policy2 = ottoscaleriov1alpha1.Policy{} + Expect(k8sClient.Get(ctx, types.NamespacedName{Name: "policy2"}, &policy2)).Should(Succeed()) policy2.Spec.IsDefault = true err := k8sClient.Update(ctx, &policy2) Expect(err).ToNot(HaveOccurred()) diff --git a/pkg/controller/policyrecommendation_controller_test.go b/pkg/controller/policyrecommendation_controller_test.go index f4d44d2..df689a4 100644 --- a/pkg/controller/policyrecommendation_controller_test.go +++ b/pkg/controller/policyrecommendation_controller_test.go @@ -4,6 +4,8 @@ import ( "context" "encoding/json" "fmt" + "time" + argov1alpha1 "github.com/argoproj/argo-rollouts/pkg/apis/rollouts/v1alpha1" v1alpha1 "github.com/flipkart-incubator/ottoscalr/api/v1alpha1" . "github.com/onsi/ginkgo/v2" @@ -13,7 +15,6 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/types" "sigs.k8s.io/controller-runtime/pkg/client" - "time" ) var _ = Describe("PolicyrecommendationController", func() { @@ -92,23 +93,37 @@ var _ = Describe("PolicyrecommendationController", func() { updatedPolicy = &v1alpha1.PolicyRecommendation{} }) AfterEach(func() { - createdPolicy.Finalizers = nil - Expect(k8sClient.Update(ctx, createdPolicy)).Should(Succeed()) Expect(k8sClient.Delete(ctx, createdPolicy)).Should(Succeed()) + Expect(k8sClient.Delete(ctx, &safestPolicy)).Should(Succeed()) + Expect(k8sClient.Delete(ctx, &policy1)).Should(Succeed()) + Expect(k8sClient.Delete(ctx, &policy2)).Should(Succeed()) + Expect(k8sClient.Delete(ctx, &policy3)).Should(Succeed()) + safestPolicy = v1alpha1.Policy{} + Expect(k8sClient.Get(ctx, types.NamespacedName{Name: "safest-policy"}, &safestPolicy)).Should(Succeed()) + Expect(safestPolicy.DeletionTimestamp).ShouldNot(BeNil()) safestPolicy.Finalizers = nil Expect(k8sClient.Update(ctx, &safestPolicy)).Should(Succeed()) - safestPolicy = v1alpha1.Policy{} - Expect(k8sClient.Get(ctx, types.NamespacedName{Name: safestPolicy.Name, Namespace: safestPolicy.Namespace}, &safestPolicy)).Should(Succeed()) - Expect(k8sClient.Delete(ctx, &safestPolicy)).Should(Succeed()) + time.Sleep(1 * time.Second) + + policy1 = v1alpha1.Policy{} + Expect(k8sClient.Get(ctx, types.NamespacedName{Name: "policy-1"}, &policy1)).Should(Succeed()) + Expect(policy1.DeletionTimestamp).ShouldNot(BeNil()) policy1.Finalizers = nil Expect(k8sClient.Update(ctx, &policy1)).Should(Succeed()) - Expect(k8sClient.Delete(ctx, &policy1)).Should(Succeed()) + time.Sleep(1 * time.Second) + + policy2 = v1alpha1.Policy{} + Expect(k8sClient.Get(ctx, types.NamespacedName{Name: "policy-2"}, &policy2)).Should(Succeed()) + Expect(policy2.DeletionTimestamp).ShouldNot(BeNil()) policy2.Finalizers = nil Expect(k8sClient.Update(ctx, &policy2)).Should(Succeed()) - Expect(k8sClient.Delete(ctx, &policy2)).Should(Succeed()) + time.Sleep(1 * time.Second) + policy3 = v1alpha1.Policy{} + Expect(k8sClient.Get(ctx, types.NamespacedName{Name: "policy-3"}, &policy3)).Should(Succeed()) + Expect(policy3.DeletionTimestamp).ShouldNot(BeNil()) policy3.Finalizers = nil Expect(k8sClient.Update(ctx, &policy3)).Should(Succeed()) - Expect(k8sClient.Delete(ctx, &policy3)).Should(Succeed()) + time.Sleep(1 * time.Second) }) It("Lifecycle of a PolicyRecommendation with a default policy", func() { @@ -268,18 +283,31 @@ var _ = Describe("PolicyrecommendationController", func() { updatedPolicy = &v1alpha1.PolicyRecommendation{} }) AfterEach(func() { - createdPolicy.Finalizers = nil - Expect(k8sClient.Update(ctx, createdPolicy)).Should(Succeed()) Expect(k8sClient.Delete(ctx, createdPolicy)).Should(Succeed()) + Expect(k8sClient.Delete(ctx, &safestPolicy)).Should(Succeed()) + Expect(k8sClient.Delete(ctx, &policy1)).Should(Succeed()) + Expect(k8sClient.Delete(ctx, &policy2)).Should(Succeed()) + + safestPolicy = v1alpha1.Policy{} + Expect(k8sClient.Get(ctx, types.NamespacedName{Name: "safest-policy"}, &safestPolicy)).Should(Succeed()) + Expect(safestPolicy.DeletionTimestamp).ShouldNot(BeNil()) safestPolicy.Finalizers = nil Expect(k8sClient.Update(ctx, &safestPolicy)).Should(Succeed()) - Expect(k8sClient.Delete(ctx, &safestPolicy)).Should(Succeed()) + time.Sleep(1 * time.Second) + + policy1 = v1alpha1.Policy{} + Expect(k8sClient.Get(ctx, types.NamespacedName{Name: "policy-1"}, &policy1)).Should(Succeed()) + Expect(policy1.DeletionTimestamp).ShouldNot(BeNil()) policy1.Finalizers = nil Expect(k8sClient.Update(ctx, &policy1)).Should(Succeed()) - Expect(k8sClient.Delete(ctx, &policy1)).Should(Succeed()) + time.Sleep(1 * time.Second) + + policy2 = v1alpha1.Policy{} + Expect(k8sClient.Get(ctx, types.NamespacedName{Name: "policy-2"}, &policy2)).Should(Succeed()) + Expect(policy2.DeletionTimestamp).ShouldNot(BeNil()) policy2.Finalizers = nil Expect(k8sClient.Update(ctx, &policy2)).Should(Succeed()) - Expect(k8sClient.Delete(ctx, &policy2)).Should(Succeed()) + time.Sleep(1 * time.Second) }) It("With a conservative target reco", func() { @@ -555,15 +583,23 @@ var _ = Describe("PolicyrecommendationController", func() { updatedPolicy = &v1alpha1.PolicyRecommendation{} }) AfterEach(func() { - createdPolicy.Finalizers = nil - Expect(k8sClient.Update(ctx, createdPolicy)).Should(Succeed()) Expect(k8sClient.Delete(ctx, createdPolicy)).Should(Succeed()) + Expect(k8sClient.Delete(ctx, &safestPolicy)).Should(Succeed()) + Expect(k8sClient.Delete(ctx, &policy1)).Should(Succeed()) + + safestPolicy = v1alpha1.Policy{} + Expect(k8sClient.Get(ctx, types.NamespacedName{Name: "safest-policy"}, &safestPolicy)).Should(Succeed()) + Expect(safestPolicy.DeletionTimestamp).ShouldNot(BeNil()) safestPolicy.Finalizers = nil Expect(k8sClient.Update(ctx, &safestPolicy)).Should(Succeed()) - Expect(k8sClient.Delete(ctx, &safestPolicy)).Should(Succeed()) + time.Sleep(1 * time.Second) + + policy1 = v1alpha1.Policy{} + Expect(k8sClient.Get(ctx, types.NamespacedName{Name: "policy-1"}, &policy1)).Should(Succeed()) + Expect(policy1.DeletionTimestamp).ShouldNot(BeNil()) policy1.Finalizers = nil Expect(k8sClient.Update(ctx, &policy1)).Should(Succeed()) - Expect(k8sClient.Delete(ctx, &policy1)).Should(Succeed()) + time.Sleep(1 * time.Second) }) It("With an aggressive target reco", func() { @@ -693,9 +729,14 @@ var _ = Describe("PolicyrecommendationController", func() { }) AfterEach(func() { + Expect(k8sClient.Delete(ctx, safestPolicy)).Should(Succeed()) + safestPolicy = &v1alpha1.Policy{} + Expect(k8sClient.Get(ctx, types.NamespacedName{Name: "safest-policy"}, safestPolicy)).Should(Succeed()) + Expect(safestPolicy.DeletionTimestamp).ShouldNot(BeNil()) safestPolicy.Finalizers = nil Expect(k8sClient.Update(ctx, safestPolicy)).Should(Succeed()) - Expect(k8sClient.Delete(ctx, safestPolicy)).Should(Succeed()) + time.Sleep(1 * time.Second) + }) It("Should create basic policyreco with 'true' QueueForExecution", func() { @@ -862,19 +903,30 @@ var _ = Describe("PolicyrecommendationController", func() { }) AfterEach(func() { - createdPolicy.Finalizers = nil - Expect(k8sClient.Update(ctx, createdPolicy)).Should(Succeed()) - Expect(k8sClient.Delete(ctx, createdPolicy)).Should(Succeed()) + Expect(k8sClient.Delete(ctx, deployment)).Should(Succeed()) + Expect(k8sClient.Delete(ctx, &safestPolicy)).Should(Succeed()) + Expect(k8sClient.Delete(ctx, &policy1)).Should(Succeed()) + Expect(k8sClient.Delete(ctx, &policy2)).Should(Succeed()) + safestPolicy = v1alpha1.Policy{} + Expect(k8sClient.Get(ctx, types.NamespacedName{Name: "safest-policy"}, &safestPolicy)).Should(Succeed()) + Expect(safestPolicy.DeletionTimestamp).ShouldNot(BeNil()) safestPolicy.Finalizers = nil Expect(k8sClient.Update(ctx, &safestPolicy)).Should(Succeed()) - Expect(k8sClient.Delete(ctx, &safestPolicy)).Should(Succeed()) + time.Sleep(1 * time.Second) + + policy1 = v1alpha1.Policy{} + Expect(k8sClient.Get(ctx, types.NamespacedName{Name: "policy-1"}, &policy1)).Should(Succeed()) + Expect(policy1.DeletionTimestamp).ShouldNot(BeNil()) policy1.Finalizers = nil Expect(k8sClient.Update(ctx, &policy1)).Should(Succeed()) - Expect(k8sClient.Delete(ctx, &policy1)).Should(Succeed()) + time.Sleep(1 * time.Second) + + policy2 = v1alpha1.Policy{} + Expect(k8sClient.Get(ctx, types.NamespacedName{Name: "policy-2"}, &policy2)).Should(Succeed()) + Expect(policy2.DeletionTimestamp).ShouldNot(BeNil()) policy2.Finalizers = nil Expect(k8sClient.Update(ctx, &policy2)).Should(Succeed()) - Expect(k8sClient.Delete(ctx, &policy2)).Should(Succeed()) - + time.Sleep(1 * time.Second) }) It("Should Create a new PolicyRecommendation with Initialized,RecoTaskQueued,Recommendation Generated and Target Reco Achieved Status Conditions", func() { By("By creating a new Deployment") @@ -992,17 +1044,9 @@ var _ = Describe("PolicyrecommendationController", func() { }) AfterEach(func() { - createdPolicy.Finalizers = nil - Expect(k8sClient.Update(ctx, createdPolicy)).Should(Succeed()) - Expect(k8sClient.Delete(ctx, createdPolicy)).Should(Succeed()) - safestPolicy.Finalizers = nil - Expect(k8sClient.Update(ctx, &safestPolicy)).Should(Succeed()) + Expect(k8sClient.Delete(ctx, deployment)).Should(Succeed()) Expect(k8sClient.Delete(ctx, &safestPolicy)).Should(Succeed()) - policy1.Finalizers = nil - Expect(k8sClient.Update(ctx, &policy1)).Should(Succeed()) Expect(k8sClient.Delete(ctx, &policy1)).Should(Succeed()) - policy2.Finalizers = nil - Expect(k8sClient.Update(ctx, &policy2)).Should(Succeed()) Expect(k8sClient.Delete(ctx, &policy2)).Should(Succeed()) }) It("Should Create a new PolicyRecommendation with Initialized,RecoTaskQueued,Recommendation Generated and Target Reco Achieved Status Conditions", func() { diff --git a/pkg/controller/policyrecommendation_registrar_test.go b/pkg/controller/policyrecommendation_registrar_test.go index 5b41c07..bf10fca 100644 --- a/pkg/controller/policyrecommendation_registrar_test.go +++ b/pkg/controller/policyrecommendation_registrar_test.go @@ -3,6 +3,8 @@ package controller import ( "context" "fmt" + "time" + rolloutv1alpha1 "github.com/argoproj/argo-rollouts/pkg/apis/rollouts/v1alpha1" ottoscaleriov1alpha1 "github.com/flipkart-incubator/ottoscalr/api/v1alpha1" . "github.com/onsi/ginkgo/v2" @@ -12,7 +14,6 @@ import ( "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/types" - "time" ) //+kubebuilder:docs-gen:collapse=Imports @@ -412,6 +413,9 @@ var _ = Describe("PolicyRecommendationRegistrar controller", func() { types.NamespacedName{Name: DeploymentName, Namespace: DeploymentNamespace}, createdPolicy)).Should(Succeed()) + time.Sleep(3 * time.Second) + Expect(k8sClient.Get(ctx, types.NamespacedName{Name: DeploymentName, Namespace: DeploymentNamespace}, + createdDeployment)).Should(Succeed()) labels := map[string]string{"app1": "test-app-1"} createdDeployment.Labels = labels From 555e52b8ecfc62ae9e87b1e14c40db77bd0c4037 Mon Sep 17 00:00:00 2001 From: deefreak Date: Mon, 6 Nov 2023 16:34:25 +0530 Subject: [PATCH 5/7] added finalizer and handled delete/update operations for policies --- pkg/controller/policy_controller_test.go | 1 - 1 file changed, 1 deletion(-) diff --git a/pkg/controller/policy_controller_test.go b/pkg/controller/policy_controller_test.go index 7579421..da9ef9b 100644 --- a/pkg/controller/policy_controller_test.go +++ b/pkg/controller/policy_controller_test.go @@ -2,7 +2,6 @@ package controller import ( "context" - "fmt" "time" ottoscaleriov1alpha1 "github.com/flipkart-incubator/ottoscalr/api/v1alpha1" From 3135f7303cfc17816f2f66e058ece9c19ed07939 Mon Sep 17 00:00:00 2001 From: deefreak Date: Wed, 8 Nov 2023 14:04:00 +0530 Subject: [PATCH 6/7] added unit tests for policy controller reconciliation --- pkg/controller/policy_controller.go | 11 +- pkg/controller/policy_controller_test.go | 157 +++++++++++++++--- .../policyrecommendation_controller_test.go | 80 --------- pkg/controller/suite_test.go | 9 +- 4 files changed, 144 insertions(+), 113 deletions(-) diff --git a/pkg/controller/policy_controller.go b/pkg/controller/policy_controller.go index 9ab6fc3..dea23c2 100644 --- a/pkg/controller/policy_controller.go +++ b/pkg/controller/policy_controller.go @@ -86,7 +86,6 @@ func (r *PolicyWatcher) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.R logger.Error(err, "Error adding finalizer to policy") return ctrl.Result{}, err } - //Handle Reconcile //If it is a delete event or update in the spec //Requeue all policyRecommendations having the request Policy object as a reference @@ -160,10 +159,10 @@ func (r *PolicyWatcher) SetupWithManager(mgr ctrl.Manager) error { return true }, UpdateFunc: func(e event.UpdateEvent) bool { - newPolicy := e.ObjectNew.(*ottoscaleriov1alpha1.Policy) - oldPolicy := e.ObjectOld.(*ottoscaleriov1alpha1.Policy) + oldObj := e.ObjectOld.(*ottoscaleriov1alpha1.Policy) + newObj := e.ObjectNew.(*ottoscaleriov1alpha1.Policy) - return !reflect.DeepEqual(newPolicy.Spec, oldPolicy.Spec) + return !reflect.DeepEqual(oldObj.Spec, newObj.Spec) }, GenericFunc: func(e event.GenericEvent) bool { return false @@ -173,7 +172,7 @@ func (r *PolicyWatcher) SetupWithManager(mgr ctrl.Manager) error { return ctrl.NewControllerManagedBy(mgr). Watches(&source.Kind{Type: &ottoscaleriov1alpha1.Policy{}}, &handler.EnqueueRequestForObject{}, - builder.WithPredicates(reconcilePredicate)). + builder.WithPredicates(predicate.Or(predicate.GenerationChangedPredicate{}, reconcilePredicate))). Named(PolicyWatcherCtrl). Complete(r) } @@ -200,6 +199,8 @@ func (r *PolicyWatcher) handleReconcilation(ctx context.Context, policy ottoscal // Requeue all PolicyRecommendation objects having the Policy object as a reference for _, policyRecommendation := range policyRecommendations.Items { + logger.Info("Requeueing PolicyRecommendation as some update/delete in seen the policy field", "policyRecommendation", policyRecommendation.Name, "Namespace", policyRecommendation.Namespace, + "policy", policyRecommendation.Spec.Policy) r.requeueOneFunc(types.NamespacedName{Namespace: policyRecommendation.Namespace, Name: policyRecommendation.Name}) } diff --git a/pkg/controller/policy_controller_test.go b/pkg/controller/policy_controller_test.go index da9ef9b..d5d4a76 100644 --- a/pkg/controller/policy_controller_test.go +++ b/pkg/controller/policy_controller_test.go @@ -2,6 +2,7 @@ package controller import ( "context" + "fmt" "time" ottoscaleriov1alpha1 "github.com/flipkart-incubator/ottoscalr/api/v1alpha1" @@ -13,8 +14,10 @@ import ( var _ = Describe("PolicyWatcher controller", func() { const ( - timeout = time.Second * 10 - interval = time.Millisecond * 250 + timeout = time.Second * 10 + interval = time.Millisecond * 250 + PolicyRecoName = "test-deployment-afgre" + PolicyRecoNamespace = "default" ) BeforeEach(func() { @@ -22,30 +25,7 @@ var _ = Describe("PolicyWatcher controller", func() { }) var policy1, policy2, policy3 ottoscaleriov1alpha1.Policy Context("When updating default policy", func() { - AfterEach(func() { - Expect(k8sClient.Delete(ctx, &policy1)).Should(Succeed()) - Expect(k8sClient.Delete(ctx, &policy2)).Should(Succeed()) - Expect(k8sClient.Delete(ctx, &policy3)).Should(Succeed()) - policy1 = ottoscaleriov1alpha1.Policy{} - Expect(k8sClient.Get(ctx, types.NamespacedName{Name: "policy1"}, &policy1)).Should(Succeed()) - Expect(policy1.DeletionTimestamp).ShouldNot(BeNil()) - policy1.Finalizers = nil - Expect(k8sClient.Update(ctx, &policy1)).Should(Succeed()) - time.Sleep(1 * time.Second) - policy2 = ottoscaleriov1alpha1.Policy{} - Expect(k8sClient.Get(ctx, types.NamespacedName{Name: "policy2"}, &policy2)).Should(Succeed()) - Expect(policy2.DeletionTimestamp).ShouldNot(BeNil()) - policy2.Finalizers = nil - Expect(k8sClient.Update(ctx, &policy2)).Should(Succeed()) - time.Sleep(1 * time.Second) - policy3 = ottoscaleriov1alpha1.Policy{} - Expect(k8sClient.Get(ctx, types.NamespacedName{Name: "policy3"}, &policy3)).Should(Succeed()) - Expect(policy3.DeletionTimestamp).ShouldNot(BeNil()) - policy3.Finalizers = nil - Expect(k8sClient.Update(ctx, &policy3)).Should(Succeed()) - time.Sleep(1 * time.Second) - }) It("Should mark other policies as non-default and requeue all policy recommendations ", func() { By("Seeding all policies") ctx := context.TODO() @@ -119,6 +99,133 @@ var _ = Describe("PolicyWatcher controller", func() { By("Testing that queuedAllRecos was called") Eventually(Expect(queuedAllRecos).Should(BeTrue())) + + Expect(k8sClient.Delete(ctx, &policy1)).Should(Succeed()) + Expect(k8sClient.Delete(ctx, &policy2)).Should(Succeed()) + Expect(k8sClient.Delete(ctx, &policy3)).Should(Succeed()) + time.Sleep(1 * time.Second) + }) + }) + Context("When handling Add/Update/Delete on a Policy", func() { + policyreco1 := &ottoscaleriov1alpha1.PolicyRecommendation{} + policyreco2 := &ottoscaleriov1alpha1.PolicyRecommendation{} + + It("Should add finalizer if not present and requeue all policyrecommendations having this policy ", func() { + By("Seeding all policies") + ctx := context.TODO() + + policy1 = ottoscaleriov1alpha1.Policy{ + ObjectMeta: metav1.ObjectMeta{Name: "policy1", Namespace: "defualt"}, + Spec: ottoscaleriov1alpha1.PolicySpec{ + IsDefault: false, + RiskIndex: 10, + MinReplicaPercentageCut: 100, + TargetUtilization: 15, + }, + } + policy2 = ottoscaleriov1alpha1.Policy{ + ObjectMeta: metav1.ObjectMeta{Name: "policy2", Namespace: "defualt"}, + Spec: ottoscaleriov1alpha1.PolicySpec{ + IsDefault: true, + RiskIndex: 20, + MinReplicaPercentageCut: 100, + TargetUtilization: 20, + }, + } + policy3 = ottoscaleriov1alpha1.Policy{ + ObjectMeta: metav1.ObjectMeta{Name: "policy3", Namespace: "defualt"}, + Spec: ottoscaleriov1alpha1.PolicySpec{ + IsDefault: false, + RiskIndex: 30, + MinReplicaPercentageCut: 100, + TargetUtilization: 30, + }, + } + + Expect(k8sClient.Create(ctx, &policy1)).Should(Succeed()) + Expect(k8sClient.Create(ctx, &policy2)).Should(Succeed()) + Expect(k8sClient.Create(ctx, &policy3)).Should(Succeed()) + + time.Sleep(2 * time.Second) + + //check if finalizer is added + policy1 = ottoscaleriov1alpha1.Policy{} + Expect(k8sClient.Get(ctx, types.NamespacedName{Name: "policy1"}, &policy1)).Should(Succeed()) + Expect(policy1.Finalizers).ShouldNot(BeNil()) + Expect(policy1.Finalizers).Should(ContainElement(policyFinalizerName)) + + policy2 = ottoscaleriov1alpha1.Policy{} + Expect(k8sClient.Get(ctx, types.NamespacedName{Name: "policy2"}, &policy2)).Should(Succeed()) + Expect(policy2.Finalizers).ShouldNot(BeNil()) + Expect(policy2.Finalizers).Should(ContainElement(policyFinalizerName)) + + policy3 = ottoscaleriov1alpha1.Policy{} + Expect(k8sClient.Get(ctx, types.NamespacedName{Name: "policy3"}, &policy3)).Should(Succeed()) + Expect(policy3.Finalizers).ShouldNot(BeNil()) + Expect(policy3.Finalizers).Should(ContainElement(policyFinalizerName)) + + //create two policy recommendations for policy2 + now := metav1.Now() + policyreco1 = &ottoscaleriov1alpha1.PolicyRecommendation{ + ObjectMeta: metav1.ObjectMeta{ + Name: PolicyRecoName, + Namespace: PolicyRecoNamespace, + }, + Spec: ottoscaleriov1alpha1.PolicyRecommendationSpec{ + WorkloadMeta: ottoscaleriov1alpha1.WorkloadMeta{ + Name: PolicyRecoName, + }, + Policy: policy2.Name, + GeneratedAt: &now, + TransitionedAt: &now, + QueuedForExecution: &falseBool, + }, + } + policyreco2 = &ottoscaleriov1alpha1.PolicyRecommendation{ + ObjectMeta: metav1.ObjectMeta{ + Name: PolicyRecoName + "2", + Namespace: PolicyRecoNamespace, + }, + Spec: ottoscaleriov1alpha1.PolicyRecommendationSpec{ + WorkloadMeta: ottoscaleriov1alpha1.WorkloadMeta{ + Name: PolicyRecoName + "2", + }, + Policy: policy2.Name, + GeneratedAt: &now, + TransitionedAt: &now, + QueuedForExecution: &falseBool, + }, + } + + Expect(k8sClient.Create(ctx, policyreco1)).Should(Succeed()) + Expect(k8sClient.Create(ctx, policyreco2)).Should(Succeed()) + + time.Sleep(2 * time.Second) + + //update policy2 now + policy2.Spec.RiskIndex = 4 + err := k8sClient.Update(ctx, &policy2) + Expect(err).ToNot(HaveOccurred()) + + time.Sleep(2 * time.Second) + //check if policyreco1 and policyreco2 are queued for execution + Expect(len(queuedOneReco)).Should(Equal(2)) + Expect(queuedOneReco[0]).Should(Equal(true)) + Expect(queuedOneReco[1]).Should(Equal(true)) + + //delete policy2 now, again check if policyreco1 and policyreco2 are queued for execution + queuedOneReco = nil + policy2 := ottoscaleriov1alpha1.Policy{} + Expect(k8sClient.Get(ctx, types.NamespacedName{Name: "policy2"}, &policy2)).Should(Succeed()) + fmt.Println(policy2) + Expect(k8sClient.Delete(ctx, &policy2)).Should(Succeed()) + time.Sleep(5 * time.Second) + Expect(len(queuedOneReco)).Should(Equal(2)) + Expect(queuedOneReco[0]).Should(Equal(true)) + Expect(queuedOneReco[1]).Should(Equal(true)) + + Expect(k8sClient.Delete(ctx, &policy1)).Should(Succeed()) + Expect(k8sClient.Delete(ctx, &policy3)).Should(Succeed()) }) }) }) diff --git a/pkg/controller/policyrecommendation_controller_test.go b/pkg/controller/policyrecommendation_controller_test.go index df689a4..cddfb6b 100644 --- a/pkg/controller/policyrecommendation_controller_test.go +++ b/pkg/controller/policyrecommendation_controller_test.go @@ -98,31 +98,6 @@ var _ = Describe("PolicyrecommendationController", func() { Expect(k8sClient.Delete(ctx, &policy1)).Should(Succeed()) Expect(k8sClient.Delete(ctx, &policy2)).Should(Succeed()) Expect(k8sClient.Delete(ctx, &policy3)).Should(Succeed()) - safestPolicy = v1alpha1.Policy{} - Expect(k8sClient.Get(ctx, types.NamespacedName{Name: "safest-policy"}, &safestPolicy)).Should(Succeed()) - Expect(safestPolicy.DeletionTimestamp).ShouldNot(BeNil()) - safestPolicy.Finalizers = nil - Expect(k8sClient.Update(ctx, &safestPolicy)).Should(Succeed()) - time.Sleep(1 * time.Second) - - policy1 = v1alpha1.Policy{} - Expect(k8sClient.Get(ctx, types.NamespacedName{Name: "policy-1"}, &policy1)).Should(Succeed()) - Expect(policy1.DeletionTimestamp).ShouldNot(BeNil()) - policy1.Finalizers = nil - Expect(k8sClient.Update(ctx, &policy1)).Should(Succeed()) - time.Sleep(1 * time.Second) - - policy2 = v1alpha1.Policy{} - Expect(k8sClient.Get(ctx, types.NamespacedName{Name: "policy-2"}, &policy2)).Should(Succeed()) - Expect(policy2.DeletionTimestamp).ShouldNot(BeNil()) - policy2.Finalizers = nil - Expect(k8sClient.Update(ctx, &policy2)).Should(Succeed()) - time.Sleep(1 * time.Second) - policy3 = v1alpha1.Policy{} - Expect(k8sClient.Get(ctx, types.NamespacedName{Name: "policy-3"}, &policy3)).Should(Succeed()) - Expect(policy3.DeletionTimestamp).ShouldNot(BeNil()) - policy3.Finalizers = nil - Expect(k8sClient.Update(ctx, &policy3)).Should(Succeed()) time.Sleep(1 * time.Second) }) @@ -288,25 +263,6 @@ var _ = Describe("PolicyrecommendationController", func() { Expect(k8sClient.Delete(ctx, &policy1)).Should(Succeed()) Expect(k8sClient.Delete(ctx, &policy2)).Should(Succeed()) - safestPolicy = v1alpha1.Policy{} - Expect(k8sClient.Get(ctx, types.NamespacedName{Name: "safest-policy"}, &safestPolicy)).Should(Succeed()) - Expect(safestPolicy.DeletionTimestamp).ShouldNot(BeNil()) - safestPolicy.Finalizers = nil - Expect(k8sClient.Update(ctx, &safestPolicy)).Should(Succeed()) - time.Sleep(1 * time.Second) - - policy1 = v1alpha1.Policy{} - Expect(k8sClient.Get(ctx, types.NamespacedName{Name: "policy-1"}, &policy1)).Should(Succeed()) - Expect(policy1.DeletionTimestamp).ShouldNot(BeNil()) - policy1.Finalizers = nil - Expect(k8sClient.Update(ctx, &policy1)).Should(Succeed()) - time.Sleep(1 * time.Second) - - policy2 = v1alpha1.Policy{} - Expect(k8sClient.Get(ctx, types.NamespacedName{Name: "policy-2"}, &policy2)).Should(Succeed()) - Expect(policy2.DeletionTimestamp).ShouldNot(BeNil()) - policy2.Finalizers = nil - Expect(k8sClient.Update(ctx, &policy2)).Should(Succeed()) time.Sleep(1 * time.Second) }) @@ -587,18 +543,6 @@ var _ = Describe("PolicyrecommendationController", func() { Expect(k8sClient.Delete(ctx, &safestPolicy)).Should(Succeed()) Expect(k8sClient.Delete(ctx, &policy1)).Should(Succeed()) - safestPolicy = v1alpha1.Policy{} - Expect(k8sClient.Get(ctx, types.NamespacedName{Name: "safest-policy"}, &safestPolicy)).Should(Succeed()) - Expect(safestPolicy.DeletionTimestamp).ShouldNot(BeNil()) - safestPolicy.Finalizers = nil - Expect(k8sClient.Update(ctx, &safestPolicy)).Should(Succeed()) - time.Sleep(1 * time.Second) - - policy1 = v1alpha1.Policy{} - Expect(k8sClient.Get(ctx, types.NamespacedName{Name: "policy-1"}, &policy1)).Should(Succeed()) - Expect(policy1.DeletionTimestamp).ShouldNot(BeNil()) - policy1.Finalizers = nil - Expect(k8sClient.Update(ctx, &policy1)).Should(Succeed()) time.Sleep(1 * time.Second) }) @@ -730,11 +674,6 @@ var _ = Describe("PolicyrecommendationController", func() { }) AfterEach(func() { Expect(k8sClient.Delete(ctx, safestPolicy)).Should(Succeed()) - safestPolicy = &v1alpha1.Policy{} - Expect(k8sClient.Get(ctx, types.NamespacedName{Name: "safest-policy"}, safestPolicy)).Should(Succeed()) - Expect(safestPolicy.DeletionTimestamp).ShouldNot(BeNil()) - safestPolicy.Finalizers = nil - Expect(k8sClient.Update(ctx, safestPolicy)).Should(Succeed()) time.Sleep(1 * time.Second) }) @@ -907,25 +846,6 @@ var _ = Describe("PolicyrecommendationController", func() { Expect(k8sClient.Delete(ctx, &safestPolicy)).Should(Succeed()) Expect(k8sClient.Delete(ctx, &policy1)).Should(Succeed()) Expect(k8sClient.Delete(ctx, &policy2)).Should(Succeed()) - safestPolicy = v1alpha1.Policy{} - Expect(k8sClient.Get(ctx, types.NamespacedName{Name: "safest-policy"}, &safestPolicy)).Should(Succeed()) - Expect(safestPolicy.DeletionTimestamp).ShouldNot(BeNil()) - safestPolicy.Finalizers = nil - Expect(k8sClient.Update(ctx, &safestPolicy)).Should(Succeed()) - time.Sleep(1 * time.Second) - - policy1 = v1alpha1.Policy{} - Expect(k8sClient.Get(ctx, types.NamespacedName{Name: "policy-1"}, &policy1)).Should(Succeed()) - Expect(policy1.DeletionTimestamp).ShouldNot(BeNil()) - policy1.Finalizers = nil - Expect(k8sClient.Update(ctx, &policy1)).Should(Succeed()) - time.Sleep(1 * time.Second) - - policy2 = v1alpha1.Policy{} - Expect(k8sClient.Get(ctx, types.NamespacedName{Name: "policy-2"}, &policy2)).Should(Succeed()) - Expect(policy2.DeletionTimestamp).ShouldNot(BeNil()) - policy2.Finalizers = nil - Expect(k8sClient.Update(ctx, &policy2)).Should(Succeed()) time.Sleep(1 * time.Second) }) It("Should Create a new PolicyRecommendation with Initialized,RecoTaskQueued,Recommendation Generated and Target Reco Achieved Status Conditions", func() { diff --git a/pkg/controller/suite_test.go b/pkg/controller/suite_test.go index 6beb7f9..f710cf8 100644 --- a/pkg/controller/suite_test.go +++ b/pkg/controller/suite_test.go @@ -18,6 +18,8 @@ package controller import ( "errors" + "time" + rolloutv1alpha1 "github.com/argoproj/argo-rollouts/pkg/apis/rollouts/v1alpha1" "github.com/flipkart-incubator/ottoscalr/pkg/reco" "github.com/flipkart-incubator/ottoscalr/pkg/testutil" @@ -28,11 +30,11 @@ import ( "golang.org/x/net/context" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/types" - "time" - ctrl "sigs.k8s.io/controller-runtime" "testing" + ctrl "sigs.k8s.io/controller-runtime" + "k8s.io/client-go/kubernetes/scheme" "k8s.io/client-go/rest" "sigs.k8s.io/controller-runtime/pkg/client" @@ -54,6 +56,7 @@ var ( cancel context.CancelFunc queuedAllRecos = false + queuedOneReco []bool recommender *MockRecommender deploymentTriggerControllerEnv *testutil.TestEnvironment excludedNamespaces []string @@ -138,7 +141,7 @@ var _ = BeforeSuite(func() { queuedAllRecos = true }, requeueOneFunc: func(namespacedName types.NamespacedName) { - queuedAllRecos = true + queuedOneReco = append(queuedOneReco, true) }, }).SetupWithManager(k8sManager) Expect(err).ToNot(HaveOccurred()) From 7f81af744027c3f1d28036dcdc111c46a1eb49ed Mon Sep 17 00:00:00 2001 From: deefreak Date: Wed, 8 Nov 2023 14:27:15 +0530 Subject: [PATCH 7/7] added unit tests for policy controller reconciliation --- pkg/controller/policy_controller_test.go | 1 + pkg/controller/suite_test.go | 4 +++- 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/pkg/controller/policy_controller_test.go b/pkg/controller/policy_controller_test.go index d5d4a76..9124259 100644 --- a/pkg/controller/policy_controller_test.go +++ b/pkg/controller/policy_controller_test.go @@ -204,6 +204,7 @@ var _ = Describe("PolicyWatcher controller", func() { //update policy2 now policy2.Spec.RiskIndex = 4 + queuedOneReco = nil err := k8sClient.Update(ctx, &policy2) Expect(err).ToNot(HaveOccurred()) diff --git a/pkg/controller/suite_test.go b/pkg/controller/suite_test.go index f710cf8..40283e3 100644 --- a/pkg/controller/suite_test.go +++ b/pkg/controller/suite_test.go @@ -141,7 +141,9 @@ var _ = BeforeSuite(func() { queuedAllRecos = true }, requeueOneFunc: func(namespacedName types.NamespacedName) { - queuedOneReco = append(queuedOneReco, true) + if namespacedName.Name == "test-deployment-afgre" || namespacedName.Name == "test-deployment-afgre2" { + queuedOneReco = append(queuedOneReco, true) + } }, }).SetupWithManager(k8sManager) Expect(err).ToNot(HaveOccurred())