openstack/neutron
Code Issues Proposed changes

Retry dhcp_release on failures

Browse Source 
Sometimes calls to dhcp_release(6) do not result in removal
of a lease from the leases file, for example, when the release
packet is not received by dnsmasq.  Trying more than once is
recommended in this case.

Instead of blindly trying some number of times, we monitor
the lease file contents, and retry the dhcp_release(6) call
when an entry still remains.  This is possible since
dhcp_release(6) is being run from the DHCP server itself.
We try three times and wait 0.3 seconds between tries.

We also now check for any stale leases in the leases file
that are unknown to neutron, also trying to remove them.

Change-Id: Ic1864f7efbc94db1369ac7f3e2879fda86f95a11
Closes-bug: #1764481
This commit is contained in:
Brian Haley 2018-04-11 21:22:23 -04:00
parent 2f3fb39fa2
commit fab032b426
2 changed files with 308 additions and 69 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

89
neutron/agent/linux/dhcp.py
View File

@ -58,6 +58,8 @@ METADATA_PORT = 80
WIN2k3_STATIC_DNS = 249
NS_PREFIX = 'qdhcp-'
DNSMASQ_SERVICE_NAME = 'dnsmasq'
DHCP_RELEASE_TRIES = 3
DHCP_RELEASE_TRIES_SLEEP = 0.3
class DictModel(dict):
@ -470,10 +472,10 @@ class Dnsmasq(DhcpLocalProcess):
"will not call it again.")
return self._IS_DHCP_RELEASE6_SUPPORTED
def _release_lease(self, mac_address, ip, client_id=None,
def _release_lease(self, mac_address, ip, ip_version, client_id=None,
server_id=None, iaid=None):
"""Release a DHCP lease."""
if netaddr.IPAddress(ip).version == constants.IP_VERSION_6:
if ip_version == constants.IP_VERSION_6:
if not self._is_dhcp_release6_supported():
return
cmd = ['dhcp_release6', '--iface', self.interface_name,
@ -754,15 +756,12 @@ class Dnsmasq(DhcpLocalProcess):
LOG.debug('Error while reading hosts file %s', filename)
return leases
def _read_v6_leases_file_leases(self, filename):
def _read_leases_file_leases(self, filename, ip_version=None):
"""
reading information from leases file which is needed to pass to
Read information from leases file, which is needed to pass to
dhcp_release6 command line utility if some of these leases are not
needed anymore
in this method ipv4 entries in leases file are ignored, as info in
hosts file is enough
each line in dnsmasq leases file is one of the following
* duid entry: duid server_duid
There MUST be single duid entry per file
@ -791,7 +790,8 @@ class Dnsmasq(DhcpLocalProcess):
dnsmasq-discuss/2016q2/010595.html
:param filename: leases file
:return: dict, keys are IPv6 addresses, values are dicts containing
:param ip_version: IP version of entries to return, or None for all
:return: dict, keys are IP(v6) addresses, values are dicts containing
iaid, client_id and server_id
"""
leases = {}
@ -812,7 +812,8 @@ class Dnsmasq(DhcpLocalProcess):
parts = l.strip().split()
(iaid, ip, client_id) = parts[1], parts[2], parts[4]
ip = ip.strip('[]')
if netaddr.IPAddress(ip).version == constants.IP_VERSION_4:
if (ip_version and
netaddr.IPAddress(ip).version != ip_version):
continue
leases[ip] = {'iaid': iaid,
'client_id': client_id,
@ -824,26 +825,68 @@ class Dnsmasq(DhcpLocalProcess):
filename = self.get_conf_file_name('host')
old_leases = self._read_hosts_file_leases(filename)
leases_filename = self.get_conf_file_name('leases')
# here is dhcpv6 stuff needed to craft dhcpv6 packet
v6_leases = self._read_v6_leases_file_leases(leases_filename)
cur_leases = self._read_leases_file_leases(leases_filename)
if not cur_leases:
return
v4_leases = set()
for (k, v) in cur_leases.items():
# IPv4 leases have a MAC, IPv6 ones do not, so we must ignore
if netaddr.IPAddress(k).version == constants.IP_VERSION_4:
# treat '*' as None, see note in _read_leases_file_leases()
client_id = v['client_id']
if client_id is '*':
client_id = None
v4_leases.add((k, v['iaid'], client_id))
new_leases = set()
for port in self.network.ports:
client_id = self._get_client_id(port)
for alloc in port.fixed_ips:
new_leases.add((alloc.ip_address, port.mac_address, client_id))
for ip, mac, client_id in old_leases - new_leases:
entry = v6_leases.get(ip, None)
version = netaddr.IPAddress(ip).version
if entry:
# must release IPv6 lease
self._release_lease(mac, ip, entry['client_id'],
entry['server_id'], entry['iaid'])
# must release only if v4 lease. If we have ipv6 address missing
# in old_leases, that means it's released already and nothing to do
# here
elif version == constants.IP_VERSION_4:
self._release_lease(mac, ip, client_id)
# If an entry is in the leases or host file(s), but doesn't have
# a fixed IP on a corresponding neutron port, consider it stale.
entries_to_release = (v4_leases | old_leases) - new_leases
if not entries_to_release:
return
# Try DHCP_RELEASE_TRIES times to release a lease, re-reading the
# file each time to see if it's still there. We loop +1 times to
# check the lease file one last time before logging any remaining
# entries.
for i in range(DHCP_RELEASE_TRIES + 1):
entries_not_present = set()
for ip, mac, client_id in entries_to_release:
try:
entry = cur_leases[ip]
except KeyError:
entries_not_present.add((ip, mac, client_id))
continue
# if not the final loop, try and release
if i < DHCP_RELEASE_TRIES:
ip_version = netaddr.IPAddress(ip).version
if ip_version == constants.IP_VERSION_6:
client_id = entry['client_id']
self._release_lease(mac, ip, ip_version, client_id,
entry['server_id'], entry['iaid'])
# Remove elements that were not in the current leases file,
# no need to look for them again, and see if we're done.
entries_to_release -= entries_not_present
if not entries_to_release:
break
if i < DHCP_RELEASE_TRIES:
time.sleep(DHCP_RELEASE_TRIES_SLEEP)
cur_leases = self._read_leases_file_leases(leases_filename)
if not cur_leases:
break
else:
LOG.warning("Could not release DHCP leases for these IP "
"addresses after %d tries: %s",
DHCP_RELEASE_TRIES,
', '.join(ip for ip, m, c in entries_to_release))
def _output_addn_hosts_file(self):
"""Writes a dnsmasq compatible additional hosts file.

288
neutron/tests/unit/agent/linux/test_dhcp.py
View File

@ -1915,18 +1915,26 @@ class TestDnsmasq(TestBase):
mac1 = '00:00:80:aa:bb:cc'
ip2 = '192.168.1.3'
mac2 = '00:00:80:cc:bb:aa'
ip3 = '0001:0002:0003:004:0005:0006:0007:0008'
ip3 = '0001:0002:0003:0004:0005:0006:0007:0008'
mac3 = '00:00:80:bb:aa:cc'
old_leases = {(ip1, mac1, None), (ip2, mac2, None), (ip3, mac3, None)}
dnsmasq._read_hosts_file_leases = mock.Mock(return_value=old_leases)
dnsmasq._read_v6_leases_file_leases = mock.Mock(
return_value={
'0001:0002:0003:004:0005:0006:0007:0008':
{'iaid': 0xff,
'client_id': 'client_id',
'server_id': 'server_id'}}
)
# Because the lease release code could fire multiple times, the
# second read of the lease file must not have the entries that
# would have been released.
dnsmasq._read_leases_file_leases = mock.Mock(
side_effect=[{ip1: {'iaid': mac1,
'client_id': 'client_id',
'server_id': 'server_id'},
ip2: {'iaid': mac2,
'client_id': 'client_id',
'server_id': 'server_id'},
ip3: {'iaid': 0xff,
'client_id': 'client_id',
'server_id': 'server_id'}
},
{}])
dnsmasq._output_hosts_file = mock.Mock()
dnsmasq._release_lease = mock.Mock()
@ -1935,12 +1943,16 @@ class TestDnsmasq(TestBase):
dnsmasq._release_unused_leases()
dnsmasq._release_lease.assert_has_calls([mock.call(mac1, ip1, None),
mock.call(mac2, ip2, None),
dnsmasq._release_lease.assert_has_calls([mock.call(mac1, ip1,
constants.IP_VERSION_4,
None, 'server_id', mac1),
mock.call(mac2, ip2,
constants.IP_VERSION_4,
None, 'server_id', mac2),
mock.call(mac3, ip3,
'client_id',
'server_id',
0xff),
constants.IP_VERSION_6,
'client_id', 'server_id',
0xff),
],
any_order=True)
@ -1952,17 +1964,24 @@ class TestDnsmasq(TestBase):
ip2 = '192.168.1.3'
mac2 = '00:00:80:cc:bb:aa'
old_leases = set([(ip1, mac1, None), (ip2, mac2, None)])
old_leases = set([(ip1, mac1, 'client_id'), (ip2, mac2, None)])
dnsmasq._read_hosts_file_leases = mock.Mock(return_value=old_leases)
dnsmasq._read_v6_leases_file_leases = mock.Mock(
return_value={'fdca:3ba5:a17a::1': {'iaid': 0xff,
'client_id': 'client_id',
'server_id': 'server_id'}
})
# Because the lease release code could fire multiple times, the
# second read of the lease file must not have the entries that
# would have been released.
dnsmasq._read_leases_file_leases = mock.Mock(
side_effect=[{ip1: {'iaid': 0xff,
'client_id': 'client_id',
'server_id': 'server_id'},
ip2: {'iaid': mac2,
'client_id': None,
'server_id': 'server_id'}
},
{}])
ipw = mock.patch(
'neutron.agent.linux.ip_lib.IpNetnsCommand.execute').start()
dnsmasq._release_unused_leases()
# Verify that dhcp_release is called both for ipv4 and ipv6 addresses.
# Verify that dhcp_release is called both for ipv4 and ipv6 addresses.
self.assertEqual(2, ipw.call_count)
ipw.assert_has_calls([mock.call(['dhcp_release6',
'--iface', None, '--ip', ip1,
@ -1981,7 +2000,7 @@ class TestDnsmasq(TestBase):
old_leases = set([(ip1, mac1, None)])
dnsmasq._read_hosts_file_leases = mock.Mock(return_value=old_leases)
dnsmasq._read_v6_leases_file_leases = mock.Mock(
dnsmasq._read_leases_file_leases = mock.Mock(
return_value={'fdca:3ba5:a17a::1': {'iaid': 0xff,
'client_id': 'client_id',
'server_id': 'server_id'}
@ -2003,7 +2022,7 @@ class TestDnsmasq(TestBase):
old_leases = set([(ip1, mac1, None), (ip2, mac2, None)])
dnsmasq._read_hosts_file_leases = mock.Mock(return_value=old_leases)
dnsmasq._read_v6_leases_file_leases = mock.Mock(
dnsmasq._read_leases_file_leases = mock.Mock(
return_value={ip6: {'iaid': 0xff,
'client_id': 'client_id',
'server_id': 'server_id'}
@ -2031,11 +2050,24 @@ class TestDnsmasq(TestBase):
old_leases = set([(ip1, mac1, client_id1), (ip2, mac2, client_id2)])
dnsmasq._read_hosts_file_leases = mock.Mock(return_value=old_leases)
dnsmasq._read_v6_leases_file_leases = mock.Mock(
return_value={ip6: {'iaid': 0xff,
# Because the lease release code could fire multiple times, the
# second read of the lease file must not have the entries that
# would have been released.
dnsmasq._read_leases_file_leases = mock.Mock(
side_effect=[{ip6: {'iaid': 0xff,
'client_id': 'client_id',
'server_id': 'server_id'},
ip1: {'iaid': mac1,
'client_id': client_id1,
'server_id': 'server_id'},
ip2: {'iaid': mac2,
'client_id': client_id2,
'server_id': 'server_id'}
},
{ip6: {'iaid': 0xff,
'client_id': 'client_id',
'server_id': 'server_id'}
})
}])
dnsmasq._output_hosts_file = mock.Mock()
dnsmasq._release_lease = mock.Mock()
dnsmasq.network.ports = []
@ -2043,8 +2075,10 @@ class TestDnsmasq(TestBase):
dnsmasq._release_unused_leases()
dnsmasq._release_lease.assert_has_calls(
[mock.call(mac1, ip1, client_id1),
mock.call(mac2, ip2, client_id2)],
[mock.call(mac1, ip1, constants.IP_VERSION_4, client_id1,
'server_id', mac1),
mock.call(mac2, ip2, constants.IP_VERSION_4, client_id2,
'server_id', mac2)],
any_order=True)
def test_release_unused_leases_one_lease(self):
@ -2058,11 +2092,21 @@ class TestDnsmasq(TestBase):
old_leases = set([(ip1, mac1, None), (ip2, mac2, None)])
dnsmasq._read_hosts_file_leases = mock.Mock(return_value=old_leases)
dnsmasq._read_v6_leases_file_leases = mock.Mock(
return_value={ip6: {'iaid': 0xff,
# Because the lease release code could fire multiple times, the
# second read of the lease file must not have the entries that
# would have been released.
dnsmasq._read_leases_file_leases = mock.Mock(
side_effect=[{ip6: {'iaid': 0xff,
'client_id': 'client_id',
'server_id': 'server_id'},
ip2: {'iaid': mac2,
'client_id': None,
'server_id': 'server_id'}
},
{ip6: {'iaid': 0xff,
'client_id': 'client_id',
'server_id': 'server_id'}
})
}])
dnsmasq._output_hosts_file = mock.Mock()
dnsmasq._release_lease = mock.Mock()
dnsmasq.network.ports = [FakePort1()]
@ -2070,7 +2114,7 @@ class TestDnsmasq(TestBase):
dnsmasq._release_unused_leases()
dnsmasq._release_lease.assert_called_once_with(
mac2, ip2, None)
mac2, ip2, constants.IP_VERSION_4, None, 'server_id', mac2)
def test_release_unused_leases_one_lease_with_client_id(self):
dnsmasq = self._get_dnsmasq(FakeDualNetwork())
@ -2086,18 +2130,138 @@ class TestDnsmasq(TestBase):
old_leases = set([(ip1, mac1, client_id1), (ip2, mac2, client_id2)])
dnsmasq._read_hosts_file_leases = mock.Mock(return_value=old_leases)
dnsmasq._output_hosts_file = mock.Mock()
dnsmasq._read_v6_leases_file_leases = mock.Mock(
return_value={ip6: {'iaid': 0xff,
# Because the lease release code could fire multiple times, the
# second read of the lease file must not have the entries that
# would have been released.
dnsmasq._read_leases_file_leases = mock.Mock(
side_effect=[{ip6: {'iaid': 0xff,
'client_id': 'client_id',
'server_id': 'server_id'},
ip1: {'iaid': mac1,
'client_id': client_id1,
'server_id': 'server_id'}
},
{ip6: {'iaid': 0xff,
'client_id': 'client_id',
'server_id': 'server_id'}
})
}])
dnsmasq._release_lease = mock.Mock()
dnsmasq.network.ports = [FakePort5()]
dnsmasq._release_unused_leases()
dnsmasq._release_lease.assert_called_once_with(
mac1, ip1, client_id1)
mac1, ip1, constants.IP_VERSION_4, client_id1, 'server_id', mac1)
def test_release_unused_leases_one_lease_from_leases_file(self):
# leases file has a stale entry that is not in the host file
dnsmasq = self._get_dnsmasq(FakeDualNetwork())
ip1 = '192.168.0.2'
mac1 = '00:00:80:aa:bb:cc'
ip2 = '192.168.0.3'
mac2 = '00:00:80:cc:bb:aa'
ip6 = '2001:0db8:11a3:09d7:1f34:8a2e:07a0:765d'
old_leases = set([(ip1, mac1, None)])
dnsmasq._read_hosts_file_leases = mock.Mock(return_value=old_leases)
# Because the lease release code could fire multiple times, the
# second read of the lease file must not have the entries that
# would have been released.
dnsmasq._read_leases_file_leases = mock.Mock(
side_effect=[{ip6: {'iaid': 0xff,
'client_id': 'client_id',
'server_id': 'server_id'},
ip2: {'iaid': mac2,
'client_id': None,
'server_id': 'server_id'}
},
{ip6: {'iaid': 0xff,
'client_id': 'client_id',
'server_id': 'server_id'}
}])
dnsmasq._output_hosts_file = mock.Mock()
dnsmasq._release_lease = mock.Mock()
dnsmasq.network.ports = [FakePort1()]
dnsmasq._release_unused_leases()
dnsmasq._release_lease.assert_called_once_with(
mac2, ip2, constants.IP_VERSION_4, None, 'server_id', mac2)
@mock.patch.object(dhcp.LOG, 'warn')
def _test_release_unused_leases_one_lease_mult_times(self, mock_log_warn,
removed):
# Simulate a dhcp_release failure where the lease remains in the
# lease file despite multiple dhcp_release calls
dnsmasq = self._get_dnsmasq(FakeDualNetwork())
ip1 = '192.168.0.2'
mac1 = '00:00:80:aa:bb:cc'
ip2 = '192.168.0.3'
mac2 = '00:00:80:cc:bb:aa'
ip6 = '2001:0db8:11a3:09d7:1f34:8a2e:07a0:765d'
old_leases = set([(ip1, mac1, None), (ip2, mac2, None)])
dnsmasq._read_hosts_file_leases = mock.Mock(return_value=old_leases)
# Because the lease release code could fire multiple times, the
# second and subsequent reads of the lease file must have the
# entries that were not released.
side_effect = [{ip6: {'iaid': 0xff,
'client_id': 'client_id',
'server_id': 'server_id'},
ip2: {'iaid': mac2,
'client_id': None,
'server_id': 'server_id'}
},
{ip6: {'iaid': 0xff,
'client_id': 'client_id',
'server_id': 'server_id'},
ip2: {'iaid': mac2,
'client_id': None,
'server_id': 'server_id'}
},
{ip6: {'iaid': 0xff,
'client_id': 'client_id',
'server_id': 'server_id'},
ip2: {'iaid': mac2,
'client_id': None,
'server_id': 'server_id'}
}]
# entry did/didn't go away after final dhcp_release try
if not removed:
side_effect.append(
{ip6: {'iaid': 0xff,
'client_id': 'client_id',
'server_id': 'server_id'},
ip2: {'iaid': mac2,
'client_id': None,
'server_id': 'server_id'}
})
else:
side_effect.append({})
dnsmasq._read_leases_file_leases = mock.Mock(side_effect=side_effect)
dnsmasq._output_hosts_file = mock.Mock()
dnsmasq._release_lease = mock.Mock()
dnsmasq.network.ports = [FakePort1()]
dnsmasq._release_unused_leases()
self.assertEqual(dhcp.DHCP_RELEASE_TRIES,
dnsmasq._release_lease.call_count)
self.assertEqual(dhcp.DHCP_RELEASE_TRIES + 1,
dnsmasq._read_leases_file_leases.call_count)
if not removed:
self.assertTrue(mock_log_warn.called)
def test_release_unused_leases_one_lease_mult_times_not_removed(self):
self._test_release_unused_leases_one_lease_mult_times(False)
def test_release_unused_leases_one_lease_mult_times_removed(self):
self._test_release_unused_leases_one_lease_mult_times(True)
def test_read_hosts_file_leases(self):
filename = '/path/to/file'
@ -2145,12 +2309,12 @@ class TestDnsmasq(TestBase):
("fdca:3ba5:a17a::1", "00:00:80:aa:bb:cc",
'client2')]), leases)
def test_read_v6_leases_file_leases(self):
def _test_read_leases_file_leases(self, ip_version):
filename = '/path/to/file'
lines = [
"1472673289 aa:bb:cc:00:00:01 192.168.1.2 host-192-168-1-2 *",
"1472673289 aa:bb:cc:00:00:01 192.168.1.3 host-192-168-1-3 *",
"1472673289 aa:bb:cc:00:00:01 192.168.1.4 host-192-168-1-4 *",
"1472673289 aa:bb:cc:00:00:02 192.168.1.2 host-192-168-1-2 *",
"1472673289 aa:bb:cc:00:00:03 192.168.1.3 host-192-168-1-3 *",
"1472673289 aa:bb:cc:00:00:04 192.168.1.4 host-192-168-1-4 *",
"duid 00:01:00:01:02:03:04:05:06:07:08:09:0a:0b",
"1472597740 1044800001 [2001:DB8::a] host-2001-db8--a "
"00:04:4a:d0:d2:34:19:2b:49:08:84:e8:34:bd:0c:dc:b9:3b",
@ -2164,14 +2328,13 @@ class TestDnsmasq(TestBase):
dnsmasq = self._get_dnsmasq(FakeDualNetwork())
with mock.patch('os.path.exists', return_value=True):
leases = dnsmasq._read_v6_leases_file_leases(filename)
leases = dnsmasq._read_leases_file_leases(filename, ip_version)
server_id = '00:01:00:01:02:03:04:05:06:07:08:09:0a:0b'
entry1 = {'iaid': '1044800001',
'client_id': '00:04:4a:d0:d2:34:19:2b:49:08:84:'
'e8:34:bd:0c:dc:b9:3b',
'server_id': server_id
}
entry2 = {'iaid': '1044800002',
'client_id': '00:04:ce:96:53:3d:f2:c2:4c:4c:81:'
'7d:db:c9:8d:d2:74:22:3b:0a',
@ -2182,13 +2345,46 @@ class TestDnsmasq(TestBase):
'7f:5c:33:31:37:5d:80:77:b4',
'server_id': server_id
}
expected = {'2001:DB8::a': entry1,
'2001:DB8::b': entry2,
'2001:DB8::c': entry3
}
v6_expected = {'2001:DB8::a': entry1,
'2001:DB8::b': entry2,
'2001:DB8::c': entry3
}
entry4 = {'iaid': 'aa:bb:cc:00:00:02',
'client_id': '*',
'server_id': None
}
entry5 = {'iaid': 'aa:bb:cc:00:00:03',
'client_id': '*',
'server_id': None
}
entry6 = {'iaid': 'aa:bb:cc:00:00:04',
'client_id': '*',
'server_id': None
}
v4_expected = {'192.168.1.2': entry4,
'192.168.1.3': entry5,
'192.168.1.4': entry6
}
expected = {}
if not ip_version or ip_version == constants.IP_VERSION_6:
expected.update(v6_expected)
if not ip_version or ip_version == constants.IP_VERSION_4:
expected.update(v4_expected)
self.assertEqual(expected, leases)
mock_open.assert_called_once_with(filename)
self.assertEqual(expected, leases)
def test_read_v6_leases_file_leases(self):
self._test_read_leases_file_leases(constants.IP_VERSION_6)
def test_read_v4_leases_file_leases(self):
self._test_read_leases_file_leases(constants.IP_VERSION_4)
def test_read_all_leases_file_leases(self):
self._test_read_leases_file_leases(None)
def test_make_subnet_interface_ip_map(self):
with mock.patch('neutron.agent.linux.ip_lib.IPDevice') as ip_dev: