fleet icon indicating copy to clipboard operation
fleet copied to clipboard

Support MySQL 8 and drop support for MySQL 5.7

Open zwass opened this issue 1 year ago • 26 comments

Goal

User story
As a Fleet contributor,
I want to update Fleet to support MySQL 8 and drop support for MySQL 5.7
so that I can ship stable Fleet releases that use MySQL 8 features.

Context

  • Requestor(s): @zwass
  • Product designer: @lukeheath

Changes

Product

There's a huge chunk of work that has to do with communication:

  • [ ] Documenting this in fleetdm.com
  • [ ] Document this as part of the release notes
  • [ ] Let all our customers and community know
  • [ ] Support any customer that needs help with the migration

Engineering

All our queries are compatible with MySQL 8. We shouldn't scope modifying any of queries to make use of the MySQL 8 features.

  • [ ] Modify the root docker-compose.yml file to use MySQL 8 by default
  • [ ] Delete the GitHub action that tests in MySQL 5.7
  • [ ] Document the supported version in our internal docs
  • [ ] Let all the team know so they can update their local environments

ℹ️  Please read this issue carefully and understand it. Pay special attention to UI wireframes, especially "dev notes".

QA

Risk assessment

  • Risk level: Low but needs really good communication

Manual testing steps

  1. Step 1
  2. Step 2
  3. Step 3

Testing notes

Confirmation

  1. [ ] Engineer (@____): Added comment to user story confirming successful completion of QA.
  2. [ ] QA (@____): Added comment to user story confirming successful completion of QA.

zwass avatar Feb 28 '24 18:02 zwass

What will need to change in the docs?

nonpunctual avatar Feb 28 '24 18:02 nonpunctual

@nonpunctual search shows some instances of 5.7 referenced in the docs: https://github.com/search?q=repo%3Afleetdm%2Ffleet%20mysql%205.7&type=code

(It also pulls up some code comments where we are doing extra work to support 5.7)

zwass avatar Feb 28 '24 18:02 zwass

@zwass as you asked for pain points related to mysql 5.7, there's a recent one where mysql 8 supports more advanced check constraints that 5.7 parses syntactically, but otherwise ignores: https://github.com/fleetdm/fleet/blob/2d4a183789c5336373665516839b9314cd056a1e/server/datastore/mysql/migrations/tables/20240126020642_AddMDMProfileLabelsTable.go#L47-L51

mna avatar Feb 28 '24 20:02 mna

And another point that comes to mind, that is not an issue per se at the moment but could become one if we maintained 5.7 support, is for the INSERT .. ON DUPLICATE KEY statements that we use a lot. The UPDATE part of the statement must use the form VALUES(column_name) to set the column to whatever the value was provided to the INSERT part in mysql 5.7 and that's supported in mysql 8.0 but it became officially deprecated in 8.0.20: https://dev.mysql.com/doc/refman/8.0/en/insert-on-duplicate.html (see the "NOTE" part mid-page).

As they document:

deprecated beginning with MySQL 8.0.20, and is subject to removal in a future version of MySQL. Instead, use row and column aliases, as described in the next few paragraphs of this section.

The row and column aliases is not supported by 5.7, so there could be a point where supporting 5.7 prevents us from supporting 8.X (that is, the point at which mysql decides to stop supporting VALUES(...)).

mna avatar Feb 28 '24 20:02 mna

a few more:

  1. For labels, software titles and hosts we're doing variations of this due to the lack of JSON aggregation functions:

https://github.com/fleetdm/fleet/blob/f131d35807868a7e1f08efbedd9b8c9ac2f8b6f2/server/datastore/mysql/hosts.go#L980

  1. Also due to the lack of JSON aggregation functions, in other places we do manipulations like:

https://github.com/fleetdm/fleet/blob/f131d35807868a7e1f08efbedd9b8c9ac2f8b6f2/server/datastore/mysql/software_titles.go#L117-L126

  1. Lack of CTEs makes us do various tricks like complex subqueries

roperzh avatar Feb 28 '24 21:02 roperzh

Thanks all! Really helpful examples / pain points above. Let's weigh it during feature fest next week.

Remove MySQL 5.7 support from documentation. Start using MySQL 8 features and drop code that supports 5.7.

Can we remove 5.7 from the docs and start using MySQL 8 w/o dropping code that supports 5.7?

(drop code later) Trying to think of ways to make the first pass work smaller.

Does that even make sense to do?

noahtalerman avatar Feb 29 '24 14:02 noahtalerman

@noahtalerman @zwass Maybe in the docs where 5.7 is referenced the 1st step should be to add note or text block that says its EOL & won't be supported in upcoming versions without a specific time or version reference? We could put the same note at every mention...

nonpunctual avatar Feb 29 '24 16:02 nonpunctual

Hey @zwass, @mna, @roperzh heads up, this story was prioritized during feature fest.

Aiming to ship an improvement in the next 6 weeks.

noahtalerman avatar Mar 08 '24 15:03 noahtalerman

Moved the original issue description here:

Problem

MySQL 5.7 has been retired and is no longer receiving security updates. In Fleet's Terraform and Cloud instances, MySQL 8.0 is used. Continuing to document support for MySQL 5.7 may encourage our users to use the (insecure) older database version, and also constrains our own development efforts (eg. @getvictor mentioned poor unicode support in 5.7 that prevents us from using newer features that will work more like users expect).

Potential solutions

  1. Check in with self-hosted customers and work with them on an upgrade plan.
  2. Remove MySQL 5.7 support from documentation.
  3. Start using MySQL 8 features and drop code that supports 5.7.

noahtalerman avatar Mar 11 '24 23:03 noahtalerman

@noahtalerman I am happy to help with this if there is an action item for me here otherwise can you let me know if there isn't one so I can unsubscribe from issue? Thanks all!

nonpunctual avatar Mar 11 '24 23:03 nonpunctual

Hey @nonpunctual, I don't think we need any action from you at this time.

If it's helpful, I don't think you need to subscribe to any issues in fleetdm/fleet or fleetdm/confidential.

With the Toast app in Slack, you get notified every time you're @ mentioned in an issue in fleetdm/fleet or fleetdm/confidential.

noahtalerman avatar Mar 13 '24 15:03 noahtalerman

another data point, we just spent almost a whole day with Sarah chasing a bug that's only present in MySQL 5.7.17

https://github.com/fleetdm/fleet/blob/cab98fe4c31bccd17ad2e6b0063619539c803812/server/datastore/mysql/apple_mdm.go#L2389-L2398

roperzh avatar Mar 27 '24 00:03 roperzh

Hey @zwass heads up, we didn't get to this estimated in the last design sprint.

Bringing it back to feature fest.

What's the business impact of this change? How does it affect our users?

noahtalerman avatar Mar 28 '24 18:03 noahtalerman

@noahtalerman please see numerous customer and business impacting issues above.

@lukeheath not sure if there's a process for engineering to prioritize things like this but sounds like it may be useful to do it sooner than later.

zwass avatar Mar 28 '24 19:03 zwass

@zwass Will do, agree we want to action this soon. I'll speak to Noah later today about what's needed to get this estimated.

lukeheath avatar Mar 28 '24 20:03 lukeheath

Hey @lukeheath want to take over as PM/PD for this one? :)

If yes, please add it to the drafting board and assign yourself.

Removing this from the feature fest board.

noahtalerman avatar Mar 29 '24 21:03 noahtalerman

@noahtalerman Sure, happy to! I'll drive this forward next week.

lukeheath avatar Mar 29 '24 21:03 lukeheath

@georgekarrv I'm not sure what additional specs this story needs, so I moved to the "Specified" column so y'all can consider it during the next estimation. This could go to either product group, but because @mna and @roperzh have been most involved in this issue I thought they'd be best suited to implement.

lukeheath avatar Apr 10 '24 22:04 lukeheath

Absolutely, we will bring this around at our next estimation session.

georgekarrv avatar Apr 10 '24 22:04 georgekarrv

@lukeheath @georgekarrv @mna All our queries are compatible with MySQL 8.

IMO we shouldn't scope modifying the list of queries we listed above to make use of the MySQL 8 features.

Engineering wise, I think what should be done is mostly cleanup (unless I'm missing something):

  1. Modify the root docker-compose.yml file to use MySQL 8 by default
  2. Delete the GitHub action that tests in MySQL 5.7
  3. Document the supported version in our internal docs

There's a huge chunk of work that has to do with communication:

  1. Documenting this in fleetdm.com
  2. Document this as part of the release notes
  3. Let all our customers and community know
  4. Support any customer that needs help with the migration

roperzh avatar Apr 10 '24 22:04 roperzh

Thanks! @roperzh or @georgekarrv Please update the ticket description to reflect the research above so it's at the top and easy to see.

lukeheath avatar Apr 12 '24 17:04 lukeheath

@lukeheath @georgekarrv I updated the issue, I put the communication changes under "Product", lmk if there's a better section for that or if it should be handled by Engineering.

roperzh avatar Apr 12 '24 17:04 roperzh

Hey team! Please add your planning poker estimate with Zenhub @dantecatalfamo @ghernandez345 @gillespi314 @mna @roperzh

georgekarrv avatar Apr 17 '24 16:04 georgekarrv

Another example where dropping MySQL 5.7 support would have lead to simpler code: https://github.com/fleetdm/fleet/pull/19654/files#diff-b2dbd5eb9d667567e564e494777c3d3662362ba5300cf6cb3669df059d51f73aR19

lucasmrod avatar Jun 17 '24 13:06 lucasmrod

I will argue this should have a P2 next sprint if we don't finish it this week.

georgekarrv avatar Jun 17 '24 13:06 georgekarrv

As a related ask, can we specify the minimum supported patch version of MySQL 8.0? An example:

MySQL 8.0.19, you can specify a time zone offset when inserting TIMESTAMP and DATETIME values into a table

getvictor avatar Jun 19 '24 15:06 getvictor

@zayhanlon just a heads up this will merge on monday so it will 'officially' be dropped in 4.55.0 (mid sprint release)

georgekarrv avatar Jul 12 '24 20:07 georgekarrv

i'm going to notify the customer and community base early @georgekarrv @ddribeiro @ksatter @pacamaster FYI

zayhanlon avatar Jul 15 '24 15:07 zayhanlon

QA Notes:

  • Restarted my docker environment (after merging latest main) to update my MySQL version and confirmed I'm now on 8.0.36
  • I'll continue testing issues for this sprint and will run smoke tests as well

@xpkoala if you haven't already, it would be great if you were on ver8 too to test against EPOPS

PezHub avatar Jul 23 '24 22:07 PezHub

Smoke Tests completed for both teams and passed.

PezHub avatar Jul 25 '24 18:07 PezHub