Merge lp:~evarlast/charms/trusty/mongodb/fix-backup into lp:charms/trusty/mongodb

Proposed by Jay R. Wren
Status: Merged
Merge reported by: Stuart Bishop
Merged at revision: not available
Proposed branch: lp:~evarlast/charms/trusty/mongodb/fix-backup
Merge into: lp:charms/trusty/mongodb
Diff against target: 47 lines (+11/-3)
2 files modified
hooks/hooks.py (+1/-1)
templates/backup.py.tpl (+10/-2)
To merge this branch: bzr merge lp:~evarlast/charms/trusty/mongodb/fix-backup
Reviewer Review Type Date Requested Status
Stuart Bishop (community) Approve
Brad Crittenden (community) Approve
Cory Johns Needs Information
Review Queue (community) automated testing Needs Fixing
Marco Ceppi Pending
Charles Butler Pending
Review via email: mp+300671@code.launchpad.net
To post a comment you must log in.
Revision history for this message
uosci-testing-bot (uosci-testing-bot) wrote :

charm_lint_check #3183 mongodb for evarlast mp300671
    LINT OK: passed

Build: http://10.245.162.36:8080/job/charm_lint_check/3183/

Revision history for this message
uosci-testing-bot (uosci-testing-bot) wrote :

charm_unit_test #2347 mongodb for evarlast mp300671
    UNIT OK: passed

Build: http://10.245.162.36:8080/job/charm_unit_test/2347/

Revision history for this message
uosci-testing-bot (uosci-testing-bot) wrote :

charm_amulet_test #818 mongodb for evarlast mp300671
    AMULET FAIL: amulet-test failed

AMULET Results (max last 2 lines):
make: *** [functional_test] Error 1
ERROR:root:Make target returned non-zero.

Full amulet test output: http://paste.ubuntu.com/20238679/
Build: http://10.245.162.36:8080/job/charm_amulet_test/818/

Revision history for this message
Review Queue (review-queue) wrote :

This item has failed automated testing! Results available here http://juju-ci.vapour.ws/job/charm-bundle-test-lxc/4741/

review: Needs Fixing (automated testing)
Revision history for this message
Review Queue (review-queue) wrote :

This item has failed automated testing! Results available here http://juju-ci.vapour.ws/job/charm-bundle-test-aws/4901/

review: Needs Fixing (automated testing)
Revision history for this message
Kevin W Monroe (kwmonroe) wrote :

Hi Chuck/Marco, you're both listed as mongodb maintainers. Will one of you move the ~charmers/mongodb charm into a non-charmers namespace? We currently can't move forward on this until the charm moves out of ~charmers.

Revision history for this message
Cory Johns (johnsca) wrote :

Instead of simply ignoring the failure, what about using shutil.rmtree() to ensure that they do get deleted?

That said, I seem to recall that the maintainers of this charm are working on a refactor. Do we want to move forward with this fix in the meantime?

Revision history for this message
Cory Johns (johnsca) :
review: Needs Information
Revision history for this message
uosci-testing-bot (uosci-testing-bot) wrote :

charm_lint_check #13 mongodb for evarlast mp300671
    LINT OK: passed

Build: http://10.245.162.208:8080/job/charm_lint_check/13/

Revision history for this message
uosci-testing-bot (uosci-testing-bot) wrote :

charm_unit_test #5 mongodb for evarlast mp300671
    UNIT FAIL: unit-test failed

UNIT Results (max last 2 lines):
Makefile:35: recipe for target 'test' failed
ERROR:root:Make target returned non-zero.

Full unit test output: http://paste.ubuntu.com/23110020/
Build: http://10.245.162.208:8080/job/charm_unit_test/5/

Revision history for this message
Jay R. Wren (evarlast) wrote :

Amulet is terrible.

On Mon, Aug 29, 2016 at 10:47 PM, uosci-testing-bot <
<email address hidden>> wrote:

> charm_unit_test #5 mongodb for evarlast mp300671
> UNIT FAIL: unit-test failed
>
> UNIT Results (max last 2 lines):
> Makefile:35: recipe for target 'test' failed
> ERROR:root:Make target returned non-zero.
>
> Full unit test output: http://paste.ubuntu.com/23110020/
> Build: http://10.245.162.208:8080/job/charm_unit_test/5/
> --
> https://code.launchpad.net/~evarlast/charms/trusty/
> mongodb/fix-backup/+merge/300671
> You are the owner of lp:~evarlast/charms/trusty/mongodb/fix-backup.
>

Revision history for this message
Jay R. Wren (evarlast) wrote :

Please take another look.

I'd rather NOT use shutil.rmtree because anything could be in that directory and a charm should not IMO delete trees of files it did not create.

Revision history for this message
Brad Crittenden (bac) :
review: Approve
Revision history for this message
Tom Haddon (mthaddon) wrote :

I've set up https://launchpad.net/mongodb-charm and https://launchpad.net/~mongodb-charmers. We'll work on getting that set up as the new home for the MongoDB charm and then figure out which teams will be involved in the maintenance of the charm.

Revision history for this message
Stuart Bishop (stub) wrote :

The bzr branch has been moved to git, details at https://code.launchpad.net/mongodb-charm

I can land this bzr branch there once approved, or the MP can be redone with git branches.

Revision history for this message
Stuart Bishop (stub) wrote :

Wearing my reviewers hat, we should not be removing unexpected files and agree that warning is the best option. Unexpected files were most likely made manually by admins and removing them would be a dataloss bug.

I also agree with bac's inline comment, and that we should be taking more care to only remove files we are reasonably confident were created by the charm. However, that would be a new feature and does not need to land with this branch.

I think this is fine to land as is. It fixes a crash, and otherwise does not change existing behaviour.

review: Approve
Revision history for this message
Stuart Bishop (stub) wrote :

I have landed this on https://git.launchpad.net/mongodb-charm , so will set this MP to merged.

Promulgation needs to happen so this and any other pending updates can get released.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'hooks/hooks.py'
2--- hooks/hooks.py 2016-02-26 14:00:17 +0000
3+++ hooks/hooks.py 2016-07-20 21:43:43 +0000
4@@ -862,7 +862,7 @@
5
6
7 def backup_cronjob(disable=False):
8- """Generate the cronjob to backup with mongodbump."""
9+ """Generate the cronjob to backup with mongodump."""
10 juju_log('Setting up cronjob')
11 config_data = config()
12 backupdir = config_data['backup_directory']
13
14=== modified file 'templates/backup.py.tpl'
15--- templates/backup.py.tpl 2013-04-25 15:58:45 +0000
16+++ templates/backup.py.tpl 2016-07-20 21:43:43 +0000
17@@ -1,6 +1,7 @@
18 #!/usr/bin/env python
19-"""Generate the cronjob to backup with mongodbump."""
20+"""Generate the cronjob to backup with mongodump."""
21
22+import logging
23 from os import chdir
24 from os import listdir
25 from os import remove
26@@ -11,6 +12,9 @@
27 import subprocess
28 from tempfile import mkdtemp
29
30+logging.basicConfig()
31+# use backup.py as logger name because __name__ will be __main__
32+logger = logging.getLogger('backup.py')
33
34 when = datetime.now()
35 backupdir = '$backup_directory'
36@@ -28,7 +32,11 @@
37 current_backups = listdir(backupdir)
38 current_backups.sort()
39 for file in current_backups[0:-$backup_copies]:
40- remove(join(backupdir, file))
41+ fname = join(backupdir, file)
42+ try:
43+ remove(fname)
44+ except Exception as e:
45+ logger.warning("could not remove %s:%s", fname, e)
46
47 chdir(tmpdir)
48

Subscribers

People subscribed via source and target branches