[node-patches] Change in ovirt-node[master]: umount partitions before mounting them

[rbarry at redhat.com]

Ryan Barry has posted comments on this change.

Change subject: umount partitions before mounting them
......................................................................


Patch Set 2:

(1 comment)

http://gerrit.ovirt.org/#/c/34611/2/src/ovirtnode/install.py
File src/ovirtnode/install.py:

Line 471:             if not _functions.system(e2label_cmd):
Line 472:                 logger.error("Failed to label new Root partition")
Line 473:                 return False
Line 474:         _functions.system("umount -l %s" % candidate_dev)
Line 475:         _functions.system("umount -l /liveos")
> hey Ryan, just question: what about check the value of the above system cal
There are a few difficulties:

First is that the old codebase has unexpected interactions, and I don't to change the behavior of ovirtfunctions.system to return anything other than what it does.

Second is that ovirtfunctions.system logged the output of a command, but didn't log stderr. In the case of the particular bug we're trying to fix here, seeing "mount /dev/${device} /liveos" in the log without any stderr or debug information from the context in which it was run doesn't do us a lot, mostly because the old codebase uses globals and class-level variables to pass data around in many cases, and cleanup can still happen which leaves it in a different state.

Third is that every time we change the installer codebas, it has unexpected interactions. It may be possible that in come cases (iscsi boot, stateless, upgrades, etc), the call will fail, but the install succeeds anyway. One of those problems with starting from code that doesn't check the return code of a call that doesn't raise exceptions if it fails. I'm wary of changing the behavior at alland causing regressions.

If we end up having problems with next-gen and need to stick with the old install codebase instead of using anaconda, I'd love to see if we can redo the entire module. If not, I'd rather leave it alone as much as possible.
Line 476:         mount_cmd = "mount \"%s\" /liveos" % candidate_dev
Line 477:         if not _functions.system(mount_cmd):
Line 478:             logger.error("Failed to mount %s on /liveos" % candidate_dev)
Line 479:             _functions.system("lsof")


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Icb87ee4153842e90472508f0fa6594f204e84268
Gerrit-PatchSet: 2
Gerrit-Project: ovirt-node
Gerrit-Branch: master
Gerrit-Owner: Ryan Barry <rbarry at redhat.com>
Gerrit-Reviewer: Douglas Schilling Landgraf <dougsland at redhat.com>
Gerrit-Reviewer: Ryan Barry <rbarry at redhat.com>
Gerrit-Reviewer: automation at ovirt.org
Gerrit-HasComments: Yes