Skip to content

Use patch to replace update#149

Merged
k8s-ci-robot merged 1 commit intokubernetes-csi:masterfrom
cwdsuzhou:json_patch
May 17, 2019
Merged

Use patch to replace update#149
k8s-ci-robot merged 1 commit intokubernetes-csi:masterfrom
cwdsuzhou:json_patch

Conversation

@cwdsuzhou
Copy link
Copy Markdown
Contributor

@cwdsuzhou cwdsuzhou commented May 10, 2019 •
edited by jsafrane
Loading

What type of PR is this?

Uncomment only one /kind <> line, hit enter to put that in a new line, and remove leading whitespaces from that line:

/kind feature

What this PR does / why we need it:

Use patch to replace update

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

NONE

@k8s-ci-robot k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. release-note-none Denotes a PR that doesn't merit a release note. kind/feature Categorizes issue or PR as related to a new feature. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels May 10, 2019
@k8s-ci-robot k8s-ci-robot requested review from saad-ali and sbezverk May 10, 2019 15:36
@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels May 11, 2019
@cwdsuzhou cwdsuzhou force-pushed the json_patch branch 2 times, most recently from 918822a to cf46a7c Compare May 13, 2019 02:48
@cwdsuzhou
Copy link
Copy Markdown
Contributor Author

cwdsuzhou commented May 13, 2019

/assign @msau42 @jsafrane

@cwdsuzhou cwdsuzhou changed the title [WIP] Use patch to replace update Use patch to replace update May 13, 2019
@k8s-ci-robot 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 May 13, 2019
@cwdsuzhou cwdsuzhou force-pushed the json_patch branch 2 times, most recently from 4a2cbb6 to e5f7f00 Compare May 13, 2019 08:14
jsafrane
jsafrane reviewed May 15, 2019
View reviewed changes
Copy link
Copy Markdown
Contributor

@jsafrane jsafrane left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work!

Comment thread pkg/controller/csi_handler.go Outdated
va, finalizerAdded := h.prepareVAFinalizer(va)
va, nodeIDAdded := h.prepareVANodeID(va, nodeID)

patch, err := createMergePatch(originalVA, va)
Copy link
Copy Markdown
Contributor

@jsafrane jsafrane May 15, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

move this under if finalizerAdded || nodeIDAdded so we don't create useless patches.

Comment thread pkg/controller/csi_handler.go Outdated
}
newVa, err := h.client.StorageV1beta1().VolumeAttachments().Update(clone)

patch, err := createMergePatch(va, clone)
Copy link
Copy Markdown
Contributor

@jsafrane jsafrane May 15, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be possible to move createMergePatch() && h.client.StorageV1beta1().VolumeAttachments().Patch() into a separate function so it's not repeated all over the place? Existing saveVA() could be a good candidate (just move klog.V(4) from there to the caller).

Comment thread pkg/controller/csi_handler_test.go
// Finalizer is saved first
core.NewUpdateAction(vaGroupResourceVersion, metav1.NamespaceNone, va(false /*attached*/, fin, ann)),
core.NewUpdateAction(vaGroupResourceVersion, metav1.NamespaceNone, va(true /*attached*/, fin, ann)),
core.NewPatchAction(vaGroupResourceVersion, metav1.NamespaceNone, testPVName+"-"+testNodeName,
Copy link
Copy Markdown
Contributor

@jsafrane jsafrane May 15, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wish there was a way to pass only the new VA - the code that compares the expected actions with reality could get old VA from reactor, get patch from API call and compute new VA. I am not sure how hard would it be and if it's even feasible. Would you please check this possibility? Don't spend too much time on it, this PR is mostly OK even without it.

Copy link
Copy Markdown
Contributor Author

@cwdsuzhou cwdsuzhou May 15, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, I review the codes, this may be possible. Since I do not have too much time, I would like to send anther PR to do this later. Do you think it is ok?

Other two places you advised have changed.

@k8s-ci-robot 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 May 15, 2019
@jsafrane
Copy link
Copy Markdown
Contributor

jsafrane commented May 17, 2019

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 17, 2019
@k8s-ci-robot
Copy link
Copy Markdown
Contributor

k8s-ci-robot commented May 17, 2019

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cwdsuzhou, jsafrane

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

The pull request process is described here

Details Needs approval from an approver in each of these files:

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

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 17, 2019
@k8s-ci-robot k8s-ci-robot merged commit 3cb3d2f into kubernetes-csi:master May 17, 2019
@jsafrane jsafrane mentioned this pull request May 27, 2019
Closed
@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. release-note-action-required Denotes a PR that introduces potentially breaking changes that require user action. and removed release-note-none Denotes a PR that doesn't merit a release note. release-note Denotes a PR that will be considered when it comes time to generate release notes. labels Jun 13, 2019
This was referenced Jun 13, 2019
Merged
Merged
@k8s-ci-robot k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. and removed release-note-action-required Denotes a PR that introduces potentially breaking changes that require user action. labels Jun 18, 2019
@jsafrane jsafrane mentioned this pull request Aug 16, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jsafrane jsafrane jsafrane left review comments

@saad-ali saad-ali Awaiting requested review from saad-ali

@sbezverk sbezverk Awaiting requested review from sbezverk

Assignees

@jsafrane jsafrane

@msau42 msau42

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/feature Categorizes issue or PR as related to a new feature. lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note-none Denotes a PR that doesn't merit a release note. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@cwdsuzhou @jsafrane @k8s-ci-robot @msau42