CQ: add landmines to delete database after unclean shutdown

commit-queue/commit_queue.py

 #!/usr/bin/env python
 # Copyright (c) 2012 The Chromium Authors. All rights reserved.
 # Use of this source code is governed by a BSD-style license that can be
 # found in the LICENSE file.
 """Commit queue executable.
 
 Reuse Rietveld and the Chromium Try Server to process and automatically commit
 patches.
 """
 
@@ skipping 105 matching lines @@
   logging_rotating_file = logging.handlers.RotatingFileHandler(
       filename=os.path.join(log_directory, 'commit_queue.log'),
       maxBytes= 10*1024*1024,
       backupCount=50)
   logging_rotating_file.setLevel(logging.DEBUG)
   logging_rotating_file.setFormatter(logging.Formatter(
       '%(asctime)s %(levelname)-8s %(module)15s(%(lineno)4d): %(message)s'))
   logging.getLogger().addHandler(logging_rotating_file)
 
 
+class SignalInterrupt(Exception):
+  """Exception that indicates being interrupted by a caught signal."""

[iannucci, 2013/09/25 19:47:07]

It might be nice to eventually encapsulate the actual signal type which we
caught, but I think right now it's only SIGINT.

[Paweł Hajdan Jr., 2013/09/25 21:53:36]

Done.

+  pass
+
+
 def main():
   parser = optparse.OptionParser(
       description=sys.modules['__main__'].__doc__)
   project_choices = projects.supported_projects()
   parser.add_option('-v', '--verbose', action='store_true')
   parser.add_option(
       '--no-dry-run',
       action='store_false',
       dest='dry_run',
       default=True,
@@ skipping 92 matching lines @@
         # Disable the checkout.
         print 'Using no checkout'
         pc.context.checkout = FakeCheckout()
       else:
         print 'Using read-only checkout'
         pc.context.checkout = checkout.ReadOnlyCheckout(pc.context.checkout)
       # Save pushed events on disk.
       print 'Using read-only chromium-status interface'
       pc.context.status = async_push.AsyncPushStore()
 
+    landmine_path = os.path.join(work_dir,
+                                 pc.context.checkout.project_name + '.landmine')
     db_path = os.path.join(work_dir, pc.context.checkout.project_name + '.json')
     if os.path.isfile(db_path):
-      try:
-        pc.load(db_path)
-      except ValueError:
+      if os.path.isfile(landmine_path):
+        # Remove saved database if previous shutdown was not clean.
+        # this helps to avoid running with inconsistent database
+        # and potentially running changes without verification.
+        logging.warning('Deleting database because previous shutdown '
+                        'was unclean.')
         os.remove(db_path)

[iannucci, 2013/09/25 19:47:07]

Can we save the db off to a mktemp backup instead, and then note that in the log
message? Like db.crashed.Wm3fks ?

[Paweł Hajdan Jr., 2013/09/25 21:53:36]

Done.

+      else:
+        try:
+          pc.load(db_path)
+        except ValueError:
+          os.remove(db_path)

[iannucci, 2013/09/25 19:47:07]

same here with backup/log message

[Paweł Hajdan Jr., 2013/09/25 21:53:36]

Done.

+
+    # Create a file to indicate unclean shutdown.
+    with open(landmine_path, 'w') as _landmine_file:

[iannucci, 2013/09/25 19:47:07]

you can just leave off the 'as ...' part.

[Paweł Hajdan Jr., 2013/09/25 21:53:36]

Done.

+      pass
 
     sig_handler.installHandlers(
         signal.SIGINT,
         signal.SIGHUP
     )
 
     # Sync every 5 minutes.
     SYNC_DELAY = 5*60
     try:
       if options.query_only:
         pc.look_for_new_pending_commit()
         pc.update_status()
         print(str(pc.queue))
+        os.remove(landmine_path)
         return 0
 
       now = time.time()
       next_loop = now + options.poll_interval
       # First sync is on second loop.
       next_sync = now + options.poll_interval * 2
       while True:
         # In theory, we would gain in performance to parallelize these tasks. In
         # practice I'm not sure it matters.
         pc.look_for_new_pending_commit()
         pc.process_new_pending_commit()
         pc.update_status()
         pc.scan_results()
         if sig_handler.getTriggeredSignals():
-          raise KeyboardInterrupt()
+          raise SignalInterrupt()
         # Save the db at each loop. The db can easily be in the 1mb range so
         # it's slowing down the CQ a tad but it in the 100ms range even for that
         # size.
         pc.save(db_path)
 
         # More than a second to wait and due to sync.
         now = time.time()
         if (next_loop - now) >= 1 and (next_sync - now) <= 0:
           if sys.stdout.isatty():
             sys.stdout.write('Syncing while waiting                \r')
             sys.stdout.flush()
           try:
             pc.context.checkout.prepare(None)
           except subprocess2.CalledProcessError as e:
             # Don't crash, most of the time it's the svn server that is dead.
             # How fun. Send a stack trace to annoy the maintainer.
             errors.send_stack(e)
           next_sync = time.time() + SYNC_DELAY
 
         now = time.time()
         next_loop = max(now, next_loop)
         while True:
           # Abort if any signals are set
           if sig_handler.getTriggeredSignals():
-            raise KeyboardInterrupt()
+            raise SignalInterrupt()
           delay = next_loop - now
           if delay <= 0:
             break
           if sys.stdout.isatty():
             sys.stdout.write('Sleeping for %1.1f seconds          \r' % delay)
             sys.stdout.flush()
           time.sleep(min(delay, 0.1))
           now = time.time()
         if sys.stdout.isatty():
           sys.stdout.write('Running (please do not interrupt)   \r')
           sys.stdout.flush()
         next_loop = time.time() + options.poll_interval
     except:  # Catch all fatal exit conditions.
       logging.exception('CQ loop terminating')
       raise
     finally:
       logging.warning('Saving db...')
       pc.save(db_path)
       pc.close()
       logging.warning('db save successful.')
+  except SignalInterrupt as e:

[iannucci, 2013/09/25 19:47:07]

unless you want to do something with e, you should just

except SignalInterrupt:

[Paweł Hajdan Jr., 2013/09/25 21:53:36]

Done.

+    # This is considered a clean shutdown: we only throw this exception
+    # from selected places in the code where the database should be
+    # in a known and consistent state.
+    os.remove(landmine_path)
+
+    print 'By bye'

[iannucci, 2013/09/25 19:47:07]

'Bye bye: caught SignalInterrupt'

Also, should these prints be logs instead?

[Paweł Hajdan Jr., 2013/09/25 21:53:36]

Not sure, they were prints before so I left it that way.

There are potentially many more things to be fixed with CQ while I'm there.

+    # 23 is an arbitrary value to signal loop.sh that it must stop looping.
+    return 23
   except KeyboardInterrupt as e:

[iannucci, 2013/09/25 19:47:07]

same here re: 'as e'

[Paweł Hajdan Jr., 2013/09/25 21:53:36]

Done.

+    # This is actually an unclean shutdown. Do not remove the landmine file.
+    # One example of this is user hitting ctrl-c twice at an arbitrary point
+    # inside the CQ loop. There are no guarantees about consistent state
+    # of the database then.
+
     print 'Bye bye'

[iannucci, 2013/09/25 19:47:07]

maybe 'Unclean shutdown! caught KeyboardInterrupt'

when I did the keyboard interrupt stuff, I wanted to put interrupt points in
more places so that the cq would always terminate basically immediately, but M-A
said this would be very hard to do right so I never bothered :/

[Paweł Hajdan Jr., 2013/09/25 21:53:36]

Done.

     # 23 is an arbitrary value to signal loop.sh that it must stop looping.
     return 23
   except errors.ConfigurationError as e:
     parser.error(str(e))

[iannucci, 2013/09/25 19:47:07]

Can we log here?

[Paweł Hajdan Jr., 2013/09/25 21:53:36]

I'd prefer not to clutter the patch with random cleanups around it.

[iannucci, 2013/09/25 22:15:48]

Actually I think parser.error will log/print anyhow

     return 1
+
+  # CQ generally doesn't exit by itself, but if we ever get here, it looks
+  # like a clean shutdown so remove the landmine file.
+  os.remove(landmine_path)

[iannucci, 2013/09/25 19:47:07]

Maybe add a log/print here

[Paweł Hajdan Jr., 2013/09/25 21:53:36]

I don't think it's needed - if you can come up with a reasonable reason to log I
can add it - other than it _might_ be useful.

[iannucci, 2013/09/25 22:15:48]

Just reacting to the comment. If we never expect to get here, I would be more
inclined to assert False.

[Paweł Hajdan Jr., 2013/09/25 23:27:17]

Added a TODO. It seems too risky to change behavior beyond scope of this patch.

   return 0
 
 
 if __name__ == '__main__':
   fix_encoding.fix_encoding()
   sys.exit(main())