From 159d4ae1d5ff7e08a7ce98350ba83e5ce36b51a6 Mon Sep 17 00:00:00 2001 From: fdfzcq Date: Wed, 15 Mar 2023 13:42:19 +0100 Subject: [PATCH 1/8] Do not block deletion if neg status is empty --- controllers/service_controller.go | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/controllers/service_controller.go b/controllers/service_controller.go index e7652d2..11fa682 100644 --- a/controllers/service_controller.go +++ b/controllers/service_controller.go @@ -93,7 +93,7 @@ func (r *ServiceReconciler) Reconcile(req ctrl.Request) (ctrl.Result, error) { intendedStatus.NEGStatus = NEGStatus{} } - if reflect.DeepEqual(status.status, intendedStatus) { + if reflect.DeepEqual(status.status, intendedStatus) && !deleting { // Equal, no reconciliation necessary return reconcile.Result{}, nil } @@ -177,9 +177,7 @@ func (r *ServiceReconciler) SetupWithManager(mgr ctrl.Manager) error { Complete(r) } -// // Helper functions to check and remove string from a slice of strings. -// func containsString(slice []string, s string) bool { for _, item := range slice { if item == s { From 1738b67471d4ec184786ae43d16b027e159ccb94 Mon Sep 17 00:00:00 2001 From: fdfzcq Date: Thu, 16 Mar 2023 13:12:50 +0100 Subject: [PATCH 2/8] Add test --- controllers/controller_test.go | 107 ++++++++++++++++++++++++++++++ controllers/service_controller.go | 2 + controllers/suite_test.go | 26 ++++++++ 3 files changed, 135 insertions(+) create mode 100644 controllers/controller_test.go diff --git a/controllers/controller_test.go b/controllers/controller_test.go new file mode 100644 index 0000000..db236a7 --- /dev/null +++ b/controllers/controller_test.go @@ -0,0 +1,107 @@ +package controllers + +import ( + "context" + "log" + "reflect" + "time" + + . "github.com/onsi/ginkgo" + . "github.com/onsi/gomega" + apps "k8s.io/api/apps/v1" + corev1 "k8s.io/api/core/v1" + v1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + "sigs.k8s.io/controller-runtime/pkg/client" +) + +var _ = Describe("Run autoneg Controller", func() { + + ctx := context.Background() + + serviceKey := client.ObjectKey{ + Name: "old-service", + Namespace: "namespace", + } + + Context("Create a service resource with autoneg annotations", func() { + + It("should succeed", func() { + + annotations := make(map[string]string) + annotations[negAnnotation] = "{\"exposed_ports\":{\"4242\":{}}}" + annotations[autonegAnnotation] = "{\"backend_services\":{\"4242\":[{\"max_rate_per_endpoint\":4242}]}}" + + ports := make([]corev1.ServicePort, 1) + ports[0] = corev1.ServicePort{ + Port: 4242, + Protocol: corev1.ProtocolTCP, + } + + service := &corev1.Service{ + ObjectMeta: v1.ObjectMeta{ + Name: "old-service", + Namespace: "namespace", + Annotations: annotations, + }, + Spec: corev1.ServiceSpec{ + Ports: ports, + }, + } + + err := k8sClient.Create(ctx, service) + Expect(err).NotTo(HaveOccurred(), "failed to create service resource") + + time.Sleep(time.Second * 5) + + createdService := &corev1.Service{} + err = k8sClient.Get(ctx, serviceKey, createdService) + Expect(err).NotTo(HaveOccurred(), "failed to retrieve service resource") + + updatedAnnos := createdService.Annotations + log.Printf("%v", updatedAnnos) + Expect(updatedAnnos[autonegStatusAnnotation]).To(Equal( + "{\"backend_services\":{\"4242\":{\"namespace-old-service-4242-de64ba2d\":" + + "{\"name\":\"namespace-old-service-4242-de64ba2d\",\"max_rate_per_endpoint\":4242}}}," + + "\"network_endpoint_groups\":null,\"zones\":null}")) + Expect(updatedAnnos[negStatusAnnotation]).To(BeEmpty()) + }) + + Context("Remove the service", func() { + + It("should succeed", func() { + createdService := &corev1.Service{} + err := k8sClient.Get(ctx, serviceKey, createdService) + Expect(err).NotTo(HaveOccurred(), "failed to retrieve service resource") + + err = k8sClient.Delete(ctx, createdService) + Expect(err).NotTo(HaveOccurred(), "failed to delete service resource") + + time.Sleep(time.Second * 5) + + err = k8sClient.Get(ctx, serviceKey, &corev1.Service{}) + + var e *errNotFound + Expect(err).To(HaveOccurred()) + Expect(reflect.TypeOf(err).Kind()).To(Equal(reflect.TypeOf(e).Kind())) + }) + + }) + }) +}) + +func getResourceFunc(ctx context.Context, key client.ObjectKey, obj runtime.Object) func() error { + return func() error { + return k8sClient.Get(ctx, key, obj) + } +} + +func getDeploymentReplicasFunc(ctx context.Context, key client.ObjectKey) func() int32 { + return func() int32 { + depl := &apps.Deployment{} + err := k8sClient.Get(ctx, key, depl) + Expect(err).NotTo(HaveOccurred(), "failed to get Deployment resource") + + return *depl.Spec.Replicas + } +} diff --git a/controllers/service_controller.go b/controllers/service_controller.go index 11fa682..3c387ce 100644 --- a/controllers/service_controller.go +++ b/controllers/service_controller.go @@ -20,6 +20,7 @@ import ( "context" "encoding/json" "errors" + "log" "reflect" "github.com/go-logr/logr" @@ -50,6 +51,7 @@ func (r *ServiceReconciler) Reconcile(req ctrl.Request) (ctrl.Result, error) { ctx := context.Background() logger := r.Log.WithValues("service", req.NamespacedName) + log.Printf("reconciling") // for debugging svc := &corev1.Service{} err := r.Get(ctx, req.NamespacedName, svc) if err != nil { diff --git a/controllers/suite_test.go b/controllers/suite_test.go index 8486297..cb6ca9f 100644 --- a/controllers/suite_test.go +++ b/controllers/suite_test.go @@ -19,13 +19,16 @@ package controllers import ( "path/filepath" "testing" + "time" . "github.com/onsi/ginkgo" . "github.com/onsi/gomega" + "github.com/onsi/gomega/gexec" corev1 "k8s.io/api/core/v1" "k8s.io/client-go/kubernetes/scheme" "k8s.io/client-go/rest" + ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/envtest" logf "sigs.k8s.io/controller-runtime/pkg/log" @@ -38,6 +41,7 @@ import ( var cfg *rest.Config var k8sClient client.Client +var k8sManager ctrl.Manager var testEnv *envtest.Environment func TestAPIs(t *testing.T) { @@ -70,11 +74,33 @@ var _ = BeforeSuite(func(done Done) { Expect(err).ToNot(HaveOccurred()) Expect(k8sClient).ToNot(BeNil()) + k8sManager, err := ctrl.NewManager(cfg, ctrl.Options{ + Scheme: scheme.Scheme, + }) + Expect(err).ToNot(HaveOccurred()) + + err = (&ServiceReconciler{ + Client: k8sManager.GetClient(), + BackendController: &BackendController{}, + Recorder: k8sManager.GetEventRecorderFor("autoneg-controller"), + Log: ctrl.Log.WithName("controllers").WithName("Service"), + ServiceNameTemplate: serviceNameTemplate, + AllowServiceName: true, + }).SetupWithManager(k8sManager) + Expect(err).ToNot(HaveOccurred()) + + go func() { + defer GinkgoRecover() + err = k8sManager.Start(ctrl.SetupSignalHandler()) + Expect(err).ToNot(HaveOccurred()) + }() + close(done) }, 60) var _ = AfterSuite(func() { By("tearing down the test environment") + gexec.KillAndWait(5 * time.Second) err := testEnv.Stop() Expect(err).ToNot(HaveOccurred()) }) From b4ee2cd908e5ec86111642518b0dc226295190c4 Mon Sep 17 00:00:00 2001 From: fdfzcq Date: Thu, 16 Mar 2023 13:14:24 +0100 Subject: [PATCH 3/8] Remove debugging logs --- controllers/controller_test.go | 3 +-- controllers/service_controller.go | 2 -- 2 files changed, 1 insertion(+), 4 deletions(-) diff --git a/controllers/controller_test.go b/controllers/controller_test.go index db236a7..645425d 100644 --- a/controllers/controller_test.go +++ b/controllers/controller_test.go @@ -2,7 +2,6 @@ package controllers import ( "context" - "log" "reflect" "time" @@ -59,7 +58,7 @@ var _ = Describe("Run autoneg Controller", func() { Expect(err).NotTo(HaveOccurred(), "failed to retrieve service resource") updatedAnnos := createdService.Annotations - log.Printf("%v", updatedAnnos) + Expect(updatedAnnos[autonegStatusAnnotation]).To(Equal( "{\"backend_services\":{\"4242\":{\"namespace-old-service-4242-de64ba2d\":" + "{\"name\":\"namespace-old-service-4242-de64ba2d\",\"max_rate_per_endpoint\":4242}}}," + diff --git a/controllers/service_controller.go b/controllers/service_controller.go index 3c387ce..11fa682 100644 --- a/controllers/service_controller.go +++ b/controllers/service_controller.go @@ -20,7 +20,6 @@ import ( "context" "encoding/json" "errors" - "log" "reflect" "github.com/go-logr/logr" @@ -51,7 +50,6 @@ func (r *ServiceReconciler) Reconcile(req ctrl.Request) (ctrl.Result, error) { ctx := context.Background() logger := r.Log.WithValues("service", req.NamespacedName) - log.Printf("reconciling") // for debugging svc := &corev1.Service{} err := r.Get(ctx, req.NamespacedName, svc) if err != nil { From c7f36c7434e89df2c96efe472e56d09e253c85c3 Mon Sep 17 00:00:00 2001 From: fdfzcq Date: Thu, 16 Mar 2023 14:04:42 +0100 Subject: [PATCH 4/8] fast async assertion instead of time.sleep --- controllers/controller_test.go | 19 ++++++++++++------- controllers/suite_test.go | 4 +--- 2 files changed, 13 insertions(+), 10 deletions(-) diff --git a/controllers/controller_test.go b/controllers/controller_test.go index 645425d..a950fe9 100644 --- a/controllers/controller_test.go +++ b/controllers/controller_test.go @@ -51,11 +51,15 @@ var _ = Describe("Run autoneg Controller", func() { err := k8sClient.Create(ctx, service) Expect(err).NotTo(HaveOccurred(), "failed to create service resource") - time.Sleep(time.Second * 5) - createdService := &corev1.Service{} - err = k8sClient.Get(ctx, serviceKey, createdService) - Expect(err).NotTo(HaveOccurred(), "failed to retrieve service resource") + + Eventually(func() string { + err = k8sClient.Get(ctx, serviceKey, createdService) + Expect(err).NotTo(HaveOccurred(), "failed to retrieve service resource") + annos := createdService.Annotations + autonegStatus := annos[autonegStatusAnnotation] + return autonegStatus + }, time.Second*5, time.Second).ShouldNot(BeEmpty()) updatedAnnos := createdService.Annotations @@ -76,9 +80,10 @@ var _ = Describe("Run autoneg Controller", func() { err = k8sClient.Delete(ctx, createdService) Expect(err).NotTo(HaveOccurred(), "failed to delete service resource") - time.Sleep(time.Second * 5) - - err = k8sClient.Get(ctx, serviceKey, &corev1.Service{}) + Eventually(func() error { + err = k8sClient.Get(ctx, serviceKey, &corev1.Service{}) + return err + }, time.Second*5, time.Second).ShouldNot(BeNil()) var e *errNotFound Expect(err).To(HaveOccurred()) diff --git a/controllers/suite_test.go b/controllers/suite_test.go index cb6ca9f..dfecbd7 100644 --- a/controllers/suite_test.go +++ b/controllers/suite_test.go @@ -19,11 +19,9 @@ package controllers import ( "path/filepath" "testing" - "time" . "github.com/onsi/ginkgo" . "github.com/onsi/gomega" - "github.com/onsi/gomega/gexec" corev1 "k8s.io/api/core/v1" "k8s.io/client-go/kubernetes/scheme" @@ -100,7 +98,7 @@ var _ = BeforeSuite(func(done Done) { var _ = AfterSuite(func() { By("tearing down the test environment") - gexec.KillAndWait(5 * time.Second) + //gexec.KillAndWait(5 * time.Second) err := testEnv.Stop() Expect(err).ToNot(HaveOccurred()) }) From f48b3fea31d3778ed3e7d304e4d2b9d3af2e5055 Mon Sep 17 00:00:00 2001 From: Sophy Cao Date: Thu, 16 Mar 2023 14:10:00 +0100 Subject: [PATCH 5/8] Remove commented line --- controllers/suite_test.go | 1 - 1 file changed, 1 deletion(-) diff --git a/controllers/suite_test.go b/controllers/suite_test.go index dfecbd7..0b78cc2 100644 --- a/controllers/suite_test.go +++ b/controllers/suite_test.go @@ -98,7 +98,6 @@ var _ = BeforeSuite(func(done Done) { var _ = AfterSuite(func() { By("tearing down the test environment") - //gexec.KillAndWait(5 * time.Second) err := testEnv.Stop() Expect(err).ToNot(HaveOccurred()) }) From cf98ea5641c0bc7db4e9e7fbdc8421c4cdf42928 Mon Sep 17 00:00:00 2001 From: sophyc Date: Thu, 16 Mar 2023 14:55:55 +0100 Subject: [PATCH 6/8] Fix failing tests --- controllers/controller_test.go | 29 ++++++++++------------------- controllers/suite_test.go | 14 ++++++++++---- 2 files changed, 20 insertions(+), 23 deletions(-) diff --git a/controllers/controller_test.go b/controllers/controller_test.go index a950fe9..9a13d7d 100644 --- a/controllers/controller_test.go +++ b/controllers/controller_test.go @@ -7,10 +7,8 @@ import ( . "github.com/onsi/ginkgo" . "github.com/onsi/gomega" - apps "k8s.io/api/apps/v1" corev1 "k8s.io/api/core/v1" v1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "k8s.io/apimachinery/pkg/runtime" "sigs.k8s.io/controller-runtime/pkg/client" ) @@ -27,6 +25,15 @@ var _ = Describe("Run autoneg Controller", func() { It("should succeed", func() { + namespace := &corev1.Namespace{ + ObjectMeta: v1.ObjectMeta{ + Name: "namespace", + }, + } + + err := k8sClient.Create(ctx, namespace) + Expect(err).NotTo(HaveOccurred()) + annotations := make(map[string]string) annotations[negAnnotation] = "{\"exposed_ports\":{\"4242\":{}}}" annotations[autonegAnnotation] = "{\"backend_services\":{\"4242\":[{\"max_rate_per_endpoint\":4242}]}}" @@ -48,7 +55,7 @@ var _ = Describe("Run autoneg Controller", func() { }, } - err := k8sClient.Create(ctx, service) + err = k8sClient.Create(ctx, service) Expect(err).NotTo(HaveOccurred(), "failed to create service resource") createdService := &corev1.Service{} @@ -93,19 +100,3 @@ var _ = Describe("Run autoneg Controller", func() { }) }) }) - -func getResourceFunc(ctx context.Context, key client.ObjectKey, obj runtime.Object) func() error { - return func() error { - return k8sClient.Get(ctx, key, obj) - } -} - -func getDeploymentReplicasFunc(ctx context.Context, key client.ObjectKey) func() int32 { - return func() int32 { - depl := &apps.Deployment{} - err := k8sClient.Get(ctx, key, depl) - Expect(err).NotTo(HaveOccurred(), "failed to get Deployment resource") - - return *depl.Spec.Replicas - } -} diff --git a/controllers/suite_test.go b/controllers/suite_test.go index c249f69..5cae226 100644 --- a/controllers/suite_test.go +++ b/controllers/suite_test.go @@ -17,11 +17,14 @@ limitations under the License. package controllers import ( + "context" "path/filepath" "testing" + "time" . "github.com/onsi/ginkgo" . "github.com/onsi/gomega" + "github.com/onsi/gomega/gexec" corev1 "k8s.io/api/core/v1" "k8s.io/client-go/kubernetes/scheme" "k8s.io/client-go/rest" @@ -41,6 +44,7 @@ var cfg *rest.Config var k8sClient client.Client var k8sManager ctrl.Manager var testEnv *envtest.Environment +var cancel context.CancelFunc func TestAPIs(t *testing.T) { RegisterFailHandler(Fail) @@ -53,6 +57,9 @@ func TestAPIs(t *testing.T) { var _ = BeforeSuite(func() { logf.SetLogger(zap.New(zap.WriteTo(GinkgoWriter), zap.UseDevMode(true))) + var ctx context.Context + ctx, cancel = context.WithCancel(context.TODO()) + By("bootstrapping test environment") testEnv = &envtest.Environment{ CRDDirectoryPaths: []string{filepath.Join("..", "config", "crd", "bases")}, @@ -81,7 +88,6 @@ var _ = BeforeSuite(func() { Client: k8sManager.GetClient(), BackendController: &BackendController{}, Recorder: k8sManager.GetEventRecorderFor("autoneg-controller"), - Log: ctrl.Log.WithName("controllers").WithName("Service"), ServiceNameTemplate: serviceNameTemplate, AllowServiceName: true, }).SetupWithManager(k8sManager) @@ -89,15 +95,15 @@ var _ = BeforeSuite(func() { go func() { defer GinkgoRecover() - err = k8sManager.Start(ctrl.SetupSignalHandler()) + err = k8sManager.Start(ctx) Expect(err).ToNot(HaveOccurred()) }() - - close(done) }, 60) var _ = AfterSuite(func() { + cancel() By("tearing down the test environment") + gexec.KillAndWait(4 * time.Second) err := testEnv.Stop() Expect(err).NotTo(HaveOccurred()) }) From 07ebff63e39919514565b56cc3ba0cb721811257 Mon Sep 17 00:00:00 2001 From: sophyc Date: Thu, 16 Mar 2023 15:06:38 +0100 Subject: [PATCH 7/8] remove useless line --- controllers/suite_test.go | 3 --- 1 file changed, 3 deletions(-) diff --git a/controllers/suite_test.go b/controllers/suite_test.go index 5cae226..5f2a8c9 100644 --- a/controllers/suite_test.go +++ b/controllers/suite_test.go @@ -20,11 +20,9 @@ import ( "context" "path/filepath" "testing" - "time" . "github.com/onsi/ginkgo" . "github.com/onsi/gomega" - "github.com/onsi/gomega/gexec" corev1 "k8s.io/api/core/v1" "k8s.io/client-go/kubernetes/scheme" "k8s.io/client-go/rest" @@ -103,7 +101,6 @@ var _ = BeforeSuite(func() { var _ = AfterSuite(func() { cancel() By("tearing down the test environment") - gexec.KillAndWait(4 * time.Second) err := testEnv.Stop() Expect(err).NotTo(HaveOccurred()) }) From c927fcc96f0cf1192ab9801acb1282c53a62c98e Mon Sep 17 00:00:00 2001 From: sophyc Date: Thu, 16 Mar 2023 16:27:14 +0100 Subject: [PATCH 8/8] tidy up the logic --- controllers/service_controller.go | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/controllers/service_controller.go b/controllers/service_controller.go index befc25d..d839012 100644 --- a/controllers/service_controller.go +++ b/controllers/service_controller.go @@ -100,9 +100,7 @@ func (r *ServiceReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ct if deleting { intendedStatus.NEGStatus = NEGStatus{} - } - - if reflect.DeepEqual(status.status, intendedStatus) && !deleting { + } else if reflect.DeepEqual(status.status, intendedStatus) { // Equal, no reconciliation necessary return reconcile.Result{}, nil }