Add missing options to CHANGE MASTER TO #63

Jorge-Rodriguez · 2020-11-21T17:37:57Z

SUMMARY

Added all missing options for CHANGE MASTER TO, according to https://dev.mysql.com/doc/refman/8.0/en/change-master-to.html

Fixes #59

ISSUE TYPE

Feature Pull Request

COMPONENT NAME

mysql_replication

Jorge-Rodriguez · 2020-11-21T17:39:23Z

@Andersson007 @bmalynovytch @bmildren I'm not quite sure how to write tests for this, other than running the module with the new options and check that the sent query matches expectation and the server doesn't spit an error. Do you have any suggestions?

codecov · 2020-11-21T17:46:26Z

Codecov Report

Merging #63 (00fa058) into main (00fa058) will not change coverage.
The diff coverage is n/a.

❗ Current head 00fa058 differs from pull request most recent head f07e389. Consider uploading reports for the commit f07e389 to get more accurate results

@@           Coverage Diff           @@
##             main      #63   +/-   ##
=======================================
  Coverage   77.88%   77.88%           
=======================================
  Files          28       28           
  Lines        2347     2347           
  Branches      552      552           
=======================================
  Hits         1828     1828           
  Misses        362      362           
  Partials      157      157

Flag	Coverage Δ
integration	`75.05% <0.00%> (ø)`
sanity	`15.30% <0.00%> (ø)`
units	`32.16% <0.00%> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

Jorge-Rodriguez · 2020-11-21T19:27:23Z

@Andersson007 the sanity checks say I can't define choices for an integer parameter. What's the proper way to define an int within a specific range of values?

Andersson007 · 2020-11-23T10:25:33Z

changelogs/fragments/59-mysql_replication_verify_server_cert.yml

@@ -0,0 +1,2 @@
+minor_changes:
+- mysql_replication - add missing CHANGE MASTER TO options (https://github.com/ansible-collections/community.mysql/issues/59).


Suggested change

- mysql_replication - add missing CHANGE MASTER TO options (https://github.com/ansible-collections/community.mysql/issues/59).

- "mysql_replication - add the new options: ``master_bind``,... (https://github.com/ansible-collections/community.mysql/issues/59)."

alternatively, we could use separate entries for each parameter

Andersson007 · 2020-11-23T10:26:36Z

plugins/modules/mysql_replication.py

@@ -42,6 +43,11 @@
    - resetslave
    - resetslaveall
    default: getslave
+  master_bind:
+    description:
+    - same as mysql_variable.


Suggested change

- same as mysql_variable.

- same as mysql_variable.

Suggested change

- same as mysql_variable.

- Same as mysql variable.

Argh, IDE autocomplete. Nicely spotted. 👍

Andersson007 · 2020-11-23T10:37:11Z

@Andersson007 the sanity checks say I can't define choices for an integer parameter. What's the proper way to define an int within a specific range of values?

I've never seen int ranges, there aren't a lot to list the whole sequence, i.e. [1, 2, ...] , or maybe we could just document this and check in the code if a passed value is in the range.

Andersson007 · 2020-11-23T11:12:33Z

@Andersson007 @bmalynovytch @bmildren I'm not quite sure how to write tests for this, other than running the module with the new options and check that the sent query matches expectation and the server doesn't spit an error. Do you have any suggestions?

Can we run the 3d instance? As i can see there are mysql_primary_port and mysql_replica1_port there now.
Maybe to run one more as a replica with mysql_replica2_port and switch it to mysql_replica1_port as a master.
I'm not sure if it works with MySQL but in Postgres we can use a "cascade" of replicas and switch between them so that one will become a master to another being a replica of another one without rebuilding.
If it's complicated, your suggestion, imo, also works.

but @bmildren and @bmalynovytch , as experts, know better:)

bmalynovytch · 2020-11-23T15:27:25Z

Absolutely, you may have 1 master with 2 slaves and switch one of the slaves to master and attach the second one as new slave of the new master.

Jorge-Rodriguez · 2020-11-23T19:08:33Z

@bmalynovytch since all of the options to CHANGE MASTER TO are optional and we aren't testing the MASTER_HOST or MASTER_PORT options (those are already tested) couldn't we test everything against the running master?

The other, more complicated question is what kind of effects to look for to verify that the command succeeded. But in that case I would assume that if the query sent to the server matches the expectation and the server doesn't err then the server has done as requested.
It could be the case that there server remains silent from the module's PoV but didn't honour the request, but my knowledge of mysql replication is not deep enough to foresee these cases.

Jorge-Rodriguez · 2020-11-23T19:12:27Z

@Andersson007 the sanity checks say I can't define choices for an integer parameter. What's the proper way to define an int within a specific range of values?

I've never seen int ranges, there aren't a lot to list the whole sequence, i.e. [1, 2, ...] , or maybe we could just document this and check in the code if a passed value is in the range.

Maybe we can define it as a list of string representations of integers. At the end of the day however the value is written in YAML didn't make that much of a difference, and we end up turning it into a string for the cursor query anyway.

Andersson007 · 2020-11-24T05:58:24Z

@Andersson007 the sanity checks say I can't define choices for an integer parameter. What's the proper way to define an int within a specific range of values?

I've never seen int ranges, there aren't a lot to list the whole sequence, i.e. [1, 2, ...] , or maybe we could just document this and check in the code if a passed value is in the range.

Maybe we can define it as a list of string representations of integers. At the end of the day however the value is written in YAML didn't make that much of a difference, and we end up turning it into a string for the cursor query anyway.

For me personally the best approach is documenting the acceptable range, removing choices, and checking on the passed value in the code with raising a helpful message if the value is out of the borders.

Jorge-Rodriguez · 2021-03-15T11:55:34Z

I'm going to abandon this PR draft, as it relates to replication and we have modifications regarding that component and the language used.
That being said, I'd like to keep this PR still open for reference until I get the chance to tackle the issue again.

Andersson007 · 2021-03-16T13:54:31Z

@Jorge-Rodriguez sure, it's marked as DRAFT, so, people will be warned. Maybe also would be good to put something like "TEMPORARY FROZEN" to the description.

Andersson007 · 2023-01-20T08:18:06Z

@Jorge-Rodriguez any updates on the PR?

Jorge-Rodriguez · 2023-01-22T06:56:32Z

I haven't gotten the chance to work on this for quite some time. I'll pick it up, seems there's only the "enum" issue to solve and to write appropriate tests.

Thanks for the ping.

Jorge-Rodriguez · 2023-01-22T08:46:13Z

@Andersson007 I have just brought the code of this PR back to date. Still pending changes to the "enum" issue and testing, so I'm not moving it from draft status yet.

Jorge-Rodriguez · 2023-01-23T13:46:21Z

I am unable to run integration tests locally, When running on docker the first get_url task fails with HTTP 403. The URL is otherwise OK and it works, so it would seem an issue with docker?

Jorge-Rodriguez · 2023-01-24T09:55:05Z

I found out that all our tests were run with the install_mariadb flag set to true.

Jorge-Rodriguez · 2023-01-24T10:54:45Z

Tests are broken because downloading the MySQL tarball consistently fails with HTTP 403. The reason is unknown as of this writing.

Jorge-Rodriguez · 2023-01-24T11:06:51Z

It seems that the old dev.mysql.com URL is not working anymore and the resources have been moved to cdn.mysql.com. For some reason, the request from the get_url module does not get redirected.

Andersson007 · 2023-01-24T12:21:27Z

@Jorge-Rodriguez great work, thank you!
And thanks for investigating the reason of 403.

Would you like to raise a separate PR for the URL change you did here?
Do you think we need those master/slave aliases? I think we left them for older options not to break stuff in users WFs. As these are new options, not sure if we should add the aliases. Ideas?

Jorge-Rodriguez · 2023-01-24T12:36:51Z

Yeah, it might be a good idea not to introduce futile backwards compatibility on new features, I'll remove the aliases.

Andersson007 · 2023-01-24T12:53:14Z

@Jorge-Rodriguez thanks! please let me know when it's ready for review.
There are a lot of parameters. Are they all supported by all MySQL/MariaDB supported versions?
If not, i'm not sure if we should document minimal supporting versions for each new argument or not? Maybe it's an extra thing. Ideas?

Jorge-Rodriguez · 2023-01-24T13:15:16Z

I've just rebased the whole thing to make the commit history clearer. I'm still keeping the CI fixup so the tests run correctly, I'll remove them when #491 is merged.

@Andersson007 no need for review just yet. I still have to write tests for the new parameters, I'm going to keep the PR as a draft until those are ready.

Jorge-Rodriguez · 2023-01-26T10:38:10Z

@Andersson007 What's the proper way to document an option that is only available since a specific version of MySQL?

Andersson007 · 2023-01-26T11:45:42Z

@Andersson007 What's the proper way to document an option that is only available since a specific version of MySQL?

@Jorge-Rodriguez imo just to add another row to each option saying something like - Available (or supported maybe) since MySQL <version > / MariaDB <version> would be enough.

@laurent-indermuehle @betanummeric @bmalynovytch other thoughts ^ ?

laurent-indermuehle · 2023-01-26T12:21:07Z

If used on older version, will it be ignored or crash?
if used on another engine (mysql vs mariadb), will it be ignored or crash?

I hope that 'Plugins CI' will tell us once it runs ;)

Andersson007 · 2023-01-26T12:29:28Z

If used on older version, will it be ignored or crash?

if used on another engine (mysql vs mariadb), will it be ignored or crash?

I hope that 'Plugins CI' will tell us once it runs ;)

I believe just crashing is OK as soon as the versions are documented. We, of course, can check versions in our code, etc. but shouldn't this be user's responsibility? If a user runs unsupported SQL, RDBMS just shows errors (invalid syntax or something), doesn't it?

laurent-indermuehle · 2023-01-26T13:39:02Z

I believe just crashing is OK as soon as the versions are documented. We, of course, can check versions in our code, etc. but shouldn't this be user's responsibility? If a user runs unsupported SQL, RDBMS just shows errors (invalid syntax or something), doesn't it?
Right, ansible-doc will show the user that we documented the minimum version and his logs will tell him what went wrong.

I searched the repo and found many occurences:

Supported from MariaDB 10.0.1.
Multi-source replication is supported from MySQL 5.7.
Roles are supported since MySQL 8.0.0 and MariaDB 10.0.5.
Supported by B(MariaDB).
Is not supported by MariaDB and is silently ignored when working with MariaDB.
Supported by MySQL 8.0 or later.

English is not my first language, so I don't know what the most correct form is. But I know I don't like seen a dot at the end of the version.

Jorge-Rodriguez · 2023-01-27T20:09:47Z

I'm planning to spot the problematic options as I progress with test writing and have the module fail gracefully with an appropriate message.

Jorge-Rodriguez · 2023-01-27T20:12:33Z

In a different order of things, it seems that since we started making modificaciones to the replication module, MySQL has settled on replacing the master term with source instead of our chosen primary so while I'm at this I'm going to introduce that rename too

Jorge-Rodriguez · 2023-01-27T20:21:36Z

If used on older version, will it be ignored or crash?

if used on another engine (mysql vs mariadb), will it be ignored or crash?

I hope that 'Plugins CI' will tell us once it runs ;)

I believe just crashing is OK as soon as the versions are documented. We, of course, can check versions in our code, etc. but shouldn't this be user's responsibility? If a user runs unsupported SQL, RDBMS just shows errors (invalid syntax or something), doesn't it?

The problem with delegating the error to the connector is that it can become very unclear. As a user you'd get a message such as

[...] PLEASE CHECK YOUR SYNTAX NEAR [...]

But the user didn't construct that query, the module did, so it would require intimate knowledge of the module's inner workings to determine what is causing the problem.

IMO it'd be better to have the module do the version checking and fail with a proper message like

You're using DB version A and argument 'you_jumped_the_gun' isn't supported until version B.

Jorge-Rodriguez · 2023-01-27T20:36:18Z

I believe just crashing is OK as soon as the versions are documented. We, of course, can check versions in our code, etc. but shouldn't this be user's responsibility? If a user runs unsupported SQL, RDBMS just shows errors (invalid syntax or something), doesn't it?
Right, ansible-doc will show the user that we documented the minimum version and his logs will tell him what went wrong.

I searched the repo and found many occurences:

Supported from MariaDB 10.0.1.

Multi-source replication is supported from MySQL 5.7.

Roles are supported since MySQL 8.0.0 and MariaDB 10.0.5.

Supported by B(MariaDB).

Is not supported by MariaDB and is silently ignored when working with MariaDB.

Supported by MySQL 8.0 or later.

English is not my first language, so I don't know what the most correct form is. But I know I don't like seen a dot at the end of the version.

It's a linguistics problem because we're mixing the syntaxes of versioning and the English language, the latter being infamous for being anything but formal. We could of course do some language gymnastics and rewrite it as something like

Supported from version 10.0.5 of MariaDB.

although it's a bit clunky. The last option is probably best since it keeps the same semantics while removing the version number from the end of the sentence

Supported by MySQL version 8.0 and later.

We even get the chance to use Oxford commas

Supported by MySQL version 8.0.0 and later, and MariaDB version 10.0.5 and later.

Or, we leverage markdown notation

Supported since MySQL version `8.0`.

Andersson007 · 2023-01-31T08:50:43Z

@Jorge-Rodriguez

In a different order of things, it seems that since we started making modificaciones to the replication module, MySQL has settled on replacing the master term with source instead of our chosen primary so while I'm at this I'm going to introduce that rename too

I hope MariaDB will not introduce other aliases, there'll be too many imo in that case:))

IMO it'd be better to have the module do the version checking and fail with a proper message like

if you're happy to write all the checks, WFM:)

Supported by MySQL 8.0 or later.

I like this one ^. Plus I would add the markdown to highlight the versions a bit, so something like:

Supported by MySQL C(8.0) or later

Though I would suggest to conduct the rewording in a separate PR:)
I'm a strong supporter of the principle Split all that can be split into separate PRs

Jorge-Rodriguez · 2023-01-31T09:17:36Z

MariaDB od still stuck in the political correctness flame war of 2020, so I don't expect much progress on the rewording front any time soon

laurent-indermuehle · 2023-01-31T09:25:40Z

Since MariaDB 10.5 you can write show replica status\G instead of show slave status\G. So, we may need a compatibility table to keep up :/

Jorge-Rodriguez · 2023-01-31T14:14:35Z

Since MariaDB 10.5 you can write show replica status\G instead of show slave status\G. So, we may need a compatibility table to keep up :/

Well, that would be in line with MySQL terms.

Jorge-Rodriguez · 2023-02-12T19:34:33Z

This is still in progress, I want to be thorough with the test suite so it's taking some time.

It seems that MariaDB is going with primary instead of source, but they still haven't made changes to the change master command or its options.

I will introduce support for MySQL's change replica source and its options but I'll leave the new aliases for a separate PR.

Andersson007 · 2023-02-13T09:15:01Z

This is still in progress, I want to be thorough with the test suite so it's taking some time.

It seems that MariaDB is going with primary instead of source, but they still haven't made changes to the change master command or its options.

I will introduce support for MySQL's change replica source and its options but I'll leave the new aliases for a separate PR.

@Jorge-Rodriguez are we talking about reflecting the differences in naming in aliases? If yes, i wonder if we really need them. Maybe we should just use one set of names? Maybe we could just mention in notes: that, say, options containing primary can mean source in some versions of the RDBMSes?

Jorge-Rodriguez · 2023-02-13T10:33:48Z

This is still in progress, I want to be thorough with the test suite so it's taking some time.
It seems that MariaDB is going with primary instead of source, but they still haven't made changes to the change master command or its options.
I will introduce support for MySQL's change replica source and its options but I'll leave the new aliases for a separate PR.

@Jorge-Rodriguez are we talking about reflecting the differences in naming in aliases? If yes, i wonder if we really need them. Maybe we should just use one set of names? Maybe we could just mention in notes: that, say, options containing primary can mean source in some versions of the RDBMSes?

While not strictly necessary, I believe the option aliases would work better in the future for users who are used to the newer terminology. I also think an alias would probably work better than an explanation in notes that might not be read.
I'm any case none of this will concern this particular PR.

Andersson007 · 2023-02-13T11:39:30Z

This is still in progress, I want to be thorough with the test suite so it's taking some time.
It seems that MariaDB is going with primary instead of source, but they still haven't made changes to the change master command or its options.
I will introduce support for MySQL's change replica source and its options but I'll leave the new aliases for a separate PR.

@Jorge-Rodriguez are we talking about reflecting the differences in naming in aliases? If yes, i wonder if we really need them. Maybe we should just use one set of names? Maybe we could just mention in notes: that, say, options containing primary can mean source in some versions of the RDBMSes?

While not strictly necessary, I believe the option aliases would work better in the future for users who are used to the newer terminology. I also think an alias would probably work better than an explanation in notes that might not be read. I'm any case none of this will concern this particular PR.

I'm not strictly against of the aliases if it helps though i think we shouldn't overuse them:) OK, let's discuss this later in a dedicated PR/Issue/Discussion

simonpahl · 2024-02-05T11:30:14Z

I would like to use the MASTER_BIND with the module, would it be acceptable to extract parts of this PR to support this parameter?

laurent-indermuehle · 2024-02-05T18:06:24Z

Hi @simonpahl, I think it's a question for @Jorge-Rodriguez.
But if he doesn't respond, I think it would be ok to create a new PR based on his work.
It's an old PR and probably needs a lot of rebasing anyway.

Jorge-Rodriguez marked this pull request as draft November 22, 2020 19:35

Andersson007 reviewed Nov 23, 2020

View reviewed changes

Jorge-Rodriguez changed the title ~~Add missing options to CHANGE MASTER TO~~ Add missing options to CHANGE MASTER TO [Temporarily frozen] Mar 27, 2021

Jorge-Rodriguez force-pushed the 59-verify-server-cert branch from bc19516 to 278f10f Compare January 22, 2023 08:43

Jorge-Rodriguez changed the title ~~Add missing options to CHANGE MASTER TO [Temporarily frozen]~~ Add missing options to CHANGE MASTER TO Jan 22, 2023

Jorge-Rodriguez force-pushed the 59-verify-server-cert branch 3 times, most recently from cc2ac60 to eb761f4 Compare January 24, 2023 10:30

Jorge-Rodriguez changed the title ~~Add missing options to CHANGE MASTER TO~~ Add missing options to CHANGE MASTER TO [BLOCKED] Jan 24, 2023

Jorge-Rodriguez added the tests label Jan 24, 2023

Jorge-Rodriguez force-pushed the 59-verify-server-cert branch from 7b99668 to 7664da7 Compare January 24, 2023 13:12

Jorge-Rodriguez force-pushed the 59-verify-server-cert branch from 7664da7 to d337f8c Compare January 24, 2023 14:46

Jorge-Rodriguez mentioned this pull request Jan 24, 2023

Draft: Add options to start and stop group replication #93

Closed

Jorge-Rodriguez changed the title ~~Add missing options to CHANGE MASTER TO [BLOCKED]~~ Add missing options to CHANGE MASTER TO Jan 24, 2023

Jorge-Rodriguez added 2 commits January 25, 2023 10:05

Add missing options to CHANGE MASTER TO

000fca2

Add changelog fragment

cd29b79

Jorge-Rodriguez force-pushed the 59-verify-server-cert branch from d337f8c to cd29b79 Compare January 25, 2023 08:05

Jorge-Rodriguez added 2 commits January 25, 2023 13:47

Add two missing options

d60693c

First CHANGE MASTER TO tests battery

f07e389

		@@ -0,0 +1,2 @@
		minor_changes:
		- mysql_replication - add missing CHANGE MASTER TO options (https://github.com/ansible-collections/community.mysql/issues/59).

	- mysql_replication - add missing CHANGE MASTER TO options (https://github.com/ansible-collections/community.mysql/issues/59).
	- "mysql_replication - add the new options: ``master_bind``,... (https://github.com/ansible-collections/community.mysql/issues/59)."

Add missing options to CHANGE MASTER TO #63

Are you sure you want to change the base?

Add missing options to CHANGE MASTER TO #63

Conversation

Jorge-Rodriguez commented Nov 21, 2020

SUMMARY

ISSUE TYPE

COMPONENT NAME

Jorge-Rodriguez commented Nov 21, 2020

codecov bot commented Nov 21, 2020 • edited Loading

Codecov Report

Jorge-Rodriguez commented Nov 21, 2020

Andersson007 Nov 23, 2020

Choose a reason for hiding this comment

Andersson007 Nov 23, 2020

Choose a reason for hiding this comment

Jorge-Rodriguez Nov 23, 2020

Choose a reason for hiding this comment

Andersson007 commented Nov 23, 2020 • edited Loading

Andersson007 commented Nov 23, 2020

bmalynovytch commented Nov 23, 2020

Jorge-Rodriguez commented Nov 23, 2020

Jorge-Rodriguez commented Nov 23, 2020

Andersson007 commented Nov 24, 2020

Jorge-Rodriguez commented Mar 15, 2021

Andersson007 commented Mar 16, 2021

Andersson007 commented Jan 20, 2023

Jorge-Rodriguez commented Jan 22, 2023

Jorge-Rodriguez commented Jan 22, 2023

Jorge-Rodriguez commented Jan 23, 2023

Jorge-Rodriguez commented Jan 24, 2023

Jorge-Rodriguez commented Jan 24, 2023

Jorge-Rodriguez commented Jan 24, 2023

Andersson007 commented Jan 24, 2023

Jorge-Rodriguez commented Jan 24, 2023

Andersson007 commented Jan 24, 2023

Jorge-Rodriguez commented Jan 24, 2023

Jorge-Rodriguez commented Jan 26, 2023

Andersson007 commented Jan 26, 2023

laurent-indermuehle commented Jan 26, 2023

Andersson007 commented Jan 26, 2023

laurent-indermuehle commented Jan 26, 2023

Jorge-Rodriguez commented Jan 27, 2023

Jorge-Rodriguez commented Jan 27, 2023

Jorge-Rodriguez commented Jan 27, 2023

Jorge-Rodriguez commented Jan 27, 2023 • edited Loading

Andersson007 commented Jan 31, 2023

Jorge-Rodriguez commented Jan 31, 2023

laurent-indermuehle commented Jan 31, 2023

Jorge-Rodriguez commented Jan 31, 2023

Jorge-Rodriguez commented Feb 12, 2023

Andersson007 commented Feb 13, 2023

Jorge-Rodriguez commented Feb 13, 2023

Andersson007 commented Feb 13, 2023

simonpahl commented Feb 5, 2024

laurent-indermuehle commented Feb 5, 2024

codecov bot commented Nov 21, 2020 •

edited

Loading

Andersson007 commented Nov 23, 2020 •

edited

Loading

Jorge-Rodriguez commented Jan 27, 2023 •

edited

Loading