Change in vdsm[master]: Fix loop device handling for mount command

Saggi Mizrahi has uploaded a new change for review.

Change subject: Fix loop device handling for mount command
......................................................................

Fix loop device handling for mount command

Change-Id: Ib3ac6e93892ec625331dd56da0ecad6f0e39d7bc
Signed-off-by: Saggi Mizrahi <smizrahi(a)redhat.com&gt;
---
M tests/Makefile.am
M vdsm/storage/mount.py
2 files changed, 40 insertions(+), 6 deletions(-)


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

Gerrit-MessageType: newchange
Gerrit-Change-Id: Ib3ac6e93892ec625331dd56da0ecad6f0e39d7bc
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Saggi Mizrahi <smizrahi(a)redhat.com&gt;

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: Fix loop device handling for mount command
......................................................................


Patch Set 7: Verified; Looks good to me, approved

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ib3ac6e93892ec625331dd56da0ecad6f0e39d7bc
Gerrit-PatchSet: 7
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Saggi Mizrahi <smizrahi(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Saggi Mizrahi <smizrahi(a)redhat.com&gt;
Gerrit-Reviewer: Shahar Havivi <shavivi(a)redhat.com&gt;

[Dan Kenigsberg]

Dan Kenigsberg has submitted this change and it was merged.

Change subject: Fix loop device handling for mount command
......................................................................


Fix loop device handling for mount command

loop devices sometimes appear as themselves and sometimes appear as the
backing file. This canonicalizes the behaviour to always show the backing
file.

Change-Id: Ib3ac6e93892ec625331dd56da0ecad6f0e39d7bc
Signed-off-by: Saggi Mizrahi <smizrahi(a)redhat.com&gt;
---
M Makefile.am
M tests/Makefile.am
A tests/mountTests.py
M vdsm/storage/mount.py
4 files changed, 150 insertions(+), 15 deletions(-)

Approvals:
  Dan Kenigsberg: Verified; Looks good to me, approved


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

Gerrit-MessageType: merged
Gerrit-Change-Id: Ib3ac6e93892ec625331dd56da0ecad6f0e39d7bc
Gerrit-PatchSet: 7
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Saggi Mizrahi <smizrahi(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Saggi Mizrahi <smizrahi(a)redhat.com&gt;
Gerrit-Reviewer: Shahar Havivi <shavivi(a)redhat.com&gt;

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: Fix loop device handling for mount command
......................................................................


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

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ib3ac6e93892ec625331dd56da0ecad6f0e39d7bc
Gerrit-PatchSet: 7
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Saggi Mizrahi <smizrahi(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Saggi Mizrahi <smizrahi(a)redhat.com&gt;
Gerrit-Reviewer: Shahar Havivi <shavivi(a)redhat.com&gt;

Shahar Havivi has posted comments on this change.

Change subject: Fix loop device handling for mount command
......................................................................


Patch Set 6: Verified

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ib3ac6e93892ec625331dd56da0ecad6f0e39d7bc
Gerrit-PatchSet: 6
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Saggi Mizrahi <smizrahi(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Saggi Mizrahi <smizrahi(a)redhat.com&gt;
Gerrit-Reviewer: Shahar Havivi <shavivi(a)redhat.com&gt;

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: Fix loop device handling for mount command
......................................................................


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

(3 inline comments)

....................................................
File tests/mountTests.py
Line 60:                 m.umount()
rmdir(mpath) would be more polite.

....................................................
File vdsm/storage/mount.py
Line 52:         st = os.stat(path)
minor: there is no need to stat unless path[0]=='/'.

Line 73:             loopOpt = "loop=%s" % path
it is very hard to get away without complete parsing :-(

what if mtab has a mountpoit that happens to be called "/mnt/loop=/dev/bla" ?

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ib3ac6e93892ec625331dd56da0ecad6f0e39d7bc
Gerrit-PatchSet: 6
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Saggi Mizrahi <smizrahi(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Saggi Mizrahi <smizrahi(a)redhat.com&gt;
Gerrit-Reviewer: Shahar Havivi <shavivi(a)redhat.com&gt;

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: Fix loop device handling for mount command
......................................................................


Patch Set 5: (2 inline comments)

Saggi, Shahar is right, this fails in EL6.

....................................................
File tests/Makefile.am
Line 30: 	mountTests \
missing.py here

....................................................
File vdsm/storage/mount.py
Line 63:     loopdir = "/sys/dev/block/%d:%d/loop" % (major, minor)
that's missing in el6's Linux 2.6.32

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ib3ac6e93892ec625331dd56da0ecad6f0e39d7bc
Gerrit-PatchSet: 5
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Saggi Mizrahi <smizrahi(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Saggi Mizrahi <smizrahi(a)redhat.com&gt;
Gerrit-Reviewer: Shahar Havivi <shavivi(a)redhat.com&gt;

Shahar Havivi has posted comments on this change.

Change subject: Fix loop device handling for mount command
......................................................................


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

two problems:
1. test fail:
$ ./run_tests.sh mountTests.py
MountTests
    testLoopMount                                               FAIL

======================================================================
FAIL: testLoopMount (mountTests.MountTests)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/root/git/vdsm/tests/mountTests.py", line 58, in testLoopMount
    self.assertTrue(m.isMounted())
AssertionError

----------------------------------------------------------------------
Ran 1 test in 0.097s

FAILED (failures=1)


2. build not working:
-----------------------------------------------
Making all in tests
make[1]: Entering directory `/root/git/vdsm/tests'
make[1]: *** No rule to make target `mountTests', needed by `all-am'.  Stop.
make[1]: Leaving directory `/root/git/vdsm/tests'
make: *** [all-recursive] Error 1
-----------------------------------------------

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ib3ac6e93892ec625331dd56da0ecad6f0e39d7bc
Gerrit-PatchSet: 5
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Saggi Mizrahi <smizrahi(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Saggi Mizrahi <smizrahi(a)redhat.com&gt;
Gerrit-Reviewer: Shahar Havivi <shavivi(a)redhat.com&gt;

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: Fix loop device handling for mount command
......................................................................


Patch Set 5: (1 inline comment)

....................................................
File vdsm/storage/mount.py
Line 52:         st = os.stat(path)
ok, I changing self.fs_spec, which is part of the Mount obj identity, is very ugly.

and I understand that "path" here is not fs_file which is the hangable
mountpoint. just a minor comment remains: there is no need to stat unless
path[0]=='/'.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ib3ac6e93892ec625331dd56da0ecad6f0e39d7bc
Gerrit-PatchSet: 5
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Saggi Mizrahi <smizrahi(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Saggi Mizrahi <smizrahi(a)redhat.com&gt;
Gerrit-Reviewer: Shahar Havivi <shavivi(a)redhat.com&gt;

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: Fix loop device handling for mount command
......................................................................


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

(1 inline comment)

....................................................
File vdsm/storage/mount.py
Line 52:         st = os.stat(path)
I do not how my comment here was lost:

This call hangs for nfs mount that went missing. This means that iterMount will go D state
just for one bad mount - even if unrelated to Vdsm.

I do not understand why you say Mount object do not have state. What is Mount.fs_spec if
not "state"? I suggest making sure that .fs_spec fits the /proc/mount, not the
args to __init__().

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ib3ac6e93892ec625331dd56da0ecad6f0e39d7bc
Gerrit-PatchSet: 5
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Saggi Mizrahi <smizrahi(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Saggi Mizrahi <smizrahi(a)redhat.com&gt;
Gerrit-Reviewer: Shahar Havivi <shavivi(a)redhat.com&gt;

Saggi Mizrahi has posted comments on this change.

Change subject: Fix loop device handling for mount command
......................................................................


Patch Set 3: (2 inline comments)

....................................................
File vdsm/storage/mount.py
Line 50: def getRealName(path):
path being "proc" or server:/export

Line 62:     # backing file. As the mount command does the resolution so must we.
No, mount object don't have state

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ib3ac6e93892ec625331dd56da0ecad6f0e39d7bc
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Saggi Mizrahi <smizrahi(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Saggi Mizrahi <smizrahi(a)redhat.com&gt;
Gerrit-Reviewer: Shahar Havivi <shavivi(a)redhat.com&gt;

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: Fix loop device handling for mount command
......................................................................


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

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ib3ac6e93892ec625331dd56da0ecad6f0e39d7bc
Gerrit-PatchSet: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Saggi Mizrahi <smizrahi(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Saggi Mizrahi <smizrahi(a)redhat.com&gt;
Gerrit-Reviewer: Shahar Havivi <shavivi(a)redhat.com&gt;

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: Fix loop device handling for mount command
......................................................................


Patch Set 3: (7 inline comments)

....................................................
File tests/Makefile.am
Line 27: 	hooksTests.py
missing backslash

Line 34: 	resourceManagerTests.py \
extra one

....................................................
File vdsm/storage/mount.py
Line 42: class MountError(RuntimeError):
you probably wanted to add this module to PEP8_WHITELIST

Line 50: def getRealName(path):
wouldn't you want to keep this function local?

Line 53:     except:
Edu would explode on this... Which exceptions do you expect here?

Line 62:     # backing file. As the mount command does the resolution so must we.
just a thought - maybe this fs_spec conversion SHOULD be done once on mount(), and not on
iter?

Line 68:         return f.read().strip()
strip? what if the backing file name ends with a space?

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ib3ac6e93892ec625331dd56da0ecad6f0e39d7bc
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Saggi Mizrahi <smizrahi(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Saggi Mizrahi <smizrahi(a)redhat.com&gt;
Gerrit-Reviewer: Shahar Havivi <shavivi(a)redhat.com&gt;