[node-patches] Change in ovirt-node[master]: Mechanism to detect changed runtime images at boot

[fabiand at fedoraproject.org]

Fabian Deutsch has posted comments on this change.

Change subject: Mechanism to detect changed runtime images at boot
......................................................................


Patch Set 1:

(3 comments)

Dan, thanks for your review! An updated set will follow.

http://gerrit.ovirt.org/#/c/29005/1/src/ovirt/node/utils/hooks.py
File src/ovirt/node/utils/hooks.py:

Line 38:     known = ["pre-upgrade", "post-upgrade", "rollback", "on-boot",
Line 39:              "on-changed-boot-image"]
Line 40: 
Line 41:     legacy_hooks_directory = "/etc/ovirt-config-boot.d/"
Line 42:     hooks_path_tpl = "/usr/libexec/hooks/{name}"
> A more "polite" directory name would be
Ah thanks for that catch. That is a big bug, the path should contain our "namespace", thus it would be as you suggest
Line 43: 
Line 44:     @staticmethod
Line 45:     def post_auto_install():
Line 46:         Hooks.__run(Hooks.legacy_hooks_directory)


http://gerrit.ovirt.org/#/c/29005/1/src/ovirt/node/utils/image.py
File src/ovirt/node/utils/image.py:

Line 62:         """The "fingerprint" of an image can be used to distinguish two images
Line 63:         Or to identify an image.
Line 64:         """
Line 65:         imagefile = os.path.join(self.mountpoint(), "LiveOS", "squashfs.img")
Line 66:         squashinfo = process.check_output(["unsquashfs", "-s", imagefile])
> Isn't this method terribly slow? I do not think we need cryptographically-s
Yes, this isn't the quickest way (tho still in the area of ms), but it seemed to be safe.

But taking another look revealed that I can use a "version" file, which should also be distinct to images.

The sha sum is used, to ensure that only one string needs to be stored on our fs for a later comparison.


http://gerrit.ovirt.org/#/c/29005/1/src/ovirt/node/utils/security.py
File src/ovirt/node/utils/security.py:

Line 261:         'fcde2b2edba56bf408601fb721fe9b5c338d10ee429ea04fae5511b68fbf8fb9'
Line 262:         """
Line 263:         hasher = {"sha256": hashlib.sha256,
Line 264:                   "sha512": hashlib.sha512}[self._algorithm]()
Line 265:         hasher.update(data)
> I personally do not see the benefit of this wrapper class over a direct cal
Yes …
I somewhat agree in this now.

When I started ot code, it was a wrapper around openssl's hash functions, and also it was run on binary files, that seemed to be enough reaosn sfor me to creat ethe wrapper.


-- 
To view, visit http://gerrit.ovirt.org/29005
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I120326e25de0a0de3e117204777891591b91c604
Gerrit-PatchSet: 1
Gerrit-Project: ovirt-node
Gerrit-Branch: master
Gerrit-Owner: Fabian Deutsch <fabiand at fedoraproject.org>
Gerrit-Reviewer: Dan Kenigsberg <danken at redhat.com>
Gerrit-Reviewer: Fabian Deutsch <fabiand at fedoraproject.org>
Gerrit-Reviewer: automation at ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes