Change in vdsm[master]: nfs-check improvements

Deepak C Shetty has uploaded a new change for review.

Change subject: nfs-check improvements
......................................................................

nfs-check improvements

Adds nfs trouble shooting wiki link and shows
the same in case of an error and also fixes the
wrong uid:gid issue when removing testfile.

Change-Id: I939476dbbad0f97b43b8dff4dc2c5913cb3d1a1e
---
M contrib/nfs-check.py
1 file changed, 25 insertions(+), 1 deletion(-)


  git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/81/3081/1
--
To view, visit http://gerrit.ovirt.org/3081
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: I939476dbbad0f97b43b8dff4dc2c5913cb3d1a1e
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Deepak C Shetty <deepakcs(a)linux.vnet.ibm.com&gt;

[Dan Kenigsberg]

Dan Kenigsberg has submitted this change and it was merged.

Change subject: nfs-check improvements
......................................................................


nfs-check improvements

Adds nfs trouble shooting wiki link and shows
the same in case of an error and also fixes the
wrong uid:gid issue when removing testfile.

Change-Id: I939476dbbad0f97b43b8dff4dc2c5913cb3d1a1e
---
M AUTHORS
M contrib/nfs-check.py
2 files changed, 36 insertions(+), 12 deletions(-)

Approvals:
  Dan Kenigsberg: Looks good to me, approved
  Deepak C Shetty: Verified


--
To view, visit http://gerrit.ovirt.org/3081
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: merged
Gerrit-Change-Id: I939476dbbad0f97b43b8dff4dc2c5913cb3d1a1e
Gerrit-PatchSet: 5
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Deepak C Shetty <deepakcs(a)linux.vnet.ibm.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Deepak C Shetty <deepakcs(a)linux.vnet.ibm.com&gt;
Gerrit-Reviewer: Douglas Schilling Landgraf <dougsland(a)redhat.com&gt;

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: nfs-check improvements
......................................................................


Patch Set 5: Looks good to me, approved

(1 inline comment)

....................................................
File contrib/nfs-check.py
Line 230:             errorMsg  = process.communicate()[1]
thanks for dropping the strip. it is just a matter of good practice: you created a
general-purpose function to exec external apps. some apps have significant whitespace in
stderrs. QED.

--
To view, visit http://gerrit.ovirt.org/3081
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I939476dbbad0f97b43b8dff4dc2c5913cb3d1a1e
Gerrit-PatchSet: 5
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Deepak C Shetty <deepakcs(a)linux.vnet.ibm.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Deepak C Shetty <deepakcs(a)linux.vnet.ibm.com&gt;
Gerrit-Reviewer: Douglas Schilling Landgraf <dougsland(a)redhat.com&gt;

Deepak C Shetty has posted comments on this change.

Change subject: nfs-check improvements
......................................................................


Patch Set 5: Verified

--
To view, visit http://gerrit.ovirt.org/3081
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I939476dbbad0f97b43b8dff4dc2c5913cb3d1a1e
Gerrit-PatchSet: 5
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Deepak C Shetty <deepakcs(a)linux.vnet.ibm.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Deepak C Shetty <deepakcs(a)linux.vnet.ibm.com&gt;
Gerrit-Reviewer: Douglas Schilling Landgraf <dougsland(a)redhat.com&gt;

Deepak C Shetty has abandoned this change.

Change subject: nfs-check improvements
......................................................................


Patch Set 4: Restored

Hmm,new to gerrit, i thought abondoning just abandons
the current patch set, but the status of the overall patch
itself got changed to Abondoned. So reverting back. Not sure what is the right way to
revoke a patchset, without affecting the status.

--
To view, visit http://gerrit.ovirt.org/3081
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: abandon
Gerrit-Change-Id: I939476dbbad0f97b43b8dff4dc2c5913cb3d1a1e
Gerrit-PatchSet: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Deepak C Shetty <deepakcs(a)linux.vnet.ibm.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Deepak C Shetty <deepakcs(a)linux.vnet.ibm.com&gt;
Gerrit-Reviewer: Douglas Schilling Landgraf <dougsland(a)redhat.com&gt;

Deepak C Shetty has abandoned this change.

Change subject: nfs-check improvements
......................................................................


Patch Set 4: Abandoned

Forgot to remove cmdRet = [] in tests function.
will re-post the patch.

--
To view, visit http://gerrit.ovirt.org/3081
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: abandon
Gerrit-Change-Id: I939476dbbad0f97b43b8dff4dc2c5913cb3d1a1e
Gerrit-PatchSet: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Deepak C Shetty <deepakcs(a)linux.vnet.ibm.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Deepak C Shetty <deepakcs(a)linux.vnet.ibm.com&gt;
Gerrit-Reviewer: Douglas Schilling Landgraf <dougsland(a)redhat.com&gt;

Deepak C Shetty has posted comments on this change.

Change subject: nfs-check improvements
......................................................................


Patch Set 3: Verified

--
To view, visit http://gerrit.ovirt.org/3081
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I939476dbbad0f97b43b8dff4dc2c5913cb3d1a1e
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Deepak C Shetty <deepakcs(a)linux.vnet.ibm.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Deepak C Shetty <deepakcs(a)linux.vnet.ibm.com&gt;
Gerrit-Reviewer: Douglas Schilling Landgraf <dougsland(a)redhat.com&gt;

Deepak C Shetty has posted comments on this change.

Change subject: nfs-check improvements
......................................................................


Patch Set 3: (3 inline comments)

....................................................
File contrib/nfs-check.py
Line 181:             process = cmdRet[0]
Done

Line 238:             errorMsg  = process.communicate()[1].strip()
Can you provide some error example where the whitespaces are useful ?
Also removing the strip() causes the output as below ( when i simulated a error) ...

python nfs-check.py llm19.in.ibm.com:/tmp/nfstest
Current hostname: llm65.in.ibm.com - IP addr 9.124.111.96
Trying to /bin/mount -t nfs llm19.in.ibm.com:/tmp/nfstest...
Executing NFS tests..
Permission denied: vdsm user as kvm cannot create a file into /tmp/tmpRsr5ZV
Suggestions: please verify the permissions of target (chmod or/and selinux booleans)
return = 1 error msg = /bin/touch: cannot touch `/tmp/tmpRsr5ZV/vdsmTest': Permission
denied

Status of tests [Failed]
For more troubleshooting tips, visit
http://www.ovirt.org/wiki/Troubleshooting_NFS_Storage_Issues
Disconnecting from NFS Server..
Done!

Notice the extra blank line just above "Status of tests [Failed]", which is not
looking too good. I think thats caused due to the \n present at the end of the errorMsg
and python print also adds \n.

Assuming there are instances where whitespaces might be needed in errorMsg,
i will remove strip() from the func, but still use it for all errorMsg reporting, so that
we don't see the extra newline as above.  Hope that is ok.

Line 247:         return cmdRet
Done

--
To view, visit http://gerrit.ovirt.org/3081
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I939476dbbad0f97b43b8dff4dc2c5913cb3d1a1e
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Deepak C Shetty <deepakcs(a)linux.vnet.ibm.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Deepak C Shetty <deepakcs(a)linux.vnet.ibm.com&gt;
Gerrit-Reviewer: Douglas Schilling Landgraf <dougsland(a)redhat.com&gt;

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: nfs-check improvements
......................................................................


Patch Set 3: I would prefer that you didn't submit this

(3 inline comments)

and please add yourself as an AUTHOR.

....................................................
File contrib/nfs-check.py
Line 181:             process = cmdRet[0]
I know it is "only" a contrib script, and that it does command-line building
(arghh), but still:

  process, errorMsg, ret = self.runCommand(cmdTouch)

would be so much cleaner!

Line 238:             errorMsg  = process.communicate()[1].strip()
it is a bad practice for a function to assume that trailing whitespace of all cmd is
meaningless. I'd suggest to strip the error outside (if needed).

Line 247:         return cmdRet
simpler and nicer:

  return process, errorMsg, ret

and loos cmdRet.

--
To view, visit http://gerrit.ovirt.org/3081
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I939476dbbad0f97b43b8dff4dc2c5913cb3d1a1e
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Deepak C Shetty <deepakcs(a)linux.vnet.ibm.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Deepak C Shetty <deepakcs(a)linux.vnet.ibm.com&gt;
Gerrit-Reviewer: Douglas Schilling Landgraf <dougsland(a)redhat.com&gt;

[Douglas Schilling Landgraf]

Douglas Schilling Landgraf has posted comments on this change.

Change subject: nfs-check improvements
......................................................................


Patch Set 3: Verified; Looks good to me, but someone else must approve

--
To view, visit http://gerrit.ovirt.org/3081
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I939476dbbad0f97b43b8dff4dc2c5913cb3d1a1e
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Deepak C Shetty <deepakcs(a)linux.vnet.ibm.com&gt;
Gerrit-Reviewer: Deepak C Shetty <deepakcs(a)linux.vnet.ibm.com&gt;
Gerrit-Reviewer: Douglas Schilling Landgraf <dougsland(a)redhat.com&gt;

Deepak C Shetty has posted comments on this change.

Change subject: nfs-check improvements
......................................................................


Patch Set 2: (1 inline comment)

Done - deepak.

....................................................
File contrib/nfs-check.py
Line 248:         
Done

--
To view, visit http://gerrit.ovirt.org/3081
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I939476dbbad0f97b43b8dff4dc2c5913cb3d1a1e
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Deepak C Shetty <deepakcs(a)linux.vnet.ibm.com&gt;
Gerrit-Reviewer: Deepak C Shetty <deepakcs(a)linux.vnet.ibm.com&gt;
Gerrit-Reviewer: Douglas Schilling Landgraf <dougsland(a)redhat.com&gt;

[Douglas Schilling Landgraf]

Douglas Schilling Landgraf has posted comments on this change.

Change subject: nfs-check improvements
......................................................................


Patch Set 2: I would prefer that you didn't submit this

(1 inline comment)

....................................................
File contrib/nfs-check.py
Line 248:         
Hello Deepak,

Can you please remove this space?
You can use git diff --check before commit your patch to check this kind of
"problem".

Thanks!

--
To view, visit http://gerrit.ovirt.org/3081
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I939476dbbad0f97b43b8dff4dc2c5913cb3d1a1e
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Deepak C Shetty <deepakcs(a)linux.vnet.ibm.com&gt;
Gerrit-Reviewer: Deepak C Shetty <deepakcs(a)linux.vnet.ibm.com&gt;
Gerrit-Reviewer: Douglas Schilling Landgraf <dougsland(a)redhat.com&gt;

[Douglas Schilling Landgraf]

Douglas Schilling Landgraf has posted comments on this change.

Change subject: nfs-check improvements
......................................................................


Patch Set 1: I would prefer that you didn't submit this

--
To view, visit http://gerrit.ovirt.org/3081
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I939476dbbad0f97b43b8dff4dc2c5913cb3d1a1e
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Deepak C Shetty <deepakcs(a)linux.vnet.ibm.com&gt;
Gerrit-Reviewer: Douglas Schilling Landgraf <dougsland(a)redhat.com&gt;

[Douglas Schilling Landgraf]

Douglas Schilling Landgraf has posted comments on this change.

Change subject: nfs-check improvements
......................................................................


Patch Set 1: (2 inline comments)

Hi Deepak,

Thanks a lot for your contribution. I am using -1 here since I have two suggestions to
your patch. Can you please check? Let me know if you would like to implement these
changes, otherwise I will go forward. 

Thanks!
Douglas

....................................................
File contrib/nfs-check.py
Line 194:                              
"http://www.ovirt.org/wiki/Troubleshooting_NFS_Storage_Issues"
I would suggest we move this print to this check:

ret = nfs.tests(LOCALPATH)
if ret != 0:
    print "Status of tests [Failed]"
    here <----

I am suggesting this approach since we can have more then "Permission denied"
error.

Line 209:                     cmdRemove = "/bin/rm " + fileTest
Since the remove and create file are the same approach (subprocess.Popen) what about we
create a function which receive the command by argument? (just to avoid duplicating the
code).

--
To view, visit http://gerrit.ovirt.org/3081
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I939476dbbad0f97b43b8dff4dc2c5913cb3d1a1e
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Deepak C Shetty <deepakcs(a)linux.vnet.ibm.com&gt;
Gerrit-Reviewer: Douglas Schilling Landgraf <dougsland(a)redhat.com&gt;