launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #10730
[Merge] lp:~jtv/maas/register-new-leases-in-omshell into lp:maas
Jeroen T. Vermeulen has proposed merging lp:~jtv/maas/register-new-leases-in-omshell into lp:maas.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
For more details, see:
https://code.launchpad.net/~jtv/maas/register-new-leases-in-omshell/+merge/118714
Pre-imped with Julian. Originally the plan was to have the MAAS server tell the worker to do this, whenever the worker made an update_leases request to the API. But there's really no point to the round-trip. Better to kick off the Omshell update and the update_leases from the same place. I had to build some worker-side retention logic for the omshell credentials (in another branch, already landed), but we need that logic for API credentials anyway. The Omshell registration is added in such a way that if it fails, the worker more or less forgets that it ever made the attempt. It will simply retry next time. This is different for the update_leases call, since that call is non-incremental — a call to update_leases does not care at all whether any previous calls failed or succeeded.
A lot of the work on this branch was actually the fight to get the fakes and such right in the tests: making sure that the Omshell doesn't really fork any processes, getting the right fake leases file back after simulating an update, and so on. I also unified the small bit of code that was repeated between update_leases and upload_leases into a new function process_leases, which affected several tests.
I found it a bit hard to get granularities right for all these interacting functions. Generally I wanted at least some integration testing across multiple levels of call, and most of these functions don't have much control flow to unit-test. So you'll see that in many tests, the call tree goes about as deep as it can before hitting a test double.
You'll also notice that send_leases is still empty. That's a separate card on the board.
Something that's clearly inefficient here is that on startup, the worker will re-register all its existing DHCP leases with omshell. If that turns out to be a problem we may have to get clever. But for now it's comforting to know that the algorithm is too stupid to be fooled.
Jeroen
--
https://code.launchpad.net/~jtv/maas/register-new-leases-in-omshell/+merge/118714
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~jtv/maas/register-new-leases-in-omshell into lp:maas.
=== modified file 'src/provisioningserver/dhcp/leases.py'
--- src/provisioningserver/dhcp/leases.py 2012-08-08 02:55:48 +0000
+++ src/provisioningserver/dhcp/leases.py 2012-08-08 11:10:25 +0000
@@ -101,9 +101,47 @@
recorded_leases = leases
+def identify_new_leases(current_leases):
+ """Return a dict of those leases that weren't previousl recorded.
+
+ :param current_leases: A dict mapping IP addresses to the respective
+ MAC addresses that own them.
+ """
+ if recorded_leases is None:
+ return current_leases
+ else:
+ return {
+ ip: current_leases[ip]
+ for ip in set(current_leases).difference(recorded_leases)}
+
+
+def register_new_leases(current_leases):
+ """Register new DHCP leases with the OMAPI.
+
+ :param current_leases: A dict mapping IP addresses to the respective
+ MAC addresses that own them.
+ """
+ # Avoid circular imports.
+ from provisioningserver.tasks import add_new_dhcp_host_map
+
+ omapi_key = recorded_omapi_shared_key
+ if omapi_key is not None:
+ new_leases = identify_new_leases(current_leases)
+ add_new_dhcp_host_map(new_leases, 'localhost', omapi_key)
+
+
def send_leases(leases):
- """Send snapshot of current leases to the MAAS server."""
- # TODO: Implement API call for uploading leases.
+ """Send lease updates to the server API."""
+
+
+def process_leases(timestamp, leases):
+ """Register leases with the DHCP server, and send to the MAAS server."""
+ # Register new leases before recording them. That way, any
+ # failure to register a lease will cause it to be retried at the
+ # next opportunity.
+ register_new_leases(leases)
+ record_lease_state(timestamp, leases)
+ send_leases(leases)
def upload_leases():
@@ -115,8 +153,7 @@
way to the DNS server.
"""
timestamp, leases = parse_leases_file()
- record_lease_state(timestamp, leases)
- send_leases(leases)
+ process_leases(timestamp, leases)
def update_leases():
@@ -129,5 +166,4 @@
updated_lease_info = check_lease_changes()
if updated_lease_info is not None:
timestamp, leases = updated_lease_info
- record_lease_state(timestamp, leases)
- send_leases(leases)
+ process_leases(timestamp, leases)
=== modified file 'src/provisioningserver/dhcp/tests/test_leases.py'
--- src/provisioningserver/dhcp/tests/test_leases.py 2012-08-08 03:14:10 +0000
+++ src/provisioningserver/dhcp/tests/test_leases.py 2012-08-08 11:10:25 +0000
@@ -28,12 +28,17 @@
from provisioningserver.dhcp import leases as leases_module
from provisioningserver.dhcp.leases import (
check_lease_changes,
+ identify_new_leases,
parse_leases_file,
+ process_leases,
record_lease_state,
record_omapi_shared_key,
+ register_new_leases,
update_leases,
upload_leases,
)
+from provisioningserver.omshell import Omshell
+from testtools.testcase import ExpectedException
class TestHelpers(TestCase):
@@ -156,12 +161,13 @@
record_lease_state(datetime.utcnow(), leases.copy())
self.assertIsNone(check_lease_changes())
- def test_update_leases_sends_leases_if_changed(self):
+ def test_update_leases_processes_leases_if_changed(self):
record_lease_state(None, None)
send_leases = FakeMethod()
self.patch(leases_module, 'send_leases', send_leases)
leases = self.make_lease()
self.fake_leases_file(leases)
+ self.patch(Omshell, 'create', FakeMethod())
update_leases()
self.assertSequenceEqual([(leases, )], send_leases.extract_args())
@@ -173,25 +179,60 @@
record_lease_state(get_write_time(leases_file), leases.copy())
self.assertSequenceEqual([], send_leases.calls)
- def test_update_leases_records_update(self):
+ def test_process_leases_records_update(self):
record_lease_state(None, None)
- self.fake_leases_file()
+ self.patch(leases_module, 'recorded_omapi_shared_key', None)
+ self.fake_leases_file({})
self.patch(leases_module, 'send_leases', FakeMethod())
- update_leases()
+ new_leases = {
+ factory.getRandomIPAddress(): factory.getRandomMACAddress(),
+ }
+ self.fake_leases_file(new_leases)
+ process_leases(datetime.utcnow(), new_leases)
self.assertIsNone(check_lease_changes())
- def test_update_leases_records_state_before_sending(self):
+ def test_process_leases_records_state_before_sending(self):
record_lease_state(None, None)
- self.fake_leases_file()
+ self.patch(Omshell, 'create', FakeMethod())
+ self.fake_leases_file({})
self.patch(
leases_module, 'send_leases', FakeMethod(failure=StopExecuting()))
+ new_leases = {
+ factory.getRandomIPAddress(): factory.getRandomMACAddress(),
+ }
try:
- update_leases()
+ process_leases(datetime.utcnow(), new_leases)
except StopExecuting:
pass
+ self.fake_leases_file(new_leases)
self.assertIsNone(check_lease_changes())
- def test_upload_leases_sends_leases_unconditionally(self):
+ def test_process_leases_registers_new_leases(self):
+ record_lease_state(None, None)
+ self.patch(Omshell, 'create', FakeMethod())
+ ip = factory.getRandomIPAddress()
+ mac = factory.getRandomMACAddress()
+ process_leases(datetime.utcnow(), {ip: mac})
+ self.assertEqual([(ip, mac)], Omshell.create.extract_args())
+
+ def test_process_leases_retries_failed_registrations(self):
+
+ class OmshellFailure(Exception):
+ pass
+
+ record_lease_state(None, None)
+ self.patch(Omshell, 'create', FakeMethod(failure=OmshellFailure()))
+ ip = factory.getRandomIPAddress()
+ mac = factory.getRandomMACAddress()
+ with ExpectedException(OmshellFailure):
+ process_leases(datetime.utcnow(), {ip: mac})
+ # At this point {ip: mac} has not been successfully registered.
+ # But if we re-run process_leases later, it will retry.
+ self.patch(Omshell, 'create', FakeMethod())
+ process_leases(datetime.utcnow(), {ip: mac})
+ self.assertEqual([(ip, mac)], Omshell.create.extract_args())
+
+ def test_upload_leases_processes_leases_unconditionally(self):
send_leases = FakeMethod()
leases = self.make_lease()
leases_file = self.fake_leases_file(leases)
@@ -200,24 +241,6 @@
upload_leases()
self.assertSequenceEqual([(leases, )], send_leases.extract_args())
- def test_upload_leases_records_update(self):
- record_lease_state(None, None)
- self.fake_leases_file()
- self.patch(leases_module, 'send_leases', FakeMethod())
- upload_leases()
- self.assertIsNone(check_lease_changes())
-
- def test_upload_leases_records_state_before_sending(self):
- record_lease_state(None, None)
- self.fake_leases_file()
- self.patch(
- leases_module, 'send_leases', FakeMethod(failure=StopExecuting()))
- try:
- upload_leases()
- except StopExecuting:
- pass
- self.assertIsNone(check_lease_changes())
-
def test_parse_leases_file_parses_leases(self):
params = {
'ip': factory.getRandomIPAddress(),
@@ -236,3 +259,60 @@
self.assertEqual(
(get_write_time(leases_file), {params['ip']: params['mac']}),
parse_leases_file())
+
+ def test_identify_new_leases_identifies_everything_first_time(self):
+ self.patch(leases_module, 'recorded_leases', None)
+ current_leases = {
+ factory.getRandomIPAddress(): factory.getRandomMACAddress(),
+ factory.getRandomIPAddress(): factory.getRandomMACAddress(),
+ }
+ self.assertEqual(current_leases, identify_new_leases(current_leases))
+
+ def test_identify_new_leases_identifies_previously_unknown_leases(self):
+ self.patch(leases_module, 'recorded_leases', {})
+ current_leases = {
+ factory.getRandomIPAddress(): factory.getRandomMACAddress(),
+ }
+ self.assertEqual(current_leases, identify_new_leases(current_leases))
+
+ def test_identify_new_leases_leaves_out_previously_known_leases(self):
+ old_leases = {
+ factory.getRandomIPAddress(): factory.getRandomMACAddress(),
+ }
+ self.patch(leases_module, 'recorded_leases', old_leases)
+ new_leases = {
+ factory.getRandomIPAddress(): factory.getRandomMACAddress(),
+ }
+ current_leases = old_leases.copy()
+ current_leases.update(new_leases)
+ self.assertEqual(new_leases, identify_new_leases(current_leases))
+
+ def test_register_new_leases_registers_new_leases(self):
+ self.patch(Omshell, 'create', FakeMethod())
+ self.patch(leases_module, 'recorded_leases', None)
+ self.patch(leases_module, 'recorded_omapi_shared_key', 'omapi-key')
+ ip = factory.getRandomIPAddress()
+ mac = factory.getRandomMACAddress()
+ register_new_leases({ip: mac})
+ [create_args] = Omshell.create.extract_args()
+ self.assertEqual((ip, mac), create_args)
+
+ def test_register_new_leases_ignores_known_leases(self):
+ self.patch(Omshell, 'create', FakeMethod())
+ self.patch(leases_module, 'recorded_omapi_shared_key', 'omapi-key')
+ old_leases = {
+ factory.getRandomIPAddress(): factory.getRandomMACAddress(),
+ }
+ self.patch(leases_module, 'recorded_leases', old_leases)
+ register_new_leases(old_leases)
+ self.assertEqual([], Omshell.create.calls)
+
+ def test_register_new_leases_does_nothing_without_omapi_key(self):
+ self.patch(Omshell, 'create', FakeMethod())
+ self.patch(leases_module, 'recorded_leases', None)
+ self.patch(leases_module, 'recorded_omapi_shared_key', None)
+ new_leases = {
+ factory.getRandomIPAddress(): factory.getRandomMACAddress(),
+ }
+ register_new_leases(new_leases)
+ self.assertEqual([], Omshell.create.calls)
=== modified file 'src/provisioningserver/tasks.py'
--- src/provisioningserver/tasks.py 2012-08-08 05:54:08 +0000
+++ src/provisioningserver/tasks.py 2012-08-08 11:10:25 +0000
@@ -29,8 +29,10 @@
from celery.task import task
from celeryconfig import DHCP_CONFIG_FILE
-from provisioningserver.dhcp import config
-from provisioningserver.dhcp.leases import record_omapi_shared_key
+from provisioningserver.dhcp import (
+ config,
+ leases,
+ )
from provisioningserver.dns.config import (
DNSConfig,
execute_rndc_command,
@@ -45,7 +47,7 @@
# For each item passed to refresh_secrets, a refresh function to give it to.
refresh_functions = {
- 'omapi_shared_key': record_omapi_shared_key,
+ 'omapi_shared_key': leases.record_omapi_shared_key,
}
