Fix admin_state_up for loadbalancer and listener

The admin-state-up=False action for loadbalancer and listener
failed to affect the appropriate change.  This patch corrects that
as well as removes an un-necessary call to the amphora-agent.

Change-Id: I698f964f584d150f162f6c8cb41c65f5c5556b52
Closes-Bug: #1619449

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)

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

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)
 
     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.start_listener = functools.partial(self._action,
-        self.stop_listener = functools.partial(self._action, 'stop')
+                                                consts.AMP_ACTION_START)
-        self.reload_listener = functools.partial(self._action, 'reload')
+        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.start_vrrp = functools.partial(self._vrrp_action,
-        self.stop_vrrp = functools.partial(self._vrrp_action, 'stop')
+                                            consts.AMP_ACTION_START)
-        self.reload_vrrp = functools.partial(self._vrrp_action, 'reload')
+        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

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'

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):

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))

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(

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.