Change in vdsm[master]: vdsm: move watchdog default params to WatchdogDevice

Martin Polednik has uploaded a new change for review.

Change subject: vdsm: move watchdog default params to WatchdogDevice
......................................................................

vdsm: move watchdog default params to WatchdogDevice

Watchdog device creation is currently for no reason in buildConfDevices
while it belongs to WatchdogDevice getXML method

Change-Id: Ic76f040bf78cbec3129569eb30fbf14447f742d6
Signed-off-by: Martin Polednik 
---
M vdsm/vm.py
1 file changed, 2 insertions(+), 10 deletions(-)

git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/31/19331/1

diff --git a/vdsm/vm.py b/vdsm/vm.py
index 92d274e..ba268f6 100644
--- a/vdsm/vm.py
+++ b/vdsm/vm.py
@@ -1616,8 +1616,8 @@
         </watchdog>
         """
         m = self.createXmlElem(self.type, None, ['address'])
-        m.setAttrs(model=self.specParams['model'],
-                   action=self.specParams['action'])
+        m.setAttrs(model=self.specParams.get('model', 'i6300esb'),
+                   action=self.specParams.get('action', 'none'))
         return m

@@ -1908,14 +1908,6 @@
         # libvirt only support one watchdog device
         if len(devices[WATCHDOG_DEVICES]) > 1:
             raise ValueError("only a single watchdog device is supported")
-        if len(devices[WATCHDOG_DEVICES]) == 1:
-            if not 'specParams' in devices[WATCHDOG_DEVICES][0]:
-                devices[WATCHDOG_DEVICES][0]['specParams'] = {}
-            if not 'model' in devices[WATCHDOG_DEVICES][0]['specParams']:
-                devices[WATCHDOG_DEVICES][0]['specParams']['model'] = \
-                    'i6300esb'
-            if not 'action' in devices[WATCHDOG_DEVICES][0]['specParams']:
-                devices[WATCHDOG_DEVICES][0]['specParams']['action'] = 'none'

if len(devices[CONSOLE_DEVICES]) > 1:
             raise ValueError("Only a single console device is supported")

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: vdsm: move watchdog default params to WatchdogDevice
......................................................................

Patch Set 1:

Build Successful

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit/4496/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_pep8_gerrit/4415/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit_el/3599/ : SUCCESS

Martin Polednik has posted comments on this change.

Change subject: vdsm: move watchdog default params to WatchdogDevice
......................................................................

Patch Set 1: Verified+1

Michal Skrivanek has posted comments on this change.

Change subject: vdsm: move watchdog default params to WatchdogDevice
......................................................................

Patch Set 1: Code-Review+1

Martin Polednik has posted comments on this change.

Change subject: vdsm: move watchdog default params to WatchdogDevice
......................................................................

Patch Set 1: Verified-1

needs a bigger change, currently breaks things

[Federico Simoncelli]

Federico Simoncelli has posted comments on this change.

Change subject: vdsm: move watchdog default params to WatchdogDevice
......................................................................

Patch Set 1:

(1 comment)

....................................................
File vdsm/vm.py
Line 1616:         </watchdog>
Line 1617:         """
Line 1618:         m = self.createXmlElem(self.type, None, ['address'])
Line 1619:         m.setAttrs(model=self.specParams.get('model', 'i6300esb'),
Line 1620:                    action=self.specParams.get('action', 'none'))
Fine by me but we have to keep in mind that now we won't have any info about the model and action in the VDSM recovery file. (Please double check if we had it before, it might be that we weren't saving the recovery file anyway).
Line 1621:         return m
Line 1622: 
Line 1623: 
Line 1624: class SmartCardDevice(VmDevice):

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: vdsm: move watchdog default params to WatchdogDevice
......................................................................

Patch Set 2:

Build Successful

http://jenkins.ovirt.org/job/vdsm_pep8_gerrit/4783/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit/4859/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit_el/3974/ : SUCCESS

Martin Polednik has posted comments on this change.

Change subject: vdsm: move watchdog default params to WatchdogDevice
......................................................................

Patch Set 2: Verified+1

Ayal Baron has posted comments on this change.

Change subject: vdsm: move watchdog default params to WatchdogDevice
......................................................................

Patch Set 2:

(1 comment)

....................................................
File vdsm/vm.py
Line 1631: 
Line 1632: 
Line 1633: class WatchdogDevice(VmDevice):
Line 1634:     def __init__(self, *args, **kwargs):
Line 1635:         super(WatchdogDevice, self).__init__(*args, **kwargs)
It would be a lot clearer if the properties of each device were explicit.
I don't see the point of inheriting from VmDevice as it isn't really giving us anything.
You can instead specify here the two params that a WatchDogDevice should have and set the default values if not passed.
Line 1636: 
Line 1637:         if not hasattr(self, 'specParams'):
Line 1638:             self.specParams = {}
Line 1639:

Martin Polednik has posted comments on this change.

Change subject: vdsm: move watchdog default params to WatchdogDevice
......................................................................

Patch Set 2:

(1 comment)

....................................................
File vdsm/vm.py
Line 1631: 
Line 1632: 
Line 1633: class WatchdogDevice(VmDevice):
Line 1634:     def __init__(self, *args, **kwargs):
Line 1635:         super(WatchdogDevice, self).__init__(*args, **kwargs)
VmDevice is giving us means of generating the XML
Line 1636: 
Line 1637:         if not hasattr(self, 'specParams'):
Line 1638:             self.specParams = {}
Line 1639:

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: vdsm: move watchdog default params to WatchdogDevice
......................................................................

Patch Set 2: Code-Review+2

(1 comment)

....................................................
File vdsm/vm.py
Line 1631: 
Line 1632: 
Line 1633: class WatchdogDevice(VmDevice):
Line 1634:     def __init__(self, *args, **kwargs):
Line 1635:         super(WatchdogDevice, self).__init__(*args, **kwargs)
Ayal is right. VmDevice's conversion of a dict into object attribute is a piece of bad design. I wish we never had it that way.

However, this patch is not about big refactoring of VmDevice, but about splitting a huge buildConfDevices functioninto smaller pieces. In this task, I think this patch succeeds.
Line 1636: 
Line 1637:         if not hasattr(self, 'specParams'):
Line 1638:             self.specParams = {}
Line 1639:

[Dan Kenigsberg]

Dan Kenigsberg has submitted this change and it was merged.

Change subject: vdsm: move watchdog default params to WatchdogDevice
......................................................................

vdsm: move watchdog default params to WatchdogDevice

Watchdog device creation is currently handled in
buildConfDevices, creating unnecessary code pollution.
This patch moves the creation to WatchdogDevice class,
cleaning up buildConfDevices code. Side effect is that
we lose access to specParams if they are not passed from
engine.

Change-Id: Ic76f040bf78cbec3129569eb30fbf14447f742d6
Signed-off-by: Martin Polednik 
Reviewed-on: http://gerrit.ovirt.org/19331
Reviewed-by: Dan Kenigsberg 
---
M vdsm/vm.py
1 file changed, 8 insertions(+), 10 deletions(-)

Approvals:
  Dan Kenigsberg: Looks good to me, approved
  Martin Polednik: Verified