From d31b47fd0d88235ecc5ca91235481537afa11ade Mon Sep 17 00:00:00 2001 From: Michael Johnson Date: Fri, 14 Jun 2019 16:58:04 -0700 Subject: [PATCH] Fix a python3 issue in the amphora-agent An exception handler in the amphora-agent has a python3 string comparison bug that will cause a TypeError. This patch fixes that bug and adds test coverage for the start_stop_listener. Change-Id: I6f5d95c5f875edda530f54ae72386d6495235ca6 Story: 2005898 Task: 33760 (cherry picked from commit dc459e2213089345f906c3daa313bd05c429bb54) --- .../backends/agent/api_server/listener.py | 2 +- .../backend/agent/api_server/test_server.py | 10 +- .../agent/api_server/test_listener.py | 145 ++++++++++++++++++ 3 files changed, 151 insertions(+), 6 deletions(-) diff --git a/octavia/amphorae/backends/agent/api_server/listener.py b/octavia/amphorae/backends/agent/api_server/listener.py index 1b83c47dde..5057daeab3 100644 --- a/octavia/amphorae/backends/agent/api_server/listener.py +++ b/octavia/amphorae/backends/agent/api_server/listener.py @@ -259,7 +259,7 @@ class Listener(object): try: subprocess.check_output(cmd.split(), stderr=subprocess.STDOUT) except subprocess.CalledProcessError as e: - if 'Job is already running' not in e.output: + if b'Job is already running' not in e.output: LOG.debug( "Failed to %(action)s haproxy-%(list)s service: %(err)s " "%(out)s", {'action': action, 'list': listener_id, diff --git a/octavia/tests/functional/amphorae/backend/agent/api_server/test_server.py b/octavia/tests/functional/amphorae/backend/agent/api_server/test_server.py index 837b28ea99..daf9f09d99 100644 --- a/octavia/tests/functional/amphorae/backend/agent/api_server/test_server.py +++ b/octavia/tests/functional/amphorae/backend/agent/api_server/test_server.py @@ -39,7 +39,7 @@ import octavia.tests.unit.base as base AMP_AGENT_CONF_PATH = '/etc/octavia/amphora-agent.conf' -RANDOM_ERROR = 'random error' +RANDOM_ERROR = b'random error' OK = dict(message='OK') @@ -330,7 +330,7 @@ class TestServerTestCase(base.TestCase): self.assertEqual( { 'message': 'Error starting haproxy', - 'details': RANDOM_ERROR, + 'details': RANDOM_ERROR.decode('utf-8'), }, jsonutils.loads(rv.data.decode('utf-8'))) mock_subprocess.assert_called_with( ['/usr/sbin/service', 'haproxy-123', 'start'], stderr=-2) @@ -1390,7 +1390,7 @@ class TestServerTestCase(base.TestCase): data=jsonutils.dumps(port_info)) self.assertEqual(500, rv.status_code) self.assertEqual( - {'details': RANDOM_ERROR, + {'details': RANDOM_ERROR.decode('utf-8'), 'message': 'Error plugging network'}, jsonutils.loads(rv.data.decode('utf-8'))) @@ -1929,7 +1929,7 @@ class TestServerTestCase(base.TestCase): data=jsonutils.dumps(subnet_info)) self.assertEqual(500, rv.status_code) self.assertEqual( - {'details': RANDOM_ERROR, + {'details': RANDOM_ERROR.decode('utf-8'), 'message': 'Error plugging VIP'}, jsonutils.loads(rv.data.decode('utf-8'))) @@ -2308,7 +2308,7 @@ class TestServerTestCase(base.TestCase): data=jsonutils.dumps(subnet_info)) self.assertEqual(500, rv.status_code) self.assertEqual( - {'details': RANDOM_ERROR, + {'details': RANDOM_ERROR.decode('utf-8'), 'message': 'Error plugging VIP'}, jsonutils.loads(rv.data.decode('utf-8'))) diff --git a/octavia/tests/unit/amphorae/backends/agent/api_server/test_listener.py b/octavia/tests/unit/amphorae/backends/agent/api_server/test_listener.py index 32927221cc..15a006acc4 100644 --- a/octavia/tests/unit/amphorae/backends/agent/api_server/test_listener.py +++ b/octavia/tests/unit/amphorae/backends/agent/api_server/test_listener.py @@ -12,6 +12,8 @@ # License for the specific language governing permissions and limitations # under the License. +import subprocess + import mock from oslo_utils import uuidutils @@ -190,3 +192,146 @@ class ListenerTestCase(base.TestCase): self.assertEqual( consts.OFFLINE, self.test_listener._check_haproxy_status(LISTENER_ID1)) + + @mock.patch('octavia.amphorae.backends.agent.api_server.listener.Listener.' + '_check_haproxy_status') + @mock.patch('octavia.amphorae.backends.agent.api_server.listener.Listener.' + 'vrrp_check_script_update') + @mock.patch('os.path.exists') + @mock.patch('octavia.amphorae.backends.agent.api_server.listener.Listener.' + '_check_listener_exists') + @mock.patch('subprocess.check_output') + def test_start_stop_listener(self, mock_check_output, mock_list_exists, + mock_path_exists, mock_vrrp_update, + mock_check_status): + listener_id = uuidutils.generate_uuid() + + mock_path_exists.side_effect = [False, True, True, False, False] + mock_check_status.side_effect = ['bogus', consts.OFFLINE] + + # Happy path - No VRRP + ref_command_split = ['/usr/sbin/service'] + ref_command_split.append('haproxy-{}'.format(listener_id)) + ref_command_split.append(consts.AMP_ACTION_START) + + result = self.test_listener.start_stop_listener( + listener_id, consts.AMP_ACTION_START) + + mock_check_output.assert_called_once_with(ref_command_split, + stderr=subprocess.STDOUT) + mock_list_exists.assert_called_once_with(listener_id) + mock_vrrp_update.assert_not_called() + self.assertEqual(202, result.status_code) + self.assertEqual('OK', result.json['message']) + ref_details = ('Configuration file is valid\n' + 'haproxy daemon for {0} started'.format(listener_id)) + self.assertEqual(ref_details, result.json['details']) + + # Happy path - VRRP - RELOAD + mock_list_exists.reset_mock() + mock_vrrp_update.reset_mock() + mock_check_output.reset_mock() + + ref_command_split = ['/usr/sbin/service'] + ref_command_split.append('haproxy-{}'.format(listener_id)) + ref_command_split.append(consts.AMP_ACTION_RELOAD) + + result = self.test_listener.start_stop_listener( + listener_id, consts.AMP_ACTION_RELOAD) + + mock_check_output.assert_called_once_with(ref_command_split, + stderr=subprocess.STDOUT) + mock_list_exists.assert_called_once_with(listener_id) + mock_vrrp_update.assert_called_once_with(listener_id, + consts.AMP_ACTION_RELOAD) + self.assertEqual(202, result.status_code) + self.assertEqual('OK', result.json['message']) + ref_details = ('Listener {0} {1}ed'.format(listener_id, + consts.AMP_ACTION_RELOAD)) + self.assertEqual(ref_details, result.json['details']) + + # Happy path - VRRP - RELOAD - OFFLINE + mock_list_exists.reset_mock() + mock_vrrp_update.reset_mock() + mock_check_output.reset_mock() + + ref_command_split = ['/usr/sbin/service'] + ref_command_split.append('haproxy-{}'.format(listener_id)) + ref_command_split.append(consts.AMP_ACTION_START) + + result = self.test_listener.start_stop_listener( + listener_id, consts.AMP_ACTION_RELOAD) + + mock_check_output.assert_called_once_with(ref_command_split, + stderr=subprocess.STDOUT) + mock_list_exists.assert_called_once_with(listener_id) + mock_vrrp_update.assert_called_once_with(listener_id, + consts.AMP_ACTION_RELOAD) + self.assertEqual(202, result.status_code) + self.assertEqual('OK', result.json['message']) + ref_details = ('Configuration file is valid\n' + 'haproxy daemon for {0} started'.format(listener_id)) + self.assertEqual(ref_details, result.json['details']) + + # Unhappy path - Not already running + mock_list_exists.reset_mock() + mock_vrrp_update.reset_mock() + mock_check_output.reset_mock() + + ref_command_split = ['/usr/sbin/service'] + ref_command_split.append('haproxy-{}'.format(listener_id)) + ref_command_split.append(consts.AMP_ACTION_START) + + mock_check_output.side_effect = subprocess.CalledProcessError( + output=b'bogus', returncode=-2, cmd='sit') + + result = self.test_listener.start_stop_listener( + listener_id, consts.AMP_ACTION_START) + + mock_check_output.assert_called_once_with(ref_command_split, + stderr=subprocess.STDOUT) + mock_list_exists.assert_called_once_with(listener_id) + mock_vrrp_update.assert_not_called() + self.assertEqual(500, result.status_code) + self.assertEqual('Error {}ing haproxy'.format(consts.AMP_ACTION_START), + result.json['message']) + self.assertEqual('bogus', result.json['details']) + + # Unhappy path - Already running + mock_list_exists.reset_mock() + mock_vrrp_update.reset_mock() + mock_check_output.reset_mock() + + ref_command_split = ['/usr/sbin/service'] + ref_command_split.append('haproxy-{}'.format(listener_id)) + ref_command_split.append(consts.AMP_ACTION_START) + + mock_check_output.side_effect = subprocess.CalledProcessError( + output=b'Job is already running', returncode=-2, cmd='sit') + + result = self.test_listener.start_stop_listener( + listener_id, consts.AMP_ACTION_START) + + mock_check_output.assert_called_once_with(ref_command_split, + stderr=subprocess.STDOUT) + mock_list_exists.assert_called_once_with(listener_id) + mock_vrrp_update.assert_not_called() + self.assertEqual(202, result.status_code) + self.assertEqual('OK', result.json['message']) + ref_details = ('Configuration file is valid\n' + 'haproxy daemon for {0} started'.format(listener_id)) + self.assertEqual(ref_details, result.json['details']) + + # Invalid action + mock_check_output.reset_mock() + mock_list_exists.reset_mock() + mock_path_exists.reset_mock() + mock_vrrp_update.reset_mock() + result = self.test_listener.start_stop_listener(listener_id, 'bogus') + self.assertEqual(400, result.status_code) + self.assertEqual('Invalid Request', result.json['message']) + self.assertEqual('Unknown action: bogus', result.json['details']) + mock_list_exists.assert_not_called() + mock_path_exists.assert_not_called() + mock_vrrp_update.assert_not_called() + mock_check_output.assert_not_called()