[node-patches] Change in ovirt-node[master]: Add initramfs re-generation during installation
fabiand at redhat.com
fabiand at redhat.com
Fri Jul 10 14:44:01 UTC 2015
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 <fabiand at redhat.com>
Gerrit-Reviewer: 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