From 488ed3a7a81cb7c303268b5921ba42db9c0ea980 Mon Sep 17 00:00:00 2001 From: Sridar Kandaswamy Date: Fri, 7 Aug 2020 09:20:09 -0700 Subject: [PATCH] When a subnet added to a bound SVI port, ensure it is added to SVI port When we add a subnet to a bound SVI port, we need to ensure that it is added to the SVI port as well. When the second subnet is added to a bound port on a SVI network, the SVI port will already be present - either created by the user or by the plugin when the binding happens. We check and add the subnet on the SVI port so it is present when the static path processing happens for the bound port to create the Interface Profile for each AF. Similarly, when we delete a subnet from a bound port - ensure that if this subnet is not present on the host, remove the Interface profile for the AF. Change-Id: I47cb516c3603eacb921527625120ed155dd2e465 (cherry picked from commit fc74d30ee86951587c8b3454af3e6afb608a3b0c) --- .../drivers/apic_aim/mechanism_driver.py | 116 ++++++++++++- .../unit/plugins/ml2plus/test_apic_aim.py | 157 ++++++++++++++++++ 2 files changed, 264 insertions(+), 9 deletions(-) diff --git a/gbpservice/neutron/plugins/ml2plus/drivers/apic_aim/mechanism_driver.py b/gbpservice/neutron/plugins/ml2plus/drivers/apic_aim/mechanism_driver.py index 28a291317..c0a15a1d9 100644 --- a/gbpservice/neutron/plugins/ml2plus/drivers/apic_aim/mechanism_driver.py +++ b/gbpservice/neutron/plugins/ml2plus/drivers/apic_aim/mechanism_driver.py @@ -2584,6 +2584,36 @@ class ApicMechanismDriver(api_plus.MechanismDriver, self.delete_port_id_for_ha_ipaddress( port['id'], ip, session=session) + def _update_svi_ports_for_added_subnet(self, ctx, new_subnets, network_id): + """ Subnet added to bound port on SVI net, add to SVI ports if needed. + + We have to ensure that the SVI ports are updated with the added + subnet _before_ we get into updating the static paths to generate + the Interface Profile for the AF of tne new subnet. Doing this + in the context of the BEFORE_UPDATE event guarantees that there + is no race condition. + + """ + # Get the SVI ports + filters = {'network_id': [network_id], + 'device_owner': [aim_cst.DEVICE_OWNER_SVI_PORT]} + svi_ports = self.plugin.get_ports(ctx, filters) + for svi_port in svi_ports: + subnet_added = False + svi_port_subnets = [s['subnet_id'] + for s in svi_port['fixed_ips']] + # update the SVI port if the added subnet is not present. + for s in new_subnets: + if s not in svi_port_subnets: + svi_port['fixed_ips'].append( + {'subnet_id': s}) + subnet_added = True + if subnet_added: + port_info = { + 'port': {'fixed_ips': svi_port['fixed_ips']} + } + self.plugin.update_port(ctx, svi_port['id'], port_info) + # NOTE: This event is generated by the ML2 plugin in three places, # all outside of transactions. It is generated from update_port # and from _update_individual_port_db_status without the @@ -2593,6 +2623,28 @@ class ApicMechanismDriver(api_plus.MechanismDriver, def _before_update_port(self, resource, event, trigger, context, port, original_port, orig_binding=None, new_binding=None): + if self._is_port_bound(original_port) and 'fixed_ips' in port: + # When a bound port is updated with a subnet, if the port + # is on a SVI network, we need to ensure that the SVI ports + # which are used for creating the Interface Profile for each + # AF also gets updated with the new subnet. + # + # Since we are not in the port binding workflow, can't get the + # bind_context + + curr = [s['subnet_id'] + for s in port['fixed_ips']] + orig = [s['subnet_id'] + for s in original_port['fixed_ips']] + new_subnets = list(set(curr) - set(orig)) + if not new_subnets: + return + ctx = nctx.get_admin_context() + net_db = self.plugin._get_network(ctx, original_port['network_id']) + if self._is_svi_db(net_db): + self._update_svi_ports_for_added_subnet(ctx, new_subnets, + original_port['network_id']) + # We are only interested in port update callbacks from # _commit_port_binding, in which the port is about to become # bound. Any other callbacks using this event are ignored. @@ -4681,7 +4733,8 @@ class ApicMechanismDriver(api_plus.MechanismDriver, {'port': attrs, 'ex': e}) def _update_static_path_for_svi(self, session, plugin_context, network, - l3out, static_port, remove=False): + l3out, static_port, remove=False, + del_subnet=None): """Configure static path for an SVI on a L3 Out Add, update, or delete a single static path on an SVI. Adds @@ -4699,7 +4752,7 @@ class ApicMechanismDriver(api_plus.MechanismDriver, # so if we're creating new nodes, then we need to allocate # new IDs for each one. aim_ctx = aim_context.AimContext(db_session=session) - if not remove and path: + if not remove and not del_subnet and path: for node_path in node_paths: # REVISIT: We should check to see if this node is # already present under the node profile. @@ -4731,7 +4784,6 @@ class ApicMechanismDriver(api_plus.MechanismDriver, 'mask': str(netaddr.IPNetwork( subnet['cidr']).prefixlen), 'primary_ips': []} - for node in nodes: # Get the IP of the SVI port for this node on this network. # @@ -4745,6 +4797,8 @@ class ApicMechanismDriver(api_plus.MechanismDriver, for fixed_ip in svi_ports[0]['fixed_ips']: ip_vers = netaddr.IPAddress( fixed_ip['ip_address']).version + if ip_vers not in subnets_dict: + continue subnets_dict[ip_vers]['primary_ips'].append( fixed_ip['ip_address'] + '/' + ( @@ -4805,10 +4859,26 @@ class ApicMechanismDriver(api_plus.MechanismDriver, self.aim.create(aim_ctx, aim_bgp_peer_prefix, overwrite=True) - if remove: + if remove or del_subnet: + if del_subnet: + # This subnet is not present on this host, Find the AF, + # to map the IProfile to remove. + query = BAKERY(lambda s: s.query( + models_v2.Subnet)) + query += lambda q: q.filter( + models_v2.Subnet.id == sa.bindparam('subnet_id')) + subnet = query(session).params( + subnet_id=del_subnet[0]).one_or_none() + if subnet.ip_version == 6: + if_profiles = [L3OUT_IF_PROFILE_NAME6] + else: + if_profiles = [L3OUT_IF_PROFILE_NAME] + else: + # Last port being removed so remove all IProfiles. + if_profiles = [L3OUT_IF_PROFILE_NAME, L3OUT_IF_PROFILE_NAME6] # REVISIT: Should we also delete the node profiles if there aren't # any more instances on this host? - for if_profile in [L3OUT_IF_PROFILE_NAME, L3OUT_IF_PROFILE_NAME6]: + for if_profile in if_profiles: aim_l3out_if = aim_resource.L3OutInterface( tenant_name=l3out.tenant_name, l3out_name=l3out.name, @@ -5182,7 +5252,20 @@ class ApicMechanismDriver(api_plus.MechanismDriver, LOG.debug('Port %s is not bound to any segment', port_context.current['id']) return - if remove: + + del_subnet = [] + if port_context.current and port_context.original: + # We are in a port update. + # If a subnet is deleted on this bound port, we need + # evaluate if we need to remove the corresponding + # IProfile. + curr = [s['subnet_id'] + for s in port_context.current['fixed_ips']] + orig = [s['subnet_id'] + for s in port_context.original['fixed_ips']] + del_subnet = list(set(orig) - set(curr)) + + if remove or del_subnet: # check if there are any other ports from this network on the host query = BAKERY(lambda s: s.query( models.PortBindingLevel)) @@ -5194,11 +5277,26 @@ class ApicMechanismDriver(api_plus.MechanismDriver, exist = query(session).params( host=host, segment_id=segment['id'], - port_id=port_context.current['id']).first() + port_id=port_context.current['id']).all() - if exist: + if remove and exist: + # We are removing a bound port, but there is at least + # one other port on this host so don't remove the + # IProfile. return + if del_subnet and exist: + # Subnet is being removed from the port, if there is + # at least one other port with the subnet on the host - + # Leave the IProfile for this AF intact. + host_ports = [p['port_id'] for p in exist] + if session.query(models_v2.Port).filter( + models_v2.Port.id.in_(host_ports)).filter( + models_v2.Port.fixed_ips.any( + models_v2.IPAllocation.subnet_id.in_( + del_subnet))).all(): + return + static_ports = self._get_static_ports(port_context._plugin_context, host, segment, port_context=port_context) @@ -5209,7 +5307,7 @@ class ApicMechanismDriver(api_plus.MechanismDriver, self._update_static_path_for_svi( session, port_context._plugin_context, port_context.network.current, l3out, - static_port, remove=remove) + static_port, remove=remove, del_subnet=del_subnet) else: self._update_static_path_for_network( session, port_context.network.current, diff --git a/gbpservice/neutron/tests/unit/plugins/ml2plus/test_apic_aim.py b/gbpservice/neutron/tests/unit/plugins/ml2plus/test_apic_aim.py index 9e2a8f9c2..e086d9e38 100644 --- a/gbpservice/neutron/tests/unit/plugins/ml2plus/test_apic_aim.py +++ b/gbpservice/neutron/tests/unit/plugins/ml2plus/test_apic_aim.py @@ -5109,6 +5109,163 @@ class TestPortBinding(ApicAimTestCase): bgp_peer = self.aim_mgr.get(aim_ctx, bgp_peer) self.assertIsNone(bgp_peer) + def test_dualstack_svi_opflex_agent_add_subnet(self): + aim_ctx = aim_context.AimContext(self.db_session) + + hlink1 = aim_infra.HostLink( + host_name='h1', + interface_name='eth1', + path='topology/pod-1/paths-102/pathep-[eth1/8]') + self.aim_mgr.create(aim_ctx, hlink1) + + self._register_agent('h1', AGENT_CONF_OPFLEX) + + net1 = self._make_network(self.fmt, 'net1', True, + arg_list=self.extension_attributes, + **{'apic:svi': 'True', + 'provider:network_type': u'vlan', + 'apic:bgp_enable': 'True', + 'apic:bgp_asn': '2'})['network'] + + with self.subnet(network={'network': net1}) as sub1: + with self.port(subnet=sub1) as p1: + # bind p1 to host h1 + p1 = self._bind_port_to_host(p1['port']['id'], 'h1') + vlan_p1 = self._check_binding(p1['port']['id'], + expected_binding_info=[('apic_aim', 'vlan')]) + ext_net = aim_resource.ExternalNetwork.from_dn( + net1[DN]['ExternalNetwork']) + + l3out_np = aim_resource.L3OutNodeProfile( + tenant_name=ext_net.tenant_name, + l3out_name=ext_net.l3out_name, + name=md.L3OUT_NODE_PROFILE_NAME) + l3out_np = self.aim_mgr.get(aim_ctx, l3out_np) + self.assertEqual(l3out_np.name, md.L3OUT_NODE_PROFILE_NAME) + + l3out_node = aim_resource.L3OutNode( + tenant_name=ext_net.tenant_name, + l3out_name=ext_net.l3out_name, + node_profile_name=md.L3OUT_NODE_PROFILE_NAME, + node_path='topology/pod-1/node-102') + l3out_node = self.aim_mgr.get(aim_ctx, l3out_node) + self.assertIsNotNone(l3out_node) + + l3out_ifs = [] + bgp_peers = [] + for if_profile in [md.L3OUT_IF_PROFILE_NAME, + md.L3OUT_IF_PROFILE_NAME6]: + l3out_ip = aim_resource.L3OutInterfaceProfile( + tenant_name=ext_net.tenant_name, + l3out_name=ext_net.l3out_name, + node_profile_name=md.L3OUT_NODE_PROFILE_NAME, + name=if_profile) + l3out_ip = self.aim_mgr.get(aim_ctx, l3out_ip) + + l3out_if = aim_resource.L3OutInterface( + tenant_name=ext_net.tenant_name, + l3out_name=ext_net.l3out_name, + node_profile_name=md.L3OUT_NODE_PROFILE_NAME, + interface_profile_name=if_profile, + interface_path=hlink1.path) + l3out_if = self.aim_mgr.get(aim_ctx, l3out_if) + if if_profile == md.L3OUT_IF_PROFILE_NAME: + self.assertEqual(l3out_ip.name, if_profile) + self.assertEqual(l3out_if.encap, 'vlan-%s' % vlan_p1) + self.assertEqual(l3out_if.secondary_addr_a_list, + [{'addr': '10.0.0.1/24'}]) + + primary = netaddr.IPNetwork(l3out_if.primary_addr_a) + subnet = str(primary.cidr) + bgp_peer = aim_resource.L3OutInterfaceBgpPeerP( + tenant_name=ext_net.tenant_name, + l3out_name=ext_net.l3out_name, + node_profile_name=md.L3OUT_NODE_PROFILE_NAME, + interface_profile_name=if_profile, + interface_path=hlink1.path, + addr=subnet) + bgp_peer = self.aim_mgr.get(aim_ctx, bgp_peer) + self.assertEqual(bgp_peer.asn, '2') + else: + self.assertIsNone(l3out_ip) + self.assertIsNone(l3out_if) + + sub2 = self._make_subnet(self.fmt, + {'network': net1}, '2001:db8::1', + '2001:db8::0/64', ip_version=6)['subnet'] + + p1['port']['fixed_ips'].append({'subnet_id': sub2['id']}) + port_info = {'port': {'fixed_ips': p1['port']['fixed_ips']}} + p1 = self._update('ports', p1['port']['id'], port_info) + + # The bound port now has the newly created v6 subnet, + # now we should have the v6 Interface Profile + for if_profile in [md.L3OUT_IF_PROFILE_NAME, + md.L3OUT_IF_PROFILE_NAME6]: + l3out_ip = aim_resource.L3OutInterfaceProfile( + tenant_name=ext_net.tenant_name, + l3out_name=ext_net.l3out_name, + node_profile_name=md.L3OUT_NODE_PROFILE_NAME, + name=if_profile) + l3out_ip = self.aim_mgr.get(aim_ctx, l3out_ip) + self.assertEqual(l3out_ip.name, if_profile) + + l3out_if = aim_resource.L3OutInterface( + tenant_name=ext_net.tenant_name, + l3out_name=ext_net.l3out_name, + node_profile_name=md.L3OUT_NODE_PROFILE_NAME, + interface_profile_name=if_profile, + interface_path=hlink1.path) + l3out_if = self.aim_mgr.get(aim_ctx, l3out_if) + self.assertEqual(l3out_if.encap, 'vlan-%s' % vlan_p1) + if if_profile == md.L3OUT_IF_PROFILE_NAME: + self.assertEqual(l3out_if.secondary_addr_a_list, + [{'addr': '10.0.0.1/24'}]) + else: + self.assertEqual(l3out_if.secondary_addr_a_list, + [{'addr': '2001:db8::1/64'}]) + l3out_ifs.append(l3out_if) + + primary = netaddr.IPNetwork(l3out_if.primary_addr_a) + subnet = str(primary.cidr) + bgp_peer = aim_resource.L3OutInterfaceBgpPeerP( + tenant_name=ext_net.tenant_name, + l3out_name=ext_net.l3out_name, + node_profile_name=md.L3OUT_NODE_PROFILE_NAME, + interface_profile_name=if_profile, + interface_path=hlink1.path, + addr=subnet) + bgp_peer = self.aim_mgr.get(aim_ctx, bgp_peer) + self.assertEqual(bgp_peer.asn, '2') + bgp_peers.append(bgp_peer) + + # Remove the v6 subnet from the port and verify that + # the v6 Profile is deleted. + v4_only = [d for d in p1['port']['fixed_ips'] + if not (d['subnet_id'] == sub2['id'])] + port_info = {'port': {'fixed_ips': v4_only}} + p1 = self._update('ports', p1['port']['id'], port_info) + + for l3out_if in l3out_ifs: + l3out_if = self.aim_mgr.get(aim_ctx, l3out_if) + if l3out_if and l3out_if.interface_profile_name == ( + md.L3OUT_IF_PROFILE_NAME): + self.assertEqual(l3out_if.secondary_addr_a_list, + [{'addr': '10.0.0.1/24'}]) + else: + self.assertIsNone(l3out_if) + + self._delete('ports', p1['port']['id']) + self._check_no_dynamic_segment(net1['id']) + + for l3out_if in l3out_ifs: + l3out_if = self.aim_mgr.get(aim_ctx, l3out_if) + self.assertIsNone(l3out_if) + + for bgp_peer in bgp_peers: + bgp_peer = self.aim_mgr.get(aim_ctx, bgp_peer) + self.assertIsNone(bgp_peer) + def test_bind_opflex_agent_svi(self): self._register_agent('host1', AGENT_CONF_OPFLEX) aim_ctx = aim_context.AimContext(self.db_session)