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

[fabiand at redhat.com]

Fabian Deutsch has posted comments on this change.

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


Patch Set 4:

(6 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.e
Done
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 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 simpli
I partially agree, but I'd rather do this in a spearate patch.
Line 928:         return fs
Line 929: 
Line 930:     def _tokens(self):
Line 931:         tokens = process.check_output(["blkid", "-o", "export", self.device])


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?
Done
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?
Done
Line 988:         except:
Line 989:             LOGGER.exception("Can't mount %s on %s" % (self.device,
Line 990:                              self.path))
Line 991: 


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".
Done.

And yes, a good rule of thumb for when only except is allowed.
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'
Done
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