Ido Barkan has posted comments on this change. Change subject: context manager for dhclient in functional tests ......................................................................
Patch Set 2: (7 comments) this still leaves dhcp tests flaky as before but cleans the dhclient residues. http://gerrit.ovirt.org/#/c/34366/2/tests/functional/dhcp.py File tests/functional/dhcp.py: Line 85: def _create_conf(self): Line 86: with open(self._conf_file, 'w') as f: Line 87: f.write('db-time-format {0};'.format(self._date_format)) Line 88: Line 89: def run(self): > Run is best use for starting a process and waiting for it. Here you seem to Done Line 90: rc, out, err = execCmd([_DHCLIENT_BINARY.cmd, '-v', '-lf', Line 91: self.lease_file, '-pf', self._pid_file, Line 92: '-timeout', str(_DHCLIENT_TIMEOUT), '-1', Line 93: '-cf', self._conf_file, self._interface]) Line 95: if rc: # == 2 Line 96: logging.debug(''.join(err)) Line 97: raise DhcpError('dhclient failed to obtain a lease: %d', rc) Line 98: Line 99: with open(self._pid_file) as pid_fd: > nit: strictly speaking, pid_fd is a file, not a file descriptor. Done Line 100: self.pid = int(pid_fd.read()) Line 101: Line 102: def kill(self): Line 103: if self.pid: Line 99: with open(self._pid_file) as pid_fd: Line 100: self.pid = int(pid_fd.read()) Line 101: Line 102: def kill(self): Line 103: if self.pid: > kill() should not be called before the process has been started. We should Done Line 104: try: Line 105: os.kill(self.pid, SIGKILL) Line 106: except OSError: Line 107: pass # No such process Line 103: if self.pid: Line 104: try: Line 105: os.kill(self.pid, SIGKILL) Line 106: except OSError: Line 107: pass # No such process > There are many other flavors of OSError that we would not like to ignore. P Done Line 108: Line 109: Line 110: @contextmanager Line 111: def dhclient(interface, tmpDir, dateFormat): Line 110: @contextmanager Line 111: def dhclient(interface, tmpDir, dateFormat): Line 112: dhclient_runner = _DhclientRunner(interface, tmpDir, dateFormat) Line 113: try: Line 114: dhclient_runner.run() > please take run() out of the try-block, as there's nothing to kill unless r Done, as part of Nir's tip (using running) Line 115: yield dhclient_runner Line 116: finally: Line 117: dhclient_runner.kill() Line 118: http://gerrit.ovirt.org/#/c/34366/2/tests/functional/networkTests.py File tests/functional/networkTests.py: Line 1991: with dnsmasqDhcp(server): Line 1992: Line 1993: with namedTemporaryDir(dir='/var/lib/dhclient') as dir: Line 1994: with dhcp.dhclient(client, dir, dateFormat) as \ Line 1995: _dh_client: > local variable do not require the _ prefix. Done Line 1996: dhcp4 = getDhclientIfaces([_dh_client.lease_file]) Line 1997: Line 1998: self.assertIn(client, dhcp4, 'Test iface not found in a lease file.') Line 1999: http://gerrit.ovirt.org/#/c/34366/2/tests/tcTests.py File tests/tcTests.py: Line 353: become active when the bridge is ready, and the bridge only becomes ready Line 354: when it is attached to an active device. Line 355: """ Line 356: Line 357: # ###[ Ethernet ]### > unrelated to this patch. Please rebase on master. Done Line 358: # dst = 00:1c:c0:d0:44:dc Line 359: # src = 00:21:5c:4d:42:75 Line 360: # type = IPv4 Line 361: # ###[ IP ]### -- To view, visit http://gerrit.ovirt.org/34366 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I458aa38415c697d3863e173444ff921d759166a2 Gerrit-PatchSet: 2 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Ido Barkan <[email protected]> Gerrit-Reviewer: Antoni Segura Puimedon <[email protected]> Gerrit-Reviewer: Dan Kenigsberg <[email protected]> Gerrit-Reviewer: Ido Barkan <[email protected]> Gerrit-Reviewer: Nir Soffer <[email protected]> Gerrit-Reviewer: [email protected] Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list [email protected] https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
