Merge lp:~caio1982/ubuntu-system-image/sis-nondestructive into lp:ubuntu-system-image/server

[Caio Begotti]

Folks, these changes are small but quite important for us on PES as we got our built images deleted without much alert inside our storage, which was pointing to the same publish path when we hooked it to image server. So we think this should be optional in the scripts, and also we should avoid deleting the publish path itself, otherwise all next runs of import-images and copy-images will probably fail with this:

Traceback (most recent call last):
  File "bin/import-images", line 66, in <module>
    pub = tree.Tree(conf)
  File "/tmp/system-image/bin/../lib/systemimage/tree.py", line 153, in __init__
    raise Exception("Invalid path: %s" % path)

To keep the original behaviour you just need to pass option -c to copy and import images calls in your cron jobs.

[Stéphane Graber (stgraber)]

I'm still against changing the default behavior. There are a few people using this code in production and requiring them to notice this change and update their cron entries is unacceptable.

Turn that into a --no-cleanup option and I may reconsider.

review: Disapprove

[lp:~caio1982/ubuntu-system-image/sis-nondestructive]

262. By Caio Begotti on 2015-03-11

turn the new behavior really new, respecting current "production deployments" of the code that might not be easily updated so anyone looking for the new behavior must explicetely pass the --no-cleanup option to copy or import scripts

[Caio Begotti (caio1982)]

Ok, I thought about making it --keep-orphaned/-k but I stuck with your suggestion instead, code updated.

[Caio Begotti (caio1982)]

Also, I left the short option out because there might be better uses of '-n' in the future I guess.

[Stéphane Graber (stgraber)]

This branch is regressing code coverage, please fix.

Name Stmts Miss Cover Missing
----------------------------------------------------------
lib/systemimage/__init__ 0 0 100%
lib/systemimage/config 109 0 100%
lib/systemimage/diff 110 0 100%
lib/systemimage/generators 771 0 100%
lib/systemimage/gpg 124 0 100%
lib/systemimage/tools 177 0 100%
lib/systemimage/tree 513 1 99% 251
----------------------------------------------------------
TOTAL 1804 1 99%

review: Needs Fixing

[Łukasz Zemczak (sil2100)]

Caio, could you re-visit this branch? If you're too busy I could pick it up in your stead as I would love to get this in as well. Without this flag sometimes I need to re-download a lot of images in my local staging instance when something goes wrong.

[Caio Begotti (caio1982)]

Lukasz, feel free to pick it up if it's very urgent for you but I plan
to get back to my semi-abandoned branches sometime next week!

On Fri, May 8, 2015 at 10:29 AM, Łukasz Zemczak
<email address hidden> wrote:
> Caio, could you re-visit this branch? If you're too busy I could pick it up in your stead as I would love to get this in as well. Without this flag sometimes I need to re-download a lot of images in my local staging instance when something goes wrong.
> --
> https://code.launchpad.net/~caio1982/ubuntu-system-image/sis-nondestructive/+merge/252581
> You are the owner of lp:~caio1982/ubuntu-system-image/sis-nondestructive.

Unmerged revisions

262. By Caio Begotti on 2015-03-11

turn the new behavior really new, respecting current "production deployments" of the code that might not be easily updated so anyone looking for the new behavior must explicetely pass the --no-cleanup option to copy or import scripts

261. By Caio Begotti on 2015-03-11

avoid being so destructive when running the script, as it might wipe out any artifacts in the same path of the publish path

the different from before is that to keep the old behaviour you need to pass option -c to the copy and import images commands, that is all

for the rest of us it means the default behaviour now is to never wipe out images after they are processed, this way image server can coexist with an archiver storage for instance, like capomastro needs in the PES team to build phone images

=== modified file 'bin/copy-image'
--- bin/copy-image	2015-03-11 20:55:00 +0000
+++ bin/copy-image	2015-03-11 21:27:59 +0000
@@ -36,6 +36,7 @@
     parser.add_argument("-k", "--keep-version", action="store_true",
                         help="Keep the original verison number")
     parser.add_argument("--verbose", "-v", action="count", default=0)
+    parser.add_argument("--no-cleanup", action='store_true', default=False)
 
     args = parser.parse_args()
 
@@ -302,8 +303,10 @@
     pub.sync_aliases(args.destination_channel)
 
     # Remove any orphaned file
-    logging.info("Removing orphaned files from the pool")
-    pub.cleanup_tree()
+    if not args.no_cleanup:
+        orphans = ', '.join(pub.list_orphaned_files())
+        logging.info("Removing orphaned files from the pool: %s", orphans)
+        pub.cleanup_tree()
 
     # Sync the mirrors
     logging.info("Triggering a mirror sync")
 
=== modified file 'bin/import-images'
--- bin/import-images	2015-03-11 20:55:00 +0000
+++ bin/import-images	2015-03-11 21:27:59 +0000
@@ -29,6 +29,7 @@
 if __name__ == '__main__':
     parser = argparse.ArgumentParser(description="image importer")
     parser.add_argument("--verbose", "-v", action="count", default=0)
+    parser.add_argument("--no-cleanup", action='store_true', default=False)
     args = parser.parse_args()
 
     # Setup logging
@@ -301,8 +302,10 @@
         pub.sync_aliases(channel_name)
 
     # Remove any orphaned file
-    logging.info("Removing orphaned files from the pool")
-    pub.cleanup_tree()
+    if not args.no_cleanup:
+        orphans = ', '.join(pub.list_orphaned_files())
+        logging.info("Removing orphaned files from the pool: %s", orphans)
+        pub.cleanup_tree()
 
     # Sync the mirrors
     logging.info("Triggering a mirror sync")
 
=== modified file 'lib/systemimage/tree.py'
--- lib/systemimage/tree.py	2015-03-11 20:55:00 +0000
+++ lib/systemimage/tree.py	2015-03-11 21:27:59 +0000
@@ -243,6 +243,9 @@
         """
 
         for entry in self.list_orphaned_files():
+            if entry in self.path:
+                continue
+
             if os.path.isdir(entry):
                 os.rmdir(entry)
             else: