Multiple backup storage backends #235

[mjlabe]

I have started adding the ability to have a "fallback" (i.e. 2nd restore location) for backups. I still need to finish testing, but I wanted to get feedback before I finish. Do we want a single "fallback" option like this, or do we want to support N backup options (like suggested in #61)? Updates to allow for N will be much more difficult. (closes #235 )

[jonathan-s]

Hey @mjlabe, thanks for starting to work on this. From what I gather on the two issues it seems like it would be more desirable to have these defined in a dictionary as described in #61.

That way you could do something akin to dbbackup --storage=local which would run a backup for the local storage. dbbackup would run the default storage which is always expected. And if there is no dictionary defined it'll use the way you define storages right now so that it stays backwards compatible.

It seems like this would give the most flexibility.

If you want to backup at both locations at the same time you'd need to run the backup command twice but with different storages.

[mjlabe]

@jonathan-s Better? I still need to figure out how to unit test it. If I just update the settings in the test to have 2 local storages, would that be good enough? Example:

STORAGES = {
    'default': 'django.core.files.storage.FileSystemStorage',
    's3': 'django.core.files.storage.FileSystemStorage'
}

[ZuluPro]

Hello @jonathan-s @mjlabe

To give you more context, when I had this idea,
Ii wanted to use the future feature Django that never came.
It was supposed to handle storage like we handle DB. (what you did in this PR)

Otherwise, great job!
What are your issues with tests?

[mjlabe]

I just haven't familiarized myself enough with the code base/unit tests yet to verify the backup is created/restored.

[jonathan-s]

Yeah, that's better. However, we also need to take into account that each storage has different options. So I would say that the following structure would be more apt.

DBBACKUP_STORAGES = {
    'default': {
        'storage': 'storages.backends.s3boto3.S3Boto3Storage',
        'access_key': 'my_id1',
        'secret_key': 'my_secret1',
        'bucket_name': 'my_bucket_name1',
        'default_acl': 'private1',
    },
    's3fallback': {
        'storage': 'storages.backends.s3boto3.S3Boto3Storage',
        'access_key': 'my_id',
        'secret_key': 'my_secret',
        'bucket_name': 'my_bucket_name',
        'default_acl': 'private',
    }
}

[mjlabe]

And that's where it becomes a significant pain. I can't even find those settings in the source code. Aren't they defined in settings for the specific library (i.e. S3_Bucket_Name and not DBBackup_Bucket_Name)? I don't know of a way to override Django settings dynamically while it's running.

Edit: OK. I found it in the docs. I just need to set options in the storage class like I'm setting the storage class.

DBBACKUP_STORAGE_OPTIONS = {
    'access_key': 'my_id',
    'secret_key': 'my_secret',
    'bucket_name': 'my_bucket_name',
    'default_acl': 'private',
}

That should be fine.

[mjlabe]

I found a potential problem. Are you overwriting the options in the Storage class __init__ regardless of what options were passed?

https://github.com/django-dbbackup/django-dbbackup/blob/4ad864b2ae288ae1155c1bb2b2215121379c7223/dbbackup/storage.py#L63-L65

If I removed that line (64), all unit tests still pass, so what is the purpose?

[ZuluPro]

@mjlabe

settings.STORAGE_OPTIONS can be considered a global storage conf.

[mjlabe]

Ok. updated to add global options to Storage options if it does not already exist (to prevent overwriting local setting). I will work on unit tests soon.

[jonathan-s]

@mjlabe

Aren't they defined in settings for the specific library (i.e. S3_Bucket_Name and not DBBackup_Bucket_Name)?

I believe it used to be like that, but that's no longer the case.

[benjaoming]

I don't know of a way to override Django settings dynamically while it's running.

You really never should have to do this. If we have a need for that, then the design is most likely somehow wrong.

[mjlabe]

Just an update. I have not abandoned this PR, but I've been sick since the week I opened the PR. I hope to get back to it soon.

[jonathan-s]

I hope you get well soon!

…

Sent from my mobile

On Wed, 11 Nov 2020, 17:17 mjlabe, ***@***.***> wrote: Just an update. I have not abandoned this PR, but I've been sick since the week I opened the PR. I hope to get back to it soon. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#357 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAQGYESJH6BK6QUR7CXVGXDSPK2H7ANCNFSM4SB5I5BQ> .

[mjlabe]

Started working again, but I'm getting this running unit tests on master:

CommandError: There's no backup file available.

/home/marclabelle/Projects/django-dbbackup/dbbackup/db/sqlite.py:79: UserWarning: Error in db restore: UNIQUE constraint failed: django_migrations.id
  warnings.warn("Error in db restore: {}".format(err))
/home/marclabelle/Projects/django-dbbackup/dbbackup/db/sqlite.py:77: UserWarning: Error in db restore: index testapp_manytomanymodel_field_manytomanymodel_id_charmodel_id_69e4ee78_uniq already exists
  warnings.warn("Error in db restore: {}".format(err))
/home/marclabelle/Projects/django-dbbackup/dbbackup/db/sqlite.py:79: UserWarning: Error in db restore: UNIQUE constraint failed: testapp_charmodel.id
  warnings.warn("Error in db restore: {}".format(err))

I assume the first message is part of the tests, but I should I be getting the second? All tests pass, but I wanted to make sure.

[mjlabe]

I will start working on restore tests, but Is this enough testing for backup? Is there a way to test S3?

[jonathan-s]

@mjlabe Sorry for the late reply here. Yeah, it looks likes some of the older tests can be improved. Not sure exactly what is happening there and if the 'errors' are expected or not. But it looks a bit omnious.

As for your tests I'd say that as long as you test that you get the correct configuration I'd say you are golden. You shouldn't need to test that everything is working end to end.

[mjlabe]

Ok. I've written restore tests and added more backup tests to verify the storage configuration in the handle. Please let me know if you find any issues, want changes, or want more tests. Otherwise, this PR is ready.