Dan Kenigsberg has posted comments on this change.
Change subject: netinfo: Do not mix local and universal time, improve tests ......................................................................
Patch Set 3: Code-Review-1
(6 comments)
http://gerrit.ovirt.org/#/c/24192/3//COMMIT_MSG Commit Message:
Line 5: CommitDate: 2014-02-07 03:38:44 +0100 Line 6: Line 7: netinfo: Do not mix local and universal time, improve tests Line 8: Line 9: Also go back to using dhclient's --bind-interfaces option A commit message with "also" suggests that a patch should be broken into two.
el6 support deserves its own patch and commit message and testing. Line 10: to stay compatible with RHEL 6. Line 11: Line 12: Change-Id: I1b2a33209b45001c427d5712d57326394cbeb9d0 Line 13: Bug-Url: https://bugzilla.redhat.com/987813
http://gerrit.ovirt.org/#/c/24192/3/tests/functional/dhcp.py File tests/functional/dhcp.py:
Line 59: self.proc.kill() Line 60: logging.debug(''.join(self.proc.stderr)) Line 61: Line 62: Line 63: def runDhclient(interface, confFile, leaseFile, pidFile): I appreciate your extensive unit test, but I do not think that we should expose the confFile in the signature of our function.
As far as I understand, Vdsm has no interest in the specific datetime format within dhclient lease file - we need only one, and a one that works.
I am not sure at all that we need to tweak db-time-format. But if we do, the confFile should be created here. Line 64: """Starts dhclient and hands the process over after a while.""" Line 65: rc, out, err = execCmd([_DHCLIENT_BINARY.cmd, '-lf', leaseFile, Line 66: '-pf', pidFile, '-timeout', str(_DHCLIENT_TIMEOUT), Line 67: '-1', '-cf', confFile, interface])
Line 61: Line 62: Line 63: def runDhclient(interface, confFile, leaseFile, pidFile): Line 64: """Starts dhclient and hands the process over after a while.""" Line 65: rc, out, err = execCmd([_DHCLIENT_BINARY.cmd, '-lf', leaseFile,
Of course dhclient stays up after getting a lease even with a timeout set (
Sorry, I do not understand this comment or its motivation. Line 66: '-pf', pidFile, '-timeout', str(_DHCLIENT_TIMEOUT), Line 67: '-1', '-cf', confFile, interface]) Line 68: Line 69: if rc: # == 2
http://gerrit.ovirt.org/#/c/24192/3/tests/functional/networkTests.py File tests/functional/networkTests.py:
Line 1689: @permutations([['default'], ['local']]) Line 1690: @cleanupNet Line 1691: @RequireVethMod Line 1692: @ValidateRunningAsRoot Line 1693: def testDhclientLeases(self, dateFormat=False): Why have a default value of "False", when the to valid values is "default" and "local"? Line 1694: dhcp4 = set() Line 1695: with vethIf() as (server, client): Line 1696: with avoidAnotherDhclient(client): Line 1697:
http://gerrit.ovirt.org/#/c/24192/3/tests/netinfoTests.py File tests/netinfoTests.py:
Line 296: rmtree(tempDir) Line 297: Line 298: def testGetDhclientIfaces(self): Line 299: LEASES = ( Line 300: 'lease {{\n' Why is an additional brace is suddenly needed? Line 301: ' interface "valid";\n' Line 302: ' expire {0:%w %Y/%m/%d %H:%M:%S};\n' Line 303: '}}\n' Line 304: 'lease {{\n'
Line 310: ' expire {2:%w %Y/%m/%d %H:%M:%S};\n' Line 311: '}}\n' Line 312: 'lease {{\n' Line 313: ' interface "expired2";\n' Line 314: ' expire epoch {3:.0f};\n' wouldn't it be nicer to keep a comment (any comment, even a static one)? Line 315: '}}\n' Line 316: ) Line 317: Line 318: with namedTemporaryDir() as tmpDir: