Handle errors on Get() call from server. #699

govint · 2016-11-04T11:48:01Z

The Get() returns volume status thats optional in the docker plugin API. So, don't treat errors on status fetch as fatal. This test below had to artifically create an empty KV file because thats the only way it can be made empty or corrupted.

Tests:

$ docker volume create -d vmdk --name ca-vol-7
ca-vol-7

$ docker volume inspect ca-vol-7
[
    {
        "Name": "ca-vol-7",
        "Driver": "vmdk",
        "Mountpoint": "/mnt/vmdk/ca-vol-7",
        "Status": {
            "access": "read-write",
            "attach-as": "independent_persistent",
            "capacity": {
                "allocated": "13MB",
                "size": "100MB"
            },
            "created": "Fri Nov  4 11:38:53 2016",
            "created by VM": "2",
            "datastore": "bigone",
            "diskformat": "thin",
            "fstype": "ext4",
            "status": "detached"
        },
        "Labels": {},
        "Scope": "global"
    }
]

$ docker volume inspect ca-vol-7
[
    {
        "Name": "ca-vol-7",
        "Driver": "vmdk",
        "Mountpoint": "/mnt/vmdk/ca-vol-7",
        "Labels": {},
        "Scope": "global"
    }
]

$ docker run --rm -it -w /data -v ca-vol-7:/data busybox
/data #

brunotm · 2016-11-04T12:25:58Z

vmdk_plugin/plugin.go

@@ -352,22 +352,32 @@ func (d *vmdkDriver) Mount(r volume.MountRequest) volume.Response {

 	// This is the first time we are asked to mount the volume, so comply
 	status, err := d.ops.Get(r.Name)
+
+	fstype := "ext4"


There is a constant for the default fstype fs.FstypeDefault

brunotm · 2016-11-04T12:26:03Z

vmdk_plugin/plugin.go

+			msg := fmt.Sprintf("Got invalid filesystem type for %s, assuming type as ext2.", r.Name)
+			log.WithFields(log.Fields{"name": r.Name, "error": msg}).Error("")
+			// Fail back to a default version that we can try with.
+			value = "ext4"


Same as above.

brunotm · 2016-11-04T12:29:59Z

If we cannot rely in the metadata in those cases, maybe we could have a fallback method for mounting with exec.Command and mount -t auto instead of passing the default fstype that may not be the user specified FS ?

govint · 2016-11-04T12:37:03Z

Yes, that sounds good but may be a separate change from this one.

On Fri, Nov 4, 2016 at 6:00 PM, Bruno Moura notifications@github.com
wrote:

If we cannot rely in the metadata in those cases, maybe we could have a
fallback method for mounting with exec.Command and mount -t auto instead of
passing the default fstype that may not be the user specified FS ?

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#699 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/APHsePFPy_0YRzeRvisZWgNJO2jp9ajkks5q6yVLgaJpZM4Kpc-c
.

msterin

The fix looks good , a few comments inline (the main one is that we need to print offending strings, and use warning instead of debug() is we fail to parse json.

msterin · 2016-11-04T18:55:29Z

vmdk_plugin/vmdkops/vmdkops.go

@@ -105,7 +105,7 @@ func (v VmdkOps) Get(name string) (map[string]interface{}, error) {

 	err = json.Unmarshal(str, &statusMap)
 	if err != nil {
-		return nil, err
+		log.Debugf("vmdkOps.Get failed decoding volume status for name=%s", name)


It should be warning , not debug. Admin cant do much about it but at least needs to know about it since it is why default would be applied.

msterin · 2016-11-04T18:58:45Z

esx_service/vmdk_ops.py

@@ -421,7 +421,7 @@ def getVMDK(vmdk_path, vol_name, datastore):
    except Exception as ex:
        msg = "Failed to get disk details for %s (%s)" % (vmdk_path, ex)
        logging.error(msg)
-        result = err(msg)
+        result = {}


after this fix msg is not needed and the code could be simply be as follows:

except Exception as ex: logging.error("Failed to get disk details for %s (%s)" % (vmdk_path, ex)) return None

msterin · 2016-11-04T19:00:11Z

vmdk_plugin/plugin.go

+		if !exists {
+			msg := fmt.Sprintf("Got invalid access type for %s, assuming read-write access.", r.Name)
+			log.WithFields(log.Fields{"name": r.Name, "error": msg}).Error("")
+			isReadOnly = false


would be nice to actually print the offending string

msterin · 2016-11-04T19:00:44Z

vmdk_plugin/plugin.go

+		// Check access type.
+		value, exists := status["access"].(string)
+		if !exists {
+			msg := fmt.Sprintf("Got invalid access type for %s, assuming read-write access.", r.Name)


"Got" is not needed, the msg can simply say "Invalid access type..."

msterin · 2016-11-04T19:00:50Z

vmdk_plugin/plugin.go

+		// Check file system type.
+		value, exists = status["fstype"].(string)
+		if !exists {
+			msg := fmt.Sprintf("Got invalid filesystem type for %s, assuming type as ext2.", r.Name)


"Got" is not needed, the msg can simply say "Invalid access type..."

msterin · 2016-11-04T19:01:14Z

vmdk_plugin/plugin.go

+		value, exists = status["fstype"].(string)
+		if !exists {
+			msg := fmt.Sprintf("Got invalid filesystem type for %s, assuming type as ext2.", r.Name)
+			log.WithFields(log.Fields{"name": r.Name, "error": msg}).Error("")


Would be good to print the offending string as well

@govint

Yes, that sounds good but may be a separate change from this one.

if you agree with the suggestions but want to put it in a different change, please open an issue and refer to it

msterin · 2016-11-07T19:41:13Z

LGTM when CI passes

pdhamdhere

I reviewed as well rebased to merge but never clicked merge. Please merge away.

kerneltime · 2016-11-29T08:27:41Z

Fixes #662

vmwclabot added the cla-not-required label Nov 4, 2016

brunotm reviewed Nov 4, 2016

View reviewed changes

govint force-pushed the null-status branch from b256b69 to 54f4bca Compare November 4, 2016 12:38

msterin reviewed Nov 4, 2016

View reviewed changes

govint force-pushed the null-status branch from 54f4bca to e1a2251 Compare November 7, 2016 12:33

Handle Get() errors from server.

44e16fc

pdhamdhere force-pushed the null-status branch from e1a2251 to 44e16fc Compare November 8, 2016 00:19

pdhamdhere approved these changes Nov 9, 2016

View reviewed changes

msterin merged commit 9a1b0c9 into master Nov 9, 2016

msterin deleted the null-status branch November 9, 2016 05:50

govint mentioned this pull request Nov 9, 2016

Corrupted metadata file should be handled gracefully and not terminate operations #662

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Handle errors on Get() call from server. #699

Handle errors on Get() call from server. #699

govint commented Nov 4, 2016 •

edited by kerneltime

Loading

brunotm Nov 4, 2016

brunotm Nov 4, 2016

brunotm commented Nov 4, 2016

govint commented Nov 4, 2016

msterin left a comment

msterin Nov 4, 2016

msterin Nov 4, 2016

msterin Nov 4, 2016

msterin Nov 4, 2016

msterin Nov 4, 2016

msterin Nov 4, 2016

msterin Nov 6, 2016

msterin commented Nov 7, 2016

pdhamdhere left a comment

kerneltime commented Nov 29, 2016

Handle errors on Get() call from server. #699

Handle errors on Get() call from server. #699

Conversation

govint commented Nov 4, 2016 • edited by kerneltime Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brunotm commented Nov 4, 2016

govint commented Nov 4, 2016

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

msterin commented Nov 7, 2016

pdhamdhere left a comment

Choose a reason for hiding this comment

kerneltime commented Nov 29, 2016

govint commented Nov 4, 2016 •

edited by kerneltime

Loading