launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #11074
[Merge] lp:~jtv/maas/refresh-api-url into lp:maas
Jeroen T. Vermeulen has proposed merging lp:~jtv/maas/refresh-api-url into lp:maas.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
For more details, see:
https://code.launchpad.net/~jtv/maas/refresh-api-url/+merge/120054
The refresh_workers call will now also send the MAAS URL to the workers. (Not the API URL; it should be up to the workers to include the version number that they expect).
Since this adds one more refresh item, I systematized the setup of those items in the leases test — which revealed that some tests that should have been verifying that send_leases does nothing if certain items are not known to the worker, were actually verifying that register_new_leases was doing nothing. Which was pointless because the one item that register_new_leases needed wasn't being set, and so it wouldn't run either. Setting all items made these tests break, and so I moved, fixed, and renamed them.
Jeroen
--
https://code.launchpad.net/~jtv/maas/refresh-api-url/+merge/120054
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~jtv/maas/refresh-api-url into lp:maas.
=== modified file 'src/maasserver/refresh_worker.py'
--- src/maasserver/refresh_worker.py 2012-08-17 04:28:56 +0000
+++ src/maasserver/refresh_worker.py 2012-08-17 05:27:18 +0000
@@ -15,6 +15,7 @@
]
from apiclient.creds import convert_tuple_to_string
+from django.conf import settings
from maasserver.models.user import get_creds_tuple
from provisioningserver.tasks import refresh_secrets
@@ -31,14 +32,14 @@
"""
items = {
+ 'api_credentials': convert_tuple_to_string(
+ get_creds_tuple(nodegroup.api_token)),
+ 'maas_url': settings.DEFAULT_MAAS_URL,
'nodegroup_name': nodegroup.name,
}
if nodegroup.dhcp_key is not None and len(nodegroup.dhcp_key) > 0:
items['omapi_key'] = nodegroup.dhcp_key
- items['api_credentials'] = convert_tuple_to_string(
- get_creds_tuple(nodegroup.api_token))
-
# TODO: Route this to the right worker, once we have multiple.
refresh_secrets.delay(**items)
=== modified file 'src/provisioningserver/auth.py'
--- src/provisioningserver/auth.py 2012-08-17 04:28:56 +0000
+++ src/provisioningserver/auth.py 2012-08-17 05:27:18 +0000
@@ -13,7 +13,7 @@
__all__ = [
'get_recorded_api_credentials',
'get_recorded_nodegroup_name',
- 'locate_maas_api',
+ 'get_recorded_maas_url',
'record_api_credentials',
'record_nodegroup_name',
]
@@ -21,6 +21,9 @@
from apiclient.creds import convert_string_to_tuple
from provisioningserver.cache import cache
+# Cache key for URL to the central MAAS server.
+MAAS_URL_CACHE_KEY = 'maas_url'
+
# Cache key for the API credentials as last sent by the server.
API_CREDENTIALS_CACHE_KEY = 'api_credentials'
@@ -28,10 +31,14 @@
NODEGROUP_NAME_CACHE_KEY = 'nodegroup_name'
-def locate_maas_api():
- """Return the base URL for the MAAS API."""
-# TODO: Configure this somehow. What you see here is a placeholder.
- return "http://localhost/MAAS/";
+def record_maas_url(maas_url):
+ """Record the MAAS server URL as sent by the server."""
+ cache.set(MAAS_URL_CACHE_KEY, maas_url)
+
+
+def get_recorded_maas_url():
+ """Return the base URL for the MAAS server."""
+ return cache.get(MAAS_URL_CACHE_KEY)
def record_api_credentials(api_credentials):
=== modified file 'src/provisioningserver/dhcp/leases.py'
--- src/provisioningserver/dhcp/leases.py 2012-08-17 04:28:56 +0000
+++ src/provisioningserver/dhcp/leases.py 2012-08-17 05:27:18 +0000
@@ -45,8 +45,8 @@
from celeryconfig import DHCP_LEASES_FILE
from provisioningserver.auth import (
get_recorded_api_credentials,
+ get_recorded_maas_url,
get_recorded_nodegroup_name,
- locate_maas_api,
)
from provisioningserver.cache import cache
from provisioningserver.dhcp.leases_parser import parse_leases
@@ -93,9 +93,9 @@
def check_lease_changes():
"""Has the DHCP leases file changed in any significant way?"""
- # These variables are shared between threads. A bit of
- # inconsistency due to concurrent updates is not a problem, but read
- # them both at once here to reduce the scope for trouble.
+ # These variables are shared between worker threads/processes.
+ # A bit of inconsistency due to concurrent updates is not a problem,
+ # but read them both at once here to reduce the scope for trouble.
previous_leases = cache.get(LEASES_CACHE_KEY)
previous_leases_time = cache.get(LEASES_TIME_CACHE_KEY)
@@ -126,8 +126,8 @@
:param current_leases: A dict mapping IP addresses to the respective
MAC addresses that own them.
"""
- # The recorded leases is shared between threads. Read it
- # just once to reduce the impact of concurrent changes.
+ # The recorded leases is shared between worker threads/processes.
+ # Read it just once to reduce the impact of concurrent changes.
previous_leases = cache.get(LEASES_CACHE_KEY)
if previous_leases is None:
return current_leases
@@ -146,9 +146,9 @@
# Avoid circular imports.
from provisioningserver.tasks import add_new_dhcp_host_map
- # The recorded_omapi_key is shared between threads, so read it just
- # once, atomically.
- omapi_key = cache.get(OMAPI_KEY_CACHE_KEY)
+ # The recorded_omapi_key is shared between worker threads or
+ # processes, so read it just once, atomically.
+ omapi_key = get_recorded_omapi_key()
if omapi_key is None:
task_logger.info(
"Not registering new leases: "
@@ -158,26 +158,32 @@
add_new_dhcp_host_map(new_leases, 'localhost', omapi_key)
+def list_missing_items(knowledge):
+ """Report items from dict `knowledge` that are still `None`."""
+ return [name for name, value in knowledge.items() if value is None]
+
+
def send_leases(leases):
"""Send lease updates to the server API."""
- api_credentials = get_recorded_api_credentials()
- nodegroup_name = get_recorded_nodegroup_name()
- if None in (api_credentials, nodegroup_name):
+ # Items that the server must have sent us before we can do this.
+ knowledge = {
+ 'maas_url': get_recorded_maas_url(),
+ 'api_credentials': get_recorded_api_credentials(),
+ 'nodegroup_name': get_recorded_nodegroup_name(),
+ }
+ if None in knowledge.values():
# The MAAS server hasn't sent us enough information for us to do
# this yet. Leave it for another time.
- if api_credentials is None:
- task_logger.info(
- "Not sending DHCP leases to server: "
- "No MAAS API credentials received from server yet.")
- if nodegroup_name is None:
- task_logger.info(
- "Not sending DHCP leases to server: "
- "No MAAS API URL received from server yet.")
+ task_logger.info(
+ "Not sending DHCP leases to server: not all required knowledge "
+ "received from server yet. "
+ "Missing: %s"
+ % ', '.join(list_missing_items(knowledge)))
return
- api_path = 'nodegroups/%s/' % nodegroup_name
- oauth = MAASOAuth(*api_credentials)
- MAASClient(oauth, MAASDispatcher(), locate_maas_api()).post(
+ api_path = 'nodegroups/%s/' % knowledge['nodegroup_name']
+ oauth = MAASOAuth(*knowledge['api_credentials'])
+ MAASClient(oauth, MAASDispatcher(), knowledge['maas_url']).post(
api_path, 'update_leases', leases=leases)
=== modified file 'src/provisioningserver/dhcp/tests/test_leases.py'
--- src/provisioningserver/dhcp/tests/test_leases.py 2012-08-17 04:28:56 +0000
+++ src/provisioningserver/dhcp/tests/test_leases.py 2012-08-17 05:27:18 +0000
@@ -25,7 +25,10 @@
age_file,
get_write_time,
)
-from provisioningserver.auth import NODEGROUP_NAME_CACHE_KEY
+from provisioningserver.auth import (
+ MAAS_URL_CACHE_KEY,
+ NODEGROUP_NAME_CACHE_KEY,
+ )
from provisioningserver.cache import cache
from provisioningserver.dhcp import leases as leases_module
from provisioningserver.dhcp.leases import (
@@ -125,18 +128,49 @@
key = factory.getRandomString()
cache.set(OMAPI_KEY_CACHE_KEY, key)
+ def clear_omapi_key(self):
+ """Clear the recorded omapi key."""
+ cache.set(OMAPI_KEY_CACHE_KEY, None)
+
def set_nodegroup_name(self):
"""Set the recorded nodegroup name for the duration of this test."""
name = factory.make_name('nodegroup')
cache.set(NODEGROUP_NAME_CACHE_KEY, name)
return name
+ def clear_nodegroup_name(self):
+ """Clear the recorded nodegroup name."""
+ cache.set(NODEGROUP_NAME_CACHE_KEY, None)
+
+ def set_maas_url(self):
+ """Set the recorded MAAS URL for the duration of this test."""
+ maas_url = 'http://%s.example.com/%s/' % (
+ factory.make_name('host'),
+ factory.getRandomString(),
+ )
+ cache.set(MAAS_URL_CACHE_KEY, maas_url)
+
+ def clear_maas_url(self):
+ """Clear the recorded MAAS API URL."""
+ cache.set(MAAS_URL_CACHE_KEY, None)
+
def set_api_credentials(self):
"""Set recorded API credentials for the duration of this test."""
creds_string = ':'.join(
factory.getRandomString() for counter in range(3))
cache.set('api_credentials', creds_string)
+ def clear_api_credentials(self):
+ """Clear recorded API credentials."""
+ cache.set('api_credentials', None)
+
+ def set_items_needed_for_lease_update(self):
+ """Set the recorded items required by `update_leases`."""
+ self.set_maas_url()
+ self.set_api_credentials()
+ self.set_omapi_key()
+ self.set_nodegroup_name()
+
def set_lease_state(self, time=None, leases=None):
"""Set the recorded state of DHCP leases.
@@ -260,8 +294,9 @@
def test_process_leases_registers_new_leases(self):
self.set_lease_state()
- self.set_omapi_key()
+ self.set_items_needed_for_lease_update()
self.patch(Omshell, 'create', FakeMethod())
+ self.patch(leases_module, 'send_leases', FakeMethod())
ip = factory.getRandomIPAddress()
mac = factory.getRandomMACAddress()
process_leases(datetime.utcnow(), {ip: mac})
@@ -273,7 +308,8 @@
pass
self.set_lease_state()
- self.set_omapi_key()
+ self.set_items_needed_for_lease_update()
+ self.patch(leases_module, 'send_leases', FakeMethod())
self.patch(Omshell, 'create', FakeMethod(failure=OmshellFailure()))
ip = factory.getRandomIPAddress()
mac = factory.getRandomMACAddress()
@@ -365,16 +401,8 @@
def test_register_new_leases_does_nothing_without_omapi_key(self):
self.patch(Omshell, 'create', FakeMethod())
self.set_lease_state()
- self.set_nodegroup_name()
- new_leases = {
- factory.getRandomIPAddress(): factory.getRandomMACAddress(),
- }
- register_new_leases(new_leases)
- self.assertEqual([], Omshell.create.calls)
-
- def test_register_new_leases_does_nothing_without_nodegroup_name(self):
- self.patch(Omshell, 'create', FakeMethod())
- self.set_lease_state()
+ self.set_items_needed_for_lease_update()
+ self.clear_omapi_key()
new_leases = {
factory.getRandomIPAddress(): factory.getRandomMACAddress(),
}
@@ -383,7 +411,7 @@
def test_send_leases_posts_to_API(self):
self.patch(Omshell, 'create', FakeMethod())
- self.set_api_credentials()
+ self.set_items_needed_for_lease_update()
nodegroup_name = self.set_nodegroup_name()
self.patch(MAASClient, 'post', FakeMethod())
leases = {
@@ -396,10 +424,30 @@
)],
MAASClient.post.calls)
+ def test_send_leases_does_nothing_without_maas_url(self):
+ self.patch(MAASClient, 'post', FakeMethod())
+ self.set_lease_state()
+ self.set_items_needed_for_lease_update()
+ self.clear_maas_url()
+ leases = {factory.getRandomIPAddress(): factory.getRandomMACAddress()}
+ send_leases(leases)
+ self.assertEqual([], MAASClient.post.calls)
+
def test_send_leases_does_nothing_without_credentials(self):
self.patch(MAASClient, 'post', FakeMethod())
+ self.set_items_needed_for_lease_update()
+ self.clear_api_credentials()
leases = {
factory.getRandomIPAddress(): factory.getRandomMACAddress(),
}
send_leases(leases)
self.assertEqual([], MAASClient.post.calls)
+
+ def test_send_leases_does_nothing_without_nodegroup_name(self):
+ self.patch(MAASClient, 'post', FakeMethod())
+ self.set_lease_state()
+ self.set_items_needed_for_lease_update()
+ self.clear_nodegroup_name()
+ leases = {factory.getRandomIPAddress(): factory.getRandomMACAddress()}
+ send_leases(leases)
+ self.assertEqual([], MAASClient.post.calls)
=== modified file 'src/provisioningserver/tasks.py'
--- src/provisioningserver/tasks.py 2012-08-17 04:28:56 +0000
+++ src/provisioningserver/tasks.py 2012-08-17 05:27:18 +0000
@@ -31,6 +31,7 @@
from celeryconfig import DHCP_CONFIG_FILE
from provisioningserver.auth import (
record_api_credentials,
+ record_maas_url,
record_nodegroup_name,
)
from provisioningserver.dhcp import (
@@ -52,6 +53,7 @@
# For each item passed to refresh_secrets, a refresh function to give it to.
refresh_functions = {
'api_credentials': record_api_credentials,
+ 'maas_url': record_maas_url,
'omapi_key': leases.record_omapi_key,
'nodegroup_name': record_nodegroup_name,
}
=== modified file 'src/provisioningserver/tests/test_tasks.py'
--- src/provisioningserver/tests/test_tasks.py 2012-08-17 04:28:56 +0000
+++ src/provisioningserver/tests/test_tasks.py 2012-08-17 05:27:18 +0000
@@ -89,6 +89,11 @@
def test_works_as_a_task(self):
self.assertTrue(refresh_secrets.delay().successful())
+ def test_updates_maas_url(self):
+ maas_url = 'http://example.com/%s/' % factory.getRandomString()
+ refresh_secrets(maas_url=maas_url)
+ self.assertEqual(maas_url, auth.get_recorded_maas_url())
+
def test_updates_api_credentials(self):
credentials = (
factory.make_name('key'),
