launchpad-reviewers team mailing list archive

["Jeroen T. Vermeulen" <jtv@xxxxxxxxxxxxx>]

Jeroen T. Vermeulen has proposed merging lp:~jtv/maas/bug-973075 into lp:maas.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #973075 in MAAS: "Clear node's user-data on start with user_data=None"
  https://bugs.launchpad.net/maas/+bug/973075

For more details, see:
https://code.launchpad.net/~jtv/maas/bug-973075/+merge/100733

As per discussion with Daviey: when we start a node without user_data (e.g. from the MAAS UI instead of from juju) then upon boot cloud-init should see no user_data, rather than any previous user_data the node may have had.

Basically, user_data is for one boot only.  This was always an anticipated direction, but absent a deliberate choice, what I implemented at the time (out of laziness as much as just to get some consistent, testable behaviour) was to keep the old metadata.
-- 
https://code.launchpad.net/~jtv/maas/bug-973075/+merge/100733
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~jtv/maas/bug-973075 into lp:maas.

=== modified file 'src/maasserver/models.py'
--- src/maasserver/models.py	2012-04-03 10:19:47 +0000
+++ src/maasserver/models.py	2012-04-04 06:18:19 +0000
@@ -431,9 +431,8 @@
         """
         from metadataserver.models import NodeUserData
         nodes = self.get_editable_nodes(by_user, ids=ids)
-        if user_data is not None:
-            for node in nodes:
-                NodeUserData.objects.set_user_data(node, user_data)
+        for node in nodes:
+            NodeUserData.objects.set_user_data(node, user_data)
         get_papi().start_nodes([node.system_id for node in nodes])
         return nodes
 

=== modified file 'src/maasserver/tests/test_models.py'
--- src/maasserver/tests/test_models.py	2012-04-03 10:19:47 +0000
+++ src/maasserver/tests/test_models.py	2012-04-04 06:18:19 +0000
@@ -549,12 +549,14 @@
         self.assertEqual(
             original_user_data, NodeUserData.objects.get_user_data(node))
 
-    def test_start_nodes_without_user_data_leaves_existing_data_alone(self):
+    def test_start_nodes_without_user_data_clears_existing_data(self):
         node = factory.make_node(owner=factory.make_user())
         user_data = self.make_user_data()
         NodeUserData.objects.set_user_data(node, user_data)
         Node.objects.start_nodes([node.system_id], node.owner, user_data=None)
-        self.assertEqual(user_data, NodeUserData.objects.get_user_data(node))
+        self.assertRaises(
+            NodeUserData.DoesNotExist,
+            NodeUserData.objects.get_user_data, node)
 
     def test_start_nodes_with_user_data_overwrites_existing_data(self):
         node = factory.make_node(owner=factory.make_user())

=== modified file 'src/metadataserver/models.py'
--- src/metadataserver/models.py	2012-02-29 17:31:38 +0000
+++ src/metadataserver/models.py	2012-04-04 06:18:19 +0000
@@ -131,17 +131,17 @@
     """Utility for the collection of NodeUserData items."""
 
     def set_user_data(self, node, data):
-        """Set user data for the given node."""
+        """Set user data for the given node.
+
+        If `data` is None, remove user data for the node.
+        """
         existing_entries = self.filter(node=node)
-        if len(existing_entries) == 1:
-            [entry] = existing_entries
-            entry.data = Bin(data)
-            entry.save()
-        elif len(existing_entries) == 0:
-            wrapped_data = Bin(data)
-            self.create(node=node, data=wrapped_data)
-        else:
+        if len(existing_entries) > 1:
             raise AssertionError("More than one user-data entry matches.")
+        if data is None:
+            self._remove(existing_entries)
+        else:
+            self._set(node, data, existing_entries)
 
     def get_user_data(self, node):
         """Retrieve user data for the given node."""
@@ -151,6 +151,28 @@
         """Do we have user data registered for node?"""
         return self.filter(node=node).exists()
 
+    def _set(self, node, data, existing_entries):
+        """Set actual user data for a node.  Not usable if data is None."""
+        if existing_entries.exists():
+            self._update(existing_entries, data)
+        else:
+            self._make(node, data)
+
+    def _update(self, existing_entries, data):
+        """Update an existing user-data entry with fresh user data."""
+        [entry] = existing_entries
+        entry.data = Bin(data)
+        entry.save()
+
+    def _make(self, node, data):
+        """Create a new user-data entry."""
+        wrapped_data = Bin(data)
+        self.create(node=node, data=wrapped_data)
+
+    def _remove(self, existing_entries):
+        """Remove metadata from entry, if any."""
+        existing_entries.delete()
+
 
 class NodeUserData(Model):
     """User-data portion of a node's metadata.

=== modified file 'src/metadataserver/tests/test_models.py'
--- src/metadataserver/tests/test_models.py	2012-03-21 21:40:28 +0000
+++ src/metadataserver/tests/test_models.py	2012-04-04 06:18:19 +0000
@@ -93,6 +93,17 @@
         NodeUserData.objects.set_user_data(factory.make_node(), b'unrelated')
         self.assertEqual(b'intact', NodeUserData.objects.get(node=node).data)
 
+    def test_set_user_data_to_None_removes_user_data(self):
+        node = factory.make_node()
+        NodeUserData.objects.set_user_data(node, b'original')
+        NodeUserData.objects.set_user_data(node, None)
+        self.assertItemsEqual([], NodeUserData.objects.filter(node=node))
+
+    def test_set_user_data_to_None_when_none_exists_does_nothing(self):
+        node = factory.make_node()
+        NodeUserData.objects.set_user_data(node, None)
+        self.assertItemsEqual([], NodeUserData.objects.filter(node=node))
+
     def test_get_user_data_retrieves_data(self):
         node = factory.make_node()
         data = b'splat'