CQ: add landmines to delete database after unclean shutdown

commit-queue/commit_queue.py

Index: commit-queue/commit_queue.py
===================================================================
--- commit-queue/commit_queue.py	(revision 225078)
+++ commit-queue/commit_queue.py	(working copy)
@@ -123,6 +123,11 @@
   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__)
@@ -235,13 +240,27 @@
       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
@@ -254,6 +273,7 @@
         pc.look_for_new_pending_commit()
         pc.update_status()
         print(str(pc.queue))
+        os.remove(landmine_path)
         return 0
 
       now = time.time()
@@ -268,7 +288,7 @@
         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.
@@ -293,7 +313,7 @@
         while True:
           # Abort if any signals are set
           if sig_handler.getTriggeredSignals():
-            raise KeyboardInterrupt()
+            raise SignalInterrupt()
           delay = next_loop - now
           if delay <= 0:
             break
@@ -314,13 +334,31 @@
       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