Fix ansible-operator finalizer concurrency issue (#5678)

* Fix ansible-operator finalizer concurrency issue

For ansible-based operators, this change fixes an issue that caused
finalizers to fail to run if the watched resource (CR) is deleted during
reconciliation.

Fixes #4909

Signed-off-by: Austin Macdonald <[email protected]>

* dont change constants

Signed-off-by: Austin Macdonald <[email protected]>

* change name so we dont have a half-made test

Signed-off-by: Austin Macdonald <[email protected]>

* PR cleanup

Signed-off-by: Austin Macdonald <[email protected]>

* rm extra ---

Signed-off-by: Austin Macdonald <[email protected]>

Author: asmacdo

Makefile

@@ -156,10 +156,16 @@ e2e_targets := test-e2e $(e2e_tests)
 export KIND_CLUSTER := osdk-test
 
 KUBEBUILDER_ASSETS = $(PWD)/$(shell go install sigs.k8s.io/controller-runtime/tools/setup-envtest@latest && $(shell go env GOPATH)/bin/setup-envtest use $(ENVTEST_K8S_VERSION) --bin-dir tools/bin/ -p path)
-test-e2e-setup: build
+test-e2e-setup:: build dev-install cluster-create
+
+.PHONY: cluster-create
+cluster-create::
+	[[ "`$(TOOLS_DIR)/kind get clusters`" =~ "$(KIND_CLUSTER)" ]] || $(TOOLS_DIR)/kind create cluster --image="kindest/node:v$(ENVTEST_K8S_VERSION)" --name $(KIND_CLUSTER)
+
+.PHONY: dev-install
+dev-install::
 	$(SCRIPTS_DIR)/fetch kind 0.11.0
 	$(SCRIPTS_DIR)/fetch kubectl $(ENVTEST_K8S_VERSION) # Install kubectl AFTER envtest because envtest includes its own kubectl binary
-	[[ "`$(TOOLS_DIR)/kind get clusters`" =~ "$(KIND_CLUSTER)" ]] || $(TOOLS_DIR)/kind create cluster --image="kindest/node:v$(ENVTEST_K8S_VERSION)" --name $(KIND_CLUSTER)
 
 .PHONY: test-e2e-teardown
 test-e2e-teardown:
@@ -177,7 +183,7 @@ test-e2e-go:: image/custom-scorecard-tests ## Run Go e2e tests
 test-e2e-ansible:: image/ansible-operator ## Run Ansible e2e tests
 	go test -count=1 ./internal/ansible/proxy/...
 	go test ./test/e2e/ansible -v -ginkgo.v
-test-e2e-ansible-molecule:: image/ansible-operator ## Run molecule-based Ansible e2e tests
+test-e2e-ansible-molecule:: install dev-install image/ansible-operator ## Run molecule-based Ansible e2e tests
 	go run ./hack/generate/samples/molecule/generate.go
 	./hack/tests/e2e-ansible-molecule.sh
 test-e2e-helm:: image/helm-operator ## Run Helm e2e tests

changelog/fragments/finalizerconcurrency.yaml

@@ -0,0 +1,11 @@
+# entries is a list of entries to include in
+# release notes and/or the migration guide
+entries:
+  - description: >
+      For ansible-based operators, this change fixes an issue that caused
+      finalizers to fail to run if the watched resource (CR) is deleted during
+      reconciliation.
+    kind: "bugfix"
+
+    # Is this a breaking change?
+    breaking: false

hack/generate/samples/internal/ansible/advanced_molecule.go

@@ -235,6 +235,12 @@ func (ma *AdvancedMolecule) addMocksFromTestdata() {
 	cmd = exec.Command("cp", "-r", "../../../hack/generate/samples/internal/ansible/testdata/inventory/", filepath.Join(ma.ctx.Dir, "inventory/"))
 	_, err = ma.ctx.Run(cmd)
 	pkg.CheckError("adding inventory/", err)
+
+	log.Infof("adding finalizer for finalizerconcurrencytest")
+	cmd = exec.Command("cp", "../../../hack/generate/samples/internal/ansible/testdata/playbooks/finalizerconcurrencyfinalizer.yml", filepath.Join(ma.ctx.Dir, "playbooks/finalizerconcurrencyfinalizer.yml"))
+	_, err = ma.ctx.Run(cmd)
+	pkg.CheckError("adding finalizer for finalizerconccurencytest", err)
+
 }
 
 func (ma *AdvancedMolecule) updateDockerfile() {
@@ -456,8 +462,6 @@ func (ma *AdvancedMolecule) updatePlaybooks() {
         command: '{{ exec_command }}'
       register: exec_result
 
-    - debug: var=exec_result
-
     - name: Get logs from busybox pod
       k8s_log:
         name: '{{ meta.name }}-busybox'
@@ -516,6 +520,36 @@ func (ma *AdvancedMolecule) updatePlaybooks() {
 		clusterAnnotationTest)
 	pkg.CheckError("adding playbook for clusterannotationtest", err)
 
+	log.Infof("adding playbook for finalizerconcurrencytest")
+	const finalizerConcurrencyTest = `---
+- hosts: localhost
+  gather_facts: no
+  collections:
+    - kubernetes.core
+    - operator_sdk.util
+
+  tasks:
+    - debug:
+        msg: "Pausing until configmap exists"
+
+    - name: Wait for configmap
+      k8s_info:
+        apiVersion: v1
+        kind: ConfigMap
+        name: unpause-reconciliation
+        namespace: osdk-test
+      wait: yes
+      wait_sleep: 10
+      wait_timeout: 360
+
+    - debug:
+        msg: "Unpause!"
+`
+	err = kbutil.ReplaceInFile(
+		filepath.Join(ma.ctx.Dir, "playbooks", "finalizerconcurrencytest.yml"),
+		originalPlaybookFragment,
+		finalizerConcurrencyTest)
+	pkg.CheckError("adding playbook for finalizerconcurrencytest", err)
 }
 
 func (ma *AdvancedMolecule) addPlaybooks() {
@@ -524,6 +558,7 @@ func (ma *AdvancedMolecule) addPlaybooks() {
 		"CaseTest",
 		"CollectionTest",
 		"ClusterAnnotationTest",
+		"FinalizerConcurrencyTest",
 		"ReconciliationTest",
 		"SelectorTest",
 		"SubresourcesTest",

hack/generate/samples/internal/ansible/constants.go

@@ -418,7 +418,6 @@ const targetMoleculeCheckDeployment = `- name: Wait 2 minutes for memcached depl
     )}}'`
 
 const molecuTaskToCheckConfigMap = `
-
 - name: Create ConfigMap that the Operator should delete
   k8s:
     definition:

hack/generate/samples/internal/ansible/testdata/playbooks/finalizerconcurrencyfinalizer.yml

@@ -0,0 +1,34 @@
+---
+- hosts: localhost
+  gather_facts: no
+  collections:
+    - kubernetes.core
+    - operator_sdk.util
+
+  tasks:
+    - debug:
+        msg: "Pausing until configmap exists"
+
+    - name: Wait for configmap
+      k8s_info:
+        api_version: v1
+        kind: ConfigMap
+        name: finalizer-concurrency-results
+        namespace: osdk-test
+        wait: yes
+        wait_sleep: 10
+        wait_timeout: 30
+
+    - name: Update configmap
+      k8s:
+        state: present
+        force: yes
+        definition:
+          apiVersion: v1
+          kind: ConfigMap
+          metadata:
+            name: finalizer-concurrency-results
+            namespace: osdk-test
+          data:
+            finalizer: "success"
+        wait: yes

hack/generate/samples/internal/ansible/testdata/tasks/finalizerconcurrencytest_test.yml

@@ -0,0 +1,58 @@
+---
+# TODO(asmacdo) this should be the only task. the other is getting magiced in
+- name: Create the test.example.com/v1alpha1.FinalizerConcurrencyTest
+  k8s:
+    state: present
+    definition:
+      apiVersion: test.example.com/v1alpha1
+      kind: FinalizerConcurrencyTest
+      metadata:
+        name: finalizer-concurrency-test
+        namespace: '{{ namespace }}'
+    wait: no
+
+- name: While reconcile is paused, delete the CR
+  k8s:
+    state: absent
+    definition:
+      apiVersion: test.example.com/v1alpha1
+      kind: FinalizerConcurrencyTest
+      metadata:
+        name: finalizer-concurrency-test
+        namespace: '{{ namespace }}'
+    wait: no
+
+- name: Create a configmap to allow reconciliation to unpause
+  k8s:
+    state: present
+    definition:
+      apiVersion: v1
+      kind: ConfigMap
+      metadata:
+        name: finalizer-concurrency-results
+        namespace: osdk-test
+    wait: no
+
+- name: Wait for the custom resource to be deleted
+  k8s_info:
+    api_version: test.example.com/v1alpha1
+    kind: FinalizerConcurrencyTest
+    namespace: osdk-test # TODO(asmacdo) Fixme
+    name: finalizer-concurrency-test
+  register: cr
+  retries: 10
+  delay: 6
+  until: not cr.resources
+  failed_when: cr.resources
+
+- name: Retrive the cm
+  k8s_info:
+    api_version: v1
+    kind: ConfigMap
+    name: finalizer-concurrency-results
+    namespace: osdk-test
+  register: finalizer_test
+
+- name: Assert that finalizer ran
+  assert:
+    that: finalizer_test.resources.0.data.finalizer== 'success'

hack/generate/samples/internal/ansible/testdata/tasks/subresourcestest_test.yml

@@ -18,8 +18,6 @@
       status: "True"
   register: subresources_test
 
-- debug: var=subresources_test
-
 - name: Assert stdout and stderr are properly set in status
   assert:
     that:

hack/generate/samples/internal/ansible/testdata/watches.yaml

@@ -71,4 +71,14 @@
   watchClusterScopedResources: true
   vars:
     meta: '{{ ansible_operator_meta }}'
+
+- version: v1alpha1
+  group: test.example.com
+  kind: FinalizerConcurrencyTest
+  playbook: playbooks/finalizerconcurrencytest.yml
+  finalizer:
+    name: test.example.com/finalizer
+    playbook: playbooks/finalizerconcurrencyfinalizer.yml
+  vars:
+    meta: '{{ ansible_operator_meta }}'
 #+kubebuilder:scaffold:watch

internal/ansible/controller/reconcile.go

@@ -243,15 +243,19 @@ func (r *AnsibleOperatorReconciler) Reconcile(ctx context.Context, request recon
 	// and do it at the end
 	runSuccessful := len(failureMessages) == 0
 
+	recentlyDeleted := u.GetDeletionTimestamp() != nil
+
 	// The finalizer has run successfully, time to remove it
-	deleted = u.GetDeletionTimestamp() != nil
 	if deleted && finalizerExists && runSuccessful {
 		controllerutil.RemoveFinalizer(u, finalizer)
 		err := r.Client.Update(ctx, u)
 		if err != nil {
 			logger.Error(err, "Failed to remove finalizer")
 			return reconcileResult, err
 		}
+	} else if recentlyDeleted && finalizerExists {
+		// If the CR was deleted after the reconcile began, we need to requeue for the finalizer.
+		reconcileResult.Requeue = true
 	}
 	if r.ManageStatus {
 		errmark := r.markDone(ctx, request.NamespacedName, u, statusEvent, failureMessages)