Fabian Deutsch has posted comments on this change. Change subject: Add initramfs re-generation during installation ......................................................................
Patch Set 2: (11 comments) Thanks for the review, I think I addressed all points and found another issue: Because the space ion /boot is limited, the new initrd needs to be written elsewhere. https://gerrit.ovirt.org/#/c/42912/2/scripts/ovirt-node-update-initramfs File scripts/ovirt-node-update-initramfs: 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? Good point. 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 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 i Absolutely, good catch. Done. 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 Done 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 w Done 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 Done 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: Doesn't _re_generate imply that the current initramfs is re-created? However, I'll look how it fits. 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? actually it should be stderr=process.STDOUT - now also the var name matches. (stderr=PIPE can lead to a deadlock) 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 looks liek ti's cleaning up behind itself, from dracut: # clean up after ourselves no matter how we die. trap ' ret=$?; [[ $keep ]] && echo "Not removing $initdir." >&2 || { [[ $initdir ]] && rm -rf -- "$initdir"; }; [[ $keep ]] && echo "Not removing $early_cpio_dir." >&2 || { [[ $early_cpio_dir ]] && rm -Rf -- "$early_cpio_dir"; }; [[ $_dlogdir ]] && rm -Rf -- "$_dlogdir"; exit $ret; ' EXIT 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, i Done I actually also needed to relocate the temporary image, otherwise we risked to fill up /boot - it's now kept in /var/tmp 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 no Done 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 Done 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: 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
