Skip to content
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

[bitnami/mysql] Feat mysqldump master database #54038

Merged
merged 15 commits into from
Jan 16, 2024

Conversation

michel-silva
Copy link
Contributor

Description of the change

Implement mysql dump to cases of missing binary files

Benefits

The dump will be executed before replication start. If binary files is missing will not broken replication

Possible drawbacks

The master server must be used only this process

Applicable issues

#54028

Signed-off-by: Michel Silva <[email protected]>
Signed-off-by: Michel Silva <[email protected]>
Signed-off-by: Michel Silva <[email protected]>
Signed-off-by: Michel Silva <[email protected]>
@github-actions github-actions bot added mysql triage Triage is needed labels Dec 22, 2023
@github-actions github-actions bot requested a review from javsalgar December 22, 2023 03:49
@carrodher
Copy link
Member

carrodher commented Dec 22, 2023

Thank you for initiating this pull request. We appreciate your effort. Just a friendly reminder that it's important to sign your commits. Adding your signature certifies that you either authored the patch or have the necessary rights to contribute to the changes. You can find detailed information on how to do this in the “Sign your work” section of our contributing guidelines.

In the same way, could you please check the markdown issue reported by the linter?

Feel free to reach out if you have any questions or need assistance with the signing process.

Signed-off-by: Michel Silva <[email protected]>
@michel-silva michel-silva force-pushed the feat-mysqldump-master-database branch from 11ceb84 to 12bef92 Compare December 22, 2023 12:25
@michel-silva
Copy link
Contributor Author

@carrodher Thank you for your fast reply.
I've resolved the lint messages and removed my commit that doesn't have the signature.

Could you check if it's all right? Let me know if I could make something to merge

Signed-off-by: Michel Silva <[email protected]>
@michel-silva michel-silva force-pushed the feat-mysqldump-master-database branch from 861a498 to 55dd33c Compare December 22, 2023 13:18
@carrodher carrodher added verify Execute verification workflow for these changes in-progress labels Dec 25, 2023
@github-actions github-actions bot removed the triage Triage is needed label Dec 25, 2023
@github-actions github-actions bot removed the request for review from javsalgar December 25, 2023 17:18
@github-actions github-actions bot requested a review from dgomezleon December 25, 2023 17:18
Copy link
Member

@dgomezleon dgomezleon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for contributing. I added some suggestions.

bitnami/mysql/README.md Outdated Show resolved Hide resolved
bitnami/mysql/README.md Outdated Show resolved Hide resolved
bitnami/mysql/README.md Outdated Show resolved Hide resolved
bitnami/mysql/docker-compose-replication.yml Outdated Show resolved Hide resolved
Signed-off-by: Michel Silva <[email protected]>
Signed-off-by: Michel Silva <[email protected]>
Signed-off-by: Michel Silva <[email protected]>
Signed-off-by: Michel Silva <[email protected]>
Signed-off-by: Michel Silva <[email protected]>
@michel-silva
Copy link
Contributor Author

@dgomezleon Thank you for your review. I made the changes, could you check again?

@github-actions github-actions bot assigned FraPazGal and unassigned dgomezleon Dec 27, 2023
@github-actions github-actions bot requested a review from FraPazGal December 27, 2023 10:01
@carrodher carrodher removed the request for review from FraPazGal December 27, 2023 10:09
@carrodher carrodher assigned dgomezleon and unassigned FraPazGal Dec 27, 2023
Copy link
Member

@dgomezleon dgomezleon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for applying the changes. I detected a couple of minor typos. Could you please take a look?

Signed-off-by: Michel Silva <[email protected]>
Signed-off-by: Michel Silva <[email protected]>
@michel-silva michel-silva force-pushed the feat-mysqldump-master-database branch 2 times, most recently from a103e44 to 6b49f3e Compare December 27, 2023 11:30
@michel-silva
Copy link
Contributor Author

@dgomezleon Thank you for the new review. The changes have been applied.

Copy link
Member

@dgomezleon dgomezleon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks again. Could you please check my last question?

mysql_exec_initial_dump() {
info "MySQL dump master data start..."

mysql -h "$DB_MASTER_HOST" -P "$DB_MASTER_PORT_NUMBER" -u "$DB_MASTER_ROOT_USER" -p"$DB_MASTER_ROOT_PASSWORD" -e 'RESET MASTER;'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have one last question about this. This would execute RESET MASTER for every slave. Is this necessary?
Note that RESET MASTER is already executed in mysql_initialize() . Could you please confirm?

Copy link
Contributor Author

@michel-silva michel-silva Jan 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @dgomezleon.
Sorry about my delay response.. I was on vacation and I returned in the last days.
Your question is very interesting and makes me think about how replication works. So let me explain my point of view.

I have a master setup and a slave was broken because of a crash on the slave server.. So, I make a reinstallation of the new slave server and when I up the MySQL slave server this error occurs and this error is because the Mysql master server is missing some binary files. To fix it, I could make a manual mysql dump on master and update the slave, but to do it I must login in recovery mode on MySQL slave because the mysql.user could not be transferred by binary files and we do not have any valid user to login. Analyzing this scenario, I make this approach to make an auto mysql dump on master and import to a slave database.

Analyzing your question about the mysql_initialize(), this is only exec on the first up of the container. In my scenario, the reset will not be executed and the mysql master binary files are not modified. To resolve this, I could manually get mysql dump on the master server, make the reset master and run the mysql dump in slave.

So, when the MySQL master server doesn't have all the binary files, for each slave that is added to the server the reset master will be necessary for the slave to start replicating at the point of the master server stop.

One important point that your question made me think about is: that is very important that all slave servers have the last data present in the master database, when we restart replication after reset master the slave has the same data and does not miss any data. So, I think is important to add one alert to make sure that all slaves have all data updated, before start the setup the new slave.

I don't know if it explains my point of view. But I'm open to having some discussion about this and making the better to the package.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It makes sense to me. Thanks for the detailed explanation.

Copy link

This Pull Request has been automatically marked as "stale" because it has not had recent activity (for 15 days). It will be closed if no further activity occurs. Thank you for your contribution.

@github-actions github-actions bot added the stale 15 days without activity label Jan 12, 2024
@carrodher carrodher removed stale 15 days without activity pending labels Jan 15, 2024
@dgomezleon dgomezleon merged commit 79352ad into bitnami:main Jan 16, 2024
13 checks passed
@mostafa-rz
Copy link

mostafa-rz commented Jan 16, 2024

@michel-silva Thanks for your PR. I have this problem and your feature will help me a lot. I have two questions:

  1. Why you are not using mysqldump --all-databases --master-data instead of searching for the tables? This is the way mysql suggests on their website. (Please look at 17.1.2.5.1 Creating a Data Snapshot Using mysqldump)
  2. With your current solution the user databases on the master are not copied therefore the slave is not the same as the master. Is this intentional?

@mostafa-rz
Copy link

There is also another problem. If I increase the number of slaves, the previous slaves will lose the connection to the master because of the RESET MASTER flag in this line. It is also mentioned in mysql documentation .

RESET MASTER is not intended to be used while any replicas are running. The behavior of RESET MASTER when used while replicas are running is undefined (and thus unsupported), whereas PURGE BINARY LOGS may be safely used while replicas are running.

Step to reproduce:

  1. Start with 1 slave docker compose -f docker-compose-replication.yml up
  2. Increase number of slaves docker-compose -f docker-compose-replication.yml up --detach --scale mysql-master=1 --scale mysql-slave=3
  3. Create a test database in master
  4. The test database shown in slave-2 but not in slave-1

@michel-silva
Copy link
Contributor Author

michel-silva commented Jan 19, 2024

@mostafa-rz Sorry about my delay response. I will analyse your points and make some tests. Thank you for your valuation.

@michel-silva
Copy link
Contributor Author

@mostafa-rz this PR fix the issue.
#55126

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
mysql solved verify Execute verification workflow for these changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants