[node-patches] Change in ovirt-node[master]: Add initramfs re-generation during installation

Sun Jul 12 00:17:01 UTC 2015

Nir Soffer has posted comments on this change.

Change subject: Add initramfs re-generation during installation
......................................................................

Patch Set 4:

(9 comments)

https://gerrit.ovirt.org/#/c/42912/4/scripts/ovirt-node-update-initramfs
File scripts/ovirt-node-update-initramfs:

Line 32:     try:
Line 33:         initramfs.rebuild()
Line 34:     except:
Line 35:         log.error("Initramfs regeneration failed")
Line 36:         # Raise to exit with != 0
This is not a valid reason to raise. To exit with non-zero code,  use sys.exit(1).

If you want to the traceback, use log.exception(), so the traceback is recorded in the log, instead of lost in the shell.
Line 37:         raise
Line 38: 

https://gerrit.ovirt.org/#/c/42912/4/src/ovirt/node/utils/system.py
File src/ovirt/node/utils/system.py:

Line 380:         LOGGER.warn("An error appeared while "
Line 381:                     "interacting with /boot: %s" % e)
Line 382:         raise
Line 383:     finally:
Line 384:         boot.umount()
Hopefully umount will not raise, or we may leave liveos mounted.
Line 385:         liveos.umount()
Line 386: 
Line 387:     LOGGER.info("Successfully unmounted /liveos and /boot")
Line 388: 

Line 923: 
Line 924:             fs = Filesystem(device)
Line 925: 
Line 926:         except process.CalledProcessError as e:
Line 927:             LOGGER.debug("Failed to resolve disks: %s" % e.cmd, exc_info=True)
This try except is relevant only around check_output. This will also simplify the code, similar to how mountpoints():

    try:
        device = process.check_output(...)
        return Filesystem(device)
    except process.CalledProcessError:
        log ...
        return None

This may also raise OSError (if blkid is not found) - are you ok with this error not handled here?
Line 928:         return fs
Line 929: 
Line 930:     def _tokens(self):
Line 931:         tokens = process.check_output(["blkid", "-o", "export", self.device])

Line 939:             targets = process.check_output(["findmnt", "-o", "target", "-n",
Line 940:                                             self.device]).strip().split("\n")
Line 941:             return [Mount(t.strip()) for t in targets]
Line 942:         except process.CalledProcessError:
Line 943:             return []
OSError is also possible here, you many like to handle it.
Line 944: 
Line 945: 
Line 946: class Mount(base.Base):
Line 947:     """A class to find the base mount point for a path and handle mounting

Line 977:             LOGGER.exception("Can't mount without a device specified")
Line 978:             raise RuntimeError("No device was specified when Mount() "
Line 979:                                "was initialized")
Line 980: 
Line 981:         args = ["-t", self.fstype if self.fstype else "auto"]
Why not add "mount" here?
Line 982:         if options:
Line 983:             args += ["-o" + options]
Line 984:         args += [self.device, self.path]
Line 985: 

Line 983:             args += ["-o" + options]
Line 984:         args += [self.device, self.path]
Line 985: 
Line 986:         try:
Line 987:             utils.process.check_call(["mount"] + args)
Why add "mount" only at the end?
Line 988:         except:
Line 989:             LOGGER.exception("Can't mount %s on %s" % (self.device,
Line 990:                              self.path))
Line 991: 

Line 1262: 
Line 1263:     The main obstacle is mounting the correct paths.
Line 1264:     Furthermore we are taking care that now orphans are left over.
Line 1265:     """
Line 1266:     def try_unlink(self, path, _log=lambda a: None):
Why use private argument?

Also, passing a logging function is little ugly. I don't think this abstraction is helpful. The caller want to get the exception, check the type of the error (e.errno, and log debug message or error, depending on the reason for the failure.

This can be more useful if you ignore missing file (usually not interesting), and raise other errors for the caller:

    try:
        os.unlink(path)
    except OSError as e:
        if e.errno != errno.ENOENT:
            raise
        LOGGER.debug("File %r was already removed", path)
Line 1267:         try:
Line 1268:             os.unlink(path)
Line 1269:         except:
Line 1270:             _log("Failed to remove " + path)

Line 1265:     """
Line 1266:     def try_unlink(self, path, _log=lambda a: None):
Line 1267:         try:
Line 1268:             os.unlink(path)
Line 1269:         except:
You are hiding the actual error - this is almost bad as "pass".

Also, you catch unrelated errors like KeyboardInterrupt, and other exceptions that inherit from BaseException.

There is only one case where it is valid to use bare except, when you raise again:

    try:
        something ...
    except:
        cleanup ...
        raise

Here you should use something like:

    try:
        os.unlink(path)
    except OSError as e:
        LOGGER.debug("Failed to remove %r: %s", path, e)
Line 1270:             _log("Failed to remove " + path)
Line 1271: 
Line 1272:     def _generate_new_initramfs(self, new_initrd):
Line 1273:         LOGGER.info("Generating new initramfs "

Line 1270:             _log("Failed to remove " + path)
Line 1271: 
Line 1272:     def _generate_new_initramfs(self, new_initrd):
Line 1273:         LOGGER.info("Generating new initramfs "
Line 1274:                     "'%s' (this can take a while)" % new_initrd)
You can use %r instead of '%s'
Line 1275: 
Line 1276:         rd_stdout = ""
Line 1277:         try:
Line 1278:             rd_stdout = check_output(["dracut", new_initrd],

-- 
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: 4
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