Mattermost-server: Ignore configured database timeout during migration

[esethna]

If you're interested please comment here and come join our "Contributors" community channel on our daily build server, where you can discuss questions with community members and the Mattermost core team. For technical advice or questions, please join our "Developers" community channel.

New contributors please see our Developer's Guide.

---

Notes: Jira ticket

Mattermost instances on Postgres (and possibly MySQL) may face issues on upgrade if the required schema changes take longer than the configured timeout. It probably never makes sense to timeout the upgrade.

For this ticket, first verify that is the case by discussing the ticket in the ~Developers channel on community.mattermost.com. Both @jesus.espino and @jesse.hallam would be good to discuss it with.

If verified, then implement the changes in server store code.

[joshuabezaleel]

May I take a shot on this?

[hanzei]

Sure @joshuabezaleel, thanks for taking a look. :+1:

[ConnorL33t]

I'd like to work on this!

[hanzei]

Thanks @ConnorL33t :rocket: If you have questions, feel free to ask.

[ConnorL33t]

I think I have a solution based off of the previous PR (and the comments) for this issue.

I have a couple of questions:

1. If a migration is taking a long time for some reason would this be visible to the user? (server admin)

2. Right now there's some magic numbers due to driver differences (MySQL needs a "max" val, while Postgres will use 0 appropriately), is there a place that these values (ex: MYSQL_MAX_QUERY_TIMEOUT) could be made const for added in-code documentation?

3. Does the test suite deal with migrations - does it need updates for this change?

[hanzei]

@wiersgallak Can you help with 1.?

@lieut-data Can you help with 2. and 3.?

[lieut-data]

@ConnorL33t:

1. These const values are unlikely to be used outside of the code setting them, so I'd just declare them as unexported values in that package (even in the same file where we migrate).
2. Ideally, we update the test suite to exercise a long-ish migration somehow, or at least the newly added code, to prove that they indeed don't time out.

[hanzei]

@wiersgallak Gentle reminder about the above question

[wiersgallak]

@ConnorL33t

1. We do not currently have a solution that informs the server admin if the migration is taking a long time. We would welcome a solution that incorporates this if you have some ideas on how you could do this.

[lieut-data]

Re: 1., I might suggest at most a periodic log if a long-running migration is occurring (e.g. ping the logs every 10 seconds with a "Still migrating to 5.12..." and maybe the time elapsed so far as a structured field. I personally don't think this is a requirement.

[hanzei]

@ConnorL33t Do you have any outstanding questions?

[hanzei]

Making this one available for the public again due to inactivity.

[lelandhwu]

Seeing as this hasn't been picked up for a while, I was wondering if I might be able to pick this up.
I'm extremely new to this project, and have limited knowledge of Go. I am willing to work on this ticket, but would like some references to point me in the right direction

[lieut-data]

@lelandhwu, you're very welcome to give this a stab! The basic idea is to ensure none of the migration steps in https://github.com/mattermost/mattermost-server/blob/master/store/sqlstore/upgrade.go can ever timeout.

The way to do this is to start by writing a fake migration step that /does/ timeout (e.g. SELECT SLEEP(300)), and then figure out the right way to tell the database not to timeout, but only for the duration of these migration steps. It's as much an exercise in SQL as it is Go.

[lelandhwu]

Hey guys - it looks like I won't be able to contribute to this for the time being. Sorry for the delay. It would be best to unassign me.

[hanzei]

Hey @lelandhwu,

Thanks for the heads up and thanks for looking into this issue.

[soorya54]

I'd like to take this on.

[jespino]

@soorya54 sorry the late reply. Yes you can pick this one. Thanks!

[jasonblais]

Hey @soorya54, just touching base to see how things are going and if you have any questions? :)

[rvillablanca]

Hi :hand:, I can take a look into this

[darkLord19]

in PR #14421 discussion came to stop at https://github.com/mattermost/mattermost-server/pull/14421#issuecomment-631303998

Mainly waiting for the discussion to unfold. I don't see any problem with the PR per se but not sure what problem we are trying to solve here. Apparently these timeouts are not something enabled by default nor it's suggested to do so.

As far as I can see we usually enforce query timeouts at the application level. For example, gorp does that by using context.WithTimeout on the dbMap.
Is there an actual need to do this at the db level? cc. @jespino

So what is the state of the issue? Like is this something to solve or not? If not then maybe it'd be better to close the issue I think to avoid any confusions and wasted efforts by someone. pinging @hanzei

[hanzei]

@streamer45 What do you think about the comment above?

[streamer45]

@streamer45 What do you think about the comment above?

Not sure my opinion has changed since last time. This is a rather old ticket and I am not sure I have the full context to understand the original issue it was trying to solve. Maybe @lieut-data or @jespino could help more.

[lieut-data]

@streamer45, this goes back a ways to a customer running Postgres with a 10 second timeout and who failed to apply a database migration that took longer on that particular instance. Backstory in a customer channel: https://community-daily.mattermost.com/core/pl/uaurhiudotb6pxmj6yjqieknkh. The thinking was that migrations should be exempt from any configured database timeout.

[streamer45]

@streamer45, this goes back a ways to a customer running Postgres with a 10 second timeout and who failed to apply a database migration that took longer on that particular instance. Backstory in a customer channel: https://community-daily.mattermost.com/core/pl/uaurhiudotb6pxmj6yjqieknkh. The thinking was that migrations should be exempt from any configured database timeout.

Thank you for the added info. So this is about possibly overriding (and sufficiently increasing) default query timeouts during the migration process because I am not sure we can disable them entirely (I also wouldn't suggest that).