Fix error handling when rolling back volume creation #617

brunotm · 2016-10-08T05:52:34Z

Fixes #616

govint · 2016-10-08T13:36:57Z

vmdk_plugin/plugin.go

+		errRm := d.ops.Remove(r.Name, nil)
+		if errRm != nil {
+			log.WithFields(log.Fields{"name": r.Name, "error": errRm}).Error("Remove volume failed ")
+			return volume.Response{Err: errRm.Error()}


Its preferable that the error returned on a create error is the create error itself vs. a remove error (which is confusing to the user, when they issued a create command). OTOH, how did this cause the empty response from docker to the docker command line. Logging the error is fine but the original error should be returned.

Its preferable that the error returned on a create error is the create error itself vs. a remove error (which is confusing to the user, when they issued a create command).

Agree.

OTOH, how did this cause the empty response from docker to the docker command line.

err was being reassigned to nil inside the rollback block after the attach error, and at the end we would return the volume response with an error nil since we didn't got any errors from the rollback operations. Which if we had, in any case we would return from there.

Tests in plugin.go:

line 248 //dev, err := d.ops.Attach(r.Name, nil)
line 249 err = fmt.Errorf("STUB")
line 250 dev := []byte("an")

Without the patch:
root@vsc01:/tmp/docker-volume-vsphere# docker volume create -d vmdk --name vol$$ -o size=1gb
Error response from daemon: create vol25407: Post http://%2Frun%2Fdocker%2Fplugins%2Fvmdk.sock/VolumeDriver.Create: http: ContentLength=78 with Body length 0

in log:
2016-10-08 21:32:38.880136128 +0100 WEST [INFO] Attaching volume and creating filesystem fstype=ext4 name=vol25407
2016-10-08 21:32:38.880268068 +0100 WEST [ERROR] Attach volume failed, removing the volume error=STUB name=vol25407

With the patch:
root@vsc01:/tmp/docker-volume-vsphere# docker volume create -d vmdk --name vol$$ -o size=1gb
Error response from daemon: create vol25407: VolumeDriver.Create: STUB

in log:
2016-10-08 21:37:47.746007178 +0100 WEST [INFO] Attaching volume and creating filesystem name=vol25407 fstype=ext4
2016-10-08 21:37:47.746070732 +0100 WEST [ERROR] Attach volume failed, removing the volume name=vol25407 error=STUB

brunotm · 2016-10-09T12:42:24Z

@govint
I have updated the commit to address your comment about returning from the main error.

govint

Looks good. Thanks for fixing this.

govint · 2016-10-08T13:40:24Z

vmdk_plugin/plugin.go

+		errRm := d.ops.Remove(r.Name, nil)
+		if errRm != nil {
+			log.WithFields(log.Fields{"name": r.Name, "error": errRm}).Error("Remove volume failed ")
+			return volume.Response{Err: errRm.Error()}


Same comment as above.

msterin

Looks good but the err* names should be consistent IMO.

msterin · 2016-10-12T01:08:02Z

vmdk_plugin/plugin.go

-			return volume.Response{Err: err.Error()}
+		errRm := d.ops.Remove(r.Name, nil)
+		if errRm != nil {
+			log.WithFields(log.Fields{"name": r.Name, "error": errRm}).Error("Remove volume failed ")


Warning(). Failed to remove here is not necessarily a big issue I think.

And about other operations in rollback? Should we also log with warning? IMO those are subsequent errors.

Yes. I think errors on rollback are generally warnings - like it's helpful to warn if something we expected to complete failed, but it does not impact success/failure of the actual operation requested. We can silently ignore but I am generally not comfortable with silent failures.

msterin · 2016-10-12T01:09:05Z

vmdk_plugin/plugin.go

-		if err != nil {
-			log.WithFields(log.Fields{"name": r.Name, "error": err}).Error("Detach volume failed ")
-			return volume.Response{Err: err.Error()}
+		errDetach := d.ops.Detach(r.Name, nil)


should not it be errInGetPath , following the pattern of err names ?

msterin · 2016-10-12T01:09:30Z

vmdk_plugin/plugin.go

-		if err != nil {
-			log.WithFields(log.Fields{"name": r.Name, "error": err}).Error("Detach volume failed ")
-			return volume.Response{Err: err.Error()}
+		errDetach := d.ops.Detach(r.Name, nil)


@govint

…l reassignment. Address @govint comments Address @msterin comments

brunotm · 2016-10-12T17:28:10Z

@msterin
I have updated the PR, it actually makes more sense to only log as Error the original issue.

msterin

A (minor) request to separate detach/remove in a function instead of copy-paste. Given it's a few lines - not a big deal. so approving.

msterin · 2016-10-12T17:42:36Z

vmdk_plugin/plugin.go

-		if err != nil {
-			log.WithFields(log.Fields{"name": r.Name, "error": err}).Error("Detach volume failed ")
-			return volume.Response{Err: err.Error()}
+			"error": errMkfs}).Error("Create filesystem failed, removing the volume ")


looks like detach/remove has to be separate in a function instead of copy-paste.

brunotm · 2016-10-14T09:12:42Z

@msterin
If it would reutilized beyond these 2 cases, i wouldn't mind.
But this PR would be the place for it?

msterin · 2016-10-14T17:07:31Z

@brunotm - for this PR it's not a problem, I am just generally sensitive to copy-n-paste :-).
At any rate, since we have 2 approved I am merging the PR. Thanks !

* master: (25 commits) Update new ESX IP added forgotten .so file Install sqlite3 py libs on ESX and load for Python2 Added py code and binaries for sqlite3 python libs Update drone security Removed accidental .pyc files Handle byte to string conversions for status command. Auth configuration and operation admission check (Auth.liping) (vmware-archive#603) Revert "Cli auth.liping" Cli auth.liping (vmware-archive#640) Handle missing or invalid fs type on mount. (vmware-archive#639) Updated Admin CLI commands to support tenants. (vmware-archive#620) Workaround older versions of e2fsprogs (vmware-archive#631) Add auth proposal Made handing of missing metafile less harsh. (vmware-archive#627) Fixed ACLs in payload bin dir (vmware-archive#624) Fixed error handling for set command. (vmware-archive#610) Use new error variables when rolling back volume creation to avoid nil reassignment. (vmware-archive#617) Change wording Fix broken link ...

brunotm changed the title ~~Fix error handling when rolling back volume creation #616~~ Fix error handling when rolling back volume creation Oct 8, 2016

govint reviewed Oct 8, 2016

View reviewed changes

brunotm force-pushed the brunotm.issue616 branch from a3c2595 to 2512516 Compare October 9, 2016 12:24

govint approved these changes Oct 10, 2016

View reviewed changes

msterin suggested changes Oct 12, 2016

View reviewed changes

Use new error variables when rolling back volume creation to avoid ni…

f70febc

…l reassignment. Address @govint comments Address @msterin comments

brunotm force-pushed the brunotm.issue616 branch from 2512516 to f70febc Compare October 12, 2016 17:25

msterin approved these changes Oct 12, 2016

View reviewed changes

msterin merged commit 53e2e24 into vmware-archive:master Oct 14, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix error handling when rolling back volume creation #617

Fix error handling when rolling back volume creation #617

brunotm commented Oct 8, 2016 •

edited

Loading

govint Oct 8, 2016 •

edited

Loading

brunotm Oct 8, 2016

brunotm commented Oct 9, 2016

govint left a comment

govint Oct 8, 2016

msterin left a comment

msterin Oct 12, 2016

brunotm Oct 12, 2016

msterin Oct 12, 2016

msterin Oct 12, 2016

msterin Oct 12, 2016

brunotm commented Oct 12, 2016

msterin left a comment

msterin Oct 12, 2016

brunotm commented Oct 14, 2016

msterin commented Oct 14, 2016

Fix error handling when rolling back volume creation #617

Fix error handling when rolling back volume creation #617

Conversation

brunotm commented Oct 8, 2016 • edited Loading

govint Oct 8, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brunotm commented Oct 9, 2016

govint left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

msterin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brunotm commented Oct 12, 2016

msterin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brunotm commented Oct 14, 2016

msterin commented Oct 14, 2016

brunotm commented Oct 8, 2016 •

edited

Loading

govint Oct 8, 2016 •

edited

Loading