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