Repository: tektoncd/operator
Pull request #339 (2021-06-30 12:37:01)

Fix EventListener instance deletion
Separate deletion of deployments from the deletion of all other resources.
So that delete call to CRDs are executed first before calling delete on controllers.
This will help to create a window before controller deletion, where controller can
handle finalizers on CRDs and CRD instances.

This is not the perfect fix, but this is good enough.

Ideal fix will be to add finalizer on controllers and remove them after making sure
all other resources have been removed. But this could create other issues such as the
CRDs watched by controllers being deleted can cause the controllers t

Signed-off-by: Nikhil Thomas <nikthoma@redhat.com>
(cherry picked from commit 962c5bfc371b19d5b7fe8e401e287890b16710e2)

# Changes

<!-- 🎉🎉🎉 Thank you for the PR!!! 🎉🎉🎉 -->

<!-- Describe your changes here- ideally you can get that description straight from
your descriptive commit message(s)! -->

# Submitter Checklist

These are the criteria that every PR should meet, please check them off as you
review them:

- [ ] Includes [tests](https://github.com/tektoncd/community/blob/master/standards.md#principles) (if functionality changed/added)
- [ ] Includes [docs](https://github.com/tektoncd/community/blob/master/standards.md#principles) (if user facing)
- [ ] Commit messages follow [commit message best practices](https://github.com/tektoncd/community/blob/master/standards.md#commit-messages)

_See [the contribution guide](https://github.com/tektoncd/operator/blob/master/CONTRIBUTING.md) for more details._

# Release Notes

<!--
Describe any user facing changes here, or delete this block.

Examples of user facing changes:
- API changes
- Bug fixes
- Any changes in behavior
- Changes requiring upgrade notices or deprecation warnings

For pull requests with a release note:

```release-note
Your release note here
```

For pull requests that require additional action from users switching to the new release, include the string "action required" (case insensitive) in the release note:

```release-note
action required: your release note here
```

For pull requests that don't need to be mentioned at release time, use the `/release-note-none` Prow command to add the `release-note-none` label to the PR. You can also write the string "NONE" as a release note in your PR description:

```release-note
NONE
```
-->


tekton-robot: [APPROVALNOTIFIER] This PR is **NOT APPROVED**

This pull-request has been approved by:
To complete the [pull request process](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process), please assign **nikhil-thomas**
You can assign the PR to them by writing `/assign @nikhil-thomas` in a comment when ready.

The full list of commands accepted by this bot can be found [here](https://go.k8s.io/bot-commands?repo=tektoncd%2Foperator).

<details open>
Needs approval from an approver in each of these files:

- **[OWNERS](https://github.com/tektoncd/operator/blob/main/OWNERS)**

Approvers can indicate their approval by writing `/approve` in a comment
Approvers can cancel approval by writing `/approve cancel` in a comment
</details>
<!-- META={"approvers":["nikhil-thomas"]} -->
tekton-robot: The following is the coverage report on the affected files.
Say `/test pull-tekton-operator-go-coverage` to re-run this coverage report

File | Old Coverage | New Coverage | Delta
---- |:------------:|:------------:|:-----:
[pkg/reconciler/common/install.go](https://storage.cloud.google.com/tekton-prow/pr-logs/pull/tektoncd_operator/339/pull-tekton-operator-go-coverage/1410215812555345920/artifacts/line-cov.html#file0) | 65.5% | 64.5% | -1.0

tekton-robot: @nikhil-thomas: The following test **failed**, say `/retest` to rerun them all:

Test name | Commit | Details | Rerun command
--- | --- | --- | ---
pull-tekton-operator-unit-tests | 2f876a54c90d414b06c932e17af1c4acd36c08b2 | [link](https://tekton-releases.appspot.com/build/tekton-prow/pr-logs/pull/tektoncd_operator/339/pull-tekton-operator-unit-tests/1410215812542763008/) | `/test pull-tekton-operator-unit-tests`

[Full PR test history](https://tekton-releases.appspot.com/pr/tektoncd_operator/339). [Your PR dashboard](https://prow.tekton.dev/pr/nikhil-thomas).

<details>

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md).  If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository. I understand the commands that are listed [here](https://go.k8s.io/bot-commands).
</details>
<!-- test report -->
tekton-robot: @nikhil-thomas: The following test **failed**, say `/retest` to rerun them all:

Test name | Commit | Details | Rerun command
--- | --- | --- | ---
pull-tekton-operator-unit-tests | 2f876a54c90d414b06c932e17af1c4acd36c08b2 | [link](https://tekton-releases.appspot.com/build/tekton-prow/pr-logs/pull/tektoncd_operator/339/pull-tekton-operator-unit-tests/1410215812542763008/) | `/test pull-tekton-operator-unit-tests`

[Full PR test history](https://tekton-releases.appspot.com/pr/tektoncd_operator/339). [Your PR dashboard](https://prow.tekton.dev/pr/nikhil-thomas).

<details>

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md).  If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository. I understand the commands that are listed [here](https://go.k8s.io/bot-commands).
</details>
<!-- test report -->
vdemeester: /approve
tekton-robot: [APPROVALNOTIFIER] This PR is **APPROVED**

This pull-request has been approved by: *<a href="https://github.com/tektoncd/operator/pull/339#issuecomment-871415810" title="Approved">vdemeester</a>*

The full list of commands accepted by this bot can be found [here](https://go.k8s.io/bot-commands?repo=tektoncd%2Foperator).

The pull request process is described [here](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process)

<details >
Needs approval from an approver in each of these files:

- ~~[OWNERS](https://github.com/tektoncd/operator/blob/main/OWNERS)~~ [vdemeester]

Approvers can indicate their approval by writing `/approve` in a comment
Approvers can cancel approval by writing `/approve cancel` in a comment
</details>
<!-- META={"approvers":[]} -->
tekton-robot: The following is the coverage report on the affected files.
Say `/test pull-tekton-operator-go-coverage` to re-run this coverage report

File | Old Coverage | New Coverage | Delta
---- |:------------:|:------------:|:-----:
[pkg/reconciler/common/install.go](https://storage.cloud.google.com/tekton-prow/pr-logs/pull/tektoncd_operator/339/pull-tekton-operator-go-coverage/1410887111313199104/artifacts/line-cov.html#file0) | 65.5% | 64.5% | -1.0

tekton-robot: @nikhil-thomas: PR needs rebase.

<details>

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md).  If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.
</details>
tekton-robot: The following is the coverage report on the affected files.
Say `/test pull-tekton-operator-go-coverage` to re-run this coverage report

File | Old Coverage | New Coverage | Delta
---- |:------------:|:------------:|:-----:
[pkg/reconciler/common/install.go](https://storage.cloud.google.com/tekton-prow/pr-logs/pull/tektoncd_operator/339/pull-tekton-operator-go-coverage/1418509165885657091/artifacts/line-cov.html#file0) | 62.5% | 61.8% | -0.7

sm43: /lgtm
/meow

tekton-robot: @sm43: ![cat image](https://cdn2.thecatapi.com/images/bt.jpg)

<details>

In response to [this](https://github.com/tektoncd/operator/pull/339#issuecomment-885535470):

>/lgtm
>/meow
>


Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md).  If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.
</details>

Patch:
diff --git a/pkg/reconciler/common/install.go b/pkg/reconciler/common/install.go
index 4e901a590..3ab956f49 100644
--- a/pkg/reconciler/common/install.go
+++ b/pkg/reconciler/common/install.go
@@ -32,6 +32,7 @@ var (
 	consoleCLIDownload    mf.Predicate = mf.Any(mf.ByKind("ConsoleCLIDownload"))
 	clusterTriggerBinding mf.Predicate = mf.Any(mf.ByKind("ClusterTriggerBinding"))
 	persistentVolumeClaim mf.Predicate = mf.Any(mf.ByKind("PersistentVolumeClaim"))
+	deployment            mf.Predicate = mf.Any(mf.ByKind("Deployment"))
 )
 
 // Install applies the manifest resources for the given version and updates the given
@@ -78,9 +79,15 @@ func Install(ctx context.Context, manifest *mf.Manifest, instance v1alpha1.Tekto
 
 // Uninstall removes all resources
 func Uninstall(ctx context.Context, manifest *mf.Manifest, instance v1alpha1.TektonComponent) error {
-	if err := manifest.Filter(mf.Not(mf.Any(role, rolebinding))).Delete(); err != nil {
+	if err := manifest.Filter(mf.Not(mf.Any(role, rolebinding, deployment))).Delete(); err != nil {
 		return fmt.Errorf("failed to remove non-crd/non-rbac resources: %w", err)
 	}
+	// delete deployment separately (after delete call to CRDs)
+	// to improve the chance of the finalizers to be handled before the controllers are deleted
+	// ref: https://github.com/tektoncd/triggers/issues/775#issuecomment-700271144
+	if err := manifest.Filter(mf.Any(deployment)).Delete(); err != nil {
+		return fmt.Errorf("failed to remove deployments: %w", err)
+	}
 	// Delete Roles last, as they may be useful for human operators to clean up.
 	if err := manifest.Filter(mf.Any(role, rolebinding)).Delete(); err != nil {
 		return fmt.Errorf("failed to remove rbac: %w", err)


Test patch:
diff --git a/pkg/reconciler/common/install_test.go b/pkg/reconciler/common/install_test.go
index b57be27e8..2430dab43 100644
--- a/pkg/reconciler/common/install_test.go
+++ b/pkg/reconciler/common/install_test.go
@@ -148,7 +148,7 @@ func TestUninstall(t *testing.T) {
 	// Deliberately mixing the order in the manifest.
 	in := []unstructured.Unstructured{crd, deployment, role, roleBinding, clusterRole, clusterRoleBinding}
 	// Expect things to be deleted, non-rbac resources first and then in reversed order.
-	want := []unstructured.Unstructured{deployment, crd, clusterRoleBinding, clusterRole, roleBinding, role}
+	want := []unstructured.Unstructured{crd, deployment, clusterRoleBinding, clusterRole, roleBinding, role}
 
 	client := &fakeClient{resourcesExist: true}
 	manifest, err := mf.ManifestFrom(mf.Slice(in), mf.UseClient(client))