mysql_replication: add option to fail on error #66252

petoju · 2020-01-07T18:49:13Z

SUMMARY

Module mysql_replication is not failing when error happens on some command call. Add a backwards compatible option to fail in such situation.

ISSUE TYPE

Feature Pull Request

COMPONENT NAME

mysql_replication

ADDITIONAL INFORMATION

Without my change:

Create mysql server, set up replication to slave
Call changemaster from mysql_replication module on running slave or without proper permissions. The call will succeed on ansible level, despite changemaster failing.

With my change, one will have ability to specify fail_on_error. I am scared to make breaking change (as this could break some users), so I am adding an option to fail, when an error happens.
That said, I don't know all error conditions and it's possible there are some cases, when the call fails on python level, but will be successful from user's POV.

ansibot · 2020-01-07T19:02:41Z

The test ansible-test sanity --test validate-modules [explain] failed with 1 error:

lib/ansible/modules/database/mysql/mysql_replication.py:0:0: option-incorrect-version-added: version_added for new option (fail_on_error) should be '2.10'. Currently StrictVersion ('0.0')

The test ansible-test sanity --test pep8 [explain] failed with 3 errors:

lib/ansible/modules/database/mysql/mysql_replication.py:328:8: E111: indentation is not a multiple of four
lib/ansible/modules/database/mysql/mysql_replication.py:329:12: E111: indentation is not a multiple of four
lib/ansible/modules/database/mysql/mysql_replication.py:330:8: E111: indentation is not a multiple of four

click here for bot help

ansibot · 2020-01-07T19:02:42Z

cc @Alexander198961 @Andersson007 @Xyon @banyek @bmalynovytch @bmildren @kurtdavis @michaelcoburn @Oneiroi @tolland
click here for bot help

Andersson007

first of all, we need to:

use module.fail_json instead of raise
a changelog fragment, see here
create integrations tests for the case by putting your options somewhere to test/integration/targets/mysql_replication/tasks/ with explicit fail_on_error: yes|no invocation
add an example of using the option to the EXAMPLE =r''' section

Andersson007 · 2020-01-09T10:14:45Z

lib/ansible/modules/database/mysql/mysql_replication.py

+  fail_on_error:
+    description:
+    - Fails on error when calling mysql.
+    - This is the "expected" behaviour if you want to rely on MySQL replication working.


Suggested change

- This is the "expected" behaviour if you want to rely on MySQL replication working.

why is there the double quotes? Anyway, IMO this is extra information

Andersson007 · 2020-01-24T11:03:02Z

@petoju are you working on this?

petoju · 2020-01-24T18:07:45Z

@Andersson007 yes, I missed the message. Changes are pushed here now - I could not run the tests now as I'm on slow Internet and it needs some MBs of images.

ansibot · 2020-01-24T18:32:11Z

The test ansible-test sanity --test pylint [explain] failed with 5 errors:

lib/ansible/modules/database/mysql/mysql_replication.py:288:12: undefined-variable: Undefined variable 'module'
lib/ansible/modules/database/mysql/mysql_replication.py:308:12: undefined-variable: Undefined variable 'module'
lib/ansible/modules/database/mysql/mysql_replication.py:328:12: undefined-variable: Undefined variable 'module'
lib/ansible/modules/database/mysql/mysql_replication.py:341:12: undefined-variable: Undefined variable 'module'
lib/ansible/modules/database/mysql/mysql_replication.py:361:12: undefined-variable: Undefined variable 'module'

click here for bot help

lib/ansible/modules/database/mysql/mysql_replication.py

Andersson007

LGTM, @petoju thank you! @bmalynovytch is it OK? (for me it looks reasonable but i'm not a guru of MySQL)
shipit

bmalynovytch · 2020-01-29T08:39:52Z

@petoju Thx for the contrib !
Although, I don't get the point of this PR.
If slave thread is already running, shouldn't Ansible just state that "nothing changed", as the thread is already running ?
I guess we miss some sort of context or even an issue explaining why you need these changes.

petoju · 2020-01-29T23:12:25Z

@bmalynovytch that's a really good point and it revealed a flaw in my code that I'll try to fix over the weekend.
Please do not merge this.

As I omitted the background, there are 3 interesting states of task execution:

changed - used when eg. new file was created. Currently, mysql_replication uses is when the command succeeded.
failed - used when the file couldn't be created. Not used in mysql_replication now for some of the cases where I'd like to see it.
ok (no change) - used eg. when creating a file and it's already there. Current mysql_replication uses it whenever there is some kind of issue with executing the command. It happens eg. when a slave is already slave (your point I missed), but also when mysql_replication cannot execute the command. There are cases, where startslave should fail: when MySQL server is not reachable, user doesn't have permissions, slave is not configured and so on.

It is really annoying, when one uses ansible and has to verify, whether execution really happened or there was some hidden error.

petoju · 2020-01-30T23:43:18Z

It should be fixed and tested now - mysql warnings still cause exceptions in current configuration (that I didn't expect). So warnings are fine now and report "ok" state now even with this option being turned on.

Andersson007 · 2020-01-31T07:31:42Z

@petoju it is usually not good to push changes forcelly, difficult to track them between commits

Andersson007 · 2020-01-31T07:33:23Z

@bmalynovytch , waiting for your confirmation

petoju · 2020-01-31T08:45:25Z

Sorry for amending, some projects have rules of one commit per PR. More changes here will be separate commits. Dňa pi 31. 1. 2020, 8:31 Andrew Klychkov <notifications@github.com> napísal(a):

…

@petoju <https://github.com/petoju> it is usually not good to push changes forcelly, difficult to track them between commits — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#66252?email_source=notifications&email_token=AAXINWFCGWJHKFCOCWL7H6TRAPHWDA5CNFSM4KD5PNC2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEKNYTTI#issuecomment-580618701>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAXINWHCNPLZUGW2YTNGEQTRAPHWDANCNFSM4KD5PNCQ> .

Andersson007 · 2020-02-05T12:36:43Z

@petoju , no problem and there are no rules:) thanks for the PR.
waiting for @bmalynovytch 's opinion about the changes.

bmalynovytch

LGTM
shipit

Andersson007 · 2020-02-11T18:06:03Z

@petoju could you close and reopen this pr. to see it merged we need to get rid of stale_ci label, ansibot will relaunch CI tests after reopening

petoju · 2020-02-11T21:24:11Z

Closing PR as requested.

petoju · 2020-02-11T21:29:26Z

@Andersson007 that PR is in #67322 now.

Andersson007 · 2020-02-12T03:50:09Z

@petoju you didn’t need to open the new one instead, just to open again this one would be enough and ok to trigger the bot to relaunch the tests.
Ok

petoju force-pushed the feature/loud_mysql_replication_error branch 2 times, most recently from 0973ad6 to 12d3efe Compare January 7, 2020 20:51

ansibot added community_review In order to be merged, this PR must follow the community review workflow. and removed ci_verified Changes made in this PR are causing tests to fail. needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. labels Jan 7, 2020

Andersson007 suggested changes Jan 9, 2020

View reviewed changes

ansibot added needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. and removed community_review In order to be merged, this PR must follow the community review workflow. needs_triage Needs a first human triage before being processed. labels Jan 9, 2020

ansibot added the stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. label Jan 17, 2020

This was referenced Jan 22, 2020

mysql_db does not support --no-create-db flag #65773

Closed

mysql_db: add skip_lock_tables option #66688

Merged

petoju force-pushed the feature/loud_mysql_replication_error branch from 12d3efe to d73508d Compare January 24, 2020 18:04

ansibot added ci_verified Changes made in this PR are causing tests to fail. and removed stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. labels Jan 24, 2020

ansibot added the test This PR relates to tests. label Jan 24, 2020

petoju force-pushed the feature/loud_mysql_replication_error branch from d73508d to 5a0ad77 Compare January 24, 2020 20:08

ansibot removed the ci_verified Changes made in this PR are causing tests to fail. label Jan 24, 2020

Andersson007 suggested changes Jan 28, 2020

View reviewed changes

lib/ansible/modules/database/mysql/mysql_replication.py Outdated Show resolved Hide resolved

petoju force-pushed the feature/loud_mysql_replication_error branch from 5a0ad77 to c3b8b78 Compare January 28, 2020 17:44

Andersson007 approved these changes Jan 28, 2020

View reviewed changes

ansibot added community_review In order to be merged, this PR must follow the community review workflow. and removed needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. labels Jan 28, 2020

petoju force-pushed the feature/loud_mysql_replication_error branch from c3b8b78 to 1ebd5bc Compare January 30, 2020 23:35

mysql_replication: add option to fail on error

f264c12

petoju force-pushed the feature/loud_mysql_replication_error branch from 1ebd5bc to f264c12 Compare January 30, 2020 23:40

bmalynovytch reviewed Feb 11, 2020

View reviewed changes

ansibot added the stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. label Feb 11, 2020

petoju closed this Feb 11, 2020

petoju deleted the feature/loud_mysql_replication_error branch February 11, 2020 21:24

ansible locked and limited conversation to collaborators Mar 10, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

mysql_replication: add option to fail on error #66252

mysql_replication: add option to fail on error #66252

petoju commented Jan 7, 2020

ansibot commented Jan 7, 2020

ansibot commented Jan 7, 2020

Andersson007 left a comment

Andersson007 Jan 9, 2020

Andersson007 commented Jan 24, 2020

petoju commented Jan 24, 2020

ansibot commented Jan 24, 2020

Andersson007 left a comment •

edited

Loading

bmalynovytch commented Jan 29, 2020

petoju commented Jan 29, 2020

petoju commented Jan 30, 2020

Andersson007 commented Jan 31, 2020

Andersson007 commented Jan 31, 2020

petoju commented Jan 31, 2020 via email

Andersson007 commented Feb 5, 2020

bmalynovytch left a comment

Andersson007 commented Feb 11, 2020

petoju commented Feb 11, 2020

petoju commented Feb 11, 2020

Andersson007 commented Feb 12, 2020

mysql_replication: add option to fail on error #66252

mysql_replication: add option to fail on error #66252

Conversation

petoju commented Jan 7, 2020

SUMMARY

ISSUE TYPE

COMPONENT NAME

ADDITIONAL INFORMATION

ansibot commented Jan 7, 2020

ansibot commented Jan 7, 2020

Andersson007 left a comment

Choose a reason for hiding this comment

Andersson007 Jan 9, 2020

Choose a reason for hiding this comment

Andersson007 commented Jan 24, 2020

petoju commented Jan 24, 2020

ansibot commented Jan 24, 2020

Andersson007 left a comment • edited Loading

Choose a reason for hiding this comment

bmalynovytch commented Jan 29, 2020

petoju commented Jan 29, 2020

petoju commented Jan 30, 2020

Andersson007 commented Jan 31, 2020

Andersson007 commented Jan 31, 2020

petoju commented Jan 31, 2020 via email

Andersson007 commented Feb 5, 2020

bmalynovytch left a comment

Choose a reason for hiding this comment

Andersson007 commented Feb 11, 2020

petoju commented Feb 11, 2020

petoju commented Feb 11, 2020

Andersson007 commented Feb 12, 2020

Andersson007 left a comment •

edited

Loading