[node-patches] Change in ovirt-node[master]: Validate "Other Device" against live device

[fabiand at fedoraproject.org]

Fabian Deutsch has posted comments on this change.

Change subject: Validate "Other Device" against live device
......................................................................


Patch Set 6:

(2 comments)

There are two bigger things, which are touched by this patch and needs to be addressed.

....................................................
File src/ovirt/node/installer/core/boot_device_page.py
Line 148: 
Line 149:     def run(self):
Line 150:         devices = utils.storage.Devices(fake=self.do_fake)
Line 151:         self.devices = devices
Line 152:         self._all_devices = devices.get_all()
Keeping the devices in self.devices is fine for me, but let us drop self._all_devices then because we can also query that informations from self.devices later on.

The bigger problem is that self.devices only get's initialized when .run() is called. (When the thread is started). It was intended that this StorageDisocvery is run async as a thread (by calling start() on the thread object), but because of problems with the UI (back then) it is run sync (by calling run() on the thread object). 

So - If we wan't to use StorageDiscover-device we need to ensure that it's initialized during the instance creation (so in the __init__ method). And that the instance creation happens early - and in the best case async as a thread.
Line 153: 
Line 154:     def all_devices(self):
Line 155:         """Return a list of all devices
Line 156:         """


....................................................
File src/ovirt/node/utils/storage.py
Line 87:             import ovirtnode.storage
Line 88:             from ovirtnode.ovirtfunctions import translate_multipath_device
Line 89:             from ovirtnode.ovirtfunctions import get_live_disk
Line 90:             self.translate_multipath_device = translate_multipath_device
Line 91:             self.get_live_disk = get_live_disk
The intention of this class is to isolate the new codebase form the old codebase.
Exposing the old functions by making them members is undermining this approach :)

What you can do is:
Add propper method members to this class which have a propper documentation, clearly defined arguments and wrap the funciton sin the old codebase.

Often it is viable to take the legacy code, clean it up a bit and reuse it in this class. What you then need to do is to point the legacy code to this new implementation. The whole point of this is only have one implementation.
Line 92:             self._storage = ovirtnode.storage.Storage()
Line 93: 
Line 94:     def live_disk_name(self):
Line 95:         """get the device name of the live-media we are booting from


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I44b87876876965418765cbb6a0c0030bf143f156
Gerrit-PatchSet: 6
Gerrit-Project: ovirt-node
Gerrit-Branch: master
Gerrit-Owner: Ryan Barry <rbarry at redhat.com>
Gerrit-Reviewer: Fabian Deutsch <fabiand at fedoraproject.org>
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes