launchpad-reviewers team mailing list archive

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

More notes:


129	=== added file 'src/maastesting/management/commands/reconcile.py'
130	--- src/maastesting/management/commands/reconcile.py	1970-01-01 00:00:00 +0000
131	+++ src/maastesting/management/commands/reconcile.py	2012-03-21 18:20:29 +0000
132	@@ -0,0 +1,84 @@
133	+# Copyright 2012 Canonical Ltd.  This software is licensed under the
134	+# GNU Affero General Public License version 3 (see the file LICENSE).
135	+
136	+"""Reconcile MAAS's view of the world with the Provisioning Server's.
137	+
138	+The Provisioning Server is currently stateless, so this actually implies
139	+reconciling with Cobbler.
140	+
141	+************************************************************************
142	+**                DO NOT USE ON A PRODUCTION SYSTEM                   **
143	+**            This is intended for use as a QA tool only              **
144	+************************************************************************
145	+"""


It is very, very nice to have this kind of documentation on top.  However it does not say in so many words which ways the data flows during reconciliation; it might be useful to say that it copies from the MAAS database to the Cobbler database as well as vice versa.


164	+def reconcile():
165	+    papi = provisioning.get_provisioning_api_proxy()
166	+    nodes_local = {node.system_id: node for node in models.Node.objects.all()}
167	+    nodes_remote = papi.get_nodes()
168	+
169	+    missing_local = set(nodes_remote).difference(nodes_local)
170	+    for name in missing_local:
171	+        print("remote:", name)
172	+        remote_node = nodes_remote[name]
173	+        local_node = models.Node(
174	+            system_id=remote_node["name"],
175	+            # TODO: Figure out the correct architecture.
176	+            architecture=models.ARCHITECTURE.amd64,


Going by the normal profile naming conventions, I think “remote_node['profile'].rsplit('-')[-1]” ought to do it.


177	+            power_type=remote_node["power_type"],
178	+            hostname=remote_node["name"])


This should probably be ["hostname"] rather than ["name"].


183	+    missing_remote = set(nodes_local).difference(nodes_remote)
184	+    for name in missing_remote:
185	+        print("local:", name)
186	+        local_node = nodes_local[name]
187	+        provisioning.provision_post_save_Node(
188	+            sender=None, instance=local_node, created=False)
189	+
190	+    present_in_both = set(nodes_local).intersection(nodes_remote)
191	+    for name in present_in_both:
192	+        print("common:", name)
193	+        node_local = nodes_local[name]
194	+        node_remote = nodes_remote[name]
195	+        # Check that MACs are the same.
196	+        macs_local = {
197	+            mac.mac_address
198	+            for mac in node_local.macaddress_set.all()
199	+            }


I'm surprised to find that a dict comprehension without values creates a set.  A set comprehension is probably clearer though.


200	+        print("- local macs:", " ".join(sorted(macs_local)))
201	+        macs_remote = {
202	+            mac for mac in node_remote["mac_addresses"]
203	+            }


Is this not equivalent to:

    macs_remote = set(node_remote["mac_addresses"])

?
-- 
https://code.launchpad.net/~allenap/maas/pserv-cobbler-reconcile/+merge/98702
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~allenap/maas/pserv-cobbler-reconcile into lp:maas.