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 (community) 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
=== 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 @@
862862
863863
864def backup_cronjob(disable=False):864def backup_cronjob(disable=False):
865 """Generate the cronjob to backup with mongodbump."""865 """Generate the cronjob to backup with mongodump."""
866 juju_log('Setting up cronjob')866 juju_log('Setting up cronjob')
867 config_data = config()867 config_data = config()
868 backupdir = config_data['backup_directory']868 backupdir = config_data['backup_directory']
869869
=== 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 @@
1#!/usr/bin/env python1#!/usr/bin/env python
2"""Generate the cronjob to backup with mongodbump."""2"""Generate the cronjob to backup with mongodump."""
33
4import logging
4from os import chdir5from os import chdir
5from os import listdir6from os import listdir
6from os import remove7from os import remove
@@ -11,6 +12,9 @@
11import subprocess12import subprocess
12from tempfile import mkdtemp13from tempfile import mkdtemp
1314
15logging.basicConfig()
16# use backup.py as logger name because __name__ will be __main__
17logger = logging.getLogger('backup.py')
1418
15when = datetime.now()19when = datetime.now()
16backupdir = '$backup_directory'20backupdir = '$backup_directory'
@@ -28,7 +32,11 @@
28current_backups = listdir(backupdir)32current_backups = listdir(backupdir)
29current_backups.sort()33current_backups.sort()
30for file in current_backups[0:-$backup_copies]:34for file in current_backups[0:-$backup_copies]:
31 remove(join(backupdir, file))35 fname = join(backupdir, file)
36 try:
37 remove(fname)
38 except Exception as e:
39 logger.warning("could not remove %s:%s", fname, e)
3240
33chdir(tmpdir)41chdir(tmpdir)
3442

Subscribers

People subscribed via source and target branches