New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Idempotent mysql_db dump #43419

Open
wants to merge 7 commits into
base: devel
from

Conversation

Projects
None yet
4 participants
@Uwila
Contributor

Uwila commented Jul 30, 2018

SUMMARY

Currently using state: dump with the mysql_db is not idempotent, which I wanted to fix. I don't know whether to mark this as a bug or a feature. It's not doing what it should currently be doing, so in that sense its a bug I guess.

The solution is simple, and comparable to that of the copy module for example. If the destination file already exists, then get the checksum, and compare it to the checksum of the new dump. If these are equal then assume that no changes were made. Unfortunately you still have to do the entire dump, so it doesn't really save time, but at least it now reports the proper result (which is useful for applications building on Ansible, it's useful because you can verify the idempotence of your own roles, and most importantly: It's correct)

This required another change however. In the 'regular' dump there's an additional timestamp of when the dump was completed, which of course ruins checksum comparisons. However, it seemed to me that this information in the database dump is undesired to begin with, and you probably always want to use the --compact option. So I added that. It can be disabled easily using compact: no.
I wasn't sure whether I should enable it by default, because defaults are often to have things turned off, at least that's my impression, but I think for most uses you want it on, and its needed for the actual feature that this PR is about.
I also wasn't sure what version to mark it as. I thought 2.7 is now in development so I used that.
I couldn't get the generation of docs working, so someone should check that they make some sense.

ISSUE TYPE
  • Bugfix Pull Request
COMPONENT NAME

mysql_db

ANSIBLE VERSION
ansible 2.7.0.dev0 (idempotent-mysql_db-dump 7b18490b7c) last updated 2018/07/30 11:19:52 (GMT +200)
  config file = None
  configured module search path = ['/home/user/.ansible/plugins/modules', '/usr/share/ansible/plugins/modules']
  ansible python module location = /home/user/documents/programming/ansible/lib/ansible
  executable location = /home/user/.venv/bin/ansible
  python version = 3.6.6 (default, Jun 27 2018, 13:11:40) [GCC 8.1.1 20180531]
ADDITIONAL INFORMATION

Take a look at the tasks below. The last two tasks are identical, and as such the last task shouldn't report changed but just ok. Old behavior is to report changed, but after this PR the last task will just report ok.

Tasks:

---
- name: MySQL packages installed
  apt:
    update-cache: true
    state: present
    name:
      - python-mysqldb
      - mysql-client
      - mysql-server
      - unzip

- name: MySQL running
  service:
    name: mysql
    state: started

- name: Sample database dump present
  get_url:
    url: http://sportsdb.org/modules/sd/assets/downloads/mlb-samples-2008.09.19.sql.zip
    dest: /database.sql.zip

- name: Unzip database
  unarchive:
    src: /database.sql.zip
    remote_src: true
    dest: /

- name: Sample database present
  mysql_db:
    name: test-db
    state: import
    target: /mlb-samples-2008.09.19.sql

- name: Dump
  mysql_db:
    name: test-db
    state: dump
    target: /tmp/dump.sql

- name: Dump again
  mysql_db:
    name: test-db
    state: dump
    target: /tmp/dump.sql

Old result:

    TASK [Gathering Facts] *********************************************************
    ok: [molecule-test-import-changed]
    
    TASK [mysql-import-changed : MySQL packages installed] *************************
    changed: [molecule-test-import-changed]
    
    TASK [mysql-import-changed : MySQL running] ************************************
    changed: [molecule-test-import-changed]
    
    TASK [mysql-import-changed : Sample database dump present] *********************
    changed: [molecule-test-import-changed]
    
    TASK [mysql-import-changed : Unzip database] ***********************************
    changed: [molecule-test-import-changed]
    
    TASK [mysql-import-changed : Sample database present] **************************
    changed: [molecule-test-import-changed]
    
    TASK [mysql-import-changed : Dump] *********************************************
    changed: [molecule-test-import-changed]
    
    TASK [mysql-import-changed : Dump again] ***************************************
    changed: [molecule-test-import-changed]
    
    PLAY RECAP *********************************************************************
    molecule-test-import-changed : ok=8    changed=7    unreachable=0    failed=0

New result:

    TASK [Gathering Facts] *********************************************************
    ok: [molecule-test-import-changed]
    
    TASK [mysql-import-changed : MySQL packages installed] *************************
    changed: [molecule-test-import-changed]
    
    TASK [mysql-import-changed : MySQL running] ************************************
    changed: [molecule-test-import-changed]
    
    TASK [mysql-import-changed : Sample database dump present] *********************
    changed: [molecule-test-import-changed]
    
    TASK [mysql-import-changed : Unzip database] ***********************************
    changed: [molecule-test-import-changed]
    
    TASK [mysql-import-changed : Sample database present] **************************
    changed: [molecule-test-import-changed]
    
    TASK [mysql-import-changed : Dump] *********************************************
    changed: [molecule-test-import-changed]
    
    TASK [mysql-import-changed : Dump again] ***************************************
    ok: [molecule-test-import-changed]
    
    PLAY RECAP *********************************************************************
    molecule-test-import-changed : ok=8    changed=6    unreachable=0    failed=0
Added `compact` option and check for checksum change
Added a compact option; the motivation for this being that a compact
dump will not include a timestamp, and as such multiple dumps will be
equal, which is needed for another feature:
If the target already exists, then the checksum of the old file and
the dump will be compared to check for change. This makes dumping the
database idempotent.

@jborean93 jborean93 removed the needs_triage label Aug 2, 2018

@mattclay

A few minor things to include. Also, could you add a changelog fragment (see changelogs/fragments/) describing the change?

@@ -62,6 +62,13 @@
required: false
default: []
version_added: "2.7"
compact:
description:
- When I(state) is C(dump), generate compact dump. Equivalent to C(--compact) flag in C(mysqldump).

This comment has been minimized.

@mattclay

mattclay Aug 8, 2018

Member

Include a note about disabling compact resulting in the module always reporting changed.

@@ -77,7 +77,6 @@
- name: assert output message backup the database
assert:
that:
- "result.changed == true"
- "result.db =='{{ db_name }}'"

This comment has been minimized.

@mattclay

mattclay Aug 8, 2018

Member

Include some integration tests to cover the new behavior.

@mattclay

This comment has been minimized.

Member

mattclay commented Aug 8, 2018

@mattclay mattclay added the ci_verified label Aug 8, 2018

@ansibot ansibot removed the ci_verified label Aug 8, 2018

@ansibot ansibot added the support:core label Aug 8, 2018

@Uwila

This comment has been minimized.

Contributor

Uwila commented Aug 9, 2018

Trying to fix the CI, I discovered that the --compact option has some more implications that might not be that nice; it gives errors when importing a dump if tables already exist, so I guess --compact will also remove IF NOT EXISTS statements and the like. There are a lot of other flags that can be set to get the desired behavior, but then I'd be making quite a mess. Perhaps it would be better if I just created a new module mysql_dump with all the flags that mysqldump supports? If so I'll make an issue for it and work on it pretty soon.

Alternatively we merge this and create a change that could break things for some people. In that case I'll just have to figure out how to get the CI working and I'm done.

Please let me know what you prefer @mattclay

@ansibot ansibot added the needs_repo label Dec 12, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment