Skip to content

Refresh MySQL config after a backup#3063

Merged
sougou merged 1 commit intovitessio:masterfrom
tinyspeck:refresh-mysql-config-after-backup
Aug 15, 2017
Merged

Refresh MySQL config after a backup#3063
sougou merged 1 commit intovitessio:masterfrom
tinyspeck:refresh-mysql-config-after-backup

Conversation

@rafael
Copy link
Copy Markdown
Member

@rafael rafael commented Aug 14, 2017

Description

Tests

  • Tested locally.

@bbeaudreault
Copy link
Copy Markdown
Contributor

This LGTM but we should probably also do on Restore?

@rafael
Copy link
Copy Markdown
Member Author

rafael commented Aug 14, 2017

Good call. Just dig into it and found that in that context it's calling ReinitConfig(), so I think it should be good.

However, now I'm not sure if I should call here ReinitConfig as well. When I was looking into this, both should work, but RefreshConfig seemed like a better fit. What do you think?

@bbeaudreault
Copy link
Copy Markdown
Contributor

Ah I forgot. That's actually where I got the inspiration for RefreshConfig. Let me take a look.

@bbeaudreault
Copy link
Copy Markdown
Contributor

bbeaudreault commented Aug 14, 2017

I think we should stick with RefreshConfig. ReinitConfig exists so that it can additionally change the mysql serverId to avoid skipping transaction after a restore. In the case of taking a backup I think we want to keep the same serverId when it comes back up.

So we can leave Backup alone.

@bbeaudreault
Copy link
Copy Markdown
Contributor

Test failures seem related to this change

@rafael
Copy link
Copy Markdown
Member Author

rafael commented Aug 14, 2017

@bbeaudreault - yeah looking into that right now.

@rafael rafael force-pushed the refresh-mysql-config-after-backup branch from 6030347 to 709b81b Compare August 15, 2017 00:33
  * Refresh MySQL config after a backup
  * Tweaks to test set up, to make sure tablet has correct
    values for extra_env
@rafael rafael force-pushed the refresh-mysql-config-after-backup branch from 709b81b to cdd759c Compare August 15, 2017 02:00
@rafael
Copy link
Copy Markdown
Member Author

rafael commented Aug 15, 2017

@bbeaudreault - I think it should be good now. Fixed the test !

@bbeaudreault
Copy link
Copy Markdown
Contributor

LGTM. cc @sougou

@sougou
Copy link
Copy Markdown
Contributor

sougou commented Aug 15, 2017

LGTM

Approved with PullApprove

@sougou sougou merged commit 32f142f into vitessio:master Aug 15, 2017
@rafael rafael deleted the refresh-mysql-config-after-backup branch August 15, 2017 16:25
@rafael
Copy link
Copy Markdown
Member Author

rafael commented Aug 24, 2017

While deploying this change to prod, we discovered an issue with this change. If you are using the following flag when starting mysqlctld: -mysqlctl_mycnf_template, you will have to pass it to the tablet as well. Otherwise you will run into this problem when running backups:

Unknown desc = TabletManager.Backup on xxx error: can't refresh mysqld config: Could not initConfig in xxx/my.cnfxxx: open /usr/local/config/mycnf/default.cnf: no such file or directory

I believe the reason is in this line. Function initConfig use by RefreshConfig() relies on that flag.

Besides documentation, I don't see any workaround for this problem.

@bbeaudreault, @sougou thoughts?

@sougou
Copy link
Copy Markdown
Contributor

sougou commented Aug 24, 2017

I feel like this needs to be fixed by a higher level design change. Until then, I'm ok with documenting it.

@bbeaudreault
Copy link
Copy Markdown
Contributor

I also ran into this. I think possibly what should happen is mysqlctld should refresh the config before starting mysql, rather than vttablet refreshing the config before calling mysqlctld.

I havent had a chance to look at the code yet to see how hard that is. If it's too complicated or doesnt work as I think then documenting is fine for now.

rafael pushed a commit to tinyspeck/vitess that referenced this pull request Jul 24, 2018
* Currently this is not having the intended behavior. Removing while adding a
  proper fix for this. This effectively reverts:

  vitessio#3063

Signed-off-by: Rafael Chacon <rafael@slack-corp.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants