diff --git a/ovn_octavia_provider/driver.py b/ovn_octavia_provider/driver.py index 8e4afdde..7d46c64c 100644 --- a/ovn_octavia_provider/driver.py +++ b/ovn_octavia_provider/driver.py @@ -272,11 +272,20 @@ class OvnProviderDriver(driver_base.ProviderDriver): self._ovn_helper.add_request(request) def member_delete(self, member): + # NOTE(froyo): OVN provider allow to create member without param + # subnet_id, in that case the driver search it according to the + # pool_id, but it is not propagated to Octavia. In this case, if + # the member is deleted, Octavia send the object without subnet_id. + subnet_id = member.subnet_id + if (isinstance(subnet_id, o_datamodels.UnsetType) or not subnet_id): + subnet_id = self._ovn_helper._get_subnet_from_pool( + member.pool_id) + request_info = {'id': member.member_id, 'address': member.address, 'protocol_port': member.protocol_port, 'pool_id': member.pool_id, - 'subnet_id': member.subnet_id} + 'subnet_id': subnet_id} request = {'type': ovn_const.REQ_TYPE_MEMBER_DELETE, 'info': request_info} self._ovn_helper.add_request(request) @@ -286,13 +295,22 @@ class OvnProviderDriver(driver_base.ProviderDriver): request_info = {'id': member.member_id, 'address': member.address, 'pool_id': member.pool_id, - 'subnet_id': member.subnet_id, + 'subnet_id': subnet_id, 'action': ovn_const.REQ_INFO_MEMBER_DELETED} request = {'type': ovn_const.REQ_TYPE_HANDLE_MEMBER_DVR, 'info': request_info} self._ovn_helper.add_request(request) def member_update(self, old_member, new_member): + # NOTE(froyo): OVN provider allow to create member without param + # subnet_id, in that case the driver search it according to the + # pool_id, but it is not propagated to Octavia. In this case, if + # the member is updated, Octavia send the object without subnet_id. + subnet_id = old_member.subnet_id + if (isinstance(subnet_id, o_datamodels.UnsetType) or not subnet_id): + subnet_id = self._ovn_helper._get_subnet_from_pool( + old_member.pool_id) + # Validate monitoring options if present self._check_member_monitor_options(new_member) if new_member.address and self._ip_version_differs(new_member): @@ -301,7 +319,7 @@ class OvnProviderDriver(driver_base.ProviderDriver): 'address': old_member.address, 'protocol_port': old_member.protocol_port, 'pool_id': old_member.pool_id, - 'subnet_id': old_member.subnet_id, + 'subnet_id': subnet_id, 'old_admin_state_up': old_member.admin_state_up} if not isinstance(new_member.admin_state_up, o_datamodels.UnsetType): request_info['admin_state_up'] = new_member.admin_state_up @@ -317,19 +335,30 @@ class OvnProviderDriver(driver_base.ProviderDriver): pool = external_ids[pool_key] existing_members = pool.split(',') if pool else [] members_to_delete = copy.copy(existing_members) + pool_subnet_id = None for member in members: if (self._check_monitor_options(member) or member.address and self._ip_version_differs(member)): skipped_members.append(member.member_id) continue - # NOTE(mjozefcz): We need to have subnet_id information. - if (isinstance(member.subnet_id, o_datamodels.UnsetType) or - not member.subnet_id): - msg = _('Subnet is required for Member creation ' - 'with OVN Provider Driver') - raise driver_exceptions.UnsupportedOptionError( - user_fault_string=msg, - operator_fault_string=msg) + # NOTE(froyo): if subnet_id not provided, lets try to get it + # from the member pool_id + subnet_id = member.subnet_id + if (isinstance(subnet_id, o_datamodels.UnsetType) or + not subnet_id): + pool_subnet_id = ( + pool_subnet_id + if pool_subnet_id + else self._ovn_helper._get_subnet_from_pool(pool_id)) + # NOTE(mjozefcz): We need to have subnet_id information. + if not pool_subnet_id: + msg = _('Subnet is required, or Loadbalancer associated ' + 'with Pool must have a subnet, for Member ' + 'creation with OVN Provider Driver') + raise driver_exceptions.UnsupportedOptionError( + user_fault_string=msg, + operator_fault_string=msg) + member.subnet_id = pool_subnet_id admin_state_up = member.admin_state_up if isinstance(admin_state_up, o_datamodels.UnsetType): admin_state_up = True diff --git a/ovn_octavia_provider/tests/functional/base.py b/ovn_octavia_provider/tests/functional/base.py index f4fed357..d82946f8 100644 --- a/ovn_octavia_provider/tests/functional/base.py +++ b/ovn_octavia_provider/tests/functional/base.py @@ -808,12 +808,21 @@ class TestOvnOctaviaBase(base.TestOVNFunctionalBase, if m.address == member_address: return m - def _update_member_and_validate(self, lb_data, pool_id, member_address): + def _update_member_and_validate(self, lb_data, pool_id, member_address, + remove_subnet_id=False): pool = self._get_pool_from_lb_data(lb_data, pool_id=pool_id) member = self._get_pool_member(pool, member_address) self._o_driver_lib.update_loadbalancer_status.reset_mock() - self.ovn_driver.member_update(member, member) + old_member = copy.deepcopy(member) + + # NOTE(froyo): In order to test update of member without passing the + # subnet_id parameter of the member, just to cover the case when a new + # member has been created without passing that argument + if remove_subnet_id: + old_member.subnet_id = None + + self.ovn_driver.member_update(old_member, member) expected_status = { 'pools': [{'id': pool.pool_id, 'provisioning_status': 'ACTIVE'}], @@ -866,7 +875,7 @@ class TestOvnOctaviaBase(base.TestOVNFunctionalBase, check_call=False) def _delete_member_and_validate(self, lb_data, pool_id, network_id, - member_address): + member_address, remove_subnet_id=False): pool = self._get_pool_from_lb_data(lb_data, pool_id=pool_id) member = self._get_pool_member(pool, member_address) pool.members.remove(member) @@ -877,7 +886,15 @@ class TestOvnOctaviaBase(base.TestOVNFunctionalBase, pool_status['operating_status'] = o_constants.OFFLINE self._o_driver_lib.update_loadbalancer_status.reset_mock() - self.ovn_driver.member_delete(member) + + # NOTE(froyo): In order to test deletion of member without passing + # the subnet_id parameter of the member, just to cover the case when + # a new member has been created without passing that argument + m_member = copy.deepcopy(member) + if remove_subnet_id: + m_member.subnet_id = None + + self.ovn_driver.member_delete(m_member) expected_status = { 'pools': [pool_status], 'members': [{"id": member.member_id, diff --git a/ovn_octavia_provider/tests/functional/test_driver.py b/ovn_octavia_provider/tests/functional/test_driver.py index b31e451c..66f55bd3 100644 --- a/ovn_octavia_provider/tests/functional/test_driver.py +++ b/ovn_octavia_provider/tests/functional/test_driver.py @@ -191,11 +191,28 @@ class TestOvnOctaviaProviderDriver(ovn_base.TestOvnOctaviaBase): self._o_driver_lib.get_pool.return_value = lb_data['pools'][0] self._o_driver_lib.get_loadbalancer.return_value = lb_data['model'] - # Test creating Member without subnet but with pool + # Test deleting a member without subnet self._create_member_and_validate( lb_data, pool_TCP_id, None, lb_data['vip_net_info'][0], '10.0.0.10', expected_subnet=lb_data['vip_net_info'][1]) + self._delete_member_and_validate( + lb_data, pool_TCP_id, lb_data['vip_net_info'][0], + '10.0.0.10', remove_subnet_id=True) + + # Test update member without subnet + self._create_member_and_validate( + lb_data, pool_TCP_id, None, + lb_data['vip_net_info'][0], '10.0.0.10', + expected_subnet=lb_data['vip_net_info'][1]) + self._update_member_and_validate( + lb_data, pool_TCP_id, "10.0.0.10", remove_subnet_id=True) + + # Test creating a Member without subnet but with pool + self._create_member_and_validate( + lb_data, pool_TCP_id, None, + lb_data['vip_net_info'][0], '10.0.0.11', + expected_subnet=lb_data['vip_net_info'][1]) # Deleting the pool should also delete the members. self._delete_pool_and_validate(lb_data, "p_TCP") diff --git a/ovn_octavia_provider/tests/unit/test_driver.py b/ovn_octavia_provider/tests/unit/test_driver.py index 59d62dd7..82cbbecf 100644 --- a/ovn_octavia_provider/tests/unit/test_driver.py +++ b/ovn_octavia_provider/tests/unit/test_driver.py @@ -343,6 +343,23 @@ class TestOvnProviderDriver(ovn_base.TestOvnOctaviaBase): self.driver.member_update(self.ref_member, self.update_member) self.mock_add_request.assert_called_once_with(expected_dict) + def test_member_update_missing_subnet_id(self): + self.driver._ovn_helper._get_subnet_from_pool.return_value = ( + self.ref_member.subnet_id) + info = {'id': self.update_member.member_id, + 'address': self.ref_member.address, + 'protocol_port': self.ref_member.protocol_port, + 'pool_id': self.ref_member.pool_id, + 'admin_state_up': self.update_member.admin_state_up, + 'old_admin_state_up': self.ref_member.admin_state_up, + 'subnet_id': self.ref_member.subnet_id} + expected_dict = {'type': ovn_const.REQ_TYPE_MEMBER_UPDATE, + 'info': info} + member = copy.copy(self.ref_member) + member.subnet_id = data_models.UnsetType() + self.driver.member_update(member, self.update_member) + self.mock_add_request.assert_called_once_with(expected_dict) + @mock.patch.object(ovn_driver.OvnProviderDriver, '_ip_version_differs') def test_member_update_no_ip_addr(self, mock_ip_differs): self.update_member.address = None @@ -388,6 +405,12 @@ class TestOvnProviderDriver(ovn_base.TestOvnOctaviaBase): self.driver.member_batch_update, self.pool_id, [self.ref_member]) + def test_member_batch_update_missing_subnet_id_get_from_pool(self): + self.driver._ovn_helper._get_subnet_from_pool.return_value = ( + self.ref_member.subnet_id) + self.ref_member.subnet_id = None + self.driver.member_batch_update(self.pool_id, [self.ref_member]) + def test_member_update_failure(self): self.assertRaises(exceptions.UnsupportedOptionError, self.driver.member_update, self.ref_member, @@ -422,6 +445,34 @@ class TestOvnProviderDriver(ovn_base.TestOvnOctaviaBase): mock.call(expected_dict_dvr)] self.mock_add_request.assert_has_calls(expected) + def test_member_delete_missing_subnet_id(self): + self.driver._ovn_helper._get_subnet_from_pool.return_value = ( + self.ref_member.subnet_id) + info = {'id': self.ref_member.member_id, + 'address': self.ref_member.address, + 'protocol_port': self.ref_member.protocol_port, + 'pool_id': self.ref_member.pool_id, + 'subnet_id': self.ref_member.subnet_id} + expected_dict = {'type': ovn_const.REQ_TYPE_MEMBER_DELETE, + 'info': info} + info_dvr = { + 'id': self.ref_member.member_id, + 'address': self.ref_member.address, + 'pool_id': self.ref_member.pool_id, + 'subnet_id': self.ref_member.subnet_id, + 'action': ovn_const.REQ_INFO_MEMBER_DELETED} + expected_dict_dvr = { + 'type': ovn_const.REQ_TYPE_HANDLE_MEMBER_DVR, + 'info': info_dvr} + + member = copy.copy(self.ref_member) + member.subnet_id = data_models.UnsetType() + self.driver.member_delete(member) + expected = [ + mock.call(expected_dict), + mock.call(expected_dict_dvr)] + self.mock_add_request.assert_has_calls(expected) + def test_listener_create(self): info = {'id': self.ref_listener.listener_id, 'protocol': self.ref_listener.protocol,