Switch from update to patch for VolumeSnapshot and Content by ggriffiths · Pull Request #480 · kubernetes-csi/external-snapshotter

ggriffiths · 2021-03-10T00:15:50Z

What type of PR is this?
/kind bug

What this PR does / why we need it:
Switches to use patch instead of update

Which issue(s) this PR fixes:
Fixes #368

Special notes for your reviewer:
n/a

Does this PR introduce a user-facing change?:

NONE

k8s-ci-robot · 2021-03-10T00:16:10Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: ggriffiths
To complete the pull request process, please assign lpabon after the PR has been reviewed.
You can assign the PR to them by writing /assign @lpabon in a comment when ready.

The full list of commands accepted by this bot can be found here.

Details

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

ggriffiths · 2021-03-25T21:05:57Z

Discovered that stategicmergepatch is not supported for CRs. Working now on an alternative

go.mod

pkg/common-controller/snapshot_create_test.go

deploy/kubernetes/csi-snapshotter/rbac-csi-snapshotter.yaml

deploy/kubernetes/snapshot-controller/rbac-snapshot-controller.yaml

pkg/utils/patch_test.go

ggriffiths · 2021-04-02T06:27:14Z

Addressed comments, ready for re-review @xing-yang

xing-yang · 2021-04-03T01:23:06Z

pkg/sidecar-controller/snapshot_controller.go

+		contentClone.Status.RestoreSize = nil
 	}
-	newContent, err := ctrl.clientset.SnapshotV1().VolumeSnapshotContents().UpdateStatus(context.TODO(), content, metav1.UpdateOptions{})
+	newContent, err := utils.PatchVolumeSnapshotContent(content, contentClone, ctrl.clientset, "status")


This string "status" is used in multiple places. Can you make it a constant?

pkg/utils/patch_test.go

ggriffiths · 2021-04-05T20:41:38Z

/retest

Comparing snapshot-controller logs with other PRs to see if we're seeing less the object has been modified errors.

Signed-off-by: Grant Griffiths <ggriffiths@purestorage.com>

xing-yang · 2021-04-06T23:56:04Z

pkg/utils/patch.go

 	}

-	return newSnapshot, nil
+	return updatedSnapshot, nil


I thought "newSnapshot" is the latest one returned from API server? Why returning "updatedSnapshot"?

this was a test commit - I was trying to see if this reduced the 'modified object' error count.

Reverting the above change as it did not lower the error count.

Can you take a look of the logs and see if you can tell why we got the "modified object" error? For example, is it because there's another API update happened before the patch?

Asked SIG API machinery per your suggestion to try and find out. I'll also check the logs to see under what scenarios we face this. The error counts are fairly consistent.

ggriffiths · 2021-04-15T19:52:08Z

Still investigating ways to lower the error count. merge patch alone with this implementation does not seem to lower the error count.
/hold

ggriffiths · 2021-04-15T22:22:32Z

Findings so far:

Strategicmergepatch is not supported for CRDs. Moving to mergepatch via json-patch does not lower error count (~250 error count)
Removing addResourceVersion from implementation does not lower error count (error count: 240)
Performing a "Get" before every "Patch" lowered error count to 24 total, but is expensive.

k8s-ci-robot · 2021-04-22T16:28:14Z

@ggriffiths: PR needs rebase.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

ggriffiths · 2021-05-21T06:38:18Z

As this PR did not actually decrease the "object has been modified" error count (see earlier comments), I am closing this for now.

I still plan to investigate how this "object has been modified" problem can be solved. It may involve some controller changes.

k8s-ci-robot requested review from lpabon and xing-yang March 10, 2021 00:16

k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Mar 10, 2021

ggriffiths force-pushed the UsePatchInsteadUpdate_2 branch from b0326c4 to 9d99874 Compare March 11, 2021 01:23

ggriffiths changed the title ~~WIP: Switch from update to patch for VolumeSnapshot and Content~~ Switch from update to patch for VolumeSnapshot and Content Mar 11, 2021

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 11, 2021

ggriffiths force-pushed the UsePatchInsteadUpdate_2 branch from 9d99874 to 1f2c2e7 Compare March 11, 2021 01:41

ggriffiths changed the title ~~Switch from update to patch for VolumeSnapshot and Content~~ WIP: Switch from update to patch for VolumeSnapshot and Content Mar 11, 2021

k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 11, 2021

ggriffiths force-pushed the UsePatchInsteadUpdate_2 branch 5 times, most recently from 7eab1e5 to e4db6cb Compare March 12, 2021 23:01

ggriffiths force-pushed the UsePatchInsteadUpdate_2 branch from e4db6cb to 34d67d2 Compare March 30, 2021 19:28

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Apr 2, 2021

ggriffiths force-pushed the UsePatchInsteadUpdate_2 branch from 462b834 to 85e6ef1 Compare April 2, 2021 00:40

ggriffiths changed the title ~~WIP: Switch from update to patch for VolumeSnapshot and Content~~ Switch from update to patch for VolumeSnapshot and Content Apr 2, 2021

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 2, 2021

ggriffiths commented Apr 2, 2021

View reviewed changes

go.mod Show resolved Hide resolved

ggriffiths commented Apr 2, 2021

View reviewed changes

pkg/common-controller/snapshot_create_test.go Outdated Show resolved Hide resolved

ggriffiths force-pushed the UsePatchInsteadUpdate_2 branch 2 times, most recently from e8d7316 to 70cc311 Compare April 2, 2021 01:19

ggriffiths force-pushed the UsePatchInsteadUpdate_2 branch from 70cc311 to 77923a9 Compare April 2, 2021 01:23

xing-yang reviewed Apr 2, 2021

View reviewed changes

ggriffiths force-pushed the UsePatchInsteadUpdate_2 branch from 77923a9 to 237d909 Compare April 2, 2021 06:25

ggriffiths requested a review from xing-yang April 2, 2021 06:26

ggriffiths force-pushed the UsePatchInsteadUpdate_2 branch from 237d909 to b78eb40 Compare April 2, 2021 18:18

xing-yang reviewed Apr 3, 2021

View reviewed changes

ggriffiths force-pushed the UsePatchInsteadUpdate_2 branch from b78eb40 to afbea95 Compare April 5, 2021 05:58

ggriffiths requested a review from xing-yang April 5, 2021 16:29

ggriffiths force-pushed the UsePatchInsteadUpdate_2 branch from afbea95 to cbf8dca Compare April 5, 2021 19:12

ggriffiths force-pushed the UsePatchInsteadUpdate_2 branch 3 times, most recently from 46aff65 to 55d6555 Compare April 6, 2021 01:45

Switch from update to patch for VolumeSnapshot and Content

acb00fb

Signed-off-by: Grant Griffiths <ggriffiths@purestorage.com>

ggriffiths force-pushed the UsePatchInsteadUpdate_2 branch from 55d6555 to acb00fb Compare April 6, 2021 04:47

xing-yang reviewed Apr 6, 2021

View reviewed changes

ggriffiths force-pushed the UsePatchInsteadUpdate_2 branch from e08f7c7 to acb00fb Compare April 7, 2021 17:29

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 15, 2021

huffmanca mentioned this pull request Apr 21, 2021

Correct snapshotContent error propagation #502

Merged

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 22, 2021

huffmanca mentioned this pull request Apr 29, 2021

UPSTREAM: 502: Bug 1949018: Correct snapshotContent error propagation openshift/csi-external-snapshotter#48

Merged

ggriffiths closed this May 21, 2021

ggriffiths mentioned this pull request May 25, 2021

Use patch instead of update #368

Closed

huffmanca mentioned this pull request May 26, 2021

Retrieve the VolumeSnapshotContent before attempting an update #524

Closed

Conversation

ggriffiths commented Mar 10, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

k8s-ci-robot commented Mar 10, 2021

Uh oh!

ggriffiths commented Mar 25, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ggriffiths commented Apr 2, 2021

Uh oh!

xing-yang Apr 3, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ggriffiths commented Apr 5, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

xing-yang Apr 6, 2021

Choose a reason for hiding this comment

Uh oh!

ggriffiths Apr 7, 2021

Choose a reason for hiding this comment

Uh oh!

ggriffiths Apr 7, 2021

Choose a reason for hiding this comment

Uh oh!

xing-yang Apr 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ggriffiths Apr 8, 2021

Choose a reason for hiding this comment

Uh oh!

ggriffiths commented Apr 15, 2021

Uh oh!

ggriffiths commented Apr 15, 2021

Uh oh!

k8s-ci-robot commented Apr 22, 2021

Uh oh!

ggriffiths commented May 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

ggriffiths commented Mar 10, 2021 •

edited

Loading

ggriffiths commented Apr 5, 2021 •

edited

Loading

xing-yang Apr 8, 2021 •

edited

Loading

ggriffiths commented May 21, 2021 •

edited

Loading