Skip to content

Commit

Permalink
Do not fail on revert PlugVIPAmphora due undefined db_lb
Browse files Browse the repository at this point in the history
- changed order: get db_lb before getting subnet info, to avoid useless
  message without db_lb.vip
- added default db_lb value for case, when DB calls are failed.
  in this case error message will be generated without information about
  VIP

Closes-Bug: 2085427

Change-Id: I97acfbd9b26fefbc38b61e36f72f483072390b23
(cherry picked from commit 7c08b63)
  • Loading branch information
skraynev committed Nov 5, 2024
1 parent c0ca1a5 commit 5a039fc
Show file tree
Hide file tree
Showing 3 changed files with 67 additions and 9 deletions.
18 changes: 9 additions & 9 deletions octavia/controller/worker/v2/tasks/network_tasks.py
Original file line number Diff line number Diff line change
Expand Up @@ -536,11 +536,10 @@ def revert(self, result, loadbalancer, amphora, subnet, *args, **kwargs):
"""Handle a failure to plumb a vip."""
if isinstance(result, failure.Failure):
return
lb_id = loadbalancer[constants.LOADBALANCER_ID]
LOG.warning("Unable to plug VIP for amphora id %s "
"load balancer id %s",
amphora.get(constants.ID),
loadbalancer[constants.LOADBALANCER_ID])

amphora.get(constants.ID), lb_id)
try:
session = db_apis.get_session()
with session.begin():
Expand All @@ -550,15 +549,16 @@ def revert(self, result, loadbalancer, amphora, subnet, *args, **kwargs):
db_amp.ha_port_id = result[constants.HA_PORT_ID]
db_subnet = self.network_driver.get_subnet(
subnet[constants.ID])
db_lb = self.loadbalancer_repo.get(
session,
id=loadbalancer[constants.LOADBALANCER_ID])

db_lb = self.loadbalancer_repo.get(session, id=lb_id)
self.network_driver.unplug_aap_port(db_lb.vip,
db_amp, db_subnet)
except Exception as e:
LOG.error('Failed to unplug AAP port. Resources may still be in '
'use for VIP: %s due to error: %s', db_lb.vip, str(e))
LOG.error(
'Failed to unplug AAP port for load balancer: %s. '
'Resources may still be in use for VRRP port: %s. '
'Due to error: %s',
lb_id, result[constants.VRRP_PORT_ID], str(e)
)


class UnplugVIP(BaseNetworkTask):
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1526,6 +1526,57 @@ def test_revert_plug_vip_amphora(self, mock_session, mock_lb_get, mock_get,
mock_driver.unplug_aap_port.assert_called_once_with(
LB.vip, self.db_amphora_mock, mockSubnet)

@mock.patch('octavia.db.repositories.AmphoraRepository.get')
@mock.patch('octavia.db.repositories.LoadBalancerRepository.get')
@mock.patch('octavia.db.api.get_session', return_value=_session_mock)
def test_revert_plug_vip_amphora_subnet_not_found(
self, mock_session, mock_lb_get, mock_get, mock_get_net_driver):
mock_driver = mock.MagicMock()
mock_lb_get.return_value = LB
mock_get.return_value = self.db_amphora_mock
mock_get_net_driver.return_value = mock_driver
net = network_tasks.PlugVIPAmphora()
amphora = {constants.ID: AMPHORA_ID,
constants.LB_NETWORK_IP: IP_ADDRESS}
subnet = {constants.ID: SUBNET_ID}
err_msg = 'Subnet not found'
mock_driver.get_subnet.side_effect = net_base.SubnetNotFound(err_msg)
result = AMPS_DATA[0].to_dict()
net.revert(result, self.load_balancer_mock, amphora, subnet)
mock_driver.unplug_aap_port.assert_not_called()
network_tasks.LOG.error.assert_called_once_with(
'Failed to unplug AAP port for load balancer: %s. '
'Resources may still be in use for VRRP port: %s. '
'Due to error: %s',
self.load_balancer_mock[constants.LOADBALANCER_ID],
result[constants.VRRP_PORT_ID], err_msg
)

@mock.patch('octavia.db.repositories.AmphoraRepository.get')
@mock.patch('octavia.db.repositories.LoadBalancerRepository.get')
@mock.patch('octavia.db.api.get_session', return_value=_session_mock)
def test_revert_plug_vip_amphora_raise_db_error(
self, mock_session, mock_lb_get, mock_get, mock_get_net_driver):
mock_driver = mock.MagicMock()
mock_lb_get.return_value = LB
err_msg = 'Some Error'
mock_get.side_effect = Exception(err_msg)
net = network_tasks.PlugVIPAmphora()
amphora = {constants.ID: AMPHORA_ID,
constants.LB_NETWORK_IP: IP_ADDRESS}
subnet = {constants.ID: SUBNET_ID}
result = AMPS_DATA[0].to_dict()
net.revert(result, self.load_balancer_mock, amphora, subnet)
mock_driver.unplug_aap_port.assert_not_called()
mock_lb_get.assert_not_called()
network_tasks.LOG.error.assert_called_once_with(
'Failed to unplug AAP port for load balancer: %s. '
'Resources may still be in use for VRRP port: %s. '
'Due to error: %s',
self.load_balancer_mock[constants.LOADBALANCER_ID],
result[constants.VRRP_PORT_ID], err_msg
)

@mock.patch('octavia.controller.worker.v2.tasks.network_tasks.DeletePort.'
'update_progress')
def test_delete_port(self, mock_update_progress, mock_get_net_driver):
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
---
fixes:
- |
Fix error on revert PlugVIPAmphora task, when db_lb is not defined
and get_subnet raises NotFound error. It could happen when Amphora
creation failed by timeout and before it VIP network was removed.
As result revert failed with exception.

0 comments on commit 5a039fc

Please sign in to comment.