Change in vdsm[master]: netinfo: Do not mix local and universal time, improve tests

Ondřej Svoboda has uploaded a new change for review.

Change subject: netinfo: Do not mix local and universal time, improve tests
......................................................................

netinfo: Do not mix local and universal time, improve tests

Also go back to using dhclient's --bind-interfaces option
to stay compatible with RHEL 6.

Change-Id: I1b2a33209b45001c427d5712d57326394cbeb9d0
Bug-Url: https://bugzilla.redhat.com/987813
Signed-off-by: Ondřej Svoboda 
---
M lib/vdsm/netinfo.py
M tests/functional/dhcp.py
M tests/functional/networkTests.py
M tests/netinfoTests.py
4 files changed, 37 insertions(+), 21 deletions(-)

git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/92/24192/1

diff --git a/lib/vdsm/netinfo.py b/lib/vdsm/netinfo.py
index 98e2462..dbacb2f 100644
--- a/lib/vdsm/netinfo.py
+++ b/lib/vdsm/netinfo.py
@@ -557,7 +557,7 @@

if expiryTime.startswith(EPOCH):
         since_epoch = expiryTime[len(EPOCH):]
-        return datetime.fromtimestamp(float(since_epoch))
+        return datetime.utcfromtimestamp(float(since_epoch))

else:
         return datetime.strptime(expiryTime, '%w %Y/%m/%d %H:%M:%S')
@@ -583,7 +583,7 @@
                     continue  # the line should always contain a ;

expiryTime = _parseExpiryTime(line[len(EXPIRE):end])
-                if datetime.now() > expiryTime:
+                if datetime.utcnow() > expiryTime:
                     insideLease = False
                     continue

diff --git a/tests/functional/dhcp.py b/tests/functional/dhcp.py
index 717b80a..3bd200a 100644
--- a/tests/functional/dhcp.py
+++ b/tests/functional/dhcp.py
@@ -26,7 +26,8 @@
 from time import sleep

_DNSMASQ_BINARY = CommandPath('dnsmasq', '/usr/sbin/dnsmasq')
-_DHCLIENT_BINARY = CommandPath('dhclient', '/usr/sbin/dhclient')
+_DHCLIENT_BINARY = CommandPath('dhclient', '/usr/sbin/dhclient',
+                               '/sbin/dhclient')
 _NM_CLI_BINARY = CommandPath('nmcli', '/usr/bin/nmcli')
 _START_CHECK_TIMEOUT = 0.5
 _DHCLIENT_TIMEOUT = 10
@@ -48,7 +49,7 @@
                             '-p', '0', '--dhcp-range=' + dhcpRangeFrom + ',' +
                             dhcpRangeTo + ',2m', '--dhcp-option=3', '-k',
                             '-i', interface, '-I', 'lo', '-d',
-                            '--bind-dynamic'], sync=False)
+                            '--bind-interfaces'], sync=False)
         sleep(_START_CHECK_TIMEOUT)
         if self.proc.returncode:
             raise DhcpError('Failed to start dnsmasq DHCP server.' +
@@ -59,11 +60,11 @@
         logging.debug(''.join(self.proc.stderr))

-def runDhclient(interface, leaseFile, pidFile):
+def runDhclient(interface, confFile, leaseFile, pidFile):
     """Starts dhclient and hands the process over after a while."""
     rc, out, err = execCmd([_DHCLIENT_BINARY.cmd, '-d', '-lf', leaseFile,
                             '-pf', pidFile, '-timeout', str(_DHCLIENT_TIMEOUT),
-                            '-1', interface])
+                            '-1', '-cf', confFile, interface])

if rc:  # == 2
         logging.debug(''.join(err))
diff --git a/tests/functional/networkTests.py b/tests/functional/networkTests.py
index 5484e99..d493192 100644
--- a/tests/functional/networkTests.py
+++ b/tests/functional/networkTests.py
@@ -1686,10 +1686,11 @@
                 self.assertEqual(status, SUCCESS, msg)
                 self.assertNetworkDoesntExist(NETWORK_NAME)

+    @permutations([['default'], ['local']])
     @cleanupNet
     @RequireVethMod
     @ValidateRunningAsRoot
-    def testDhclientLeases(self):
+    def testDhclientLeases(self, dateFormat=False):
         dhcp4 = set()
         with vethIf() as (server, client):
             with avoidAnotherDhclient(client):
@@ -1700,10 +1701,14 @@
                 with dnsmasqDhcp(server):

with namedTemporaryDir(dir='/var/lib/dhclient') as tmpDir:
+                        confFile = os.path.join(tmpDir, 'test.conf')
                         leaseFile = os.path.join(tmpDir, 'test.lease')
                         pidFile = os.path.join(tmpDir, 'test.pid')

-                        dhcp.runDhclient(client, leaseFile, pidFile)
+                        with open(confFile, 'w') as f:
+                            f.write('db-time-format {0};'.format(dateFormat))
+
+                        dhcp.runDhclient(client, confFile, leaseFile, pidFile)
                         dhcp4 = getDhclientIfaces([leaseFile])

self.assertIn(client, dhcp4, 'Test iface not found in a lease file.')
diff --git a/tests/netinfoTests.py b/tests/netinfoTests.py
index cfdf107..4ad79dc 100644
--- a/tests/netinfoTests.py
+++ b/tests/netinfoTests.py
@@ -19,9 +19,11 @@
 # Refer to the README and COPYING files for full details of the license
 #
 import os
+from datetime import datetime
 from functools import partial
 from shutil import rmtree
 import tempfile
+import time
 from xml.dom import minidom

import ethtool
@@ -295,28 +297,36 @@

def testGetDhclientIfaces(self):
         LEASES = (
-            'lease {\n'
+            'lease {{\n'
             '  interface "valid";\n'
-            '  expire 3 2037/01/29 18:25:46;\n'
-            '}\n'
-            'lease {\n'
+            '  expire {0:%w %Y/%m/%d %H:%M:%S};\n'
+            '}}\n'
+            'lease {{\n'
             '  interface "valid2";\n'
-            '  expire epoch 2117041460; # Sat Jan 31 20:04:20 2037\n'
-            '}\n'
-            'lease {\n'
+            '  expire epoch {1:.0f};\n'
+            '}}\n'
+            'lease {{\n'
             '  interface "expired";\n'
-            '  expire 3 2014/01/29 18:25:46;\n'
-            '}\n'
-            'lease {\n'
+            '  expire {2:%w %Y/%m/%d %H:%M:%S};\n'
+            '}}\n'
+            'lease {{\n'
             '  interface "expired2";\n'
-            '  expire epoch 1391195060; # Fri Jan 31 20:04:20 2014\n'
-            '}\n'
+            '  expire epoch {3:.0f};\n'
+            '}}\n'
         )

with namedTemporaryDir() as tmpDir:
             leaseFile = os.path.join(tmpDir, 'test.lease')
             with open(leaseFile, 'w') as f:
-                f.write(LEASES)
+                lastMinute = time.time() - 60
+                nextMinute = time.time() + 60
+
+                f.write(LEASES.format(
+                    datetime.utcfromtimestamp(nextMinute),
+                    nextMinute,
+                    datetime.utcfromtimestamp(lastMinute),
+                    lastMinute
+                ))

dhcp4 = getDhclientIfaces([leaseFile])

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: netinfo: Do not mix local and universal time, improve tests
......................................................................

Patch Set 1:

Build Successful

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit_el/6234/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit/7124/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/7013/ : SUCCESS

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: netinfo: Do not mix local and universal time, improve tests
......................................................................

Patch Set 2:

Build Successful

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit_el/6235/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit/7125/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/7014/ : SUCCESS

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: netinfo: Do not mix local and universal time, improve tests
......................................................................

Patch Set 3:

Build Successful

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit_el/6236/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit/7126/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/7015/ : SUCCESS

Ondřej Svoboda has posted comments on this change.

Change subject: netinfo: Do not mix local and universal time, improve tests
......................................................................

Patch Set 3: Verified+1

(1 comment)

http://gerrit.ovirt.org/#/c/24192/3/tests/functional/dhcp.py
File tests/functional/dhcp.py:

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 (it is only relevant in getting a lease for the first time).

I can live without dhclient's debug mode, can be found in the Journal.
Line 66:                             '-pf', pidFile, '-timeout', str(_DHCLIENT_TIMEOUT),
Line 67:                             '-1', '-cf', confFile, interface])
Line 68: 
Line 69:     if rc:  # == 2

[Dan Kenigsberg]

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:

Ondřej Svoboda has posted comments on this change.

Change subject: netinfo: Do not mix local and universal time, improve tests
......................................................................

Patch Set 3:

(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 tw

Okay, let us make the el6 patch standalone.
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 ex

We are still in the context of tests here :-)

I think it is useful to have functional tests simulate both date-time formats. We catch any future changes (though there should be none!) and more importantly, we mirror static tests in this regard, making sure both tests are correct now.
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,
> Sorry, I do not understand this comment or its motivation.

The '-d' switch was completely OK when running dhclient asynchronously and killing it by our means.

By observing this test hang I found out that once dhclient succesfully obtains a lease, it does not terminate itself nowadays. (Thankfully, the output was mirrored to the Journal.)

I am not sure how this behaviour was introduced and quite confident that the test indeed ended successfully before. The debug switch is unneeded and turned rogue by a combination of other parameters or I was blind.
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"

The value is indeed an 'old' one and was forgotten.

Would a default argument be needed at all? I can only issue ./run_tests.sh functional/networkTests.py:NetworkTest.testDhclientLeases(kwargs='local'), the argument cannot be omitted as far as I can see.

I was confused by testSetupNetworksAddDelBondedNetwork having an obligatory and testSetupNetworksAddOverExistingBond having an optional argument.
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?

Literal braces have to be escaped, otherwise .format() treats them as placeholders to replace.
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)?

Okay, since I caused confusion, this definitely needs a comment, though not inside the string. The comments I removed are not supposed to be parsed by a machine so generating them / leaving them unmodified would be just a different source of confusion.
Line 315:             '}}\n'
Line 316:         )
Line 317: 
Line 318:         with namedTemporaryDir() as tmpDir:

Change in vdsm[master]: netinfo: Do not mix local time and UTC, improve tests

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: netinfo: Do not mix local time and UTC, improve tests
......................................................................

Patch Set 4:

Build Successful

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit_el/6244/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit/7134/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/7023/ : SUCCESS

Change in vdsm[master]: netinfo: Do not mix local time and UTC, improve tests

Ondřej Svoboda has posted comments on this change.

Change subject: netinfo: Do not mix local time and UTC, improve tests
......................................................................

Patch Set 4: Verified+1

Tests succeed, albeit the suite takes really long to assemble and tear up on my (possibly broken) RHEL 6.5 (40 and 69 seconds). On Fedora 20, functional tests run in 6 seconds.

Change in vdsm[master]: netinfo: Do not mix local time and UTC, improve tests

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: netinfo: Do not mix local time and UTC, improve tests
......................................................................

Patch Set 4: Code-Review-1

(3 comments)

minor comment about comments remain.

http://gerrit.ovirt.org/#/c/24192/4/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):
You have convinced me that the test is useful: we test getDhclientIfaces(), and we want to make sure it works with both values of db-time-format.
Line 1694:         dhcp4 = set()
Line 1695:         with vethIf() as (server, client):
Line 1696:             with avoidAnotherDhclient(client):
Line 1697:

Line 1702: 
Line 1703:                     with namedTemporaryDir(dir='/var/lib/dhclient') as tmpDir:
Line 1704:                         confFile = os.path.join(tmpDir, 'test.conf')
Line 1705:                         leaseFile = os.path.join(tmpDir, 'test.lease')
Line 1706:                         pidFile = os.path.join(tmpDir, 'test.pid')
In a follow-up patch, please generate pidFile within dhcp.runDhclient() - we do not tweak it or care about it. it's a dhclient implementation detail.
Line 1707: 
Line 1708:                         with open(confFile, 'w') as f:
Line 1709:                             f.write('db-time-format {0};'.format(dateFormat))
Line 1710:

http://gerrit.ovirt.org/#/c/24192/4/tests/netinfoTests.py
File tests/netinfoTests.py:

Line 301:             '  interface "valid";\n'
Line 302:             '  expire {0:%w %Y/%m/%d %H:%M:%S};\n'
Line 303:             '}}\n'
Line 304:             'lease {{\n'                # in an actual lease file,
Line 305:             '  interface "valid2";\n'   # human-readable date follows:
but to simulate an actual lease file, this comment should be inside the string.
Line 306:             '  expire epoch {1:.0f};\n'
Line 307:             #  expire epoch 2117041460; # Sat Jan 31 20:04:20 2037
Line 308:             '}}\n'
Line 309:             'lease {{\n'

Change in vdsm[master]: netinfo: Do not mix local time and UTC, improve tests

Ondřej Svoboda has posted comments on this change.

Change subject: netinfo: Do not mix local time and UTC, improve tests
......................................................................

Patch Set 4:

(2 comments)

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

Line 1702: 
Line 1703:                     with namedTemporaryDir(dir='/var/lib/dhclient') as tmpDir:
Line 1704:                         confFile = os.path.join(tmpDir, 'test.conf')
Line 1705:                         leaseFile = os.path.join(tmpDir, 'test.lease')
Line 1706:                         pidFile = os.path.join(tmpDir, 'test.pid')
> In a follow-up patch, please generate pidFile within dhcp.runDhclient() - w

dhcp.runDhclient() is tightly bound to its only caller. If pidFile did not matter, I would make it an optional argument. But it is important to avoid a possible collision with another dhclient. If it was running, our own might refuse to run. Isolating any files dhclient uses is a good idea and working with them in one place is good for consistency.

Not passing the file name would require passing the path anyway. The minimum would be to pass the working path and the path to the lease file. Or refactoring the whole: move creating a tmpdir to dhcp.runDhclient and making it return a path to a lease file. But how would I clean up?
Line 1707: 
Line 1708:                         with open(confFile, 'w') as f:
Line 1709:                             f.write('db-time-format {0};'.format(dateFormat))
Line 1710:

http://gerrit.ovirt.org/#/c/24192/4/tests/netinfoTests.py
File tests/netinfoTests.py:

Line 301:             '  interface "valid";\n'
Line 302:             '  expire {0:%w %Y/%m/%d %H:%M:%S};\n'
Line 303:             '}}\n'
Line 304:             'lease {{\n'                # in an actual lease file,
Line 305:             '  interface "valid2";\n'   # human-readable date follows:
> but to simulate an actual lease file, this comment should be inside the str

Would it be acceptable to include these fixed comments in the string and make a comment outside explaining that they are mere examples?

An explanation is needed in any case, even when I would put real-looking locale-aware dates in comments, this time to say they aren't actually used...

I commented on the previous patch that the comments are not meant for a machine anyway...

Quoting man 5 dhclient.conf:

"The # symbol supplies a comment that describes what actual time this is as according to the system's configured timezone, at the time the value was written.  It is provided only for human inspection, the epoch time is the only recommended value for machine inspection."

"# %a %b %d %H:%M:%S %Y" is used as a format string in dhclient's source code.
Line 306:             '  expire epoch {1:.0f};\n'
Line 307:             #  expire epoch 2117041460; # Sat Jan 31 20:04:20 2037
Line 308:             '}}\n'
Line 309:             'lease {{\n'

Change in vdsm[master]: netinfo: Do not mix local time and UTC, improve tests

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: netinfo: Do not mix local time and UTC, improve tests
......................................................................

Patch Set 5: Verified-1

Build Failed

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit_el/6260/ : FAILURE

http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/7039/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit/7150/ : FAILURE

Change in vdsm[master]: netinfo: Do not mix local time and UTC, improve tests

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: netinfo: Do not mix local time and UTC, improve tests
......................................................................

Patch Set 6: Verified-1

Build Failed

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit_el/6262/ : FAILURE

http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/7041/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit/7152/ : FAILURE

Change in vdsm[master]: netinfo: Do not mix local time and UTC, improve tests

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: netinfo: Do not mix local time and UTC, improve tests
......................................................................

Patch Set 7:

Build Successful

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit_el/6264/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/7043/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit/7154/ : SUCCESS

Change in vdsm[master]: netinfo: Do not mix local time and UTC, improve tests

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: netinfo: Do not mix local time and UTC, improve tests
......................................................................

Patch Set 7: Code-Review+1

Change in vdsm[master]: netinfo: Do not mix local time and UTC, improve tests

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: netinfo: Do not mix local time and UTC, improve tests
......................................................................

Patch Set 4:

(2 comments)

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

Line 1702: 
Line 1703:                     with namedTemporaryDir(dir='/var/lib/dhclient') as tmpDir:
Line 1704:                         confFile = os.path.join(tmpDir, 'test.conf')
Line 1705:                         leaseFile = os.path.join(tmpDir, 'test.lease')
Line 1706:                         pidFile = os.path.join(tmpDir, 'test.pid')
> dhcp.runDhclient() is tightly bound to its only caller. If pidFile did not

I preferred to keep this refactoring to a follow up patch but I think we can let the current version be.
Line 1707: 
Line 1708:                         with open(confFile, 'w') as f:
Line 1709:                             f.write('db-time-format {0};'.format(dateFormat))
Line 1710:

http://gerrit.ovirt.org/#/c/24192/4/tests/netinfoTests.py
File tests/netinfoTests.py:

Line 301:             '  interface "valid";\n'
Line 302:             '  expire {0:%w %Y/%m/%d %H:%M:%S};\n'
Line 303:             '}}\n'
Line 304:             'lease {{\n'                # in an actual lease file,
Line 305:             '  interface "valid2";\n'   # human-readable date follows:
> Would it be acceptable to include these fixed comments in the string and ma

Yeah, a static comment is good.
Line 306:             '  expire epoch {1:.0f};\n'
Line 307:             #  expire epoch 2117041460; # Sat Jan 31 20:04:20 2037
Line 308:             '}}\n'
Line 309:             'lease {{\n'

Change in vdsm[master]: netinfo: Do not mix local time and UTC, improve tests

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: netinfo: Do not mix local time and UTC, improve tests
......................................................................

Patch Set 7: Code-Review+2

Change in vdsm[master]: netinfo: Do not mix local time and UTC, improve tests

Ondřej Svoboda has posted comments on this change.

Change subject: netinfo: Do not mix local time and UTC, improve tests
......................................................................

Patch Set 7: Verified+1

Static and functional tests pass on Fedora 20 and RHEL 6.5.

Change in vdsm[master]: netinfo: Do not mix local time and UTC, improve tests

[Dan Kenigsberg]

Dan Kenigsberg has submitted this change and it was merged.

Change subject: netinfo: Do not mix local time and UTC, improve tests
......................................................................

netinfo: Do not mix local time and UTC, improve tests

dhclient is run in the background so the functional test can
finish. Both date-time formats dhclient uses are now tested.

In the non-functional test, leases are created dynamically.

Change-Id: I1b2a33209b45001c427d5712d57326394cbeb9d0
Bug-Url: https://bugzilla.redhat.com/987813
Signed-off-by: Ondřej Svoboda 
Reviewed-on: http://gerrit.ovirt.org/24192
Reviewed-by: Dan Kenigsberg 
---
M lib/vdsm/netinfo.py
M tests/functional/dhcp.py
M tests/functional/networkTests.py
M tests/netinfoTests.py
4 files changed, 50 insertions(+), 26 deletions(-)

Approvals:
  Ondřej Svoboda: Verified
  Dan Kenigsberg: Looks good to me, approved