Change in vdsm[master]: net tests: Functional tests infrastructure
by automation＠ovirt.org
gerrit-hooks has posted comments on this change.
Change subject: net tests: Functional tests infrastructure
......................................................................
Patch Set 11:
* Update tracker: IGNORE, no Bug-Url found
* Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and is a valid url.
* Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.6', 'ovirt-4.0'])
--
To view, visit https://gerrit.ovirt.org/58329
To unsubscribe, visit https://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I4645756c575f34cabf9536ef663ffc8009655abb
Gerrit-PatchSet: 11
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Edward Haas <edwardh(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Edward Haas <edwardh(a)redhat.com>
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Ondřej Svoboda <osvoboda(a)redhat.com>
Gerrit-Reviewer: Petr Horáček <phoracek(a)redhat.com>
Gerrit-Reviewer: gerrit-hooks <automation(a)ovirt.org>
Gerrit-HasComments: No
7 years, 11 months
Change in vdsm[master]: net tests: Functional tests infrastructure
by edwardh＠redhat.com
Edward Haas has posted comments on this change.
Change subject: net tests: Functional tests infrastructure
......................................................................
Patch Set 10:
(8 comments)
https://gerrit.ovirt.org/#/c/58329/10/tests/network/func_net_creation_bas...
File tests/network/func_net_creation_basic_test.py:
Line 23:
Line 24: from .netfunctestlib import NetFuncTestCase, NOCHK
Line 25: from .nettestlib import dummy_device
Line 26:
Line 27: NETWORK_NAME = 'test-network'
> i think this should be imported as well. we use this name over and over
Even this scope is too much, I think we should randomize it eventually.
For now, I prefer it under the test module, there is no advantage to reuse it at project test level.
Line 28:
Line 29:
Line 30: @attr(type='functional')
Line 31: class NetworkCreateBasicTest(NetFuncTestCase):
Line 31: class NetworkCreateBasicTest(NetFuncTestCase):
Line 32:
Line 33: def test_add_net_based_on_nic(self):
Line 34: with dummy_device() as nic:
Line 35: NETSETUP = {NETWORK_NAME: {'nic': nic}}
> why capital?
It's a constant.
Line 36: with self.setupNetworks(NETSETUP, {}, NOCHK):
https://gerrit.ovirt.org/#/c/58329/10/tests/network/netfunctestlib.py
File tests/network/netfunctestlib.py:
Line 17: #
Line 18: # Refer to the README and COPYING files for full details of the license
Line 19: #
Line 20:
Line 21: from __future__ import absolute_import
> empty line after __future__
Done
Line 22: from copy import deepcopy
Line 23: import six
Line 24:
Line 25: import vdsm.config
Line 89: def assertHostQos(self, netname, netattrs):
Line 90: network_caps = self.netinfo.networks[netname]
Line 91: if 'hostQos' in netattrs:
Line 92: qos_caps = _normalize_qos_config(network_caps['hostQos'])
Line 93: self.assertEqual(netattrs['hostQos'], qos_caps)
> don't we have to normalize netattrs as well?
netattr is the requested config, we need to compare the request with the caps report.
The normalization here is to translate the caps to the same syntax as the request.
Line 94:
Line 95: def assertNetworkExists(self, netname):
Line 96: self.assertIn(netname, self.netinfo.networks)
Line 97:
Line 104: network_caps = self.netinfo.networks[netname]
Line 105: self.assertFalse(network_caps['bridged'])
Line 106: self.assertNotIn(netname, self.netinfo.bridges)
Line 107:
Line 108: def assertSouthboundIface(self, netname, netattrs):
> i'd call it iface. in case of OVS, vlan is northbound, southbound ifaces ar
I remind you that these are the API level tests.
SB is the iface/port to the external 'world', northbound is the iface/port/network to the host stack.
In the CAPS, the network 'iface' key represents the SB. Am, I wrong here?
In OVS a vlan is not a virtual device, it's an attribute of a port, but in caps we fake it... we it becomes a SB.
Line 109: nic = netattrs.get('nic')
Line 110: bond = netattrs.get('bonding')
Line 111: vlan = netattrs.get('vlan')
Line 112: bridged = netattrs.get('bridged', True)
Line 206: NETSETUP = {net: {'remove': True}
Line 207: for net in self.setup_networks if net in networks_caps}
Line 208: status, msg = self.vdsm_proxy.setupNetworks(NETSETUP, {}, NOCHK)
Line 209: if type is None and status != SUCCESS:
Line 210: raise SetupNetworksError(msg)
> we should raise an exception if type is not None. __exit__ suppresses all e
__exit__ does not suppress exceptions, unless you intentionally want to do it.
https://docs.python.org/3/whatsnew/2.6.html#writing-context-managers
Line 211:
Line 212:
Line 213: def _normalize_caps(netinfo_from_caps):
Line 214: """
Line 218: To allow the kernel vs running comparison, it is required to revert the
Line 219: caps data compatibility conversions (required by the oVirt Engine).
Line 220: """
Line 221: netinfo = deepcopy(netinfo_from_caps)
Line 222: # When production code drops compatibility normalization, remove this.
> add TODO here. can we drop mtu normalization now? (in another patch)
OK. I think we can drop it in a separated patch.
Line 223: for dev in six.itervalues(netinfo.networks):
Line 224: dev['mtu'] = int(dev['mtu'])
Line 225:
Line 226: return netinfo
Line 226: return netinfo
Line 227:
Line 228:
Line 229: def _normalize_qos_config(qos):
Line 230: for key, value in qos.items():
> six.itervalues(qos)
items() is compatible with py3 and I am in favor of avoiding six when possible.
I would say this is an over optimization.
(and the next)
Line 231: for curve, attrs in value.items():
Line 232: if attrs.get('m1') == 0:
Line 233: del attrs['m1']
Line 234: if attrs.get('d') == 0:
--
To view, visit https://gerrit.ovirt.org/58329
To unsubscribe, visit https://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I4645756c575f34cabf9536ef663ffc8009655abb
Gerrit-PatchSet: 10
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Edward Haas <edwardh(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Edward Haas <edwardh(a)redhat.com>
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Ondřej Svoboda <osvoboda(a)redhat.com>
Gerrit-Reviewer: Petr Horáček <phoracek(a)redhat.com>
Gerrit-Reviewer: gerrit-hooks <automation(a)ovirt.org>
Gerrit-HasComments: Yes
7 years, 11 months
Change in vdsm[master]: rpc: Use Suppressed class instead of logging workarounds
by automation＠ovirt.org
gerrit-hooks has posted comments on this change.
Change subject: rpc: Use Suppressed class instead of logging workarounds
......................................................................
Patch Set 5:
* Update tracker: IGNORE, no Bug-Url found
* Set MODIFIED::IGNORE, no Bug-Url found.
--
To view, visit https://gerrit.ovirt.org/59078
To unsubscribe, visit https://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Idaf00e557fccb8f08fa3aeb38d51cb4bbe0ffe53
Gerrit-PatchSet: 5
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Milan Zamazal <mzamazal(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com>
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Milan Zamazal <mzamazal(a)redhat.com>
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com>
Gerrit-Reviewer: Piotr Kliczewski <piotr.kliczewski(a)gmail.com>
Gerrit-Reviewer: gerrit-hooks <automation(a)ovirt.org>
Gerrit-HasComments: No
7 years, 11 months
Change in vdsm[master]: rpc: Use Suppressed class instead of logging workarounds
by Nir Soffer
Nir Soffer has submitted this change and it was merged.
Change subject: rpc: Use Suppressed class instead of logging workarounds
......................................................................
rpc: Use Suppressed class instead of logging workarounds
We have got logging workarounds for getAllVmStats RPC calls.
The workarounds are there to suppress logging large data regularly.
We'd like to get rid of the workarounds, it's better to handle the large
data in the API functions, where we can use specific handling of each
kind of large data.
This patch introduces a wrapper for large values, with the purpose of
suppressing writing them to the log. The wrapper is used for
getAllVmStats so that we get rid of the logging workarounds.
There are other API calls that may produce large outputs, we will handle
them in followup patches.
Change-Id: Idaf00e557fccb8f08fa3aeb38d51cb4bbe0ffe53
Signed-off-by: Milan Zamazal <mzamazal(a)redhat.com>
Backport-To: 4.0
Backport-To: 3.6
Reviewed-on: https://gerrit.ovirt.org/59078
Continuous-Integration: Jenkins CI
Reviewed-by: Nir Soffer <nsoffer(a)redhat.com>
---
M lib/api/vdsmapi.py
M lib/vdsm/logUtils.py
M lib/vdsm/rpc/bindingxmlrpc.py
M lib/yajsonrpc/__init__.py
M vdsm/API.py
5 files changed, 30 insertions(+), 10 deletions(-)
Approvals:
Nir Soffer: Looks good to me, approved
Jenkins CI: Passed CI tests
Milan Zamazal: Verified
--
To view, visit https://gerrit.ovirt.org/59078
To unsubscribe, visit https://gerrit.ovirt.org/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: Idaf00e557fccb8f08fa3aeb38d51cb4bbe0ffe53
Gerrit-PatchSet: 5
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Milan Zamazal <mzamazal(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com>
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Milan Zamazal <mzamazal(a)redhat.com>
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com>
Gerrit-Reviewer: Piotr Kliczewski <piotr.kliczewski(a)gmail.com>
Gerrit-Reviewer: gerrit-hooks <automation(a)ovirt.org>
7 years, 11 months
Change in vdsm[master]: rpc: Use Suppressed class instead of logging workarounds
by mzamazal＠redhat.com
Milan Zamazal has posted comments on this change.
Change subject: rpc: Use Suppressed class instead of logging workarounds
......................................................................
Patch Set 4: Verified+1
Verified that:
- (suppressed) is logged for getAllVmStats, for both jsonrpc and xmlrpc.
- There are no false reported issues with response verification.
- I can start a VM and its status is reported correctly.
--
To view, visit https://gerrit.ovirt.org/59078
To unsubscribe, visit https://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Idaf00e557fccb8f08fa3aeb38d51cb4bbe0ffe53
Gerrit-PatchSet: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Milan Zamazal <mzamazal(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com>
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Milan Zamazal <mzamazal(a)redhat.com>
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com>
Gerrit-Reviewer: Piotr Kliczewski <piotr.kliczewski(a)gmail.com>
Gerrit-Reviewer: gerrit-hooks <automation(a)ovirt.org>
Gerrit-HasComments: No
7 years, 11 months
Change in vdsm[master]: ppc64hwinfo: refactor getHardwareInfoStructure
by Martin Polednik
Martin Polednik has posted comments on this change.
Change subject: ppc64hwinfo: refactor getHardwareInfoStructure
......................................................................
Patch Set 4:
(1 comment)
https://gerrit.ovirt.org/#/c/57751/4/lib/vdsm/cpuinfo.py
File lib/vdsm/cpuinfo.py:
Line 44: fields = {}
Line 45:
Line 46: if cpuarch.is_ppc(cpuarch.real()):
Line 47: fields['flags'] = ['powernv']
Line 48: elif cpuarch.is_x86(cpuarch.real()):
> why this?
To properly "document" that the two platforms are mutually exclusive. Should be omitted from the patch.
Line 49: fields['platform'] = 'unavailable'
Line 50: fields['machine'] = 'unavailable'
Line 51: fields['ppcmodel'] = 'unavailable'
Line 52:
--
To view, visit https://gerrit.ovirt.org/57751
To unsubscribe, visit https://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I0245925f5cf3a0a2d384fdff0f5701b312b336b1
Gerrit-PatchSet: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Martin Polednik <mpolednik(a)redhat.com>
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com>
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik <mpolednik(a)redhat.com>
Gerrit-Reviewer: Milan Zamazal <mzamazal(a)redhat.com>
Gerrit-Reviewer: gerrit-hooks <automation(a)ovirt.org>
Gerrit-HasComments: Yes
7 years, 11 months
Change in vdsm[master]: ppc64hwinfo: refactor getHardwareInfoStructure
by fromani＠redhat.com
Francesco Romani has posted comments on this change.
Change subject: ppc64hwinfo: refactor getHardwareInfoStructure
......................................................................
Patch Set 4: Code-Review-1
(1 comment)
-1 for visibility because of an inline question
https://gerrit.ovirt.org/#/c/57751/4/lib/vdsm/cpuinfo.py
File lib/vdsm/cpuinfo.py:
Line 44: fields = {}
Line 45:
Line 46: if cpuarch.is_ppc(cpuarch.real()):
Line 47: fields['flags'] = ['powernv']
Line 48: elif cpuarch.is_x86(cpuarch.real()):
why this?
Line 49: fields['platform'] = 'unavailable'
Line 50: fields['machine'] = 'unavailable'
Line 51: fields['ppcmodel'] = 'unavailable'
Line 52:
--
To view, visit https://gerrit.ovirt.org/57751
To unsubscribe, visit https://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I0245925f5cf3a0a2d384fdff0f5701b312b336b1
Gerrit-PatchSet: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Martin Polednik <mpolednik(a)redhat.com>
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com>
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik <mpolednik(a)redhat.com>
Gerrit-Reviewer: Milan Zamazal <mzamazal(a)redhat.com>
Gerrit-Reviewer: gerrit-hooks <automation(a)ovirt.org>
Gerrit-HasComments: Yes
7 years, 11 months
Change in vdsm[master]: ppc64hwinfo: extend cpuinfo to platform and machine
by fromani＠redhat.com
Francesco Romani has posted comments on this change.
Change subject: ppc64hwinfo: extend cpuinfo to platform and machine
......................................................................
Patch Set 3:
(1 comment)
https://gerrit.ovirt.org/#/c/57750/3/lib/vdsm/cpuinfo.py
File lib/vdsm/cpuinfo.py:
PS3, Line 126: ppcmodel
> Very different.
Thanks for the context. Makes sense.
--
To view, visit https://gerrit.ovirt.org/57750
To unsubscribe, visit https://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I9a18b6ba49c9d0a6199dad436defae39c20f0cc5
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Martin Polednik <mpolednik(a)redhat.com>
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com>
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik <mpolednik(a)redhat.com>
Gerrit-Reviewer: Milan Zamazal <mzamazal(a)redhat.com>
Gerrit-Reviewer: gerrit-hooks <automation(a)ovirt.org>
Gerrit-HasComments: Yes
7 years, 11 months
Change in vdsm[master]: rpc: Log important info from VM stats
by michal.skrivanek＠redhat.com
Michal Skrivanek has posted comments on this change.
Change subject: rpc: Log important info from VM stats
......................................................................
Patch Set 17:
(1 comment)
https://gerrit.ovirt.org/#/c/58465/17/vdsm/API.py
File vdsm/API.py:
Line 1344: """
Line 1345: hooks.before_get_all_vm_stats()
Line 1346: statsList = self._cif.getAllVmStats()
Line 1347: statsList = hooks.after_get_all_vm_stats(statsList)
Line 1348: self._allvmstats_log.log(self.log, logging.DEBUG,
> Probably because the hook can modify statsList
indeed. that's the purpose of that hook. We have similar issue in e.g. create flow where hooks are often modifying the domain xml heavily, so we dump it right before sending to libvirt
Line 1349: "Current getAllVmStats: %s",
Line 1350: AllVmStatsValue(statsList))
Line 1351: return {'status': doneCode, 'statsList': Suppressed(statsList)}
Line 1352:
--
To view, visit https://gerrit.ovirt.org/58465
To unsubscribe, visit https://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ifcbac615323b62fb9a27e5c0f5a4e98990076146
Gerrit-PatchSet: 17
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Milan Zamazal <mzamazal(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com>
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Michal Skrivanek <michal.skrivanek(a)redhat.com>
Gerrit-Reviewer: Milan Zamazal <mzamazal(a)redhat.com>
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com>
Gerrit-Reviewer: Piotr Kliczewski <piotr.kliczewski(a)gmail.com>
Gerrit-Reviewer: gerrit-hooks <automation(a)ovirt.org>
Gerrit-HasComments: Yes
7 years, 11 months
Change in vdsm[master]: ppc64hwinfo: refactor _getFromDeviceTree
by fromani＠redhat.com
Francesco Romani has posted comments on this change.
Change subject: ppc64hwinfo: refactor _getFromDeviceTree
......................................................................
Patch Set 3: Code-Review-1
-1 for visibility
please don't forget the TODO I mentioned in my previous comment
--
To view, visit https://gerrit.ovirt.org/57749
To unsubscribe, visit https://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Id065362e54bf004281b2f84593a8b0fd2f8e90b1
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Martin Polednik <mpolednik(a)redhat.com>
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com>
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik <mpolednik(a)redhat.com>
Gerrit-Reviewer: Milan Zamazal <mzamazal(a)redhat.com>
Gerrit-Reviewer: gerrit-hooks <automation(a)ovirt.org>
Gerrit-HasComments: No
7 years, 11 months