[node-patches] Change in ovirt-node[master]: Add initramfs re-generation during installation
nsoffer at redhat.com
nsoffer at redhat.com
Mon Jun 29 22:50:44 UTC 2015
Nir Soffer has posted comments on this change.
Change subject: Add initramfs re-generation during installation
......................................................................
Patch Set 2:
(16 comments)
https://gerrit.ovirt.org/#/c/42912/2/scripts/ovirt-node-update-initramfs
File scripts/ovirt-node-update-initramfs:
Line 15: mount -oremount,ro /run/initramfs/live
Line 16: """
Line 17:
Line 18: import logging
Line 19: from ovirt.node.utils import system
Trailing whitespace
Line 20:
Line 21:
Line 22: log = logging.getLogger()
Line 23:
Line 22: log = logging.getLogger()
Line 23:
Line 24:
Line 25: if __name__ == "__main__":
Line 26: stdout = logging.StreamHandler()
This logger is using stderr - why are you calling it stdout?
Line 27: log.addHandler(stdout)
Line 28: initramfs = system.Initramfs()
Line 29: initramfs.rebuild()
Line 30:
https://gerrit.ovirt.org/#/c/42912/2/src/ovirt/node/utils/system.py
File src/ovirt/node/utils/system.py:
Line 925
Line 926
Line 927
Line 928
Line 929
Why not catch only the exception that check_call may raise?
Line 366:
Line 367: liveos = Mount("/liveos")
Line 368: boot = Mount(device="/liveos", path="/boot")
Line 369:
Line 370: liveos.remount(rw=True)
If this fails, you are leaving liveos mounted without cleanup.
Line 371: boot.mount("bind")
Line 372:
Line 373: if not os.path.ismount("/boot"):
Line 374: raise RuntimeError("Failed to mount /boot")
Line 367: liveos = Mount("/liveos")
Line 368: boot = Mount(device="/liveos", path="/boot")
Line 369:
Line 370: liveos.remount(rw=True)
Line 371: boot.mount("bind")
If this fails, you are leaving liveos mounted without cleanup.
Line 372:
Line 373: if not os.path.ismount("/boot"):
Line 374: raise RuntimeError("Failed to mount /boot")
Line 375:
Line 370: liveos.remount(rw=True)
Line 371: boot.mount("bind")
Line 372:
Line 373: if not os.path.ismount("/boot"):
Line 374: raise RuntimeError("Failed to mount /boot")
You are living liveos mounted without cleanup
Line 375:
Line 376: # Now run something in this context
Line 377: yield
Line 378:
Line 373: if not os.path.ismount("/boot"):
Line 374: raise RuntimeError("Failed to mount /boot")
Line 375:
Line 376: # Now run something in this context
Line 377: yield
You want to use try finally block, to ensure that the clean up is perform if the caller code raised an exception:
with mounted_boot():
raise Exception("no cleanup for you")
This should fix this issue:
@contextmanager
def mounted_boot():
setup ...
try:
yield
finally:
cleanup ...
Line 378:
Line 379: boot.umount()
Line 380: liveos.umount()
Line 381: LOGGER.info("Successfully unmounted /liveos and /boot")
Line 942: except:
Line 943: LOGGER.exception("Can't remount %s on %s!" % (device,
Line 944: self.path))
Line 945:
Line 946: def mount(self, options=""):
Using None will make it more clear
Line 947: if not self.device:
Line 948: LOGGER.exception("Can't mount without a device specified")
Line 949: raise RuntimeError("No device was specified when Mount() "
Line 950: "was initialized")
Line 949: raise RuntimeError("No device was specified when Mount() "
Line 950: "was initialized")
Line 951:
Line 952: fstype = self.fstype if self.fstype else "auto"
Line 953: options = ["-o" + options] if options else []
Using same variable for too different things (options value, options flag with value) works but makes the code harder to follow.
Better prepare the args separately from the call like this:
args = ["mount", "-t", fstype]
if options:
args.extend(("-o", options))
args.extend((self.device, self.path))
utils.process.check_call(args)
Line 954:
Line 955: try:
Line 956: utils.process.check_call(["mount", "-t", fstype] + options +
Line 957: [self.device, self.path])
Line 1227: return vgs
Line 1228:
Line 1229:
Line 1230: class Initramfs(base.Base):
Line 1231: """This class shallw rap the logic needed to rebuild the initramfs
shallw rap -> shall wrap
Line 1232:
Line 1233: The main obstacle is mounting the correct paths.
Line 1234: Furthermore we are taking care that now orphans are left over.
Line 1235: """
Line 1232:
Line 1233: The main obstacle is mounting the correct paths.
Line 1234: Furthermore we are taking care that now orphans are left over.
Line 1235: """
Line 1236: def _regenerate_initramfs(self):
This function is doing 2 things:
- generate new initrd
- replace the old with the new
It would be better to separate generation from the replacement, and put the logic in rebuild:
with mounted_boot():
generate_new_initrd(new_initrd)
install_new_initrd(new_initrd, pri_initrd)
Line 1237: pri_initrd = "/boot/initrd0.img"
Line 1238: new_initrd = pri_initrd + ".new"
Line 1239: backup_initrd = pri_initrd + ".old"
Line 1240:
Line 1242: "'%s' (this can take a while)" % pri_initrd)
Line 1243:
Line 1244: rd_stdout = ""
Line 1245: try:
Line 1246: rd_stdout = check_output(["dracut", new_initrd],
Isn't this stderr?
Line 1247: stderr=process.PIPE)
Line 1248: except:
Line 1249: LOGGER.warn("dracut failed to regenerate the initramfs")
Line 1250: LOGGER.warn("dracut output: %s" % rd_stdout)
Line 1247: stderr=process.PIPE)
Line 1248: except:
Line 1249: LOGGER.warn("dracut failed to regenerate the initramfs")
Line 1250: LOGGER.warn("dracut output: %s" % rd_stdout)
Line 1251: raise
If dracut fails, does it leave partly genrated initrd? do we want to remove it?
Line 1252:
Line 1253: try:
Line 1254: check_call(["mv", pri_initrd, backup_initrd])
Line 1255: check_call(["mv", new_initrd, pri_initrd])
Line 1263: try:
Line 1264: os.unlink(orph)
Line 1265: LOGGER.debug("Removed orphan: %s" % orph)
Line 1266: except:
Line 1267: pass
This upgrade logic can leave the machine without any initrd, for example, if "mv new pri" fail, and then "mv backup pri" fail. Probably unlikely - but we can do this in an atomic way:
check_call(["ln", pri_initrd, backup_initrd])
try:
check_call(["mv", new_initrd, pri_initrd])
except:
try:
os.unlink(new_initrd)
except OSError:
warn...
finally:
try:
os.unlink(backup_initrd)
except OSError:
warn...
Line 1268:
Line 1269: def rebuild(self):
Line 1270: LOGGER.info("Preparing to regenerate the initramfs")
Line 1271: LOGGER.info("The regenreation is performed in-place, "
Line 1272: "the existing initrd will be overwritten")
Line 1273: try:
Line 1274: with mounted_boot():
Line 1275: self._regenerate_initramfs()
Line 1276: LOGGER.info("Initramfs regenration completed successfully")
You did not exit from the with scope, so mounted_boot() cleanup code was not run yet.
If the cleanup code fails, you will see 2 contradicting logs:
Initramfs regenration completed successfully
(cleanup code raises)
Initramfs regenration failed
Line 1277: except:
Line 1278: LOGGER.info("Initramfs regenration failed")
Line 1279: raise
Line 1280:
Line 1274: with mounted_boot():
Line 1275: self._regenerate_initramfs()
Line 1276: LOGGER.info("Initramfs regenration completed successfully")
Line 1277: except:
Line 1278: LOGGER.info("Initramfs regenration failed")
Why log if you raise? The caller should handle this failure. You don't need this try block, just let it blow and let the caller handle it.
This should be simplified to:
....
with mounted_boot():
self._regenerate_initramfs()
LOGGER.info("Initramfs regenration completed successfully")
Line 1279: raise
Line 1280:
--
To view, visit https://gerrit.ovirt.org/42912
To unsubscribe, visit https://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I209a82ff6bf10edf0857e362584bc6370081c320
Gerrit-PatchSet: 2
Gerrit-Project: ovirt-node
Gerrit-Branch: master
Gerrit-Owner: Fabian Deutsch <fabiand at redhat.com>
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer at redhat.com>
Gerrit-Reviewer: automation at ovirt.org
Gerrit-HasComments: Yes
More information about the node-patches
mailing list