launchpad-reviewers team mailing list archive

[Gavin Panella <gavin.panella@xxxxxxxxxxxxx>]

> +# Copyright 2012 Canonical Ltd.  This software is licensed under the
> +# GNU Affero General Public License version 3 (see the file LICENSE).
> +
> +"""Reconcile MAAS's view of the world with the Provisioning Server's.
> +
> +The Provisioning Server is currently stateless, so this actually implies
> +reconciling with Cobbler.
> +
> +************************************************************************
> +**                DO NOT USE ON A PRODUCTION SYSTEM                   **
> +**            This is intended for use as a QA tool only              **
> +************************************************************************
> +"""
> 
> 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.

Good point. I've added the following paragraph:

"""
Both MAAS and the Provisioning Server will be examined. Missing nodes
will be copied in *both* directions. Nodes that exist in common will
have their MAC addresses reconciled in *both* directions. The result
should be the union of nodes in MAAS and nodes as the Provisioning
Server sees them.
"""

> +def reconcile():
> +    papi = provisioning.get_provisioning_api_proxy()
> +    nodes_local = {node.system_id: node for node in models.Node.objects.all()}
> +    nodes_remote = papi.get_nodes()
> +
> +    missing_local = set(nodes_remote).difference(nodes_local)
> +    for name in missing_local:
> +        print("remote:", name)
> +        remote_node = nodes_remote[name]
> +        local_node = models.Node(
> +            system_id=remote_node["name"],
> +            # TODO: Figure out the correct architecture.
> +            architecture=models.ARCHITECTURE.amd64,
> 
> Going by the normal profile naming conventions, I think
> “remote_node['profile'].rsplit('-')[-1]” ought to do it.

Oh brilliant, good thinking :) Thanks.

> 177     +            power_type=remote_node["power_type"],
> 178     +            hostname=remote_node["name"])
> 
> This should probably be ["hostname"] rather than ["name"].

There's no "hostname" key in the node representation returned by PAPI,
so "name" will have to suffice for now.

> +    missing_remote = set(nodes_local).difference(nodes_remote)
> +    for name in missing_remote:
> +        print("local:", name)
> +        local_node = nodes_local[name]
> +        provisioning.provision_post_save_Node(
> +            sender=None, instance=local_node, created=False)
> +
> +    present_in_both = set(nodes_local).intersection(nodes_remote)
> +    for name in present_in_both:
> +        print("common:", name)
> +        node_local = nodes_local[name]
> +        node_remote = nodes_remote[name]
> +        # Check that MACs are the same.
> +        macs_local = {
> +            mac.mac_address
> +            for mac in node_local.macaddress_set.all()
> +            }
> 
> I'm surprised to find that a dict comprehension without values
> creates a set.  A set comprehension is probably clearer though.

That _is_ a set comprehension :) It's a deliberate language feature,
for better or worse.

> +        print("- local macs:", " ".join(sorted(macs_local)))
> +        macs_remote = {
> +            mac for mac in node_remote["mac_addresses"]
> +            }
> 
> Is this not equivalent to:
> 
>     macs_remote = set(node_remote["mac_addresses"])

Yes, but I wrote it like this for symmetry with the macs_local
initialization.

-- 
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.