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 <[email protected]> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Nir Soffer <[email protected]> Gerrit-Reviewer: [email protected] Gerrit-HasComments: Yes _______________________________________________ node-patches mailing list [email protected] http://lists.ovirt.org/mailman/listinfo/node-patches
