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

Proposed by Caio Begotti on 2015-03-11
Status: Work in progress
Proposed branch: lp:~caio1982/ubuntu-system-image/sis-nondestructive
Merge into: lp:ubuntu-system-image/server
Diff against target: 63 lines (+13/-4)
3 files modified
bin/copy-image (+5/-2)
bin/import-images (+5/-2)
lib/systemimage/tree.py (+3/-0)
To merge this branch: bzr merge lp:~caio1982/ubuntu-system-image/sis-nondestructive
Reviewer Review Type Date Requested Status
Stéphane Graber 2015-03-11 Needs Fixing on 2015-04-10
Review via email: mp+252581@code.launchpad.net

Description of the change

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.

To post a comment you must log in.
Stéphane Graber (stgraber) wrote :

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

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

Caio Begotti (caio1982) wrote :

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

Stéphane Graber (stgraber) wrote :

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

Caio Begotti (caio1982) wrote :

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

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'bin/copy-image'
2--- bin/copy-image 2015-03-11 20:55:00 +0000
3+++ bin/copy-image 2015-03-11 21:27:59 +0000
4@@ -36,6 +36,7 @@
5 parser.add_argument("-k", "--keep-version", action="store_true",
6 help="Keep the original verison number")
7 parser.add_argument("--verbose", "-v", action="count", default=0)
8+ parser.add_argument("--no-cleanup", action='store_true', default=False)
9
10 args = parser.parse_args()
11
12@@ -302,8 +303,10 @@
13 pub.sync_aliases(args.destination_channel)
14
15 # Remove any orphaned file
16- logging.info("Removing orphaned files from the pool")
17- pub.cleanup_tree()
18+ if not args.no_cleanup:
19+ orphans = ', '.join(pub.list_orphaned_files())
20+ logging.info("Removing orphaned files from the pool: %s", orphans)
21+ pub.cleanup_tree()
22
23 # Sync the mirrors
24 logging.info("Triggering a mirror sync")
25
26=== modified file 'bin/import-images'
27--- bin/import-images 2015-03-11 20:55:00 +0000
28+++ bin/import-images 2015-03-11 21:27:59 +0000
29@@ -29,6 +29,7 @@
30 if __name__ == '__main__':
31 parser = argparse.ArgumentParser(description="image importer")
32 parser.add_argument("--verbose", "-v", action="count", default=0)
33+ parser.add_argument("--no-cleanup", action='store_true', default=False)
34 args = parser.parse_args()
35
36 # Setup logging
37@@ -301,8 +302,10 @@
38 pub.sync_aliases(channel_name)
39
40 # Remove any orphaned file
41- logging.info("Removing orphaned files from the pool")
42- pub.cleanup_tree()
43+ if not args.no_cleanup:
44+ orphans = ', '.join(pub.list_orphaned_files())
45+ logging.info("Removing orphaned files from the pool: %s", orphans)
46+ pub.cleanup_tree()
47
48 # Sync the mirrors
49 logging.info("Triggering a mirror sync")
50
51=== modified file 'lib/systemimage/tree.py'
52--- lib/systemimage/tree.py 2015-03-11 20:55:00 +0000
53+++ lib/systemimage/tree.py 2015-03-11 21:27:59 +0000
54@@ -243,6 +243,9 @@
55 """
56
57 for entry in self.list_orphaned_files():
58+ if entry in self.path:
59+ continue
60+
61 if os.path.isdir(entry):
62 os.rmdir(entry)
63 else:

Subscribers

People subscribed via source and target branches

to all changes: