Address the code review.

Fix return  value in the should_run_scheduled_action and the comments.
Correct the json composition in the scheduled_restart test.
Fix the delete in case there is no scheduled restart.
Fix the usage of format in the logger output.
Fix the indentation in the evaluate_scheduled_restart.
Fix the condition related to the body_is_optional in the do_POST_restart.
Fix a few typos in the error messages.
Fix the _read_json_content
Make the scheduled restart unit-tests a bit less ugly

features/steps/patroni_api.py

@@ -106,13 +106,8 @@ def scheduled_failover(context, at_url, from_host, to_host, in_seconds):
 @step('I issue a scheduled restart at {url:url} in {in_seconds:d} seconds with {data}')
 def scheduled_restart(context, url, in_seconds, data):
     data = data and json.loads(data) or {}
-    restart_options = ['"schedule": "{0}"'.format((datetime.now(pytz.utc) + timedelta(seconds=int(in_seconds))).isoformat())]
-    for key in data:
-        if key == 'schedule':
-            continue
-        restart_options.append('"{0}": "{1}"'.format(key, data[key]))
-    context.execute_steps(u"""Given I issue a POST request to {0}/restart with {{{1}}}""".
-                          format(url, ','.join(restart_options)))
+    data.update(schedule='{0}'.format((datetime.now(pytz.utc) + timedelta(seconds=int(in_seconds))).isoformat()))
+    context.execute_steps(u"""Given I issue a POST request to {0}/restart with {1}""".format(url, json.dumps(data)))
 
 
 @step('I add tag {tag:w} {value:w} to {pg_name:w} config')

patroni/api.py

@@ -119,7 +119,7 @@ class RestApiHandler(BaseHTTPRequestHandler):
 
     def _read_json_content(self, body_is_optional=False):
         if 'content-length' not in self.headers:
-            return self.send_error(411) if body_is_optional else None
+            return self.send_error(411) if not body_is_optional else {}
         try:
             content_length = int(self.headers.get('content-length'))
             if content_length == 0 and body_is_optional:
@@ -195,7 +195,9 @@ class RestApiHandler(BaseHTTPRequestHandler):
         status_code = 500
         data = 'restart failed'
         request = self._read_json_content(body_is_optional=True)
-        request = request or {}
+        if request is None:
+            # failed to parse the json
+            return
         if request:
             logger.debug("received restart request: {0}".format(request))
         for k in request:
@@ -225,7 +227,6 @@ class RestApiHandler(BaseHTTPRequestHandler):
                     status_code = 200 if status else 503
                 except Exception:
                     logger.exception('Exception during restart')
-                    data = "Schedule required for the scheduled restart"
                     status_code = 400
             else:
                 request['postmaster_start_time'] = self.server.patroni.ha.state_handler.postmaster_start_time()
@@ -239,9 +240,12 @@ class RestApiHandler(BaseHTTPRequestHandler):
 
     @check_auth
     def do_DELETE_restart(self):
-        self.server.patroni.ha.delete_future_restart()
-        data = "scheduled restart deleted"
-        code = 200
+        if self.server.patroni.ha.delete_future_restart():
+            data = "scheduled restart deleted"
+            code = 200
+        else:
+            data = "no restarts are scheduled"
+            code = 404
         self._write_response(code, data)
 
     @check_auth

patroni/ha.py

@@ -174,7 +174,7 @@ class Ha(object):
             tags = json.get('tags', dict())
             return (member, True, not is_master, xlog_location, tags)
         except:
-            logging.exception('request failed: GET %s', member.api_url)
+            logger.exception('request failed: GET %s', member.api_url)
         return (member, False, None, 0, {})
 
     def fetch_nodes_statuses(self, members):
@@ -293,33 +293,35 @@ class Ha(object):
 
     def should_run_scheduled_action(self, action_name, scheduled_at, cleanup_fn):
         if scheduled_at:
-            # If the faildover is in the far future, we shouldn't do anything and just return.
-            # If the failover is in the past, we consider the value to be stale and we remove
+            # If the scheduled action is in the far future, we shouldn't do anything and just return.
+            # If the scheduled action is in the past, we consider the value to be stale and we remove
             # the value.
-            # If the value is close to now, we initiate the failover
+            # If the value is close to now, we initiate the scheduled action
+            # Additionally, if the scheduled action cannot be executed altogether, i.e. there is an error
+            # or the action is in the past - we take care of cleaning it up.
             now = datetime.datetime.now(pytz.utc)
             try:
                 delta = (scheduled_at - now).total_seconds()
 
                 if delta > self.patroni.nap_time:
-                    logging.info('Awaiting {0} at %s (in %.0f seconds)'.format(action_name),
-                                 scheduled_at.isoformat(), delta)
+                    logger.info('Awaiting %s at %s (in %.0f seconds)',
+                                action_name, scheduled_at.isoformat(), delta)
                     return False
                 elif delta < - int(self.patroni.nap_time * 1.5):
-                    logger.warning('Found a stale {0} value, cleaning up: %s'.format(action_name),
-                                   scheduled_at.isoformat())
+                    logger.warning('Found a stale %s value, cleaning up: %s',
+                                   action_name, scheduled_at.isoformat())
                     cleanup_fn()
                     self.dcs.manual_failover('', '', index=self.cluster.failover.index)
-                    return None
+                    return False
 
                 # The value is very close to now
                 sleep(max(delta, 0))
                 logger.info('Manual scheduled {0} at %s'.format(action_name), scheduled_at.isoformat())
                 return True
             except TypeError:
-                logger.warning('Incorrect value in of scheduled_at: %s', scheduled_at)
+                logger.warning('Incorrect value of scheduled_at: %s', scheduled_at)
                 cleanup_fn()
-        return None
+        return False
 
     def process_manual_failover_from_leader(self):
         failover = self.cluster.failover
@@ -401,11 +403,11 @@ class Ha(object):
         if (restart_data and
            self.should_run_scheduled_action('restart', restart_data['schedule'], self.delete_future_restart)):
             try:
-                    ret, message = self.restart(restart_data)
-                    if ret:
-                        logger.info("Scheduled restart successfull")
-                    else:
-                        logger.warning("Scheduled restart: {0}".format(message))
+                ret, message = self.restart(restart_data)
+                if ret:
+                    logger.info("Scheduled restart successful")
+                else:
+                    logger.warning("Scheduled restart: {0}".format(message))
             finally:
                 self.delete_future_restart()
 
@@ -442,8 +444,12 @@ class Ha(object):
         return False
 
     def delete_future_restart(self):
+        ret = False
         with self._async_executor:
-            self.patroni.scheduled_restart = {}
+            if self.patroni.scheduled_restart:
+                self.patroni.scheduled_restart = {}
+                ret = True
+        return ret
 
     def immediate_restart_scheduled(self):
         return self._async_executor.scheduled_action == 'restart'

tests/test_api.py

@@ -72,7 +72,7 @@ class MockPatroni(object):
     version = '0.00'
     noloadbalance = Mock(return_value=False)
     scheduled_restart = {'schedule': dateutil.parser.parse('2016-08-29 12:45TZ+1'),
-                         'postmaster_start_time': '2016-08-20 12:00TZ+1'}
+                         'postmaster_start_time': postgresql.postmaster_start_time()}
 
     @staticmethod
     def sighup_handler():
@@ -184,24 +184,27 @@ class TestRestApiHandler(unittest.TestCase):
             MockRestApiServer(RestApiHandler, request)
 
         post = request + '\nContent-Length: '
-        # wrong version
-        request = post + '84\n\n{"schedule": "2016-08-20 12:45TZ+1", "role": "unknown", "postgres_version": "9.5.3"}'
-        MockRestApiServer(RestApiHandler, request)
+
+        def make_request(request):
+            return '{0}{1}\n\n{2}'.format(post, len(request), request)
         # wrong role
-        request = post + '85\n\n{"schedule": "2016-08-20 12:45TZ+1", "role": "master", "postgres_version": "9.5.3.1"}'
+        request = make_request('{"schedule": "2016-08-20 12:45TZ+1", "role": "unknown", "postgres_version": "9.5.3"}')
+        MockRestApiServer(RestApiHandler, request)
+        # wrong version
+        request = make_request('{"schedule": "2016-08-20 12:45TZ+1", "role": "master", "postgres_version": "9.5.3.1"}')
         MockRestApiServer(RestApiHandler, request)
         # unknown filter
-        request = post + '55\n\n{"schedule": "2016-08-29 12:45TZ+1", "batman": "lives"}'
+        request = make_request('{"schedule": "2016-08-29 12:45TZ+1", "batman": "lives"}')
         MockRestApiServer(RestApiHandler, request)
         # incorrect schedule
-        request = post + '55\n\n{"schedule": "2016-08-42 12:45TZ+1", "role": "master"}'
+        request = make_request('{"schedule": "2016-08-42 12:45TZ+1", "role": "master"}')
         MockRestApiServer(RestApiHandler, request)
         # everything fine, but the schedule is missing
-        request = post + '47\n\n{"role": "master", "postgres_version": "9.5.2"}'
+        request = make_request('{"role": "master", "postgres_version": "9.5.2"}')
         MockRestApiServer(RestApiHandler, request)
         for retval in (True, False):
             with patch.object(MockHa, 'schedule_future_restart', Mock(return_value=retval)):
-                request = post + '36\n\n{"schedule": "2016-08-29 12:45TZ+1"}'
+                request = make_request('{"schedule": "2016-08-29 12:45TZ+1"}')
                 MockRestApiServer(RestApiHandler, request)
 
     def test_do_DELETE_restart(self):