commit a1baa42b99447347857d7e52a8ad39536fa210b2
Author: Christos Stavrakakis <cstavr@grnet.gr>
Date:   Sun Jun 30 01:52:20 2013 +0300

    cyclades: Use UUIDs to update VM NICs
    
    Callback functions of snf-dispatcher updated the NICs of a VM by
    deleting all NICs from DB, and creating the new NICs that were described
    in the message. However, since NICs have UUIDs, this is no longer
    required. This commit update callbacks to update the NICs based on their
    UUID.

diff --git a/snf-cyclades-app/synnefo/logic/backend.py b/snf-cyclades-app/synnefo/logic/backend.py
index f650908..87865ca 100644
--- a/snf-cyclades-app/synnefo/logic/backend.py
+++ b/snf-cyclades-app/synnefo/logic/backend.py
@@ -32,10 +32,9 @@
 # or implied, of GRNET S.A.
 from django.conf import settings
 from django.db import transaction
-from datetime import datetime
+from datetime import datetime, timedelta
 
 from synnefo.db.models import (Backend, VirtualMachine, Network,
-                               FloatingIP,
                                BackendNetwork, BACKEND_STATUSES,
                                pooled_rapi_client, VirtualMachineDiagnostic,
                                Flavor)
@@ -57,6 +56,9 @@ _firewall_tags = {
 _reverse_tags = dict((v.split(':')[3], k) for k, v in _firewall_tags.items())
 
 
+NIC_FIELDS = ["state", "mac", "ipv4", "ipv6", "network", "firewall_profile"]
+
+
 def handle_vm_quotas(vm, job_id, job_opcode, job_status, job_fields):
     """Handle quotas for updated VirtualMachine.
 
@@ -165,10 +167,12 @@ def process_op_status(vm, etime, jobid, opcode, status, logmsg, nics=None,
         # See ticket #799 for all the details.
         if status == 'success' or (status == 'error' and
                                    not vm_exists_in_backend(vm)):
-            # VM has been deleted. Release the instance IPs
-            release_instance_ips(vm, [])
-            # And delete the releated NICs (must be performed after release!)
-            vm.nics.all().delete()
+            # VM has been deleted
+            for nic in vm.nics.all():
+                # Release the IP
+                release_nic_address(nic)
+                # And delete the NIC.
+                nic.delete()
             vm.deleted = True
             vm.operstate = state_for_success
             vm.backendtime = etime
@@ -217,104 +221,110 @@ def _process_net_status(vm, etime, nics):
     detailing the NIC configuration of a VM instance.
 
     Update the state of the VM in the DB accordingly.
-    """
 
+    """
     ganeti_nics = process_ganeti_nics(nics)
-    if not nics_changed(vm.nics.order_by('index'), ganeti_nics):
-        log.debug("NICs for VM %s have not changed", vm)
-        return
+    db_nics = dict([(nic.id, nic) for nic in vm.nics.all()])
 
     # Get X-Lock on backend before getting X-Lock on network IP pools, to
     # guarantee that no deadlock will occur with Backend allocator.
     Backend.objects.select_for_update().get(id=vm.backend_id)
 
-    # NICs have changed. Release the instance IPs
-    release_instance_ips(vm, ganeti_nics)
-    # And delete the releated NICs (must be performed after release!)
-    vm.nics.all().delete()
-
-    for nic in ganeti_nics:
-        ipv4 = nic["ipv4"]
-        net = nic['network']
-        if ipv4:
-            net.reserve_address(ipv4)
-
-        vm.nics.create(**nic)
-        # Dummy save the network, because UI uses changed-since for VMs
-        # and Networks in order to show the VM NICs
-        net.save()
+    for nic_name in set(db_nics.keys()) | set(ganeti_nics.keys()):
+        db_nic = db_nics.get(nic_name)
+        ganeti_nic = ganeti_nics.get(nic_name)
+        if ganeti_nic is None:
+            # NIC exists in DB but not in Ganeti. If the NIC is in 'building'
+            # state for more than 5 minutes, then we remove the NIC.
+            # TODO: This is dangerous as the job may be stack in the queue, and
+            # releasing the IP may lead to duplicate IP use.
+            if nic.state != "BUILDING" or (nic.state == "BUILDING" and
+               etime > nic.created + timedelta(minutes=5)):
+                release_nic_address(nic)
+                nic.delete()
+            else:
+                log.warning("Ignoring recent building NIC: %s", nic)
+        elif db_nic is None:
+            # NIC exists in Ganeti but not in DB
+            if ganeti_nic["ipv4"]:
+                network = ganeti_nic["network"]
+                network.reserve_address(ganeti_nic["ipv4"])
+            vm.nics.create(**ganeti_nic)
+        elif not nics_are_equal(db_nic, ganeti_nic):
+            # Special case where the IPv4 address has changed, because you
+            # need to release the old IPv4 address and reserve the new one
+            if db_nic.ipv4 != ganeti_nic["ipv4"]:
+                release_nic_address(db_nic)
+                if ganeti_nic["ipv4"]:
+                    ganeti_nic["network"].reserve_address(ganeti_nic["ipv4"])
+
+            # Update the NIC in DB with the values from Ganeti NIC
+            [setattr(db_nic, f, ganeti_nic[f]) for f in NIC_FIELDS]
+            db_nic.save()
+
+            # Dummy update the network, to work with 'changed-since'
+            db_nic.network.save()
 
     vm.backendtime = etime
     vm.save()
 
 
+def nics_are_equal(db_nic, gnt_nic):
+    for field in NIC_FIELDS:
+        if getattr(db_nic, field) != gnt_nic[field]:
+            return False
+    return True
+
+
 def process_ganeti_nics(ganeti_nics):
-    """Process NIC dict from ganeti hooks."""
+    """Process NIC dict from ganeti"""
     new_nics = []
-    for i, new_nic in enumerate(ganeti_nics):
-        network = new_nic.get('network', '')
-        n = str(network)
-        pk = utils.id_from_network_name(n)
-
-        net = Network.objects.get(pk=pk)
+    for index, gnic in enumerate(ganeti_nics):
+        nic_name = gnic.get("name", None)
+        if nic_name is not None:
+            nic_id = utils.id_from_nic_name(nic_name)
+        else:
+            # Put as default value the index. If it is an unknown NIC to
+            # synnefo it will be created automaticaly.
+            nic_id = "unknown-" + str(index)
+        network_name = gnic.get('network', '')
+        network_id = utils.id_from_network_name(network_name)
+        network = Network.objects.get(id=network_id)
 
         # Get the new nic info
-        mac = new_nic.get('mac')
-        ipv4 = new_nic.get('ip')
-        ipv6 = mac2eui64(mac, net.subnet6) if net.subnet6 is not None else None
+        mac = gnic.get('mac')
+        ipv4 = gnic.get('ip')
+        ipv6 = mac2eui64(mac, network.subnet6)\
+            if network.subnet6 is not None else None
 
-        firewall = new_nic.get('firewall')
+        firewall = gnic.get('firewall')
         firewall_profile = _reverse_tags.get(firewall)
-        if not firewall_profile and net.public:
+        if not firewall_profile and network.public:
             firewall_profile = settings.DEFAULT_FIREWALL_PROFILE
 
-        nic = {
-            'index': i,
-            'network': net,
+        nic_info = {
+            'index': index,
+            'network': network,
             'mac': mac,
             'ipv4': ipv4,
             'ipv6': ipv6,
             'firewall_profile': firewall_profile,
             'state': 'ACTIVE'}
 
-        new_nics.append(nic)
-    return new_nics
+        new_nics.append((nic_id, nic_info))
+    return dict(new_nics)
 
 
-def nics_changed(old_nics, new_nics):
-    """Return True if NICs have changed in any way."""
-    if len(old_nics) != len(new_nics):
-        return True
-    fields = ["ipv4", "ipv6", "mac", "firewall_profile", "index", "network"]
-    for old_nic, new_nic in zip(old_nics, new_nics):
-        for field in fields:
-            if getattr(old_nic, field) != new_nic[field]:
-                return True
-    return False
-
-
-def release_instance_ips(vm, ganeti_nics):
-    old_addresses = set(vm.nics.values_list("network", "ipv4"))
-    new_addresses = set(map(lambda nic: (nic["network"].id, nic["ipv4"]),
-                            ganeti_nics))
-    to_release = old_addresses - new_addresses
-    for (network_id, ipv4) in to_release:
-        if ipv4:
-            # Get X-Lock before searching floating IP, to exclusively search
-            # and release floating IP. Otherwise you may release a floating IP
-            # that has been just reserved.
-            net = Network.objects.select_for_update().get(id=network_id)
-            if net.floating_ip_pool:
-                try:
-                    floating_ip = net.floating_ips.select_for_update()\
-                                                  .get(ipv4=ipv4, machine=vm,
-                                                       deleted=False)
-                    floating_ip.machine = None
-                    floating_ip.save()
-                except FloatingIP.DoesNotExist:
-                    net.release_address(ipv4)
-            else:
-                net.release_address(ipv4)
+def release_nic_address(nic):
+    """Release the IPv4 address of a NIC.
+
+    Check if an instance's NIC has an IPv4 address and release it if it is not
+    a Floating IP.
+
+    """
+
+    if nic.ipv4 and not nic.ip_type == "FLOATING":
+        nic.network.release_address(nic.ipv4)
 
 
 @transaction.commit_on_success
diff --git a/snf-cyclades-app/synnefo/logic/reconciliation.py b/snf-cyclades-app/synnefo/logic/reconciliation.py
index d139f1b..a22709f 100644
--- a/snf-cyclades-app/synnefo/logic/reconciliation.py
+++ b/snf-cyclades-app/synnefo/logic/reconciliation.py
@@ -288,11 +288,21 @@ class BackendReconciler(object):
         db_nics = db_server.nics.order_by("index")
         gnt_nics = gnt_server["nics"]
         gnt_nics_parsed = backend_mod.process_ganeti_nics(gnt_nics)
-        if backend_mod.nics_changed(db_nics, gnt_nics_parsed):
-            msg = "Found unsynced NICs for server '%s'.\n\t"\
-                  "DB: %s\n\tGaneti: %s"
-            db_nics_str = ", ".join(map(format_db_nic, db_nics))
-            gnt_nics_str = ", ".join(map(format_gnt_nic, gnt_nics_parsed))
+        nics_changed = len(db_nics) != len(gnt_nics)
+        for db_nic, gnt_nic in zip(db_nics, sorted(gnt_nics_parsed.items())):
+            gnt_nic_id, gnt_nic = gnt_nic
+            if (db_nic.id == gnt_nic_id) and\
+               backend_mod.nics_are_equal(db_nic, gnt_nic):
+                continue
+            else:
+                nics_changed = True
+                break
+        if nics_changed:
+            msg = "Found unsynced NICs for server '%s'.\n"\
+                  "\tDB:\n\t\t%s\n\tGaneti:\n\t\t%s"
+            db_nics_str = "\n\t\t".join(map(format_db_nic, db_nics))
+            gnt_nics_str = "\n\t\t".join(map(format_gnt_nic,
+                                         gnt_nics_parsed.items()))
             self.log.info(msg, server_id, db_nics_str, gnt_nics_str)
             if self.options["fix_unsynced_nics"]:
                 backend_mod.process_net_status(vm=db_server,
@@ -322,15 +332,19 @@ class BackendReconciler(object):
                 self.log.info("Cleared pending task for server '%s", server_id)
 
 
+NIC_MSG = ": %s\t".join(["ID", "State", "IP", "Network", "MAC", "Firewall"])\
+    + ": %s"
+
+
 def format_db_nic(nic):
-    return "Index: %s, IP: %s Network: %s MAC: %s Firewall: %s" % (nic.index,
-           nic.ipv4, nic.network_id, nic.mac, nic.firewall_profile)
+    return NIC_MSG % (nic.id, nic.state, nic.ipv4, nic.network_id, nic.mac,
+                      nic.firewall_profile)
 
 
 def format_gnt_nic(nic):
-    return "Index: %s IP: %s Network: %s MAC: %s Firewall: %s" %\
-           (nic["index"], nic["ipv4"], nic["network"], nic["mac"],
-            nic["firewall_profile"])
+    nic_name, nic = nic
+    return NIC_MSG % (nic_name, nic["state"], nic["ipv4"], nic["network"],
+                      nic["mac"], nic["firewall_profile"])
 
 
 #
@@ -420,12 +434,13 @@ def parse_gnt_instance(instance):
 
 def nics_from_instance(i):
     ips = zip(itertools.repeat('ip'), i['nic.ips'])
+    names = zip(itertools.repeat('name'), i['nic.names'])
     macs = zip(itertools.repeat('mac'), i['nic.macs'])
-    networks = zip(itertools.repeat('network'), i['nic.networks'])
+    networks = zip(itertools.repeat('network'), i['nic.networks.names'])
     # modes = zip(itertools.repeat('mode'), i['nic.modes'])
     # links = zip(itertools.repeat('link'), i['nic.links'])
     # nics = zip(ips,macs,modes,networks,links)
-    nics = zip(ips, macs, networks)
+    nics = zip(ips, names, macs, networks)
     nics = map(lambda x: dict(x), nics)
     #nics = dict(enumerate(nics))
     tags = i["tags"]
diff --git a/snf-cyclades-app/synnefo/logic/tests/reconciliation.py b/snf-cyclades-app/synnefo/logic/tests/reconciliation.py
index d13e8a2..9fa7961 100644
--- a/snf-cyclades-app/synnefo/logic/tests/reconciliation.py
+++ b/snf-cyclades-app/synnefo/logic/tests/reconciliation.py
@@ -122,8 +122,9 @@ class ServerReconciliationTest(TestCase):
              "mtime": time(),
              "disk.sizes": [],
              "nic.ips": [],
+             "nic.names": [],
              "nic.macs": [],
-             "nic.networks": [],
+             "nic.networks.names": [],
              "tags": []}]
         self.reconciler.reconcile()
         cmrapi.DeleteInstance\
@@ -142,8 +143,9 @@ class ServerReconciliationTest(TestCase):
              "mtime": time(),
              "disk.sizes": [],
              "nic.ips": [],
+             "nic.names": [],
              "nic.macs": [],
-             "nic.networks": [],
+             "nic.networks.names": [],
              "tags": []}]
         with mocked_quotaholder():
             self.reconciler.reconcile()
@@ -168,8 +170,9 @@ class ServerReconciliationTest(TestCase):
              "mtime": time(),
              "disk.sizes": [],
              "nic.ips": [],
+             "nic.names": [],
              "nic.macs": [],
-             "nic.networks": [],
+             "nic.networks.names": [],
              "tags": []}]
         with mocked_quotaholder():
             self.reconciler.reconcile()
@@ -183,8 +186,8 @@ class ServerReconciliationTest(TestCase):
         vm1 = mfactory.VirtualMachineFactory(backend=self.backend,
                                              deleted=False,
                                              operstate="STOPPED")
-        mfactory.NetworkInterfaceFactory(machine=vm1, network=network1,
-                                         ipv4="10.0.0.0")
+        nic = mfactory.NetworkInterfaceFactory(machine=vm1, network=network1,
+                                               ipv4="10.0.0.0")
         mrapi().GetInstances.return_value =\
             [{"name": vm1.backend_vm_id,
              "beparams": {"maxmem": 2048,
@@ -193,9 +196,10 @@ class ServerReconciliationTest(TestCase):
              "oper_state": True,
              "mtime": time(),
              "disk.sizes": [],
+             "nic.names": [nic.backend_uuid],
              "nic.ips": ["192.168.2.1"],
              "nic.macs": ["aa:00:bb:cc:dd:ee"],
-             "nic.networks": [network2.backend_id],
+             "nic.networks.names": [network2.backend_id],
              "tags": []}]
         with mocked_quotaholder():
             self.reconciler.reconcile()
@@ -227,7 +231,9 @@ class NetworkReconciliationTest(TestCase):
         # Test creation if exists in Ganeti
         self.assertEqual(net1.backend_networks.count(), 0)
         mrapi().GetNetworks.return_value = [{"name": net1.backend_id,
-                                             "group_list": ["default"],
+                                             "group_list": [["default",
+                                                             "bridged",
+                                                             "prv0"]],
                                              "network": net1.subnet,
                                              "map": "....",
                                              "external_reservations": ""}]
diff --git a/snf-cyclades-app/synnefo/logic/tests/servers.py b/snf-cyclades-app/synnefo/logic/tests/servers.py
index 46af334..5e86543 100644
--- a/snf-cyclades-app/synnefo/logic/tests/servers.py
+++ b/snf-cyclades-app/synnefo/logic/tests/servers.py
@@ -128,8 +128,9 @@ class ServerTest(TransactionTestCase):
         nics = kwargs["nics"][0]
         self.assertEqual(kwargs["instance"], vm.backend_vm_id)
         self.assertEqual(nics[0], "add")
-        self.assertEqual(nics[1]["ip"], "192.168.2.2")
-        self.assertEqual(nics[1]["network"], net.backend_id)
+        self.assertEqual(nics[1], "-1")
+        self.assertEqual(nics[2]["ip"], "192.168.2.2")
+        self.assertEqual(nics[2]["network"], net.backend_id)
 
         # No dhcp
         vm = mfactory.VirtualMachineFactory(operstate="STARTED")
@@ -145,8 +146,9 @@ class ServerTest(TransactionTestCase):
         nics = kwargs["nics"][0]
         self.assertEqual(kwargs["instance"], vm.backend_vm_id)
         self.assertEqual(nics[0], "add")
-        self.assertEqual(nics[1]["ip"], None)
-        self.assertEqual(nics[1]["network"], net.backend_id)
+        self.assertEqual(nics[1], "-1")
+        self.assertEqual(nics[2]["ip"], None)
+        self.assertEqual(nics[2]["network"], net.backend_id)
 
         # Test connect to IPv6 only network
         vm = mfactory.VirtualMachineFactory(operstate="STARTED")
@@ -159,8 +161,9 @@ class ServerTest(TransactionTestCase):
         nics = kwargs["nics"][0]
         self.assertEqual(kwargs["instance"], vm.backend_vm_id)
         self.assertEqual(nics[0], "add")
-        self.assertEqual(nics[1]["ip"], None)
-        self.assertEqual(nics[1]["network"], net.backend_id)
+        self.assertEqual(nics[1], "-1")
+        self.assertEqual(nics[2]["ip"], None)
+        self.assertEqual(nics[2]["network"], net.backend_id)
 
 
 @patch("synnefo.logic.rapi_pool.GanetiRapiClient")
diff --git a/snf-cyclades-app/synnefo/logic/utils.py b/snf-cyclades-app/synnefo/logic/utils.py
index 65ccd21..50bd73d 100644
--- a/snf-cyclades-app/synnefo/logic/utils.py
+++ b/snf-cyclades-app/synnefo/logic/utils.py
@@ -55,7 +55,7 @@ def id_to_instance_name(id):
 
 
 def id_from_network_name(name):
-    """Returns Network's Django id, given a ganeti machine name.
+    """Returns Network's Django id, given a ganeti network name.
 
     Strips the ganeti prefix atm. Needs a better name!
 
@@ -73,6 +73,19 @@ def id_to_network_name(id):
     return "%snet-%s" % (settings.BACKEND_PREFIX_ID, str(id))
 
 
+def id_from_nic_name(name):
+    """Returns NIC's Django id, given a Ganeti's NIC name.
+
+    """
+    if not str(name).startswith(settings.BACKEND_PREFIX_ID):
+        raise ValueError("Invalid NIC name: %s" % name)
+    ns = str(name).replace(settings.BACKEND_PREFIX_ID + 'nic-', "", 1)
+    if not ns.isdigit():
+        raise ValueError("Invalid NIC name: %s" % name)
+
+    return int(ns)
+
+
 def get_rsapi_state(vm):
     """Returns the API state for a virtual machine
 
