Update Error in Snapshot Status by xing-yang · Pull Request #284 · kubernetes-csi/external-snapshotter

xing-yang · 2020-04-01T04:37:34Z

What type of PR is this?
/kind bug

What this PR does / why we need it:
This PR updates the Error field in the snapshot status based on the status of the content.

Which issue(s) this PR fixes:
Partial fix for #333

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

Updates Error field in the snapshot status based on the status of the content.

k8s-ci-robot · 2020-04-01T04:37:43Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: xing-yang

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~~ [xing-yang]

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

humblec · 2020-05-15T16:07:07Z

pkg/common-controller/snapshot_controller.go

+		}
+		if content.Status.Error.Message != nil {
+			msg := *content.Status.Error.Message
+			volumeSnapshotErr.Message = &msg


@xing-yang Dont we have to record this event once we set it on the VolumeSnapshot object?

We don't automatically record an event when updating the status. There's a separate call that does this when needed.

humblec · 2020-05-15T16:07:48Z

This looks good to me, however, got a question on the event submission.

saikat-royc · 2020-07-15T19:17:34Z

pkg/common-controller/snapshot_controller.go

 		readyToUse = *content.Status.ReadyToUse
 	}
+	var volumeSnapshotErr *crdv1.VolumeSnapshotError
+	if content.Status != nil && content.Status.Error != nil {


To use this change for #333 we need to clear the error in the volume snapshot object, if the content has been created or bound successfully.

volumeSnapshotErr := &crdv1.VolumeSnapshotError{} if content.status != nil && content.Status.Error != nil { // update the error time and message }

Thoughts @xing-yang ?

Yes, makes sense.

hmmm, isn't the following sufficient?
volumeSnapshotErr = content.Status.Error.DeepCopy()
I'd say just do it in the code blocks where assignment really happens and delete this whole block. WDYT?

saikat-royc · 2020-07-15T19:18:27Z

pkg/common-controller/snapshot_controller.go

 			newStatus.RestoreSize = resource.NewQuantity(*size, resource.BinarySI)
 			updated = true
 		}
+		if (newStatus.Error == nil && volumeSnapshotErr != nil) || (newStatus.Error != nil && volumeSnapshotErr != nil && newStatus.Error.Time != nil && volumeSnapshotErr.Time != nil && &newStatus.Error.Time != &volumeSnapshotErr.Time) {


new status should be updated unconditionally? so that we can clear any error in the volume snapshot object?

I am not sure I understand this if statement here: "&newStatus.Error.Time != &volumeSnapshotErr.Time"
and what if newStats.Error != nil but volumeSnapshotErr is nil? in this case, should newStatus.Error be updated to nil as well?

If the Error in both objects are identical, we don't want to make an API update call, but the logic to clear up the error is missing.

yuxiangqian · 2020-07-18T00:18:15Z

pkg/common-controller/snapshot_controller.go

 		readyToUse = *content.Status.ReadyToUse
 	}
+	var volumeSnapshotErr *crdv1.VolumeSnapshotError
+	if content.Status != nil && content.Status.Error != nil {


hmmm, isn't the following sufficient?
volumeSnapshotErr = content.Status.Error.DeepCopy()
I'd say just do it in the code blocks where assignment really happens and delete this whole block. WDYT?

yuxiangqian · 2020-07-18T00:18:58Z

pkg/common-controller/snapshot_controller.go

 		if size != nil {
 			newStatus.RestoreSize = resource.NewQuantity(*size, resource.BinarySI)
 		}
+		if volumeSnapshotErr != nil {


i.e., here could be simply:
newStats.Error = content.Status.Error.DeepCopy()?

yuxiangqian · 2020-07-18T00:24:22Z

pkg/common-controller/snapshot_controller.go

 			newStatus.RestoreSize = resource.NewQuantity(*size, resource.BinarySI)
 			updated = true
 		}
+		if (newStatus.Error == nil && volumeSnapshotErr != nil) || (newStatus.Error != nil && volumeSnapshotErr != nil && newStatus.Error.Time != nil && volumeSnapshotErr.Time != nil && &newStatus.Error.Time != &volumeSnapshotErr.Time) {


I am not sure I understand this if statement here: "&newStatus.Error.Time != &volumeSnapshotErr.Time"
and what if newStats.Error != nil but volumeSnapshotErr is nil? in this case, should newStatus.Error be updated to nil as well?

xing-yang · 2020-07-21T13:57:13Z

Review comments addressed.

saikat-royc · 2020-07-21T21:22:47Z

pkg/common-controller/snapshot_update_test.go

 			expectSuccess:    true,
 			test:             testSyncSnapshot,
 		},
+		{


Thinking of some more test cases:

vs object status == nil, vs content object status error != nil -> error expected in volume snapshot object error status.

vs object status == nil, vs content object status error == nil, no change expected in volume snapshot object error status.

Added two new test cases.
Test case 2 behaves like this: When both statuses are nil, updateSnapshotStatus will create an Status and set boundContentName and set readyToUse to false.

saikat-royc

The changes looks good to me. Added a comment for couple of new test cases.

saikat-royc · 2020-07-22T04:59:56Z

/lgtm

k8s-ci-robot requested review from lpabon and saad-ali April 1, 2020 04:37

k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Apr 1, 2020

humblec reviewed May 15, 2020

View reviewed changes

xing-yang mentioned this pull request Jul 15, 2020

Snapshot controller cannot recover from missing volume snapshot class error #333

Closed

saikat-royc reviewed Jul 15, 2020

View reviewed changes

xing-yang changed the title ~~WIP: Update Error in Snapshot Status~~ Update Error in Snapshot Status Jul 15, 2020

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 Jul 15, 2020

yuxiangqian reviewed Jul 18, 2020

View reviewed changes

xing-yang force-pushed the error_status branch from bf4d549 to 65a3f5f Compare July 18, 2020 19:20

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jul 18, 2020

xing-yang force-pushed the error_status branch 2 times, most recently from f909fcb to 976ec81 Compare July 21, 2020 02:54

saikat-royc reviewed Jul 21, 2020

View reviewed changes

Update Error in Snapshot Status

5f20906

xing-yang force-pushed the error_status branch from 976ec81 to 5f20906 Compare July 22, 2020 04:47

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jul 22, 2020

k8s-ci-robot assigned saikat-royc Jul 22, 2020

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 22, 2020

k8s-ci-robot merged commit 0c5b535 into kubernetes-csi:master Jul 22, 2020

Conversation

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

Uh oh!

Uh oh!

k8s-ci-robot commented Apr 1, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

humblec commented May 15, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xing-yang Jul 15, 2020 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xing-yang commented Jul 21, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

saikat-royc left a comment

Choose a reason for hiding this comment

Uh oh!

saikat-royc commented Jul 22, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

xing-yang commented Apr 1, 2020 •

edited

Loading

xing-yang Jul 15, 2020 •

edited

Loading