On 26 November 2014 at 11:09, Gavin Panella <email address hidden>
wrote:

> Review: Approve
>
> LGTM.
>
> Diff comments:
>
> > === modified file 'src/maasserver/api/ip_addresses.py'
> > --- src/maasserver/api/ip_addresses.py        2014-11-24 17:55:55 +0000
> > +++ src/maasserver/api/ip_addresses.py        2014-11-25 14:41:46 +0000
> > @@ -16,6 +16,7 @@
> >      'IPAddressesHandler',
> >      ]
> >
> > +from django.db import transaction
> >  from django.shortcuts import get_object_or_404
> >  from maasserver.api.support import (
> >      operation,
> > @@ -25,15 +26,23 @@
> >      get_mandatory_param,
> >      get_optional_param,
> >      )
> > +from maasserver.clusterrpc.dhcp import (
> > +    remove_host_maps,
> > +    update_host_maps,
> > +    )
> >  from maasserver.enum import (
> >      IPADDRESS_TYPE,
> >      NODEGROUP_STATUS,
> >      )
> > -from maasserver.exceptions import MAASAPIBadRequest
> > +from maasserver.exceptions import (
> > +    MAASAPIBadRequest,
> > +    StaticIPAlreadyExistsForMACAddress,
> > +    )
> >  from maasserver.models import (
> >      NodeGroupInterface,
> >      StaticIPAddress,
> >      )
> > +from maasserver.models.macaddress import MACAddress
> >  import netaddr
> >  from provisioningserver.logger import get_maas_logger
> >
> > @@ -53,19 +62,53 @@
> >      def resource_uri(cls, *args, **kwargs):
> >          return ('ipaddresses_handler', [])
> >
> > -    def claim_ip(self, user, interface, requested_address):
> > +    @transaction.atomic
> > +    def claim_ip(self, user, interface, requested_address, mac=None):
> >          """Attempt to get a USER_RESERVED StaticIPAddress for `user` on
> >          `interface`.
> >
> >          :raises StaticIPAddressExhaustion: If no IPs available.
> >          """
> > -        sip = StaticIPAddress.objects.allocate_new(
> > -            range_low=interface.static_ip_range_low,
> > -            range_high=interface.static_ip_range_high,
> > -            alloc_type=IPADDRESS_TYPE.USER_RESERVED,
> > -            requested_address=requested_address,
> > -            user=user)
> > -        maaslog.info("User %s was allocated IP %s", user.username,
> sip.ip)
> > +        if mac is not None:
>
> Might be worth switching around this conditional to remove the negative.
>

Yep.


>
> > +            mac_address, _ = MACAddress.objects.get_or_create(
> > +                mac_address=mac, cluster_interface=interface)
> > +            ips_on_interface = (
> > +                addr.ip for addr in mac_address.ip_addresses.all()
> > +                if netaddr.IPAddress(addr.ip) in interface.network)
> > +            if any(ips_on_interface):
> > +                # If this MAC already has static IPs on the interface in
> > +                # question we raise an error, since we can't sanely
> > +                # allocate more addresses for the MAC here.
> > +                raise StaticIPAlreadyExistsForMACAddress(
> > +                    "MAC address %s already has the IP address(es) %s."
> %
> > +                    (mac, ips_on_interface))
> > +
> > +            [sip] = mac_address.claim_static_ips(
> > +                alloc_type=IPADDRESS_TYPE.USER_RESERVED, user=user,
> > +                requested_address=requested_address)
> > +            # Update the DHCP host maps for the cluster so that this MAC
> > +            # gets an entry with this static IP.
> > +            host_map_updates = {
> > +                interface.nodegroup: {
> > +                    sip.ip: mac_address.mac_address,
> > +                }
> > +            }
> > +            update_host_maps_failures = list(
>
> Before doing RPC, do a commit_within_atomic_block() here. Now that it's
> possible for transactions to be routinely rejected because of serialisation
> failures, we need to commit before mutating remote systems.
>


Aaagh. Yes.



> > +                update_host_maps(host_map_updates))
> > +            if len(update_host_maps_failures) > 0:
> > +                # There will only ever be one error, so raise that.
> > +                raise update_host_maps_failures[0].value
>
> You may want to revert the claims made before the commit now. This is a
> candidate for 2-phase commit <
> http://initd.org/psycopg/docs/usage.html#two-phase-commit-protocol-support>.
> I wonder how many kittens Django will poop with that kind of shenanigans?
>

Ha. Good point on both counts.


>
> > +            maaslog.info(
> > +                "User %s was allocated IP %s for MAC address %s",
> > +                user.username, sip.ip, mac_address.mac_address)
> > +        else:
> > +            sip = StaticIPAddress.objects.allocate_new(
> > +                range_low=interface.static_ip_range_low,
> > +                range_high=interface.static_ip_range_high,
> > +                alloc_type=IPADDRESS_TYPE.USER_RESERVED,
> > +                requested_address=requested_address,
> > +                user=user)
> > +            maaslog.info("User %s was allocated IP %s", user.username,
> sip.ip)
>
> Another fun aspect of serialisation failures is that this statement isn't
> true yet.
>

Hmm. I'm not prepared to make log statements fuzzy, however :).

"User %s might have been allocated IP %s… can't be sure yet. Watch this
space."

OTOH I could try committing here :)


>
> >          return sip
> >
> >      @operation(idempotent=False)
> > @@ -86,6 +129,7 @@
> >          network = get_mandatory_param(request.POST, "network")
> >          requested_address = get_optional_param(
> >              request.POST, "requested_address")
> > +        mac_address = get_optional_param(request.POST, "mac")
> >          # Validate the passed network.
> >          try:
> >              valid_network = netaddr.IPNetwork(network)
> > @@ -103,7 +147,7 @@
> >              if valid_network == interface.network:
> >                  # Winner winner chicken dinner.
> >                  return self.claim_ip(
> > -                    request.user, interface, requested_address)
> > +                    request.user, interface, requested_address,
> mac_address)
> >          raise MAASAPIBadRequest(
> >              "No network found matching %s; you may be requesting an IP "
> >              "on a network with no static IP range defined." % network)
> > @@ -120,7 +164,30 @@
> >          ip = get_mandatory_param(request.POST, "ip")
> >          staticaddress = get_object_or_404(
> >              StaticIPAddress, ip=ip, user=request.user)
> > +
> > +        linked_mac_addresses = staticaddress.macaddress_set
> > +        linked_mac_address_interfaces = set(
> > +            mac_address.cluster_interface
> > +            for mac_address in linked_mac_addresses.all())
> > +
> > +        # Remove any hostmaps for this IP.
> > +        host_maps_to_remove = {
> > +            interface.nodegroup: [staticaddress.ip]
> > +            for interface in linked_mac_address_interfaces
> > +            }
> > +        remove_host_maps_failures = list(
>
> Same again re. committing.
>

Nothing to commit yet; it's only post-RPC that we want to commit here, so
I'm sanguine about it.


>
> > +            remove_host_maps(host_maps_to_remove))
> > +        if len(remove_host_maps_failures) > 0:
> > +            # There's only going to be one failure, so raise that.
> > +            raise remove_host_maps_failures[0].value
> > +
> > +        # Delete any MACAddress entries that are attached to this static
> > +        # IP but that *aren't* attached to a Node. With the DB isolation
> > +        # at SERIALIZABLE there will be no race here, and it's better to
> > +        # keep cruft out of the DB.
> > +        linked_mac_addresses.filter(node=None).delete()
> >          staticaddress.deallocate()
> > +
> >          maaslog.info("User %s released IP %s", request.user.username,
> ip)



> >
> >      def read(self, request):
> >
> > === modified file 'src/maasserver/api/tests/test_ipaddresses.py'
> > --- src/maasserver/api/tests/test_ipaddresses.py      2014-10-22
> 10:35:09 +0000
> > +++ src/maasserver/api/tests/test_ipaddresses.py      2014-11-25
> 14:41:46 +0000
> > @@ -18,6 +18,7 @@
> >  import json
> >
> >  from django.core.urlresolvers import reverse
> > +from maasserver.api import ip_addresses as ip_addresses_module
> >  from maasserver.enum import (
> >      IPADDRESS_TYPE,
> >      NODEGROUP_STATUS,
> > @@ -26,8 +27,15 @@
> >  from maasserver.testing.api import APITestCase
> >  from maasserver.testing.factory import factory
> >  from maasserver.testing.orm import reload_object
> > +from maastesting.matchers import MockCalledOnceWith
> >  from netaddr import IPAddress
> > -from testtools.matchers import Equals
> > +from provisioningserver.rpc.exceptions import NoConnectionsAvailable
> > +from testtools.matchers import (
> > +    Equals,
> > +    Is,
> > +    Not,
> > +    )
> > +from twisted.python.failure import Failure
> >
> >
> >  class TestNetworksAPI(APITestCase):
> > @@ -36,19 +44,22 @@
> >          cluster = factory.make_NodeGroup(status=status, **kwargs)
> >          return factory.make_NodeGroupInterface(cluster)
> >
> > -    def post_reservation_request(self, net, requested_address=None):
> > +    def post_reservation_request(self, net, requested_address=None,
> mac=None):
> >          params = {
> >              'op': 'reserve',
> >              'network': unicode(net),
> >          }
> >          if requested_address is not None:
> >              params["requested_address"] = requested_address
> > +        if mac is not None:
> > +            params["mac"] = mac
> >          return self.client.post(reverse('ipaddresses_handler'), params)
> >
> > -    def post_release_request(self, ip):
> > +    def post_release_request(self, ip, mac=None):
> >          params = {
> >              'op': 'release',
> >              'ip': ip,
> > +            'mac': mac,
> >          }
> >          return self.client.post(reverse('ipaddresses_handler'), params)
> >
> > @@ -84,6 +95,75 @@
> >              IPADDRESS_TYPE.USER_RESERVED, staticipaddress.alloc_type)
> >          self.assertEqual(self.logged_in_user, staticipaddress.user)
> >
> > +    def test_POST_reserve_with_MAC_links_MAC_to_ip_address(self):
> > +        update_host_maps = self.patch(ip_addresses_module,
> 'update_host_maps')
> > +        interface = self.make_interface()
> > +        net = interface.network
> > +        mac = factory.make_mac_address()
> > +
> > +        response = self.post_reservation_request(net, mac=mac)
> > +        self.assertEqual(httplib.OK, response.status_code)
> > +        returned_address = json.loads(response.content)
> > +        [staticipaddress] = StaticIPAddress.objects.all()
> > +        self.expectThat(
> > +            staticipaddress.macaddress_set.first().mac_address,
> > +            Equals(mac))
> > +
> > +        # DHCP Host maps have been updated.
> > +        self.expectThat(
> > +            update_host_maps,
> > +            MockCalledOnceWith(
> > +                {interface.nodegroup: {returned_address['ip']: mac}}))
> > +
> > +    def
> test_POST_reserve_with_MAC_returns_503_if_hostmap_update_fails(self):
> > +        update_host_maps = self.patch(ip_addresses_module,
> 'update_host_maps')
> > +        # We a specific exception here because update_host_maps() will
> > +        # fail with RPC-specific errors.
> > +        update_host_maps.return_value = [
> > +            Failure(
> > +                NoConnectionsAvailable(
> > +                    "Are you sure you're not Elvis?"))
> > +            ]
> > +        interface = self.make_interface()
> > +        net = interface.network
> > +        mac = factory.make_mac_address()
> > +
> > +        response = self.post_reservation_request(net, mac=mac)
> > +        self.expectThat(
> > +            response.status_code, Equals(httplib.SERVICE_UNAVAILABLE))
> > +        # No static IP has been created.
> > +        self.expectThat(
> > +            StaticIPAddress.objects.all().count(),
> > +            Equals(0))
>
> The HasLength matcher might be good here too:
>
>     self.expectThat(StaticIPAddress.objects.all(), HasLength(0))
>
> so that you can see the objects if the assertion fails. I don't remember
> if the reprs/strs of model objects are useful by default though.
>

Yep, fixed.


>
> > +
> > +    def
> test_POST_returns_CONFLICT_when_static_ip_for_MAC_already_exists(self):
> > +        interface = self.make_interface()
> > +        mac = factory.make_MACAddress(cluster_interface=interface)
> > +        mac.claim_static_ips()
> > +        net = interface.network
> > +
> > +        response = self.post_reservation_request(net,
> mac=mac.mac_address)
> > +        self.expectThat(
> > +            response.status_code, Equals(httplib.CONFLICT))
> > +        # No new static IP has been created.
> > +        self.expectThat(
> > +            StaticIPAddress.objects.all().count(),
> > +            Equals(1))
> > +
> > +    def
> test_POST_allows_claiming_of_new_static_ips_for_existing_MAC(self):
> > +        self.patch(ip_addresses_module, 'update_host_maps')
> > +
> > +        interface = self.make_interface()
> > +        net = interface.network
> > +        mac = factory.make_MACAddress(cluster_interface=interface)
> > +
> > +        response = self.post_reservation_request(net,
> mac=mac.mac_address)
> > +        self.expectThat(response.status_code, Equals(httplib.OK))
> > +        [staticipaddress] = StaticIPAddress.objects.all()
> > +        self.assertEqual(
> > +            staticipaddress.macaddress_set.first().mac_address,
> > +            mac.mac_address)
> > +
> >      def test_POST_reserve_errors_for_no_matching_interface(self):
> >          interface = self.make_interface()
> >          net = factory.make_ipv4_network(but_not=[interface.network])
> > @@ -202,6 +282,70 @@
> >          self.assertEqual(httplib.OK, response.status_code,
> response.content)
> >          self.assertIsNone(reload_object(ipaddress))
> >
> > +    def test_POST_release_deletes_floating_MAC_address(self):
> > +        self.patch(ip_addresses_module, 'remove_host_maps')
> > +
> > +        interface = self.make_interface()
> > +        floating_mac =
> factory.make_MACAddress(cluster_interface=interface)
> > +        [ipaddress] = floating_mac.claim_static_ips(
> > +            alloc_type=IPADDRESS_TYPE.USER_RESERVED,
> user=self.logged_in_user)
> > +
> > +        self.post_release_request(ipaddress.ip)
> > +        self.assertIsNone(reload_object(floating_mac))
> > +
> > +    def test_POST_does_not_delete_MACs_linked_to_nodes(self):
>
> POST_release?
>

Oops. Yep.


>
> > +        self.patch(ip_addresses_module, 'remove_host_maps')
> > +
> > +        interface = self.make_interface()
> > +        node = factory.make_Node(nodegroup=interface.nodegroup)
> > +        attached_mac = factory.make_MACAddress(
> > +            node=node, cluster_interface=interface)
> > +        [ipaddress] = attached_mac.claim_static_ips(
> > +            alloc_type=IPADDRESS_TYPE.USER_RESERVED,
> user=self.logged_in_user)
> > +
> > +        self.post_release_request(ipaddress.ip)
> > +        self.assertEqual(attached_mac, reload_object(attached_mac))
> > +
> > +    def test_POST_release_updates_DNS_and_DHCP(self):
> > +        remove_host_maps = self.patch(ip_addresses_module,
> 'remove_host_maps')
> > +
> > +        interface = self.make_interface()
> > +        floating_mac =
> factory.make_MACAddress(cluster_interface=interface)
> > +        [ipaddress] = floating_mac.claim_static_ips(
> > +            alloc_type=IPADDRESS_TYPE.USER_RESERVED,
> user=self.logged_in_user)
> > +
> > +        self.post_release_request(ipaddress.ip)
> > +        self.expectThat(
> > +            remove_host_maps, MockCalledOnceWith(
> > +                {interface.nodegroup: [ipaddress.ip]}))
> > +
> > +    def test_POST_raises_503_if_removing_host_maps_errors(self):
> > +        remove_host_maps = self.patch(ip_addresses_module,
> 'remove_host_maps')
> > +        # Failures in remove_host_maps() will be RPC-related exceptions,
> > +        # so we use one of those explicitly.
> > +        remove_host_maps.return_value = [
> > +            Failure(
> > +                NoConnectionsAvailable(
> > +                    "The wizard's staff has a knob on the end."))
> > +            ]
> > +
> > +        interface = self.make_interface()
> > +        floating_mac =
> factory.make_MACAddress(cluster_interface=interface)
> > +        [ipaddress] = floating_mac.claim_static_ips(
> > +            alloc_type=IPADDRESS_TYPE.USER_RESERVED,
> user=self.logged_in_user)
> > +
> > +        response = self.post_release_request(ipaddress.ip)
> > +        self.expectThat(
> > +            response.status_code, Equals(httplib.SERVICE_UNAVAILABLE))
> > +
> > +        # The static IP hasn't been deleted.
> > +        self.expectThat(
> > +            reload_object(ipaddress), Not(Is(None)))
> > +
> > +        # Neither has the DHCPHost.
> > +        self.expectThat(
> > +            reload_object(floating_mac), Not(Is(None)))
> > +
> >      def test_POST_release_does_not_delete_IP_that_I_dont_own(self):
> >          ipaddress =
> factory.make_StaticIPAddress(user=factory.make_User())
> >          response = self.post_release_request(ipaddress.ip)
> >
> > === modified file 'src/maasserver/exceptions.py'
> > --- src/maasserver/exceptions.py      2014-10-08 09:43:51 +0000
> > +++ src/maasserver/exceptions.py      2014-11-25 14:41:46 +0000
> > @@ -140,6 +140,12 @@
> >      api_error = httplib.CONFLICT
> >
> >
> > +class StaticIPAlreadyExistsForMACAddress(MAASAPIException):
> > +    """Raised when trying to allocate a static IP for a non-node MAC
> > +    where a node with that MAC already exists."""
> > +    api_error = httplib.CONFLICT
> > +
> > +
> >  class NodeActionError(MAASException):
> >      """Raised when there is an error performing a NodeAction."""
> >
> >
> > === modified file 'src/maasserver/models/macaddress.py'
> > --- src/maasserver/models/macaddress.py       2014-11-21 20:25:54 +0000
> > +++ src/maasserver/models/macaddress.py       2014-11-25 14:41:46 +0000
> > @@ -204,7 +204,7 @@
> >          return allocations
> >
> >      def _allocate_static_address(self, cluster_interface, alloc_type,
> > -                                 requested_address=None):
> > +                                 requested_address=None, user=None):
> >          """Allocate a `StaticIPAddress` for this MAC."""
> >          # Avoid circular imports.
> >          from maasserver.models import (
> > @@ -215,12 +215,13 @@
> >          new_sip = StaticIPAddress.objects.allocate_new(
> >              cluster_interface.static_ip_range_low,
> >              cluster_interface.static_ip_range_high,
> > -            alloc_type, requested_address=requested_address)
> > +            alloc_type, requested_address=requested_address,
> > +            user=user)
> >          MACStaticIPAddressLink(mac_address=self,
> ip_address=new_sip).save()
> >          return new_sip
> >
> >      def claim_static_ips(self, alloc_type=IPADDRESS_TYPE.AUTO,
> > -                         requested_address=None):
> > +                         requested_address=None, user=None):
> >          """Assign static IP addresses to this MAC.
> >
> >          Allocates one address per managed cluster interface connected
> to this
> > @@ -235,6 +236,8 @@
> >              the range defined on some cluster interface to which this
> >              MACAddress is related.  If given, no allocations will be
> made on
> >              any other cluster interfaces the MAC may be connected to.
> > +        :param user: Optional User who will be given ownership of any
> > +            `StaticIPAddress`es claimed.
> >          :return: A list of :class:`StaticIPAddress`.  Returns empty if
> >              the cluster_interface is not yet known, or the
> >              static_ip_range_low/high values values are not set on the
> > @@ -257,9 +260,13 @@
> >          # different representations for "none" values in IP addresses.
> >          if self.cluster_interface is None:
> >              # No known cluster interface.  Nothing we can do.
> > +            if self.node is not None:
> > +                hostname_string = "%s: " % self.node.hostname
> > +            else:
> > +                hostname_string = ""
> >              maaslog.error(
> > -                "%s: Tried to allocate an IP to MAC %s but its cluster "
> > -                "interface is not known", self.node.hostname, self)
> > +                "%sTried to allocate an IP to MAC %s but its cluster "
> > +                "interface is not known", hostname_string, self)
> >              return []
> >          cluster_interfaces = [
> >              interface
> > @@ -301,7 +308,7 @@
> >              if allocations[interface] is None:
> >                  # No IP address yet on this cluster interface.  Get one.
> >                  allocations[interface] = self._allocate_static_address(
> > -                    interface, alloc_type, requested_address)
> > +                    interface, alloc_type, requested_address, user=user)
> >
> >          # We now have a static IP allocated to each of our cluster
> interfaces.
> >          # Ignore the clashes.  Return the ones that have the right
> type: those
> >
> > === modified file 'src/maasserver/models/tests/test_macaddress.py'
> > --- src/maasserver/models/tests/test_macaddress.py    2014-11-25
> 14:23:55 +0000
> > +++ src/maasserver/models/tests/test_macaddress.py    2014-11-25
> 14:41:46 +0000
> > @@ -635,6 +635,16 @@
> >          [sip] = allocation
> >          self.assertEqual(IPAddress(requested_ip), IPAddress(sip.ip))
> >
> > +    def test__links_static_ip_to_user_if_passed(self):
> > +        cluster = factory.make_NodeGroup()
> > +        cluster_interface = factory.make_NodeGroupInterface(cluster)
> > +        mac_address = factory.make_MACAddress(
> > +            cluster_interface=cluster_interface)
> > +        user = factory.make_User()
> > +        [sip] = mac_address.claim_static_ips(
> > +            user=user, alloc_type=IPADDRESS_TYPE.USER_RESERVED)
> > +        self.assertEqual(sip.user, user)
> > +
> >
> >  class TestGetClusterInterfaces(MAASServerTestCase):
> >      """Tests for `MACAddress.get_cluster_interfaces`."""
> >
> > === modified file 'src/maasserver/testing/factory.py'
> > --- src/maasserver/testing/factory.py 2014-11-25 14:23:55 +0000
> > +++ src/maasserver/testing/factory.py 2014-11-25 14:41:46 +0000
> > @@ -461,8 +461,6 @@
> >      def make_MACAddress(self, address=None, node=None, networks=None,
> >                          **kwargs):
> >          """Create a `MACAddress` model object."""
> > -        if node is None:
> > -            node = self.make_Node()
> >          if address is None:
> >              address = self.make_mac_address()
> >          mac = MACAddress(mac_address=MAC(address), node=node, **kwargs)
> >
>
>
> --
>
> https://code.launchpad.net/~gmb/maas/add-api-reserve-sip-for-mac-bug-1387239/+merge/242788
> You are the owner of lp:~gmb/maas/add-api-reserve-sip-for-mac-bug-1387239.
>