Return VolumeSnapshotContent from various functions instead of nil

[ggriffiths]

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

What type of PR is this?
/kind bug

What this PR does / why we need it:
When removeAnnVolumeSnapshotBeingCreated fails, updatedContent is not returned.

Which issue(s) this PR fixes:
Part of #525

Special notes for your reviewer:
This does not completely fix the problem, but it does seem to lower the count.

Does this PR introduce a user-facing change?:

NONE

[ggriffiths]

@jsafrane I have fixed the issue you mentioned with removeAnnVolumeSnapshotBeingCreated. Let me know what you think.

This seems to lower the "object has been modified" count a bit, but it's still not consistently low yet. Either way, I think this is probably heading the right direction.

[jsafrane]

It's a step in the right direction. Every time the sidecar issues Update / UpdateStatus against the API server, it must use the returned objects (with updated ResourceVersion) in all subsequent Update / UpdateStatus calls.

• setAnnVolumeSnapshotBeingCreated must return content it got from the API server
• Whole createSnapshotWrapper should not return nil, but some content.
• When it calls createSnapshotWrapper, it should use the returned object in subsequent updateContentErrorStatusWithEvent and not the original one, i.e. use contentObj instead of content in both these places:

  • external-snapshotter/pkg/sidecar-controller/snapshot_controller.go

    Line 104 in 8751eb0

    ctrl.updateContentErrorStatusWithEvent(content, v1.EventTypeWarning, "SnapshotCreationFailed", fmt.Sprintf("Failed to create snapshot: %v", err))

  • external-snapshotter/pkg/sidecar-controller/snapshot_controller.go

    Line 121 in 8751eb0

    ctrl.updateContentErrorStatusWithEvent(content, v1.EventTypeWarning, "SnapshotContentCheckandUpdateFailed", fmt.Sprintf("Failed to check and update snapshot content: %v", err))

And please check all error paths for returning nil from createSnapshotWrapper / checkandUpdateContentStatusOperation.

[ggriffiths]

Ok, I plan to update this PR soon with the above feedback.

[ggriffiths]

@jsafrane ready for re-review, I've addressed your comments above.

I've also done the following:

• Searched for return nil, and added a content/snapshot object if possible
• Searched for all Update/UpdateStatus calls and made sure we were returning the updated objects.

I still see around the same number "object has been modified" error count in our PR tests, but I think these changes are good to do regardless.

[xing-yang]

@ggriffiths are you saying we don't see the "object has been modified" count getting reduced at all with changes in this PR?

[ggriffiths]

@xing-yang I wrote that comment before the final build ran. Up until then, the error count seemed to not be lowered after each commit.

However, with the most recent commit, it seems to be lowered:

1.18 - 10 occurrences of object has been modified - link
1.19 - 28 occurrences of object has been modified - link
1.20 - 42 occurrences of object has been modified - link
1.21 - 26 occurrences of object has been modified - link

[jsafrane]

This declares a new variable content, shadowing the existing one. return at line 310 below therefore returns the old content.

With following code, I don't see any "the object has been modified":

var removeAnnotationErr error
if content, removeAnnotationErr = ctrl.removeAnnVolumeSnapshotBeingCreated(...)

[ggriffiths]

good catch, let me address this

[ggriffiths]

Addressed this

[jsafrane]

I found one issue in the PR, but the rest looks good to me and it reduces nr. of conflicts, at least in my test case (taking snapshot of a non-existing CSI hostpath volume - the driver constantly returns NotFound).

[jsafrane]

/lgtm
Thanks a lot for this PR!

[ggriffiths]

/assign @xing-yang

[jsafrane]

/approve

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ggriffiths, 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:

• OWNERS [jsafrane]

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