Repository: knative/pkg
Pull request #2403 (2022-01-27 02:44:41)

Do not block NotFound error during patching resources
storage-version-migration-serving crashes if ksvc deleted during
migration.

Please refer to the following error especially
`services.serving.knative.dev \"receiver261\" not found` message.

```
{"severity":"INFO","timestamp":"2022-01-25T14:17:19.840090884Z","caller":"logging/config.go:116","message":"Successfully created the logger."}
{"severity":"INFO","timestamp":"2022-01-25T14:17:19.840350704Z","caller":"logging/config.go:117","message":"Logging level set to: info"}
{"severity":"INFO","timestamp":"2022-01-25T14:17:19.840399959Z","caller":"logging/config.go:79","message":"Fetch GitHub commit ID from kodata failed","error":"\"KO_DATA_PATH\" does not exist or is empty"}
{"severity":"INFO","timestamp":"2022-01-25T14:17:19.854296148Z","logger":"storage-migrator","caller":"migrate/main.go:60","message":"Migrating 4 group resources"}
{"severity":"INFO","timestamp":"2022-01-25T14:17:19.85439718Z","logger":"storage-migrator","caller":"migrate/main.go:63","message":"Migrating group resource services.serving.knative.dev"}
{"severity":"EMERGENCY","timestamp":"2022-01-25T14:17:47.601002153Z","logger":"storage-migrator","caller":"migrate/main.go:65","message":"Failed to migrate: unable to patch resource kc-broker-newsubs-kn-0/receiver261 (gvr: serving.knative.dev/v1, Resource=services) - services.serving.knative.dev \"receiver261\" not found","stacktrace":"main.main\n\t/opt/app-root/src/go/src/knative.dev/serving/vendor/knative.dev/pkg/apiextensions/storageversion/cmd/migrate/main.go:65\nruntime.main\n\t/usr/lib/golang/src/runtime/proc.go:255"}
```

It could be a rare case but it happens when an user deletes their
resource during storage-version-migration-serving is running.

This patch fixes the issue by ignoring the NotFound error during
patching resources.

Thank you so much @maschmid for reporting the issue.

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

This pull-request has been approved by: *<a href="https://github.com/knative/pkg/pull/2403#" title="Author self-approved">nak3</a>*
To complete the [pull request process](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process), please assign **dprotaso** after the PR has been reviewed.
You can assign the PR to them by writing `/assign @dprotaso` 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=knative%2Fpkg).

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

- **[apiextensions/OWNERS](https://github.com/knative/pkg/blob/main/apiextensions/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":["dprotaso"]} -->
codecov[bot]: # [Codecov](https://codecov.io/gh/knative/pkg/pull/2403?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative) Report
> Merging [#2403](https://codecov.io/gh/knative/pkg/pull/2403?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative) (4c00829) into [main](https://codecov.io/gh/knative/pkg/commit/d7b329c36019326ccb19a90f0573db16b83b822e?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative) (d7b329c) will **decrease** coverage by `0.04%`.
> The diff coverage is `100.00%`.

[![Impacted file tree graph](https://codecov.io/gh/knative/pkg/pull/2403/graphs/tree.svg?width=650&height=150&src=pr&token=rlqpDnfxbO&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative)](https://codecov.io/gh/knative/pkg/pull/2403?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative)

```diff
@@            Coverage Diff             @@
##             main    #2403      +/-   ##
==========================================
- Coverage   63.53%   63.49%   -0.05%     
==========================================
  Files         226      226              
  Lines        9886     9886              
==========================================
- Hits         6281     6277       -4     
- Misses       3320     3322       +2     
- Partials      285      287       +2     
```


| [Impacted Files](https://codecov.io/gh/knative/pkg/pull/2403?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative) | Coverage Δ | |
|---|---|---|
| [apiextensions/storageversion/migrator.go](https://codecov.io/gh/knative/pkg/pull/2403/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative#diff-YXBpZXh0ZW5zaW9ucy9zdG9yYWdldmVyc2lvbi9taWdyYXRvci5nbw==) | `94.44% <100.00%> (ø)` | |
| [test/gcs/mock/mock.go](https://codecov.io/gh/knative/pkg/pull/2403/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative#diff-dGVzdC9nY3MvbW9jay9tb2NrLmdv) | `91.39% <0.00%> (-2.16%)` | :arrow_down: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/knative/pkg/pull/2403?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/knative/pkg/pull/2403?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative). Last update [d7b329c...4c00829](https://codecov.io/gh/knative/pkg/pull/2403?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative).

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

This pull-request has been approved by: *<a href="https://github.com/knative/pkg/pull/2403#" title="Author self-approved">nak3</a>*, *<a href="https://github.com/knative/pkg/pull/2403#pullrequestreview-864411764" title="LGTM">pierDipi</a>*
To complete the [pull request process](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process), please assign **dprotaso** after the PR has been reviewed.
You can assign the PR to them by writing `/assign @dprotaso` 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=knative%2Fpkg).

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

- **[apiextensions/OWNERS](https://github.com/knative/pkg/blob/main/apiextensions/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":["dprotaso"]} -->
dprotaso: /approve
knative-prow-robot: [APPROVALNOTIFIER] This PR is **APPROVED**

This pull-request has been approved by: *<a href="https://github.com/knative/pkg/pull/2403#issuecomment-1024302530" title="Approved">dprotaso</a>*, *<a href="https://github.com/knative/pkg/pull/2403#" title="Author self-approved">nak3</a>*, *<a href="https://github.com/knative/pkg/pull/2403#pullrequestreview-864411764" title="LGTM">pierDipi</a>*

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

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:

- ~~[apiextensions/OWNERS](https://github.com/knative/pkg/blob/main/apiextensions/OWNERS)~~ [dprotaso]

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":[]} -->

Patch:
diff --git a/apiextensions/storageversion/migrator.go b/apiextensions/storageversion/migrator.go
index 2d1cb47ac..7ac1cc932 100644
--- a/apiextensions/storageversion/migrator.go
+++ b/apiextensions/storageversion/migrator.go
@@ -22,6 +22,7 @@ import (
 
 	apix "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1"
 	apixclient "k8s.io/apiextensions-apiserver/pkg/client/clientset/clientset"
+	apierrs "k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/runtime/schema"
@@ -93,7 +94,7 @@ func (m *Migrator) migrateResources(ctx context.Context, gvr schema.GroupVersion
 		_, err := client.Namespace(item.GetNamespace()).
 			Patch(ctx, item.GetName(), types.MergePatchType, []byte("{}"), metav1.PatchOptions{})
 
-		if err != nil {
+		if err != nil && !apierrs.IsNotFound(err) {
 			return fmt.Errorf("unable to patch resource %s/%s (gvr: %s) - %w",
 				item.GetNamespace(), item.GetName(),
 				gvr, err)


Test patch:
diff --git a/apiextensions/storageversion/migrator_test.go b/apiextensions/storageversion/migrator_test.go
index b779995f5..8129d456c 100644
--- a/apiextensions/storageversion/migrator_test.go
+++ b/apiextensions/storageversion/migrator_test.go
@@ -24,6 +24,7 @@ import (
 	"github.com/google/go-cmp/cmp"
 	apix "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1"
 	apixFake "k8s.io/apiextensions-apiserver/pkg/client/clientset/clientset/fake"
+	apierrs "k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
 	runtime "k8s.io/apimachinery/pkg/runtime"
@@ -97,6 +98,7 @@ func TestMigrate_Errors(t *testing.T) {
 		name string
 		crd  func(*k8stesting.Fake)
 		dyn  func(*k8stesting.Fake)
+		pass bool
 	}{{
 		name: "failed to fetch CRD",
 		crd: func(fake *k8stesting.Fake) {
@@ -129,6 +131,16 @@ func TestMigrate_Errors(t *testing.T) {
 					return true, nil, errors.New("failed to patch definition")
 				})
 		},
+	}, {
+		name: "patching unexisting resource",
+		dyn: func(fake *k8stesting.Fake) {
+			fake.PrependReactor("patch", "*",
+				func(k8stesting.Action) (bool, runtime.Object, error) {
+					return true, nil, apierrs.NewNotFound(fakeGR, "resource-removed")
+				})
+		},
+		// Resouce not found error should not block the storage migration.
+		pass: true,
 	},
 	// todo paging fails
 	}
@@ -148,7 +160,7 @@ func TestMigrate_Errors(t *testing.T) {
 			}
 
 			m := NewMigrator(dclient, cclient)
-			if err := m.Migrate(context.Background(), fakeGR); err == nil {
+			if err := m.Migrate(context.Background(), fakeGR); test.pass != (err == nil) {
 				t.Error("Migrate should have returned an error")
 			}
 		})