Change in vdsm[master]: Functional test for Multiple Gateways source routing

ibarkan at redhat.com ibarkan at redhat.com
Mon Oct 6 11:28:05 UTC 2014


Ido Barkan has posted comments on this change.

Change subject: Functional test for Multiple Gateways source routing
......................................................................


Patch Set 4:

(10 comments)

http://gerrit.ovirt.org/#/c/33612/4/tests/functional/dhcp.py
File tests/functional/dhcp.py:

Line 51:         self.proc = execCmd([
Line 52:             _DNSMASQ_BINARY.cmd, '--dhcp-authoritative',
Line 53:             '-p', '0', '--dhcp-range=' + dhcpRangeFrom + ',' +
Line 54:             dhcpRangeTo + ',2m',
Line 55:             '--dhcp-option=3,%s' % (router,) if router else '--dhcp-option=3',
> Consider
Done
Line 56:             '-O', '6',
Line 57:             '-i', interface, '-I', 'lo', '-d',
Line 58:             '--bind-interfaces'], sync=False)
Line 59:         sleep(_START_CHECK_TIMEOUT)


http://gerrit.ovirt.org/#/c/33612/4/tests/functional/networkTests.py
File tests/functional/networkTests.py:

Line 94:         dummy.remove(nic)
Line 95: 
Line 96: 
Line 97: @contextmanager
Line 98: def dnsmasqDhcp(interface, router=None):
> +1
Done
Line 99:     """Manages the life cycle of dnsmasq as a DHCP server."""
Line 100:     dhcpServer = dhcp.Dnsmasq()
Line 101:     try:
Line 102:         dhcpServer.start(interface, DHCP_RANGE_FROM, DHCP_RANGE_TO,


Line 94:         dummy.remove(nic)
Line 95: 
Line 96: 
Line 97: @contextmanager
Line 98: def dnsmasqDhcp(interface, router=None):
> Could you remind me why the new art is unavoidable? Cannot we always pass r
Done
Line 99:     """Manages the life cycle of dnsmasq as a DHCP server."""
Line 100:     dhcpServer = dhcp.Dnsmasq()
Line 101:     try:
Line 102:         dhcpServer.start(interface, DHCP_RANGE_FROM, DHCP_RANGE_TO,


Line 1938:                     self.assertIn(right, devs)
Line 1939:                     self.assertEqual(devs[right]['cfg']['BOOTPROTO'], 'dhcp')
Line 1940:                     device_name = right
Line 1941: 
Line 1942:                 device_ip_addr = netaddr.IPAddress(net['addr'])
> You're replicating logic from what code you're testing... Source routing co
Done
Line 1943:                 routing_table = str(device_ip_addr.value)
Line 1944:                 rule1 = Rule(table=routing_table,
Line 1945:                              destination=IP_NETWORK_AND_CIDR,
Line 1946:                              srcDevice=str(device_name))


Line 1939:                     self.assertEqual(devs[right]['cfg']['BOOTPROTO'], 'dhcp')
Line 1940:                     device_name = right
Line 1941: 
Line 1942:                 device_ip_addr = netaddr.IPAddress(net['addr'])
Line 1943:                 routing_table = str(device_ip_addr.value)
> You're repeating yourself here and in line 1656.
Done
Line 1944:                 rule1 = Rule(table=routing_table,
Line 1945:                              destination=IP_NETWORK_AND_CIDR,
Line 1946:                              srcDevice=str(device_name))
Line 1947:                 rule2 = Rule(table=routing_table,


Line 1946:                              srcDevice=str(device_name))
Line 1947:                 rule2 = Rule(table=routing_table,
Line 1948:                              source=IP_NETWORK_AND_CIDR)
Line 1949: 
Line 1950:                 # DHCP source routing configuration is async hence we wait
> Where's the patient wait? Didn't you need to use until.retry?
Done
Line 1951:                 # patiently
Line 1952:                 self.assertTrue(ruleExists(rule1))
Line 1953:                 self.assertTrue(ruleExists(rule2))
Line 1954:                 routes = [Route.fromText(r)


Line 1946:                              srcDevice=str(device_name))
Line 1947:                 rule2 = Rule(table=routing_table,
Line 1948:                              source=IP_NETWORK_AND_CIDR)
Line 1949: 
Line 1950:                 # DHCP source routing configuration is async hence we wait
> 'blockingdhcp' is set to True in line 1918. Very convenient for testing! Th
Done
Line 1951:                 # patiently
Line 1952:                 self.assertTrue(ruleExists(rule1))
Line 1953:                 self.assertTrue(ruleExists(rule2))
Line 1954:                 routes = [Route.fromText(r)


Line 1949: 
Line 1950:                 # DHCP source routing configuration is async hence we wait
Line 1951:                 # patiently
Line 1952:                 self.assertTrue(ruleExists(rule1))
Line 1953:                 self.assertTrue(ruleExists(rule2))
> Why not use routeExists?
Done
Line 1954:                 routes = [Route.fromText(r)
Line 1955:                           for r in routeShowTable(routing_table)]
Line 1956:                 default_route = Route('0.0.0.0/0', via=IP_GATEWAY,
Line 1957:                                       device=str(device_name))


Line 1961:                 self.assertIn(default_route, routes)
Line 1962:                 self.assertIn(specific_route, routes)
Line 1963: 
Line 1964:                 network = {NETWORK_NAME: {'remove': True}}
Line 1965:                 status, msg = self.vdsm_net.setupNetworks(network, {}, NOCHK)
> We really wanna check that the routes and rules are cleaned up properly. We
Done
Line 1966:                 self.assertEqual(status, SUCCESS, msg)
Line 1967:                 self.assertNetworkDoesntExist(NETWORK_NAME)
Line 1968: 
Line 1969:     @permutations([['default'], ['local']])


http://gerrit.ovirt.org/#/c/33612/4/tests/functional/utils.py
File tests/functional/utils.py:

Line 225:             result['info']
Line 226: 
Line 227:     def updateVmPolicy(self, vmId, vcpuLimit):
Line 228:         result = self.vdscli.updateVmPolicy([vmId, vcpuLimit])
Line 229:         return result['status']['code'], result['status']['message']
> Please avoid unrelated whitespace changes
Done


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ib510bb56c205dffc98aa2bae3f55c1b4c7f6f56f
Gerrit-PatchSet: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Ido Barkan <ibarkan at redhat.com>
Gerrit-Reviewer: Antoni Segura Puimedon <asegurap at redhat.com>
Gerrit-Reviewer: Assaf Muller <amuller at redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken at redhat.com>
Gerrit-Reviewer: Ido Barkan <ibarkan at redhat.com>
Gerrit-Reviewer: automation at ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes


More information about the vdsm-patches mailing list