NSX|P: support multiple loadbalancers on a router

The loadbalancers using the router LB service will be marked on
a new tag on the NSX service.

Also adin an admin utility to update existing Lb services with the tag.

Change-Id: I6c38b45e4d683681a6915fd07ca296264c7d2495

doc/source/admin_util.rst

@@ -640,15 +640,19 @@ NSX Policy Plugin
 
 - Replace an old tier0 (that might have been deleted) with a new one::
 
-    nsxadmin -r routers -o  update-tier0  --property old-tier0=<id> --property new-tier0=<id> 
+    nsxadmin -r routers -o  update-tier0  --property old-tier0=<id> --property new-tier0=<id>
 
 - Update the firewall_match value in neutron nat rules with a new value. Should be used when firewall_match_internal_addr config changes::
 
-    nsxadmin -r routers -o  update-nat-firewall-match --property firewall-match=external/internal 
+    nsxadmin -r routers -o  update-nat-firewall-match --property firewall-match=external/internal
 
 - Migrate networks DHCP from MP to Policy (for NSX 3.0 upgrades)::
     nsxadmin -r dhcp-binding -o migrate-to-policy --property dhcp-config=<id>
 
+- Update tags on a loadbalancer service
+    nsxadmin -r lb-services -o nsx-update-tags
+
+
 Client Certificate
 ~~~~~~~~~~~~~~~~~~
 

vmware_nsx/plugins/nsx_p/plugin.py

@@ -83,6 +83,7 @@ from vmware_nsx.services.lbaas import lb_const
 from vmware_nsx.services.lbaas.nsx_p.implementation import healthmonitor_mgr
 from vmware_nsx.services.lbaas.nsx_p.implementation import l7policy_mgr
 from vmware_nsx.services.lbaas.nsx_p.implementation import l7rule_mgr
+from vmware_nsx.services.lbaas.nsx_p.implementation import lb_utils
 from vmware_nsx.services.lbaas.nsx_p.implementation import listener_mgr
 from vmware_nsx.services.lbaas.nsx_p.implementation import loadbalancer_mgr
 from vmware_nsx.services.lbaas.nsx_p.implementation import member_mgr
@@ -2381,14 +2382,8 @@ class NsxPolicyPlugin(nsx_plugin_common.NsxPluginV3Base):
                 vlan_interfaces)
 
     def service_router_has_loadbalancers(self, context, router_id):
-        tags_to_search = [{'scope': lb_const.LR_ROUTER_TYPE, 'tag': router_id}]
+        service = lb_utils.get_router_nsx_lb_service(self.nsxpolicy, router_id)
-        router_lb_services = self.nsxpolicy.search_by_tags(
+        return True if service else False
-            tags_to_search,
-            self.nsxpolicy.load_balancer.lb_service.entry_def.resource_type()
-        )['results']
-        non_delete_services = [srv for srv in router_lb_services
-                               if not srv.get('marked_for_delete')]
-        return True if non_delete_services else False
 
     def service_router_has_vpnaas(self, context, router_id):
         """Return True if there is a vpn service attached to this router"""

vmware_nsx/services/lbaas/nsx_p/implementation/lb_utils.py

@@ -19,6 +19,7 @@ from neutron_lib import exceptions as n_exc
 from oslo_log import log as logging
 
 from vmware_nsx._i18n import _
+from vmware_nsx.common import locking
 from vmware_nsx.services.lbaas import lb_const
 from vmware_nsx.services.lbaas.nsx_p.implementation import lb_const as p_const
 from vmware_nsx.services.lbaas.nsx_v3.implementation import lb_utils
@@ -29,6 +30,10 @@ from vmware_nsxlib.v3 import utils
 
 LOG = logging.getLogger(__name__)
 ADV_RULE_NAME = 'LB external VIP advertisement'
+SERVICE_LB_TAG_SCOPE = 'loadbalancer_id'
+#TODO(asarfaty): allow more LBs on the same router by setting multiple
+# ids in the same tag
+SERVICE_LB_TAG_MAX = 20
 
 
 def get_rule_match_conditions(policy):
@@ -259,3 +264,92 @@ def setup_session_persistence(nsxpolicy, pool, pool_tags, switch_type,
 
         return pp_id, None
     return None, None
+
+
+def get_router_nsx_lb_service(nsxpolicy, router_id):
+    tags_to_search = [{'scope': lb_const.LR_ROUTER_TYPE, 'tag': router_id}]
+    router_lb_services = nsxpolicy.search_by_tags(
+        tags_to_search,
+        nsxpolicy.load_balancer.lb_service.entry_def.resource_type()
+    )['results']
+    non_delete_services = [srv for srv in router_lb_services
+                           if not srv.get('marked_for_delete')]
+    if non_delete_services:
+        return non_delete_services[0]
+
+
+def get_lb_nsx_lb_service(nsxpolicy, lb_id):
+    tags_to_search = [{'scope': SERVICE_LB_TAG_SCOPE, 'tag': lb_id}]
+    lb_services = nsxpolicy.search_by_tags(
+        tags_to_search,
+        nsxpolicy.load_balancer.lb_service.entry_def.resource_type()
+    )['results']
+    non_delete_services = [srv for srv in lb_services
+                           if not srv.get('marked_for_delete')]
+    if non_delete_services:
+        return non_delete_services[0]
+
+
+def get_service_lb_name(lb, router_id=None):
+    if router_id:
+        return utils.get_name_and_uuid('rtr', router_id)
+    else:
+        return utils.get_name_and_uuid(lb.get('name') or 'lb', lb.get('id'))
+
+
+def get_service_lb_tag(lb_id):
+    return {'scope': SERVICE_LB_TAG_SCOPE, 'tag': lb_id}
+
+
+def add_service_tag_callback(lb_id, only_first=False):
+    """Return a callback that will validate and add a tag to the lb service"""
+    def _update_calback(body):
+        count = 0
+        for tag in body.get('tags', []):
+            if tag.get('scope') == SERVICE_LB_TAG_SCOPE:
+                if only_first:
+                    msg = _("A loadbalancer tag is already attached to the"
+                            " service")
+                    raise n_exc.BadRequest(
+                        resource='lbaas-loadbalancer-create', msg=msg)
+                if tag.get('tag') == lb_id:
+                    # No need to update
+                    return
+                count = count + 1
+        if count >= SERVICE_LB_TAG_MAX:
+            msg = _("Too many loadbalancers on the same router")
+            raise n_exc.BadRequest(resource='lbaas-loadbalancer-create',
+                                   msg=msg)
+        if 'tags' in body:
+            body['tags'].append(get_service_lb_tag(lb_id))
+        else:
+            body['tags'] = [get_service_lb_tag(lb_id)]
+
+    return _update_calback
+
+
+def remove_service_tag_callback(lb_id):
+    """Return a callback that will remove the tag from the lb service
+    If it is the last tag raise an error so that the service can be deleted
+    """
+    def _update_calback(body):
+        count = 0
+        match_tag = None
+        for tag in body.get('tags', []):
+            if tag.get('scope') == SERVICE_LB_TAG_SCOPE:
+                count = count + 1
+                if tag.get('tag') == lb_id:
+                    match_tag = tag
+        if match_tag:
+            if count <= 1:
+                msg = _("This LB service should be deleted")
+                raise n_exc.BadRequest(resource='lbaas-loadbalancer-delete',
+                                       msg=msg)
+            else:
+                body['tags'].remove(match_tag)
+
+    return _update_calback
+
+
+def get_lb_rtr_lock(router_id):
+    return locking.LockManager.get_lock('lb-router-%s' % str(router_id))

vmware_nsx/services/lbaas/nsx_p/implementation/loadbalancer_mgr.py

@@ -25,7 +25,6 @@ from vmware_nsx.services.lbaas.nsx_v3.implementation import lb_utils
 from vmware_nsx.services.lbaas.octavia import constants as oct_const
 from vmware_nsxlib.v3 import exceptions as nsxlib_exc
 from vmware_nsxlib.v3.policy import utils as lib_p_utils
-from vmware_nsxlib.v3 import utils
 
 LOG = logging.getLogger(__name__)
 
@@ -51,53 +50,74 @@ class EdgeLoadBalancerManagerFromDict(base_mgr.NsxpLoadbalancerBaseManager):
                      'router') %
                    {'lb_id': lb_id, 'subnet': lb['vip_subnet_id']})
             raise n_exc.BadRequest(resource='lbaas-subnet', msg=msg)
-
-        if router_id:
-            # Validate that there is no other LB on this router
-            # as NSX does not allow it
-            if self.core_plugin.service_router_has_loadbalancers(
-                context.elevated(), router_id):
-                completor(success=False)
-                msg = (_('Cannot create a loadbalancer %(lb_id)s on router '
-                         '%(router)s, as it already has a loadbalancer') %
-                       {'lb_id': lb_id, 'router': router_id})
-                raise n_exc.BadRequest(resource='lbaas-router', msg=msg)
-
-            # Create the service router if it does not exist
-            if not self.core_plugin.service_router_has_services(
-                context.elevated(), router_id):
-                self.core_plugin.create_service_router(context, router_id)
-
-        lb_name = utils.get_name_and_uuid(lb['name'] or 'lb',
-                                          lb_id)
-        tags = p_utils.get_tags(self.core_plugin,
-                                router_id if router_id else '',
-                                lb_const.LR_ROUTER_TYPE,
-                                lb['tenant_id'], context.project_name)
-
-        lb_size = lb_utils.get_lb_flavor_size(self.flavor_plugin, context,
-                                              lb.get('flavor_id'))
-
         service_client = self.core_plugin.nsxpolicy.load_balancer.lb_service
-        try:
+        lb_service = None
-            if network and network.get('router:external'):
+        if router_id:
-                connectivity_path = None
+            # Check if a service was already created for this router
-            else:
+            lb_service = p_utils.get_router_nsx_lb_service(
-                connectivity_path = self.core_plugin.nsxpolicy.tier1.get_path(
+                self.core_plugin.nsxpolicy, router_id)
-                    router_id)
-            service_client.create_or_overwrite(
-                lb_name, lb_service_id=lb['id'], description=lb['description'],
-                tags=tags, size=lb_size, connectivity_path=connectivity_path)
 
-            # Add rule to advertise external vips
+            if lb_service:
-            if router_id:
+                # Add the new LB to the service by adding the tag.
-                p_utils.update_router_lb_vip_advertisement(
+                # At the same time verify maximum number of tags
-                    context, self.core_plugin, router_id)
+                try:
-        except Exception as e:
+                    with p_utils.get_lb_rtr_lock(router_id):
-            with excutils.save_and_reraise_exception():
+                        service_client.update_customized(
-                completor(success=False)
+                            lb_service['id'],
-                LOG.error('Failed to create loadbalancer %(lb)s for lb with '
+                            p_utils.add_service_tag_callback(lb['id']))
-                          'exception %(e)s', {'lb': lb['id'], 'e': e})
+                except n_exc.BadRequest:
+                    # This will fail if there are too many loadbalancers
+                    completor(success=False)
+                    msg = (_('Cannot create a loadbalancer %(lb_id)s on '
+                             'router %(router)s, as it already has too many '
+                             'loadbalancers') %
+                           {'lb_id': lb_id, 'router': router_id})
+                    raise n_exc.BadRequest(resource='lbaas-router', msg=msg)
+                except Exception as e:
+                    with excutils.save_and_reraise_exception():
+                        completor(success=False)
+                        LOG.error('Failed to create loadbalancer %(lb)s for '
+                                  'lb with exception %(e)s',
+                                  {'lb': lb['id'], 'e': e})
+            else:
+                # Create the Tier1 service router if it does not exist
+                if not self.core_plugin.service_router_has_services(
+                    context.elevated(), router_id):
+                    self.core_plugin.create_service_router(context, router_id)
+
+        if not lb_service:
+            lb_name = p_utils.get_service_lb_name(lb, router_id)
+            tags = p_utils.get_tags(self.core_plugin,
+                                    router_id if router_id else '',
+                                    lb_const.LR_ROUTER_TYPE,
+                                    lb['tenant_id'], context.project_name)
+            tags.append(p_utils.get_service_lb_tag(lb['id']))
+
+            lb_size = lb_utils.get_lb_flavor_size(self.flavor_plugin, context,
+                                                  lb.get('flavor_id'))
+
+            try:
+                if network and network.get('router:external'):
+                    connectivity_path = None
+                else:
+                    tier1_srv = self.core_plugin.nsxpolicy.tier1
+                    connectivity_path = tier1_srv.get_path(router_id)
+                with p_utils.get_lb_rtr_lock(router_id):
+                    service_client.create_or_overwrite(
+                        lb_name, lb_service_id=lb['id'],
+                        description=lb['description'],
+                        tags=tags, size=lb_size,
+                        connectivity_path=connectivity_path)
+
+                # Add rule to advertise external vips
+                if router_id:
+                    p_utils.update_router_lb_vip_advertisement(
+                        context, self.core_plugin, router_id)
+            except Exception as e:
+                with excutils.save_and_reraise_exception():
+                    completor(success=False)
+                    LOG.error('Failed to create loadbalancer %(lb)s for lb '
+                              'with exception %(e)s', {'lb': lb['id'], 'e': e})
 
         # Make sure the vip port is marked with a device owner
         port = self.core_plugin.get_port(
@@ -120,39 +140,57 @@ class EdgeLoadBalancerManagerFromDict(base_mgr.NsxpLoadbalancerBaseManager):
         except n_exc.SubnetNotFound:
             LOG.warning("VIP subnet %s not found while deleting "
                         "loadbalancer %s", lb['vip_subnet_id'], lb['id'])
-
         service_client = self.core_plugin.nsxpolicy.load_balancer.lb_service
-
+        service = p_utils.get_lb_nsx_lb_service(
-        if not router_id:
+            self.core_plugin.nsxpolicy, lb['id'])
-            # Try to get it from the service
+        if service:
-            try:
+            lb_service_id = service['id']
-                service = service_client.get(lb['id'])
+            if not router_id:
-            except nsxlib_exc.ResourceNotFound:
-                pass
-            else:
                 router_id = lib_p_utils.path_to_id(
                     service.get('connectivity_path', ''))
-        try:
-            service_client.delete(lb['id'])
-        except Exception as e:
-            with excutils.save_and_reraise_exception():
-                completor(success=False)
-                LOG.error('Failed to delete loadbalancer %(lb)s for lb '
-                          'with error %(err)s',
-                          {'lb': lb['id'], 'err': e})
 
-        # if no router for vip - should check the member router
+            if router_id:
-        if router_id:
+                with p_utils.get_lb_rtr_lock(router_id):
-            try:
+                    # check if this is the last lb for this router and update
-                if not self.core_plugin.service_router_has_services(
+                    # tags / delete service
-                        context.elevated(), router_id):
+                    try:
-                    self.core_plugin.delete_service_router(router_id)
+                        service_client.update_customized(
-            except Exception as e:
+                            lb_service_id,
-                with excutils.save_and_reraise_exception():
+                            p_utils.remove_service_tag_callback(lb['id']))
-                    completor(success=False)
+                    except n_exc.BadRequest:
-                    LOG.error('Failed to delete service router upon deletion '
+                        # This is the last Lb and service should be deleted
-                              'of loadbalancer %(lb)s with error %(err)s',
+                        try:
-                              {'lb': lb['id'], 'err': e})
+                            service_client.delete(lb_service_id)
+                        except Exception as e:
+                            with excutils.save_and_reraise_exception():
+                                completor(success=False)
+                                LOG.error('Failed to delete loadbalancer '
+                                          '%(lb)s for lb with error %(err)s',
+                                          {'lb': lb['id'], 'err': e})
+                        # Remove the service router if no more services
+                        if not self.core_plugin.service_router_has_services(
+                                context.elevated(), router_id):
+                            try:
+                                self.core_plugin.delete_service_router(
+                                    router_id)
+                            except Exception as e:
+                                with excutils.save_and_reraise_exception():
+                                    completor(success=False)
+                                    LOG.error('Failed to delete service '
+                                              'router upon deletion '
+                                              'of loadbalancer %(lb)s with '
+                                              'error %(err)s',
+                                              {'lb': lb['id'], 'err': e})
+            else:
+                # LB without router (meaning external vip and no members)
+                try:
+                    service_client.delete(lb_service_id)
+                except Exception as e:
+                    with excutils.save_and_reraise_exception():
+                        completor(success=False)
+                        LOG.error('Failed to delete loadbalancer %(lb)s for '
+                                  'lb with error %(err)s',
+                                  {'lb': lb['id'], 'err': e})
 
         # Make sure the vip port is not marked with a vmware device owner
         try:

vmware_nsx/services/lbaas/nsx_p/implementation/member_mgr.py

@@ -61,20 +61,17 @@ class EdgeMemberManagerFromDict(base_mgr.NsxpLoadbalancerBaseManager):
         # attach it to a router. If not, set the LB service connectivity path
         # to the member subnet's router.
         service_client = self.core_plugin.nsxpolicy.load_balancer.lb_service
-        service = service_client.get(lb['id'])
+        service = p_utils.get_lb_nsx_lb_service(
+            self.core_plugin.nsxpolicy, lb['id'])
+        if not service:
+            completor(success=False)
+            msg = (_('Cannot find loadbalancer %(lb_id)s service') %
+                   {'lb_id': lb['id']})
+            raise n_exc.BadRequest(resource='lbaas-router', msg=msg)
+
         if not service.get('connectivity_path'):
             router_id = lb_utils.get_router_from_network(
                 context, self.core_plugin, member['subnet_id'])
-            # Validate that there is no other LB on this router
-            # as NSX does not allow it
-            if self.core_plugin.service_router_has_loadbalancers(
-                context.elevated(), router_id):
-                completor(success=False)
-                msg = (_('Cannot attach a loadbalancer %(lb_id)s on router '
-                         '%(router)s, as it already has a loadbalancer') %
-                       {'lb_id': lb['id'], 'router': router_id})
-                raise n_exc.BadRequest(resource='lbaas-router', msg=msg)
-
             if not self.core_plugin.service_router_has_services(context,
                                                                 router_id):
                 self.core_plugin.create_service_router(context, router_id)
@@ -84,21 +81,39 @@ class EdgeMemberManagerFromDict(base_mgr.NsxpLoadbalancerBaseManager):
             tags = p_utils.get_tags(self.core_plugin,
                                     router_id,
                                     lb_const.LR_ROUTER_TYPE,
-                                    lb.get('tenant_id'), context.project_name)
+                                    member.get('tenant_id'),
-            try:
+                                    context.project_name)
-                service_client.update(lb['id'],
+            tags.append(p_utils.get_service_lb_tag(lb['id']))
-                                      tags=tags,
+            lb_name = p_utils.get_service_lb_name(lb, router_id)
-                                      connectivity_path=connectivity_path)
+
-                p_utils.update_router_lb_vip_advertisement(
+            # Validate that there is no other LB on this router
-                    context, self.core_plugin, router_id)
+            # as NSX does not allow it
-            except Exception as e:
+            with p_utils.get_lb_rtr_lock(router_id):
-                with excutils.save_and_reraise_exception():
+                if self.core_plugin.service_router_has_loadbalancers(
+                    context.elevated(), router_id):
                     completor(success=False)
-                    LOG.error('Failed to set connectivity for loadbalancer '
+                    msg = (_('Cannot attach a loadbalancer %(lb_id)s on '
-                              '%(lb)s on subnet %(sub)s with error %(err)s',
+                             'router %(router)s, as it already has a '
-                              {'lb': lb['id'],
+                             'loadbalancer') %
-                               'sub': member['subnet_id'],
+                           {'lb_id': lb['id'], 'router': router_id})
-                               'err': e})
+                    raise n_exc.BadRequest(resource='lbaas-router', msg=msg)
+
+                try:
+                    service_client.update(service['id'],
+                                          name=lb_name,
+                                          tags=tags,
+                                          connectivity_path=connectivity_path)
+                    p_utils.update_router_lb_vip_advertisement(
+                        context, self.core_plugin, router_id)
+                except Exception as e:
+                    with excutils.save_and_reraise_exception():
+                        completor(success=False)
+                        LOG.error('Failed to set connectivity for '
+                                  'loadbalancer  %(lb)s on subnet %(sub)s '
+                                  'with error %(err)s',
+                                  {'lb': lb['id'],
+                                   'sub': member['subnet_id'],
+                                   'err': e})
 
     def create(self, context, member, completor):
         pool_client = self.core_plugin.nsxpolicy.load_balancer.lb_pool

vmware_nsx/shell/admin/plugins/nsxp/resources/loadbalancer.py

@@ -0,0 +1,61 @@
+# Copyright 2020 VMware, Inc.  All rights reserved.
+#
+#    Licensed under the Apache License, Version 2.0 (the "License"); you may
+#    not use this file except in compliance with the License. You may obtain
+#    a copy of the License at
+#
+#         http://www.apache.org/licenses/LICENSE-2.0
+#
+#    Unless required by applicable law or agreed to in writing, software
+#    distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+#    WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+#    License for the specific language governing permissions and limitations
+#    under the License.
+
+from neutron_lib.callbacks import registry
+from neutron_lib import exceptions as n_exc
+from oslo_log import log as logging
+
+from vmware_nsx.services.lbaas.nsx_p.implementation import lb_utils
+from vmware_nsx.shell.admin.plugins.common import constants
+from vmware_nsx.shell.admin.plugins.common import utils as admin_utils
+from vmware_nsx.shell.admin.plugins.nsxp.resources import utils as p_utils
+from vmware_nsx.shell import resources as shell
+
+LOG = logging.getLogger(__name__)
+
+
+@admin_utils.output_header
+def update_lb_service_tags(resource, event, trigger, **kwargs):
+    """Update the LB id tag on existing LB services"""
+    nsxpolicy = p_utils.get_connected_nsxpolicy()
+    service_client = nsxpolicy.load_balancer.lb_service
+    services = service_client.list()
+    n_updated = 0
+    for lb_service in services:
+        # First make sure it i a neutron service
+        is_neutron = False
+        for tag in lb_service.get('tags', []):
+            if tag['scope'] == 'os-api-version':
+                is_neutron = True
+                break
+        if is_neutron:
+            # Add a tag with the id of this resource as the first Lb
+            # creates the service with its id
+            try:
+                service_client.update_customized(
+                    lb_service['id'],
+                    lb_utils.add_service_tag_callback(lb_service['id'],
+                                                      only_first=True))
+            except n_exc.BadRequest:
+                LOG.warning("Lb service %s already has a loadbalancer tag",
+                            lb_service['id'])
+            else:
+                n_updated = n_updated + 1
+
+    LOG.info("Done updating %s Lb services.", n_updated)
+
+
+registry.subscribe(update_lb_service_tags,
+                   constants.LB_SERVICES,
+                   shell.Operations.NSX_UPDATE_TAGS.value)

vmware_nsx/shell/resources.py

@@ -55,6 +55,7 @@ class Operations(enum.Enum):
     NSX_UPDATE_STATE = 'nsx-update-state'
     NSX_ENABLE_STANDBY_RELOCATION = 'nsx-enable-standby-relocation'
     NSX_UPDATE_IP = 'nsx-update-ip'
+    NSX_UPDATE_TAGS = 'nsx-update-tags'
     NSX_RECREATE = 'nsx-recreate'
     NSX_REDISTRIBURE = 'nsx-redistribute'
     NSX_REORDER = 'nsx-reorder'
@@ -269,6 +270,8 @@ nsxp_resources = {
                                 [Operations.LIST.value,
                                  Operations.UPDATE_TIER0.value,
                                  Operations.UPDATE_FIREWALL_MATCH.value]),
+    constants.LB_SERVICES: Resource(constants.LB_SERVICES,
+                                [Operations.NSX_UPDATE_TAGS.value]),
     constants.CERTIFICATE: Resource(constants.CERTIFICATE,
                                     [Operations.GENERATE.value,
                                      Operations.SHOW.value,

vmware_nsx/tests/unit/services/lbaas/test_nsxp_driver.py

@@ -19,6 +19,7 @@ from neutron_lib import context
 from neutron_lib import exceptions as n_exc
 
 from vmware_nsx.services.lbaas import base_mgr
+from vmware_nsx.services.lbaas import lb_const
 from vmware_nsx.services.lbaas.nsx_p.implementation import healthmonitor_mgr
 from vmware_nsx.services.lbaas.nsx_p.implementation import l7policy_mgr
 from vmware_nsx.services.lbaas.nsx_p.implementation import l7rule_mgr
@@ -133,6 +134,10 @@ class BaseTestEdgeLbaasV2(base.BaseTestCase):
         self.last_completor_succees = success
         self.last_completor_called = True
 
+    def reset_completor(self):
+        self.last_completor_succees = False
+        self.last_completor_called = False
+
     def setUp(self):
         super(BaseTestEdgeLbaasV2, self).setUp()
 
@@ -296,15 +301,18 @@ class TestEdgeLbaasV2Loadbalancer(BaseTestEdgeLbaasV2):
                                return_value=LB_NETWORK), \
             mock.patch.object(lb_utils, 'get_router_from_network',
                               return_value=ROUTER_ID),\
+            mock.patch.object(lb_utils, 'get_tags', return_value=[]),\
             mock.patch.object(self.core_plugin, 'get_router',
                               return_value=neutron_router), \
             mock.patch.object(self.core_plugin, '_find_router_gw_subnets',
                               return_value=[]),\
             mock.patch.object(self.core_plugin,
-                              'service_router_has_loadbalancers',
+                              'service_router_has_services',
-                              return_value=False) as plugin_has_lb,\
+                              return_value=False) as plugin_has_sr,\
             mock.patch.object(self.service_client, 'get_router_lb_service',
                               return_value=None),\
+            mock.patch.object(self.core_plugin.nsxpolicy, 'search_by_tags',
+                              return_value={'results': []}),\
             mock.patch.object(self.service_client, 'create_or_overwrite'
                               ) as create_service:
 
@@ -318,9 +326,49 @@ class TestEdgeLbaasV2Loadbalancer(BaseTestEdgeLbaasV2):
                 description=self.lb_dict['description'],
                 tags=mock.ANY, size='SMALL',
                 connectivity_path=mock.ANY)
-            plugin_has_lb.assert_called_once_with(mock.ANY, ROUTER_ID)
+            # Verify that the tags contain the loadbalancer id
+            actual_tags = create_service.mock_calls[0][-1]['tags']
+            found_tag = False
+            for tag in actual_tags:
+                if (tag['scope'] == p_utils.SERVICE_LB_TAG_SCOPE and
+                    tag['tag'] == LB_ID):
+                    found_tag = True
+            self.assertTrue(found_tag)
+            plugin_has_sr.assert_called_once_with(mock.ANY, ROUTER_ID)
 
-    def test_create_same_router_fail(self):
+    def test_create_same_router(self):
+        self.reset_completor()
+        neutron_router = {'id': ROUTER_ID, 'name': 'dummy',
+                          'external_gateway_info': {'external_fixed_ips': []}}
+        old_lb_id = 'aaa'
+        lb_service = {'id': old_lb_id,
+                      'tags': [{'scope': p_utils.SERVICE_LB_TAG_SCOPE,
+                                'tag': old_lb_id}]}
+        with mock.patch.object(lb_utils, 'get_network_from_subnet',
+                               return_value=LB_NETWORK), \
+            mock.patch.object(lb_utils, 'get_router_from_network',
+                              return_value=ROUTER_ID),\
+            mock.patch.object(self.core_plugin, 'get_router',
+                              return_value=neutron_router), \
+            mock.patch.object(self.core_plugin, '_find_router_gw_subnets',
+                              return_value=[]),\
+            mock.patch.object(self.core_plugin.nsxpolicy, 'search_by_tags',
+                              return_value={'results': [lb_service]}),\
+            mock.patch.object(self.core_plugin,
+                              'service_router_has_services',
+                              return_value=True) as plugin_has_sr,\
+            mock.patch.object(self.service_client,
+                              'update_customized') as service_update:
+            self.edge_driver.loadbalancer.create(
+                self.context, self.lb_dict, self.completor)
+            self.assertTrue(self.last_completor_called)
+            self.assertTrue(self.last_completor_succees)
+            plugin_has_sr.assert_not_called()
+            service_update.assert_called_once()
+
+    def test_create_same_router_many_fail(self):
+        lb_service = {'id': 'first_lb', 'tags': []}
+        self.reset_completor()
         neutron_router = {'id': ROUTER_ID, 'name': 'dummy',
                           'external_gateway_info': {'external_fixed_ips': []}}
         with mock.patch.object(lb_utils, 'get_network_from_subnet',
@@ -331,9 +379,11 @@ class TestEdgeLbaasV2Loadbalancer(BaseTestEdgeLbaasV2):
                               return_value=neutron_router), \
             mock.patch.object(self.core_plugin, '_find_router_gw_subnets',
                               return_value=[]),\
-            mock.patch.object(self.core_plugin,
+            mock.patch.object(self.core_plugin.nsxpolicy, 'search_by_tags',
-                              'service_router_has_loadbalancers',
+                              return_value={'results': [lb_service]}),\
-                              return_value=True) as plugin_has_lb,\
+            mock.patch.object(self.service_client, 'update_customized',
+                              side_effect=n_exc.BadRequest(resource='', msg='')
+                              ) as service_update,\
             mock.patch.object(self.service_client, 'get_router_lb_service',
                               return_value=None):
             self.assertRaises(
@@ -342,7 +392,7 @@ class TestEdgeLbaasV2Loadbalancer(BaseTestEdgeLbaasV2):
                 self.context, self.lb_dict, self.completor)
             self.assertTrue(self.last_completor_called)
             self.assertFalse(self.last_completor_succees)
-            plugin_has_lb.assert_called_once_with(mock.ANY, ROUTER_ID)
+            service_update.assert_called_once()
 
     def test_create_external_vip(self):
         with mock.patch.object(lb_utils, 'get_router_from_network',
@@ -366,6 +416,79 @@ class TestEdgeLbaasV2Loadbalancer(BaseTestEdgeLbaasV2):
                 tags=mock.ANY, size='SMALL',
                 connectivity_path=None)
 
+    def test_create_no_services(self):
+        self.reset_completor()
+        neutron_router = {'id': ROUTER_ID, 'name': 'dummy',
+                          'external_gateway_info': {'external_fixed_ips': []}}
+        with mock.patch.object(lb_utils, 'get_network_from_subnet',
+                               return_value=LB_NETWORK), \
+            mock.patch.object(lb_utils, 'get_router_from_network',
+                              return_value=ROUTER_ID),\
+            mock.patch.object(self.core_plugin, 'get_router',
+                              return_value=neutron_router), \
+            mock.patch.object(self.core_plugin, '_find_router_gw_subnets',
+                              return_value=[]),\
+            mock.patch.object(self.core_plugin, 'service_router_has_services',
+                              return_value=False) as plugin_has_sr, \
+            mock.patch.object(self.service_client, 'get_router_lb_service',
+                              return_value=None),\
+            mock.patch.object(self.service_client, 'create_or_overwrite'
+                              ) as create_service,\
+            mock.patch.object(self.core_plugin.nsxpolicy, 'search_by_tags',
+                              return_value={'results': []}),\
+            mock.patch.object(self.core_plugin,
+                              "create_service_router") as create_sr:
+            self.edge_driver.loadbalancer.create(
+                self.context, self.lb_dict, self.completor)
+            self.assertTrue(self.last_completor_called)
+            self.assertTrue(self.last_completor_succees)
+            # Service should be created with connectivity path
+            create_service.assert_called_once_with(
+                mock.ANY, lb_service_id=LB_ID,
+                description=self.lb_dict['description'],
+                tags=mock.ANY, size='SMALL',
+                connectivity_path=mock.ANY)
+            plugin_has_sr.assert_called_once_with(mock.ANY, ROUTER_ID)
+            create_sr.assert_called_once()
+
+    def test_create_with_port(self):
+        self.reset_completor()
+        neutron_router = {'id': ROUTER_ID, 'name': 'dummy',
+                          'external_gateway_info': {'external_fixed_ips': []}}
+        neutron_port = {'id': 'port-id', 'name': 'dummy', 'device_owner': ''}
+        with mock.patch.object(lb_utils, 'get_network_from_subnet',
+                               return_value=LB_NETWORK), \
+            mock.patch.object(lb_utils, 'get_router_from_network',
+                              return_value=ROUTER_ID),\
+            mock.patch.object(self.core_plugin, 'get_router',
+                              return_value=neutron_router), \
+            mock.patch.object(self.core_plugin, 'get_port',
+                              return_value=neutron_port), \
+            mock.patch.object(self.core_plugin, 'update_port'
+                              ) as update_port, \
+            mock.patch.object(self.core_plugin, '_find_router_gw_subnets',
+                              return_value=[]),\
+            mock.patch.object(self.core_plugin,
+                              'service_router_has_services',
+                              return_value=False) as plugin_has_sr,\
+            mock.patch.object(self.service_client, 'get_router_lb_service',
+                              return_value=None),\
+            mock.patch.object(self.service_client, 'create_or_overwrite'
+                              ) as create_service:
+
+            self.edge_driver.loadbalancer.create(
+                self.context, self.lb_dict, self.completor)
+            self.assertTrue(self.last_completor_called)
+            self.assertTrue(self.last_completor_succees)
+            # Service should be created with connectivity path
+            create_service.assert_called_once_with(
+                mock.ANY, lb_service_id=LB_ID,
+                description=self.lb_dict['description'],
+                tags=mock.ANY, size='SMALL',
+                connectivity_path=mock.ANY)
+            plugin_has_sr.assert_called_once_with(mock.ANY, ROUTER_ID)
+            update_port.assert_called_once()
+
     def test_update(self):
         new_lb = lb_models.LoadBalancer(LB_ID, 'yyy-yyy', 'lb1-new',
                                         'new-description', 'some-subnet',
@@ -377,18 +500,119 @@ class TestEdgeLbaasV2Loadbalancer(BaseTestEdgeLbaasV2):
         self.assertTrue(self.last_completor_succees)
 
     def test_delete(self):
+        self.reset_completor()
+        lb_service = {'id': LB_SERVICE_ID}
         with mock.patch.object(lb_utils, 'get_router_from_network',
                                return_value=ROUTER_ID),\
-            mock.patch.object(self.service_client, 'get'
+            mock.patch.object(self.service_client, 'update_customized',
-                              ) as mock_get_lb_service, \
+                              side_effect=n_exc.BadRequest(resource='', msg='')
+                              ) as service_update,\
+            mock.patch.object(self.core_plugin.nsxpolicy, 'search_by_tags',
+                              return_value={'results': [lb_service]}),\
+            mock.patch.object(self.service_client, 'delete'
+                              ) as mock_delete_lb_service:
+
+            self.edge_driver.loadbalancer.delete(
+                self.context, self.lb_dict, self.completor)
+
+            mock_delete_lb_service.assert_called_with(LB_SERVICE_ID)
+            service_update.assert_called_once()
+            self.assertTrue(self.last_completor_called)
+            self.assertTrue(self.last_completor_succees)
+
+    def test_delete_cascade(self):
+        self.reset_completor()
+        lb_service = {'id': LB_SERVICE_ID}
+        with mock.patch.object(lb_utils, 'get_router_from_network',
+                               return_value=ROUTER_ID),\
+            mock.patch.object(self.service_client, 'update_customized',
+                              side_effect=n_exc.BadRequest(resource='', msg='')
+                              ) as service_update,\
+            mock.patch.object(self.core_plugin.nsxpolicy, 'search_by_tags',
+                              return_value={'results': [lb_service]}),\
+            mock.patch.object(self.service_client, 'delete'
+                              ) as mock_delete_lb_service:
+
+            self.edge_driver.loadbalancer.delete_cascade(
+                self.context, self.lb_dict, self.completor)
+
+            mock_delete_lb_service.assert_called_with(LB_SERVICE_ID)
+            service_update.assert_called_once()
+            self.assertTrue(self.last_completor_called)
+            self.assertTrue(self.last_completor_succees)
+
+    def test_delete_with_router_id(self):
+        self.reset_completor()
+        lb_service = {'id': LB_SERVICE_ID,
+                      'connectivity_path': 'infra/%s' % ROUTER_ID}
+        with mock.patch.object(lb_utils, 'get_router_from_network',
+                               return_value=None),\
+            mock.patch.object(self.service_client, 'update_customized',
+                              side_effect=n_exc.BadRequest(resource='', msg='')
+                              ) as service_update,\
+            mock.patch.object(self.core_plugin.nsxpolicy, 'search_by_tags',
+                              return_value={'results': [lb_service]}),\
             mock.patch.object(self.service_client, 'delete'
                               ) as mock_delete_lb_service:
-            mock_get_lb_service.return_value = {'id': LB_SERVICE_ID}
 
             self.edge_driver.loadbalancer.delete(self.context, self.lb_dict,
                                                  self.completor)
 
             mock_delete_lb_service.assert_called_with(LB_SERVICE_ID)
+            service_update.assert_called_once()
+            self.assertTrue(self.last_completor_called)
+            self.assertTrue(self.last_completor_succees)
+
+    def test_delete_no_services(self):
+        self.reset_completor()
+        lb_service = {'id': LB_SERVICE_ID,
+                      'connectivity_path': 'infra/%s' % ROUTER_ID}
+        with mock.patch.object(lb_utils, 'get_router_from_network',
+                               return_value=None),\
+            mock.patch.object(self.service_client, 'update_customized',
+                              side_effect=n_exc.BadRequest(resource='', msg='')
+                              ) as service_update,\
+            mock.patch.object(self.core_plugin.nsxpolicy, 'search_by_tags',
+                              return_value={'results': [lb_service]}),\
+            mock.patch.object(self.core_plugin, 'service_router_has_services',
+                              return_value=False), \
+            mock.patch.object(self.core_plugin,
+                              'delete_service_router') as delete_sr, \
+            mock.patch.object(self.service_client, 'delete'
+                              ) as mock_delete_lb_service:
+            self.edge_driver.loadbalancer.delete(self.context, self.lb_dict,
+                                                 self.completor)
+
+            mock_delete_lb_service.assert_called_with(LB_SERVICE_ID)
+            delete_sr.assert_called_once_with(ROUTER_ID)
+            service_update.assert_called_once()
+            self.assertTrue(self.last_completor_called)
+            self.assertTrue(self.last_completor_succees)
+
+    def test_delete_with_port(self):
+        self.reset_completor()
+        lb_service = {'id': LB_SERVICE_ID}
+        neutron_port = {'id': 'port-id', 'name': 'dummy',
+                        'device_owner': lb_const.VMWARE_LB_VIP_OWNER}
+        with mock.patch.object(lb_utils, 'get_router_from_network',
+                               return_value=ROUTER_ID),\
+            mock.patch.object(self.service_client, 'update_customized',
+                              side_effect=n_exc.BadRequest(resource='', msg='')
+                              ) as service_update,\
+            mock.patch.object(self.core_plugin.nsxpolicy, 'search_by_tags',
+                              return_value={'results': [lb_service]}),\
+            mock.patch.object(self.core_plugin, 'get_port',
+                              return_value=neutron_port), \
+            mock.patch.object(self.core_plugin, 'update_port'
+                              ) as update_port, \
+            mock.patch.object(self.service_client, 'delete'
+                              ) as mock_delete_lb_service:
+            self.edge_driver.loadbalancer.delete(self.context, self.lb_dict,
+                                                 self.completor)
+
+            mock_delete_lb_service.assert_called_with(LB_SERVICE_ID)
+            service_update.assert_called_once()
+            update_port.assert_called_once()
             self.assertTrue(self.last_completor_called)
             self.assertTrue(self.last_completor_succees)
 
@@ -417,6 +641,68 @@ class TestEdgeLbaasV2Loadbalancer(BaseTestEdgeLbaasV2):
             self.assertEqual(0, len(statuses['listeners']))
             self.assertEqual(0, len(statuses['members']))
 
+    def test_add_tags_callback(self):
+        callback = p_utils.add_service_tag_callback(LB_ID)
+
+        # Add a tag
+        body = {'tags': [{'scope': p_utils.SERVICE_LB_TAG_SCOPE,
+                          'tag': 'dummy_id'}]}
+        callback(body)
+        self.assertEqual(2, len(body['tags']))
+
+        # Tag already there
+        callback(body)
+        self.assertEqual(2, len(body['tags']))
+
+        # Too many tags
+        body['tags'] = []
+        for x in range(p_utils.SERVICE_LB_TAG_MAX):
+            body['tags'].append({
+                'scope': p_utils.SERVICE_LB_TAG_SCOPE,
+                'tag': 'dummy_id_%s' % x})
+        self.assertRaises(n_exc.BadRequest, callback, body)
+
+        # No tags
+        body['tags'] = []
+        callback(body)
+        self.assertEqual(1, len(body['tags']))
+
+    def test_add_tags_callback_only_first(self):
+        callback = p_utils.add_service_tag_callback(LB_ID, only_first=True)
+
+        # No tags
+        body = {'tags': []}
+        callback(body)
+        self.assertEqual(1, len(body['tags']))
+
+        # Tag already there
+        self.assertRaises(n_exc.BadRequest, callback, body)
+
+        # Another tag exists
+        body['tags'] = [{'scope': p_utils.SERVICE_LB_TAG_SCOPE,
+                         'tag': 'dummy'}]
+        self.assertRaises(n_exc.BadRequest, callback, body)
+
+    def test_del_tags_callback(self):
+        callback = p_utils.remove_service_tag_callback(LB_ID)
+
+        # remove a tag
+        body = {'tags': [{'scope': p_utils.SERVICE_LB_TAG_SCOPE,
+                          'tag': 'dummy_id'},
+                         {'scope': p_utils.SERVICE_LB_TAG_SCOPE,
+                          'tag': LB_ID}]}
+        callback(body)
+        self.assertEqual(1, len(body['tags']))
+
+        # Tag not there there
+        callback(body)
+        self.assertEqual(1, len(body['tags']))
+
+        # Last one
+        body['tags'] = [{'scope': p_utils.SERVICE_LB_TAG_SCOPE,
+                         'tag': LB_ID}]
+        self.assertRaises(n_exc.BadRequest, callback, body)
+
 
 class TestEdgeLbaasV2Listener(BaseTestEdgeLbaasV2):
     def setUp(self):
@@ -1197,6 +1483,8 @@ class TestEdgeLbaasV2Member(BaseTestEdgeLbaasV2):
             self.assertTrue(self.last_completor_succees)
 
     def test_create_external_vip(self):
+        self.reset_completor()
+        lb_service = {'id': LB_SERVICE_ID}
         with mock.patch.object(self.lbv2_driver.plugin, 'get_pool_members'
                                ) as mock_get_pool_members, \
             mock.patch.object(lb_utils, 'get_network_from_subnet'
@@ -1205,11 +1493,14 @@ class TestEdgeLbaasV2Member(BaseTestEdgeLbaasV2):
                               ) as mock_get_router, \
             mock.patch.object(self.service_client, 'get_router_lb_service'
                               ) as mock_get_lb_service, \
-            mock.patch.object(self.service_client, 'get',
+            mock.patch.object(self.core_plugin.nsxpolicy, 'search_by_tags',
-                              return_value={}), \
+                              return_value={'results': [lb_service]}),\
+            mock.patch.object(self.core_plugin,
+                              'service_router_has_services',
+                              return_value=False) as plugin_has_sr,\
             mock.patch.object(self.core_plugin,
                               'service_router_has_loadbalancers',
-                              return_value=False) as plugin_has_lb,\
+                              return_value=False),\
             mock.patch.object(self.pool_client, 'get'
                               ) as mock_get_pool, \
             mock.patch.object(self.core_plugin, '_find_router_gw_subnets',
@@ -1237,7 +1528,42 @@ class TestEdgeLbaasV2Member(BaseTestEdgeLbaasV2):
                 admin_state='ENABLED')
             self.assertTrue(self.last_completor_called)
             self.assertTrue(self.last_completor_succees)
-            plugin_has_lb.assert_called_once_with(mock.ANY, LB_ROUTER_ID)
+            plugin_has_sr.assert_called_once_with(mock.ANY, LB_ROUTER_ID)
+
+    def test_create_external_vip_router_used(self):
+        self.reset_completor()
+        lb_service = {'id': LB_SERVICE_ID}
+        with mock.patch.object(self.lbv2_driver.plugin, 'get_pool_members'
+                               ) as mock_get_pool_members, \
+            mock.patch.object(lb_utils, 'get_network_from_subnet'
+                              ) as mock_get_network, \
+            mock.patch.object(lb_utils, 'get_router_from_network'
+                              ) as mock_get_router, \
+            mock.patch.object(self.service_client, 'get_router_lb_service'
+                              ) as mock_get_lb_service, \
+            mock.patch.object(self.core_plugin.nsxpolicy, 'search_by_tags',
+                              return_value={'results': [lb_service]}),\
+            mock.patch.object(self.core_plugin,
+                              'service_router_has_loadbalancers',
+                              return_value=True),\
+            mock.patch.object(self.pool_client, 'get'
+                              ) as mock_get_pool, \
+            mock.patch.object(self.core_plugin, '_find_router_gw_subnets',
+                              return_value=[]),\
+            mock.patch.object(self.core_plugin, 'get_floatingips',
+                              return_value=[{
+                                  'fixed_ip_address': MEMBER_ADDRESS}]):
+            mock_get_pool_members.return_value = [self.member]
+            mock_get_network.return_value = EXT_LB_NETWORK
+            mock_get_router.return_value = LB_ROUTER_ID
+            mock_get_lb_service.return_value = {'id': LB_SERVICE_ID}
+            mock_get_pool.return_value = LB_POOL
+
+            self.assertRaises(
+                n_exc.BadRequest, self.edge_driver.member.create,
+                self.context, self.member_dict, self.completor)
+            self.assertTrue(self.last_completor_called)
+            self.assertFalse(self.last_completor_succees)
 
     def test_update(self):
         new_member = lb_models.Member(MEMBER_ID, LB_TENANT_ID, POOL_ID,