Improve scheduling L3/DHCP agents, missing lower binding indexes

This patch is covering an edge case that could happen when the number
of DHCP agents ("dhcp_agents_per_network") or L3 agents
("max_l3_agents_per_router") has been reduced and there are more agents
assigned than the current number. If the user removes any agent
assignation from a L3 router or a DHCP agent, it is possible to remove
first the lower binding assigned registers.

Now the method ``get_vacant_binding_index`` calculates the number of
agents bound and the number required. If a new one is needed, the
method returns first the lower binding indexes not used.

Closes-Bug: #2006496

Conflicts:
    neutron/common/_constants.py
    neutron/objects/l3agent.py

Change-Id: I25145c088ffdca47acfcb7add02b1a4a615e4612
(cherry picked from commit 5250598)
(cherry picked from commit 0920f17)

Author: ralonsoh

neutron/common/_constants.py

@@ -78,3 +78,6 @@
 AUTO_DELETE_PORT_OWNERS = [constants.DEVICE_OWNER_DHCP,
                            constants.DEVICE_OWNER_DISTRIBUTED,
                            constants.DEVICE_OWNER_AGENT_GW]
+
+# The lowest binding index for L3 agents and DHCP agents.
+LOWEST_AGENT_BINDING_INDEX = 1

neutron/db/l3_agentschedulers_db.py

@@ -25,9 +25,9 @@
 import oslo_messaging
 
 from neutron.agent.common import utils as agent_utils
+from neutron.common import _constants as n_const
 from neutron.conf.db import l3_agentschedulers_db
 from neutron.db import agentschedulers_db
-from neutron.db.models import l3agent as rb_model
 from neutron.extensions import l3agentscheduler
 from neutron.extensions import router_availability_zone as router_az
 from neutron.objects import agent as ag_obj
@@ -522,7 +522,7 @@ def get_vacant_binding_index(self, context, router_id,
         bindings = rb_obj.RouterL3AgentBinding.get_objects(
                 context, _pager=pager, router_id=router_id)
         return base_scheduler.get_vacant_binding_index(
-            num_agents, bindings, rb_model.LOWEST_BINDING_INDEX,
+            num_agents, bindings, n_const.LOWEST_AGENT_BINDING_INDEX,
             force_scheduling=is_manual_scheduling)
 
 

neutron/db/models/l3agent.py

@@ -15,10 +15,9 @@
 import sqlalchemy as sa
 from sqlalchemy import orm
 
+from neutron.common import _constants as n_const
 from neutron.db.models import agent as agent_model
 
-LOWEST_BINDING_INDEX = 1
-
 
 class RouterL3AgentBinding(model_base.BASEV2):
     """Represents binding between neutron routers and L3 agents."""
@@ -37,5 +36,6 @@ class RouterL3AgentBinding(model_base.BASEV2):
     l3_agent_id = sa.Column(sa.String(36),
                             sa.ForeignKey("agents.id", ondelete='CASCADE'),
                             primary_key=True)
-    binding_index = sa.Column(sa.Integer, nullable=False,
-                              server_default=str(LOWEST_BINDING_INDEX))
+    binding_index = sa.Column(
+        sa.Integer, nullable=False,
+        server_default=str(n_const.LOWEST_AGENT_BINDING_INDEX))

neutron/db/network_dhcp_agent_binding/models.py

@@ -14,12 +14,10 @@
 import sqlalchemy as sa
 from sqlalchemy import orm
 
+from neutron.common import _constants as n_const
 from neutron.db.models import agent as agent_model
 
 
-LOWEST_BINDING_INDEX = 1
-
-
 class NetworkDhcpAgentBinding(model_base.BASEV2):
     """Represents binding between neutron networks and DHCP agents."""
 
@@ -38,5 +36,6 @@ class NetworkDhcpAgentBinding(model_base.BASEV2):
                               sa.ForeignKey("agents.id",
                                             ondelete='CASCADE'),
                               primary_key=True)
-    binding_index = sa.Column(sa.Integer, nullable=False,
-                              server_default=str(LOWEST_BINDING_INDEX))
+    binding_index = sa.Column(
+        sa.Integer, nullable=False,
+        server_default=str(n_const.LOWEST_AGENT_BINDING_INDEX))

neutron/objects/l3agent.py

@@ -17,6 +17,7 @@
 
 from sqlalchemy import sql
 
+from neutron.common import _constants as n_const
 from neutron.db.models import agent as agent_model
 from neutron.db.models import l3_attrs
 from neutron.db.models import l3agent
@@ -36,7 +37,7 @@ class RouterL3AgentBinding(base.NeutronDbObject):
         'router_id': common_types.UUIDField(),
         'l3_agent_id': common_types.UUIDField(),
         'binding_index': obj_fields.IntegerField(
-                             default=l3agent.LOWEST_BINDING_INDEX),
+            default=n_const.LOWEST_AGENT_BINDING_INDEX),
     }
 
     # TODO(ihrachys) return OVO objects not models

neutron/scheduler/base_scheduler.py

@@ -99,24 +99,44 @@ def get_vacant_binding_index(num_agents, bindings, lowest_binding_index,
                              always return an index, even if this number
                              exceeds the maximum configured number of agents.
     """
-    binding_indices = [b.binding_index for b in bindings]
-    all_indices = set(range(lowest_binding_index, num_agents + 1))
-    open_slots = sorted(list(all_indices - set(binding_indices)))
+    def get_open_slots(binding_indices, lowest_binding_index, max_number):
+        """Returns an ordered list of free slots
+
+        This list starts from the lowest available binding index. The number
+        of open slots and "binding_indices" (those already taken), must be
+        equal to "max_number". The list returned can be [], if
+        len(max_number) == len(binding_indices) (that means there are no free
+        slots).
+        """
+        # NOTE(ralonsoh): check LP#2006496 for more context. The DHCP/router
+        # binding indexes could not be a sequential list starting from
+        # lowest_binding_index (that is usually 1).
+        open_slots = set(binding_indices)
+        idx = lowest_binding_index
+        while len(open_slots) < max_number:
+            # Increase sequentially the "open_slots" set until we have the
+            # required number of slots, that is "num_agents".
+            open_slots.add(idx)
+            idx += 1
+
+        # Remove those indices already used.
+        open_slots -= set(binding_indices)
+        return sorted(list(open_slots))
 
+    binding_indices = [b.binding_index for b in bindings]
+    open_slots = get_open_slots(binding_indices, lowest_binding_index,
+                                num_agents)
     if open_slots:
         return open_slots[0]
 
     if not force_scheduling:
         return -1
 
-    # Last chance: if this is a manual scheduling, we're gonna allow
+    # Last chance: if this is a manual scheduling, we're going to allow
     # creation of a binding_index even if it will exceed
-    # dhcp_agents_per_network.
-    if max(binding_indices) == len(binding_indices):
-        return max(binding_indices) + 1
-    else:
-        # Find binding index set gaps and return first free one.
-        all_indices = set(range(lowest_binding_index,
-                                max(binding_indices) + 1))
-        open_slots = sorted(list(all_indices - set(binding_indices)))
-        return open_slots[0]
+    # dhcp_agents_per_network/max_l3_agents_per_router.
+    while not open_slots:
+        num_agents += 1
+        open_slots = get_open_slots(binding_indices, lowest_binding_index,
+                                    num_agents)
+    return open_slots[0]

neutron/scheduler/dhcp_agent_scheduler.py

@@ -25,7 +25,7 @@
 from oslo_log import log as logging
 
 from neutron.agent.common import utils as agent_utils
-from neutron.db.network_dhcp_agent_binding import models as ndab_model
+from neutron.common import _constants as n_const
 from neutron.objects import agent as agent_obj
 from neutron.objects import network
 from neutron.scheduler import base_resource_filter
@@ -195,7 +195,7 @@ def get_vacant_network_dhcp_agent_binding_index(
         bindings = network.NetworkDhcpAgentBinding.get_objects(
             context, network_id=network_id)
         return base_scheduler.get_vacant_binding_index(
-            num_agents, bindings, ndab_model.LOWEST_BINDING_INDEX,
+            num_agents, bindings, n_const.LOWEST_AGENT_BINDING_INDEX,
             force_scheduling=force_scheduling)
 
     def bind(self, context, agents, network_id, force_scheduling=False):
@@ -205,7 +205,7 @@ def bind(self, context, agents, network_id, force_scheduling=False):
         for agent in agents:
             binding_index = self.get_vacant_network_dhcp_agent_binding_index(
                 context, network_id, force_scheduling)
-            if binding_index < ndab_model.LOWEST_BINDING_INDEX:
+            if binding_index < n_const.LOWEST_AGENT_BINDING_INDEX:
                 LOG.debug('Unable to find a vacant binding_index for '
                           'network %(network_id)s and agent %(agent_id)s',
                           {'network_id': network_id,

neutron/scheduler/l3_agent_scheduler.py

@@ -26,6 +26,7 @@
 from oslo_db import exception as db_exc
 from oslo_log import log as logging
 
+from neutron.common import _constants as n_const
 from neutron.common import utils
 from neutron.conf.db import l3_hamode_db
 from neutron.db.models import l3agent as rb_model
@@ -185,7 +186,7 @@ def bind_router(self, plugin, context, router_id, agent_id,
             return
 
         if not is_ha:
-            binding_index = rb_model.LOWEST_BINDING_INDEX
+            binding_index = n_const.LOWEST_AGENT_BINDING_INDEX
             if rb_obj.RouterL3AgentBinding.objects_exist(
                     context, router_id=router_id, binding_index=binding_index):
                 LOG.debug('Non-HA router %s has already been scheduled',
@@ -194,7 +195,7 @@ def bind_router(self, plugin, context, router_id, agent_id,
         else:
             binding_index = plugin.get_vacant_binding_index(
                 context, router_id, is_manual_scheduling)
-            if binding_index < rb_model.LOWEST_BINDING_INDEX:
+            if binding_index < n_const.LOWEST_AGENT_BINDING_INDEX:
                 LOG.debug('Unable to find a vacant binding_index for '
                           'router %(router_id)s and agent %(agent_id)s',
                           {'router_id': router_id,

neutron/tests/unit/scheduler/test_base_scheduler.py

@@ -38,6 +38,21 @@ def test_get_vacant_binding_index_several_agents(self):
             3, [mock.Mock(binding_index=1), mock.Mock(binding_index=3)], 1)
         self.assertEqual(2, ret)
 
+        # Binding list starting in 2, two elements, required three.
+        ret = base_scheduler.get_vacant_binding_index(
+            3, [mock.Mock(binding_index=2), mock.Mock(binding_index=3)], 1)
+        self.assertEqual(1, ret)
+
+        # Binding list starting in 2, two elements, required two.
+        ret = base_scheduler.get_vacant_binding_index(
+            2, [mock.Mock(binding_index=2), mock.Mock(binding_index=3)], 1)
+        self.assertEqual(-1, ret)
+
+        # Binding list starting in 2, two elements, required one.
+        ret = base_scheduler.get_vacant_binding_index(
+            1, [mock.Mock(binding_index=2), mock.Mock(binding_index=3)], 1)
+        self.assertEqual(-1, ret)
+
     def test_get_vacant_binding_index_force_scheduling(self):
         ret = base_scheduler.get_vacant_binding_index(
             3, [mock.Mock(binding_index=1), mock.Mock(binding_index=2),
@@ -50,3 +65,9 @@ def test_get_vacant_binding_index_force_scheduling(self):
                 mock.Mock(binding_index=3), mock.Mock(binding_index=4),
                 mock.Mock(binding_index=5)], 1, force_scheduling=True)
         self.assertEqual(6, ret)
+
+        ret = base_scheduler.get_vacant_binding_index(
+            3, [mock.Mock(binding_index=2), mock.Mock(binding_index=3),
+                mock.Mock(binding_index=4), mock.Mock(binding_index=5),
+                mock.Mock(binding_index=6)], 1, force_scheduling=True)
+        self.assertEqual(1, ret)