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

[Jay R. Wren]

[uosci-testing-bot (uosci-testing-bot)]

charm_lint_check #3183 mongodb for evarlast mp300671
    LINT OK: passed

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

[uosci-testing-bot (uosci-testing-bot)]

charm_unit_test #2347 mongodb for evarlast mp300671
    UNIT OK: passed

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

[uosci-testing-bot (uosci-testing-bot)]

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/

[Review Queue (review-queue)]

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)

[Review Queue (review-queue)]

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)

[Kevin W Monroe (kwmonroe)]

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.

[Cory Johns (johnsca)]

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?

[Cory Johns (johnsca)]

review: Needs Information

[uosci-testing-bot (uosci-testing-bot)]

charm_lint_check #13 mongodb for evarlast mp300671
    LINT OK: passed

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

[uosci-testing-bot (uosci-testing-bot)]

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/

[Jay R. Wren (evarlast)]

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

[Jay R. Wren (evarlast)]

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.

[Brad Crittenden (bac)]

review: Approve

[Tom Haddon (mthaddon)]

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.

[Stuart Bishop (stub)]

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.

[Stuart Bishop (stub)]

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

[Stuart Bishop (stub)]

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.

=== modified file 'hooks/hooks.py'
--- hooks/hooks.py	2016-02-26 14:00:17 +0000
+++ hooks/hooks.py	2016-07-20 21:43:43 +0000
@@ -862,7 +862,7 @@
 
 
 def backup_cronjob(disable=False):
-    """Generate the cronjob to backup with mongodbump."""
+    """Generate the cronjob to backup with mongodump."""
     juju_log('Setting up cronjob')
     config_data = config()
     backupdir = config_data['backup_directory']

=== modified file 'templates/backup.py.tpl'
--- templates/backup.py.tpl	2013-04-25 15:58:45 +0000
+++ templates/backup.py.tpl	2016-07-20 21:43:43 +0000
@@ -1,6 +1,7 @@
 #!/usr/bin/env python
-"""Generate the cronjob to backup with mongodbump."""
+"""Generate the cronjob to backup with mongodump."""
 
+import logging
 from os import chdir
 from os import listdir
 from os import remove
@@ -11,6 +12,9 @@
 import subprocess
 from tempfile import mkdtemp
 
+logging.basicConfig()
+# use backup.py as logger name because __name__ will be __main__
+logger = logging.getLogger('backup.py')
 
 when = datetime.now()
 backupdir = '$backup_directory'
@@ -28,7 +32,11 @@
 current_backups = listdir(backupdir)
 current_backups.sort()
 for file in current_backups[0:-$backup_copies]:
-    remove(join(backupdir, file))
+    fname = join(backupdir, file)
+    try:
+        remove(fname)
+    except Exception as e:
+        logger.warning("could not remove %s:%s", fname, e)
 
 chdir(tmpdir)