Change in vdsm[master]: contrib: Adding contrib/nfs-check.py

List overview All Threads
Download

newer

older

Change in vdsm[master]: BZ#787594...

Change in vdsm[master]: BZ#773371...

Douglas Schilling Landgraf

5 Jan 2012 5 Jan '12

7:37 p.m.

Douglas Schilling Landgraf has uploaded a new change for review.

Change subject: contrib: Adding contrib/nfs-check.py ......................................................................

contrib: Adding contrib/nfs-check.py

nfs-check is a python script to validate nfs targets. Currently, nfs-check mount the nfs target, create a file and read it from the mountpoint. If somethings goes wrong it will throw the error message.

Usage: nfs-check.py [OPTIONS] --server NFS server (REQUIRED) --target NFS target (REQUIRED) --local Local mountpoint (REQUIRED) --opts NFS options (OPTIONAL) --version List version release --help This help menu

Change-Id: I4d3231077fb59e11be1d61a578a96a3988559ff3 Example: 1) nfs-check.py --server 192.168.1.103 --target /nfs/storage --local /mnt 2) nfs-check.py --server 192.168.1.103 --target /nfs/storage --local /mnt --opts soft --- A contrib/nfs-check.py 1 file changed, 157 insertions(+), 0 deletions(-)

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

Gerrit-MessageType: newchange Gerrit-Change-Id: I4d3231077fb59e11be1d61a578a96a3988559ff3 Gerrit-PatchSet: 1 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Douglas Schilling Landgraf dougsland@redhat.com

Show replies by date

Dan Kenigsberg

7 Jan 7 Jan

10:15 p.m.

Dan Kenigsberg has posted comments on this change.

Change subject: contrib: Adding contrib/nfs-check.py ......................................................................

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

(2 inline comments)

a good start, but mount should be as root, but writing - as vdsm:kvm. The point of the script is to try second-guess why mount has failed - it should check whether the export exists, its ownership and mode, etc. I got Cannot execute the command: mount -t nfs localhost:/export/ext1 /mnt mount.nfs: access denied by server while mounting localhost:/export/ext1

which does not bring me newbie any closer to understanding why nfs is not working for me.

.................................................... File contrib/nfs-check.py Line 25: print "Usage: " + sys.argv[0] + " [OPTIONS]" I'd prefer a simpler cmdline of server:/targer with optional local (create a dir under /tmp and remove it later)

Line 41: process = subprocess.Popen(cmd, shell=True, stdout=subprocess.PIPE, stderr=subprocess.PIPE) using shell=True is a bad practice. it opens the door for nasty code injection. it's not the end of the world for this script, but still.

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

Gerrit-MessageType: comment Gerrit-Change-Id: I4d3231077fb59e11be1d61a578a96a3988559ff3 Gerrit-PatchSet: 2 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Douglas Schilling Landgraf dougsland@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Douglas Schilling Landgraf dougsland@redhat.com

Douglas Schilling Landgraf

9 Jan 9 Jan

12:35 p.m.

Douglas Schilling Landgraf has posted comments on this change.

Change subject: contrib: Adding contrib/nfs-check.py ......................................................................

Patch Set 2:

Ok Dan, I will prepare a new version. Thanks!

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

smizrahi＠redhat.com

31 Jan 31 Jan

9:35 p.m.

Saggi Mizrahi has posted comments on this change.

Change subject: contrib: Adding contrib/nfs-check.py ......................................................................

Patch Set 4: Looks good to me, but someone else must approve

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

Gerrit-MessageType: comment Gerrit-Change-Id: I4d3231077fb59e11be1d61a578a96a3988559ff3 Gerrit-PatchSet: 4 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Douglas Schilling Landgraf dougsland@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Douglas Schilling Landgraf dougsland@redhat.com Gerrit-Reviewer: Saggi Mizrahi smizrahi@redhat.com

Douglas Schilling Landgraf

1 Feb 1 Feb

12:34 a.m.

Douglas Schilling Landgraf has posted comments on this change.

Change subject: contrib: Adding contrib/nfs-check.py ......................................................................

Patch Set 4: Verified

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

Dan Kenigsberg

7:48 a.m.

Dan Kenigsberg has posted comments on this change.

Change subject: contrib: Adding contrib/nfs-check.py ......................................................................

Patch Set 4: (12 inline comments)

several minor comments, few (mostly postponable) serious ones.

.................................................... File contrib/nfs-check.py Line 28: print "Usage: " + sys.argv[0] + " server:/target" please add a few words on what this script is supposed to be doing

Line 36: def handler(self,signum, frame): coma, space

Line 41: process = subprocess.Popen(['mount', "-t", "nfs", cmd, pathName], please use full path (I don't want to infuriate Yaniv Kaul)

Line 53: print "Timeout, cannot mount the nfs! Please check the status" \ I'm surprised that an exception in a signal handler reaches here. Was this flow tested?

Line 62: if not os.path.isfile("/etc/exports"): use a CONSTANT instead of typing the same string twice

Line 66: strFound = False better name: targetFound

Line 67: with open('/etc/exports', 'r') as f: why do you expect that nfs server is localhost? that's not the typical use case. run this logic only if nfsServer resolves to this host.

Line 78: print "The local path %s doesn't exist." I think it would be more informative to at least print the returncode and the error message.

Line 107: process = subprocess.Popen(['sudo', "-u", "vdsm", "touch", fileTest], we are root now, we'd better use 'su' for that.

and use full path to su.

Line 122: if ret != 0: Consider using something like supervdsmServer._runAs() so that you get the proper OSError, instead of instead of parsing locale-specifiic error messages. (can wait for future patch_

BTW you should make sure LC_ALL=C when running touch.

Line 160: print "cannot execute umount()!" print why this has failed (rc, stderr)

Line 199: os.removedirs(LOCALPATH) this should be put in a "finally" section of a "try" starting line 181.

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

Dan Kenigsberg

7:48 a.m.

Dan Kenigsberg has posted comments on this change.

Change subject: contrib: Adding contrib/nfs-check.py ......................................................................

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

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

Douglas Schilling Landgraf

2 Feb 2 Feb

2:10 a.m.

Douglas Schilling Landgraf has posted comments on this change.

Change subject: contrib: Adding contrib/nfs-check.py ......................................................................

Patch Set 4: (12 inline comments)

.................................................... File contrib/nfs-check.py Line 28: print "Usage: " + sys.argv[0] + " server:/target" Done

Line 36: def handler(self,signum, frame): Done

Line 41: process = subprocess.Popen(['mount', "-t", "nfs", cmd, pathName], Done

Line 53: print "Timeout, cannot mount the nfs! Please check the status" \ If the nfs server is not available or the address of the nfs server is wrong, the mount will keep trying to mount it... So, to avoid the 'lock stage' I have added a timeout.

Line 62: if not os.path.isfile("/etc/exports"): Done

Line 66: strFound = False Done

Line 67: with open('/etc/exports', 'r') as f: Done

Line 78: print "The local path %s doesn't exist." Done

Line 107: process = subprocess.Popen(['sudo', "-u", "vdsm", "touch", fileTest], Done

Line 122: if ret != 0: Done

Line 160: print "cannot execute umount()!" Done

Line 199: os.removedirs(LOCALPATH) Done

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

Douglas Schilling Landgraf

2:10 a.m.

Douglas Schilling Landgraf has posted comments on this change.

Change subject: contrib: Adding contrib/nfs-check.py ......................................................................

Patch Set 5: Verified

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

Gerrit-MessageType: comment Gerrit-Change-Id: I4d3231077fb59e11be1d61a578a96a3988559ff3 Gerrit-PatchSet: 5 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Douglas Schilling Landgraf dougsland@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Douglas Schilling Landgraf dougsland@redhat.com Gerrit-Reviewer: Saggi Mizrahi smizrahi@redhat.com

Dan Kenigsberg

8:30 a.m.

Dan Kenigsberg has posted comments on this change.

Change subject: contrib: Adding contrib/nfs-check.py ......................................................................

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

(4 inline comments)

few more issues open (some scribed on previous code)

.................................................... File contrib/nfs-check.py Line 46: self.nfsServer = None naming: "self" is an "Nfs" object. so attribute does not have to start with "nfs".

Line 84: if ret != 0 and localMachine == True: this begs to be a separate function

Line 180: print output printing numerical returncode is important for debugging.

Line 197: output = process.communicate()[1].strip() this is output or errput?

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

Dan Kenigsberg

8:30 a.m.

Dan Kenigsberg has posted comments on this change.

Change subject: contrib: Adding contrib/nfs-check.py ......................................................................

Patch Set 4: (2 inline comments)

.................................................... File contrib/nfs-check.py Line 53: print "Timeout, cannot mount the nfs! Please check the status" \ I understand your intention, I simply asked if you tested that it actually works.

Line 122: if ret != 0: where exactly?

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

Douglas Schilling Landgraf

3 Feb 3 Feb

6:21 p.m.

Douglas Schilling Landgraf has posted comments on this change.

Change subject: contrib: Adding contrib/nfs-check.py ......................................................................

Patch Set 5: (4 inline comments)

.................................................... File contrib/nfs-check.py Line 46: self.nfsServer = None Done

Line 84: if ret != 0 and localMachine == True: Done

Line 180: print output Done

Line 197: output = process.communicate()[1].strip() Indeed, in this case, I am looking only for error output but popen.communicate returns a tuple of stdout stderr that's why I used output. If you prefer, I can change it to errput.

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

Douglas Schilling Landgraf

6:33 p.m.

Douglas Schilling Landgraf has posted comments on this change.

Change subject: contrib: Adding contrib/nfs-check.py ......................................................................

Patch Set 7: Verified

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

Gerrit-MessageType: comment Gerrit-Change-Id: I4d3231077fb59e11be1d61a578a96a3988559ff3 Gerrit-PatchSet: 7 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Douglas Schilling Landgraf dougsland@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Douglas Schilling Landgraf dougsland@redhat.com Gerrit-Reviewer: Saggi Mizrahi smizrahi@redhat.com

Dan Kenigsberg

5 Feb 5 Feb

1:38 p.m.

Dan Kenigsberg has posted comments on this change.

Change subject: contrib: Adding contrib/nfs-check.py ......................................................................

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

(2 inline comments)

.................................................... File contrib/nfs-check.py Line 71: print "Timeout, cannot mount the nfs! Please check the status " \ I did not receive an answer - have you tested this flow (sigalarm while communicate()).

Line 212: output = process.communicate()[1].strip() you are taking the second element of communicate() (which is the stderr) and call it output. I still do not understand if this is intentional.

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

Douglas Schilling Landgraf

6 Feb 6 Feb

12:48 p.m.

Douglas Schilling Landgraf has posted comments on this change.

Change subject: contrib: Adding contrib/nfs-check.py ......................................................................

Patch Set 8: Verified

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

Gerrit-MessageType: comment Gerrit-Change-Id: I4d3231077fb59e11be1d61a578a96a3988559ff3 Gerrit-PatchSet: 8 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Douglas Schilling Landgraf dougsland@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Douglas Schilling Landgraf dougsland@redhat.com Gerrit-Reviewer: Saggi Mizrahi smizrahi@redhat.com

Douglas Schilling Landgraf

12:48 p.m.

Douglas Schilling Landgraf has posted comments on this change.

Change subject: contrib: Adding contrib/nfs-check.py ......................................................................

Patch Set 7: (2 inline comments)

.................................................... File contrib/nfs-check.py Line 71: print "Timeout, cannot mount the nfs! Please check the status " \ yes, I have tested. The test is: use as serverName a random IP address.

# nfs-check.py 192.168.1.2:/lala Trying to /bin/mount -t nfs 192.168.1.2:/ds... Timeout, cannot mount the nfs! Please check the status of NFS service or/and the Firewall settings!

Line 212: output = process.communicate()[1].strip() I have renamed to errorMsg.

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

Dan Kenigsberg

2:41 p.m.

Dan Kenigsberg has posted comments on this change.

Change subject: contrib: Adding contrib/nfs-check.py ......................................................................

Patch Set 8: Looks good to me, approved

Ok, let's improve on this script later!

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

Dan Kenigsberg

2:41 p.m.

Dan Kenigsberg has submitted this change and it was merged.

Change subject: contrib: Adding contrib/nfs-check.py ......................................................................

contrib: Adding contrib/nfs-check.py

nfs-check is a python script to validate nfs targets. Currently, nfs-check mount the nfs target, create a file as vdsm:kvm and remove it. If any of these operations fails, the script will make suggestions to make the NFS server available for the tests.

$ sudo python nfs-check.py Usage: nfs-check.py server:/target

Change-Id: I4d3231077fb59e11be1d61a578a96a3988559ff3 --- A contrib/nfs-check.py 1 file changed, 270 insertions(+), 0 deletions(-)

Approvals: Douglas Schilling Landgraf: Verified Dan Kenigsberg: Looks good to me, approved

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

Gerrit-MessageType: merged Gerrit-Change-Id: I4d3231077fb59e11be1d61a578a96a3988559ff3 Gerrit-PatchSet: 8 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Douglas Schilling Landgraf dougsland@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Douglas Schilling Landgraf dougsland@redhat.com Gerrit-Reviewer: Saggi Mizrahi smizrahi@redhat.com

5273

Age (days ago)

5305

Last active (days ago)

vdsm-patches@lists.fedorahosted.org

17 comments

3 participants

tags (0)

participants (3)

Dan Kenigsberg
Douglas Schilling Landgraf
smizrahi＠redhat.com