From ecb9cdd1ed8dcd040ae51ce137698c87aaf8c5e7 Mon Sep 17 00:00:00 2001 From: DQ Date: Mon, 14 Apr 2025 12:12:21 +1000 Subject: [PATCH 1/6] feat: Enhance AppDeployment controller with improved reconciliation logic and event recording --- .../controller/appdeployment_controller.go | 66 ++++++++++++-- .../appdeployment_controller_test.go | 86 ++++++++++++++++++- 2 files changed, 143 insertions(+), 9 deletions(-) diff --git a/internal/controller/appdeployment_controller.go b/internal/controller/appdeployment_controller.go index b504944..caa8b98 100644 --- a/internal/controller/appdeployment_controller.go +++ b/internal/controller/appdeployment_controller.go @@ -19,18 +19,25 @@ package controller import ( "context" + batchv1 "k8s.io/api/batch/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" + "k8s.io/client-go/tools/record" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client" - logf "sigs.k8s.io/controller-runtime/pkg/log" + "sigs.k8s.io/controller-runtime/pkg/controller" + "sigs.k8s.io/controller-runtime/pkg/log" - appv1 "github.com/Azure/operation-cache-controller/api/v1" + appsv1 "github.com/Azure/operation-cache-controller/api/v1" + apdutil "github.com/Azure/operation-cache-controller/internal/utils/controller/appdeployment" + "github.com/Azure/operation-cache-controller/internal/utils/reconciler" ) // AppDeploymentReconciler reconciles a AppDeployment object type AppDeploymentReconciler struct { client.Client - Scheme *runtime.Scheme + Scheme *runtime.Scheme + recorder record.EventRecorder } // +kubebuilder:rbac:groups=app.github.com,resources=appdeployments,verbs=get;list;watch;create;update;patch;delete @@ -47,17 +54,64 @@ type AppDeploymentReconciler struct { // For more details, check Reconcile and its Result here: // - https://pkg.go.dev/sigs.k8s.io/controller-runtime@v0.20.4/pkg/reconcile func (r *AppDeploymentReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) { - _ = logf.FromContext(ctx) + logger := log.FromContext(ctx).WithValues(apdutil.LogKeyAppDeploymentName, req.NamespacedName) + appdeployment := &appsv1.AppDeployment{} + if err := r.Get(ctx, req.NamespacedName, appdeployment); err != nil { + return ctrl.Result{}, client.IgnoreNotFound(err) + } - // TODO(user): your logic here + adapter := NewAppDeploymentAdapter(ctx, appdeployment, logger, r.Client, r.recorder) + return r.ReconcileHandler(ctx, adapter) +} +func (r *AppDeploymentReconciler) ReconcileHandler(ctx context.Context, adapter AppDeploymentAdapterInterface) (ctrl.Result, error) { + operations := []reconciler.ReconcileOperation{ + adapter.EnsureApplicationValid, + adapter.EnsureFinalizer, + adapter.EnsureFinalizerDeleted, + adapter.EnsureDependenciesReady, + adapter.EnsureDeployingFinished, + adapter.EnsureTeardownFinished, + } + for _, operation := range operations { + operationResult, err := operation(ctx) + if err != nil || operationResult.RequeueRequest { + return ctrl.Result{RequeueAfter: operationResult.RequeueDelay}, err + } + if operationResult.CancelRequest { + return ctrl.Result{}, nil + } + } return ctrl.Result{}, nil } +var appDeploymentOwnerKey = ".appDeployment.metadata.controller" + // SetupWithManager sets up the controller with the Manager. func (r *AppDeploymentReconciler) SetupWithManager(mgr ctrl.Manager) error { + if err := mgr.GetFieldIndexer().IndexField(context.Background(), &batchv1.Job{}, appDeploymentOwnerKey, + func(rawObj client.Object) []string { + job := rawObj.(*batchv1.Job) + owner := metav1.GetControllerOf(job) + if owner == nil { + return nil + } + if owner.APIVersion != appsv1.GroupVersion.String() || owner.Kind != "AppDeployment" { + return nil + } + return []string{owner.Name} + }); err != nil { + return err + } + + r.recorder = mgr.GetEventRecorderFor("AppDeployment") + return ctrl.NewControllerManagedBy(mgr). - For(&appv1.AppDeployment{}). + For(&appsv1.AppDeployment{}). + Owns(&batchv1.Job{}). + WithOptions(controller.Options{ + MaxConcurrentReconciles: 100, + }). Named("appdeployment"). Complete(r) } diff --git a/internal/controller/appdeployment_controller_test.go b/internal/controller/appdeployment_controller_test.go index 34d0ca7..756e601 100644 --- a/internal/controller/appdeployment_controller_test.go +++ b/internal/controller/appdeployment_controller_test.go @@ -21,8 +21,11 @@ import ( . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" + "go.uber.org/mock/gomock" "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/types" + "k8s.io/client-go/kubernetes/scheme" + ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/reconcile" batchv1 "k8s.io/api/batch/v1" @@ -30,6 +33,9 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" appv1 "github.com/Azure/operation-cache-controller/api/v1" + ctrlmocks "github.com/Azure/operation-cache-controller/internal/controller/mocks" + mockpkg "github.com/Azure/operation-cache-controller/internal/mocks" + "github.com/Azure/operation-cache-controller/internal/utils/reconciler" ) func newTestJobSpec() batchv1.JobSpec { @@ -55,9 +61,35 @@ func newTestJobSpec() batchv1.JobSpec { } var _ = Describe("AppDeployment Controller", func() { + Context("When setupWithManager is called", func() { + It("Should setup the controller with the manager", func() { + + // Create a new mock controller + mockCtrl := gomock.NewController(GinkgoT()) + defer mockCtrl.Finish() + + k8sManager, err := ctrl.NewManager(cfg, ctrl.Options{ + Scheme: scheme.Scheme, + }) + Expect(err).NotTo(HaveOccurred()) + + err = (&AppDeploymentReconciler{ + Client: k8sManager.GetClient(), + Scheme: k8sManager.GetScheme(), + recorder: k8sManager.GetEventRecorderFor("appdeployment-controller"), + }).SetupWithManager(k8sManager) + Expect(err).NotTo(HaveOccurred()) + }) + }) + Context("When reconciling a resource", func() { const resourceName = "test-resource" - + var ( + mockRecorderCtrl *gomock.Controller + mockRecorder *mockpkg.MockEventRecorder + mockAdapterCtrl *gomock.Controller + mockAdapter *ctrlmocks.MockAppDeploymentAdapterInterface + ) ctx := context.Background() typeNamespacedName := types.NamespacedName{ @@ -87,6 +119,10 @@ var _ = Describe("AppDeployment Controller", func() { }} Expect(k8sClient.Create(ctx, resource)).To(Succeed()) } + mockRecorderCtrl = gomock.NewController(GinkgoT()) + mockRecorder = mockpkg.NewMockEventRecorder(mockRecorderCtrl) + mockAdapterCtrl = gomock.NewController(GinkgoT()) + mockAdapter = ctrlmocks.NewMockAppDeploymentAdapterInterface(mockAdapterCtrl) }) AfterEach(func() { @@ -101,9 +137,18 @@ var _ = Describe("AppDeployment Controller", func() { It("should successfully reconcile the resource", func() { By("Reconciling the created resource") controllerReconciler := &AppDeploymentReconciler{ - Client: k8sClient, - Scheme: k8sClient.Scheme(), + Client: k8sClient, + Scheme: k8sClient.Scheme(), + recorder: mockRecorder, } + ctx = context.WithValue(ctx, appdeploymentAdapterContextKey{}, mockAdapter) + + mockAdapter.EXPECT().EnsureApplicationValid(gomock.Any()).Return(reconciler.OperationResult{}, nil) + mockAdapter.EXPECT().EnsureFinalizer(gomock.Any()).Return(reconciler.OperationResult{}, nil) + mockAdapter.EXPECT().EnsureFinalizerDeleted(gomock.Any()).Return(reconciler.OperationResult{}, nil) + mockAdapter.EXPECT().EnsureDependenciesReady(gomock.Any()).Return(reconciler.OperationResult{}, nil) + mockAdapter.EXPECT().EnsureDeployingFinished(gomock.Any()).Return(reconciler.OperationResult{}, nil) + mockAdapter.EXPECT().EnsureTeardownFinished(gomock.Any()).Return(reconciler.OperationResult{}, nil) _, err := controllerReconciler.Reconcile(ctx, reconcile.Request{ NamespacedName: typeNamespacedName, @@ -112,5 +157,40 @@ var _ = Describe("AppDeployment Controller", func() { // TODO(user): Add more specific assertions depending on your controller's reconciliation logic. // Example: If you expect a certain status condition after reconciliation, verify it here. }) + It("should cancel the reconcile loop", func() { + By("Reconciling the created resource") + controllerReconciler := &AppDeploymentReconciler{ + Client: k8sClient, + Scheme: k8sClient.Scheme(), + recorder: mockRecorder, + } + ctx = context.WithValue(ctx, appdeploymentAdapterContextKey{}, mockAdapter) + + mockAdapter.EXPECT().EnsureApplicationValid(gomock.Any()).Return(reconciler.OperationResult{ + CancelRequest: true, + }, nil) + + _, err := controllerReconciler.Reconcile(ctx, reconcile.Request{ + NamespacedName: typeNamespacedName, + }) + Expect(err).NotTo(HaveOccurred()) + }) + + It("should fail to reconcile the resource", func() { + By("Reconciling the created resource") + controllerReconciler := &AppDeploymentReconciler{ + Client: k8sClient, + Scheme: k8sClient.Scheme(), + recorder: mockRecorder, + } + ctx = context.WithValue(ctx, appdeploymentAdapterContextKey{}, mockAdapter) + + mockAdapter.EXPECT().EnsureApplicationValid(gomock.Any()).Return(reconciler.OperationResult{}, errors.NewServiceUnavailable("test error")) + + _, err := controllerReconciler.Reconcile(ctx, reconcile.Request{ + NamespacedName: typeNamespacedName, + }) + Expect(errors.IsServiceUnavailable(err)).To(Equal(true), "expected error is ServiceUnavailable") + }) }) }) From b7ffd3d7b67c5f44aa6d1cf4f7fc5f889d54aca5 Mon Sep 17 00:00:00 2001 From: DQ Date: Mon, 14 Apr 2025 12:22:31 +1000 Subject: [PATCH 2/6] fix: Update expectation in AppDeployment controller test for ServiceUnavailable error --- internal/controller/appdeployment_controller_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/controller/appdeployment_controller_test.go b/internal/controller/appdeployment_controller_test.go index 756e601..7f8d7de 100644 --- a/internal/controller/appdeployment_controller_test.go +++ b/internal/controller/appdeployment_controller_test.go @@ -190,7 +190,7 @@ var _ = Describe("AppDeployment Controller", func() { _, err := controllerReconciler.Reconcile(ctx, reconcile.Request{ NamespacedName: typeNamespacedName, }) - Expect(errors.IsServiceUnavailable(err)).To(Equal(true), "expected error is ServiceUnavailable") + Expect(errors.IsServiceUnavailable(err)).To(BeTrue(), "expected error is ServiceUnavailable") }) }) }) From dc67bed84733fae3fe1d29d809a3e94d9d8be85a Mon Sep 17 00:00:00 2001 From: DQ Date: Mon, 14 Apr 2025 13:56:39 +1000 Subject: [PATCH 3/6] feat: Add permissions for batch jobs and their statuses in RBAC role --- config/rbac/role.yaml | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/config/rbac/role.yaml b/config/rbac/role.yaml index 7e8af4c..ed445ec 100644 --- a/config/rbac/role.yaml +++ b/config/rbac/role.yaml @@ -39,3 +39,21 @@ rules: - get - patch - update +- apiGroups: + - batch + resources: + - jobs + verbs: + - create + - delete + - get + - list + - patch + - update + - watch +- apiGroups: + - batch + resources: + - jobs/status + verbs: + - get From a41bbf0190c0c8186b5e78900a0fdf07e91e6e03 Mon Sep 17 00:00:00 2001 From: DQ Date: Mon, 14 Apr 2025 15:25:06 +1000 Subject: [PATCH 4/6] feat: Increase Gomega output length for better test readability --- test/e2e/e2e_test.go | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/test/e2e/e2e_test.go b/test/e2e/e2e_test.go index c1b51c2..7d1dbf2 100644 --- a/test/e2e/e2e_test.go +++ b/test/e2e/e2e_test.go @@ -26,10 +26,15 @@ import ( . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" + "github.com/onsi/gomega/format" "github.com/Azure/operation-cache-controller/test/utils" ) +func init() { + format.MaxLength = 10000 // Or however much you need +} + // namespace where the project is deployed in const namespace = "operation-cache-controller-system" From 3adf47715ab8943d6afbeb48bea27acb2820a97a Mon Sep 17 00:00:00 2001 From: DQ Date: Mon, 14 Apr 2025 15:37:15 +1000 Subject: [PATCH 5/6] feat: Increase Gomega output length to enhance test log readability --- test/e2e/e2e_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/e2e/e2e_test.go b/test/e2e/e2e_test.go index 7d1dbf2..358f532 100644 --- a/test/e2e/e2e_test.go +++ b/test/e2e/e2e_test.go @@ -32,7 +32,7 @@ import ( ) func init() { - format.MaxLength = 10000 // Or however much you need + format.MaxLength = 20000 } // namespace where the project is deployed in From 359e0376f8c1726605659522dc0d5434c7e9e8f3 Mon Sep 17 00:00:00 2001 From: DQ Date: Mon, 14 Apr 2025 16:21:29 +1000 Subject: [PATCH 6/6] feat: Add RBAC permissions for batch jobs and their finalizers --- config/rbac/role.yaml | 8 ++++++++ internal/controller/appdeployment_controller.go | 3 +++ 2 files changed, 11 insertions(+) diff --git a/config/rbac/role.yaml b/config/rbac/role.yaml index ed445ec..6eeca17 100644 --- a/config/rbac/role.yaml +++ b/config/rbac/role.yaml @@ -51,9 +51,17 @@ rules: - patch - update - watch +- apiGroups: + - batch + resources: + - jobs/finalizers + verbs: + - update - apiGroups: - batch resources: - jobs/status verbs: - get + - patch + - update diff --git a/internal/controller/appdeployment_controller.go b/internal/controller/appdeployment_controller.go index caa8b98..2e4aae3 100644 --- a/internal/controller/appdeployment_controller.go +++ b/internal/controller/appdeployment_controller.go @@ -43,6 +43,9 @@ type AppDeploymentReconciler struct { // +kubebuilder:rbac:groups=app.github.com,resources=appdeployments,verbs=get;list;watch;create;update;patch;delete // +kubebuilder:rbac:groups=app.github.com,resources=appdeployments/status,verbs=get;update;patch // +kubebuilder:rbac:groups=app.github.com,resources=appdeployments/finalizers,verbs=update +// +kubebuilder:rbac:groups=batch,resources=jobs,verbs=get;list;watch;create;update;patch;delete +// +kubebuilder:rbac:groups=batch,resources=jobs/status,verbs=get;update;patch +// +kubebuilder:rbac:groups=batch,resources=jobs/finalizers,verbs=update // Reconcile is part of the main kubernetes reconciliation loop which aims to // move the current state of the cluster closer to the desired state.