diff --git a/octavia/amphorae/backends/agent/api_server/keepalived.py b/octavia/amphorae/backends/agent/api_server/keepalived.py index 528cdc285f..5f7fdd8356 100644 --- a/octavia/amphorae/backends/agent/api_server/keepalived.py +++ b/octavia/amphorae/backends/agent/api_server/keepalived.py @@ -85,7 +85,9 @@ def upload_keepalived_config(): def manager_keepalived_service(action): action = action.lower() - if action not in ['start', 'stop', 'reload']: + if action not in [consts.AMP_ACTION_START, + consts.AMP_ACTION_STOP, + consts.AMP_ACTION_RELOAD]: return flask.make_response(flask.jsonify(dict( message='Invalid Request', details="Unknown action: {0}".format(action))), 400) diff --git a/octavia/amphorae/backends/agent/api_server/listener.py b/octavia/amphorae/backends/agent/api_server/listener.py index 0a8a0b1018..1025f41e84 100644 --- a/octavia/amphorae/backends/agent/api_server/listener.py +++ b/octavia/amphorae/backends/agent/api_server/listener.py @@ -164,7 +164,9 @@ def upload_haproxy_config(amphora_id, listener_id): def start_stop_listener(listener_id, action): action = action.lower() - if action not in ['start', 'stop', 'reload']: + if action not in [consts.AMP_ACTION_START, + consts.AMP_ACTION_STOP, + consts.AMP_ACTION_RELOAD]: return flask.make_response(flask.jsonify(dict( message='Invalid Request', details="Unknown action: {0}".format(action))), 400) @@ -177,6 +179,12 @@ def start_stop_listener(listener_id, action): if os.path.exists(util.keepalived_check_script_path()): vrrp_check_script_update(listener_id, action) + # HAProxy does not start the process when given a reload + # so start it if haproxy is not already running + if action == consts.AMP_ACTION_RELOAD: + if consts.OFFLINE == _check_haproxy_status(listener_id): + action = consts.AMP_ACTION_START + cmd = ("/usr/sbin/service haproxy-{listener_id} {action}".format( listener_id=listener_id, action=action)) @@ -189,7 +197,8 @@ def start_stop_listener(listener_id, action): return flask.make_response(flask.jsonify(dict( message="Error {0}ing haproxy".format(action), details=e.output)), 500) - if action in ['stop', 'reload']: + if action in [consts.AMP_ACTION_STOP, + consts.AMP_ACTION_RELOAD]: return flask.make_response(flask.jsonify( dict(message='OK', details='Listener {listener_id} {action}ed'.format( @@ -437,7 +446,7 @@ def _cert_file_path(listener_id, filename): def vrrp_check_script_update(listener_id, action): listener_ids = util.get_listeners() - if action == 'stop': + if action == consts.AMP_ACTION_STOP: listener_ids.remove(listener_id) args = [] for listener_id in listener_ids: @@ -450,3 +459,14 @@ def vrrp_check_script_update(listener_id, action): cmd = 'haproxy-vrrp-check {args}; exit $?'.format(args=' '.join(args)) with open(util.haproxy_check_script_path(), 'w') as text_file: text_file.write(cmd) + + +def _check_haproxy_status(listener_id): + if os.path.exists(util.pid_path(listener_id)): + if os.path.exists( + os.path.join('/proc', util.get_haproxy_pid(listener_id))): + return consts.ACTIVE + else: # pid file but no process... + return consts.OFFLINE + else: + return consts.OFFLINE diff --git a/octavia/amphorae/drivers/haproxy/rest_api_driver.py b/octavia/amphorae/drivers/haproxy/rest_api_driver.py index 66b7f11031..eac411b55f 100644 --- a/octavia/amphorae/drivers/haproxy/rest_api_driver.py +++ b/octavia/amphorae/drivers/haproxy/rest_api_driver.py @@ -28,13 +28,13 @@ from octavia.amphorae.drivers import driver_base as driver_base from octavia.amphorae.drivers.haproxy import exceptions as exc from octavia.amphorae.drivers.keepalived import vrrp_rest_driver from octavia.common.config import cfg -from octavia.common import constants +from octavia.common import constants as consts from octavia.common.jinja.haproxy import jinja_cfg from octavia.common.tls_utils import cert_parser from octavia.i18n import _LE, _LW LOG = logging.getLogger(__name__) -API_VERSION = constants.API_VERSION +API_VERSION = consts.API_VERSION OCTAVIA_API_CLIENT = ( "Octavia HaProxy Rest Client/{version} " "(https://wiki.openstack.org/wiki/Octavia)").format(version=API_VERSION) @@ -68,21 +68,13 @@ class HaproxyAmphoraLoadBalancerDriver( certs = self._process_tls_certificates(listener) for amp in listener.load_balancer.amphorae: - if amp.status != constants.DELETED: + if amp.status != consts.DELETED: # Generate HaProxy configuration from listener object config = self.jinja.build_config(amp, listener, certs['tls_cert']) self.client.upload_config(amp, listener.id, config) - # todo (german): add a method to REST interface to reload or - # start without having to check - # Is that listener running? - r = self.client.get_listener_status(amp, - listener.id) - if r['status'] == 'ACTIVE': - self.client.reload_listener(amp, listener.id) - else: - self.client.start_listener(amp, listener.id) + self.client.reload_listener(amp, listener.id) def upload_cert_amp(self, amp, pem): LOG.debug("Amphora %s updating cert in REST driver " @@ -92,7 +84,7 @@ class HaproxyAmphoraLoadBalancerDriver( def _apply(self, func, listener=None, *args): for amp in listener.load_balancer.amphorae: - if amp.status != constants.DELETED: + if amp.status != consts.DELETED: func(amp, listener.id, *args) def stop(self, listener, vip): @@ -114,7 +106,7 @@ class HaproxyAmphoraLoadBalancerDriver( pass def post_vip_plug(self, amphora, load_balancer, amphorae_network_config): - if amphora.status != constants.DELETED: + if amphora.status != consts.DELETED: subnet = amphorae_network_config.get(amphora.id).vip_subnet # NOTE(blogan): using the vrrp port here because that # is what the allowed address pairs network driver sets @@ -224,13 +216,19 @@ class AmphoraAPIClient(object): self.delete = functools.partial(self.request, 'delete') self.head = functools.partial(self.request, 'head') - self.start_listener = functools.partial(self._action, 'start') - self.stop_listener = functools.partial(self._action, 'stop') - self.reload_listener = functools.partial(self._action, 'reload') + self.start_listener = functools.partial(self._action, + consts.AMP_ACTION_START) + self.stop_listener = functools.partial(self._action, + consts.AMP_ACTION_STOP) + self.reload_listener = functools.partial(self._action, + consts.AMP_ACTION_RELOAD) - self.start_vrrp = functools.partial(self._vrrp_action, 'start') - self.stop_vrrp = functools.partial(self._vrrp_action, 'stop') - self.reload_vrrp = functools.partial(self._vrrp_action, 'reload') + self.start_vrrp = functools.partial(self._vrrp_action, + consts.AMP_ACTION_START) + self.stop_vrrp = functools.partial(self._vrrp_action, + consts.AMP_ACTION_STOP) + self.reload_vrrp = functools.partial(self._vrrp_action, + consts.AMP_ACTION_RELOAD) self.session = requests.Session() self.session.cert = CONF.haproxy_amphora.client_cert diff --git a/octavia/common/constants.py b/octavia/common/constants.py index 69d8a114bc..18517ed035 100644 --- a/octavia/common/constants.py +++ b/octavia/common/constants.py @@ -327,3 +327,7 @@ FLOW_DOC_TITLES = {'AmphoraFlows': 'Amphora Flows', 'L7RuleFlows': 'Layer 7 Rule Flows'} NETNS_PRIMARY_INTERFACE = 'eth1' + +AMP_ACTION_START = 'start' +AMP_ACTION_STOP = 'stop' +AMP_ACTION_RELOAD = 'reload' 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 ca26e281aa..2321c145ab 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 @@ -184,6 +184,40 @@ class TestServerTestCase(base.TestCase): mock_subprocess.assert_called_with( ['/usr/sbin/service', 'haproxy-123', 'start'], stderr=-2) + @mock.patch('os.path.exists') + @mock.patch('octavia.amphorae.backends.agent.api_server.listener.' + 'vrrp_check_script_update') + @mock.patch('octavia.amphorae.backends.agent.api_server.listener.' + '_check_haproxy_status') + @mock.patch('subprocess.check_output') + def test_reload(self, mock_subprocess, mock_haproxy_status, + mock_vrrp, mock_exists): + + # Process running so reload + mock_exists.return_value = True + mock_haproxy_status.return_value = consts.ACTIVE + rv = self.app.put('/' + api_server.VERSION + '/listeners/123/reload') + self.assertEqual(202, rv.status_code) + self.assertEqual( + {'message': 'OK', + 'details': 'Listener 123 reloaded'}, + json.loads(rv.data.decode('utf-8'))) + mock_subprocess.assert_called_with( + ['/usr/sbin/service', 'haproxy-123', 'reload'], stderr=-2) + + # Process not running so start + mock_exists.return_value = True + mock_haproxy_status.return_value = consts.OFFLINE + rv = self.app.put('/' + api_server.VERSION + '/listeners/123/reload') + self.assertEqual(202, rv.status_code) + self.assertEqual( + {'message': 'OK', + 'details': 'Configuration file is valid\nhaproxy daemon for' + ' 123 started'}, + json.loads(rv.data.decode('utf-8'))) + mock_subprocess.assert_called_with( + ['/usr/sbin/service', 'haproxy-123', 'start'], stderr=-2) + @mock.patch('socket.gethostname') @mock.patch('subprocess.check_output') def test_info(self, mock_subbprocess, mock_hostname): 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 24a6c31496..db9e6a7c66 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 @@ -165,3 +165,23 @@ class ListenerTestCase(base.TestCase): listener.vrrp_check_script_update(LISTENER_ID1, 'start') handle = m() handle.write.assert_called_once_with(cmd) + + @mock.patch('os.path.exists') + @mock.patch('octavia.amphorae.backends.agent.api_server' + + '.util.get_haproxy_pid') + def test_check_haproxy_status(self, mock_pid, mock_exists): + mock_pid.return_value = '1245' + mock_exists.side_effect = [True, True] + self.assertEqual( + consts.ACTIVE, + listener._check_haproxy_status(LISTENER_ID1)) + + mock_exists.side_effect = [True, False] + self.assertEqual( + consts.OFFLINE, + listener._check_haproxy_status(LISTENER_ID1)) + + mock_exists.side_effect = [False] + self.assertEqual( + consts.OFFLINE, + listener._check_haproxy_status(LISTENER_ID1)) diff --git a/octavia/tests/unit/amphorae/drivers/haproxy/test_rest_api_driver.py b/octavia/tests/unit/amphorae/drivers/haproxy/test_rest_api_driver.py index 86241c625f..5036aac882 100644 --- a/octavia/tests/unit/amphorae/drivers/haproxy/test_rest_api_driver.py +++ b/octavia/tests/unit/amphorae/drivers/haproxy/test_rest_api_driver.py @@ -122,18 +122,6 @@ class TestHaproxyAmphoraLoadBalancerDriverTest(base.TestCase): self.driver.client.reload_listener.assert_called_once_with( self.amp, self.sl.id) - # listener down - self.driver.client.get_cert_md5sum.side_effect = [ - 'd41d8cd98f00b204e9800998ecf8427e'] * 3 - self.driver.jinja.build_config.side_effect = ['fake_config'] - self.driver.client.get_listener_status.side_effect = [ - dict(status='BLAH')] - - self.driver.update(self.sl, self.sv) - - self.driver.client.start_listener.assert_called_once_with( - self.amp, self.sl.id) - def test_upload_cert_amp(self): self.driver.upload_cert_amp(self.amp, six.b('test')) self.driver.client.update_cert_for_rotation.assert_called_once_with( diff --git a/releasenotes/notes/admin-state-up-fix-4aa278eac67646ae.yaml b/releasenotes/notes/admin-state-up-fix-4aa278eac67646ae.yaml new file mode 100644 index 0000000000..a59816510b --- /dev/null +++ b/releasenotes/notes/admin-state-up-fix-4aa278eac67646ae.yaml @@ -0,0 +1,7 @@ +--- +upgrade: + - To fix the admin-state-up bug you must upgrade your + amphora image. +fixes: + - Fixes admin-state-up=False action for loadbalancer + and listener.