Code review comment for ~lvoytek/ubuntu/+source/mysql-8.0:mysql-lp1899248-increase-shutdown-timeout-sru-focal

Revision history for this message
Bryce Harrington (bryce) wrote :

Glad to see the work done on this, and I think your approach is spot on. This'll fix a couple of the big issues resulting in apport bugs with mysql.

A few course corrections, however.

First, policy in Ubuntu regarding SRUs to stable releases is to make sure the changes are landed to jammy beforehand. So, please re-implement these changes against jammy's mysql package, and resubmit the MP against jammy, in favor of this one. Once the changes have landed to jammy, then effort can be put towards backporting those changes to focal (and impish) as well.

Second, it looks like there's two distinct problems being addressed here, namely the timeout and the permissions error on the socket files. It's not a problem to include multiple fixes in an MP to the -devel release, but for changes that are going to be SRU'd to focal, et al, each distinct fix is going to need its own bug report.

I agree that LP: #1899248 is an instance of both problems, however given that the jammy bug task was already closed (by me! sorry) that might make it a bit more confusing. You'll likely have best luck with SRU approvals if you locate two other bug reports (you could file brand new ones, or repurpose a couple of 1899248's dupes), and transform them into reports that focus on each of the two issues. Edit the bug descriptions and provide a discussion of the symptom or error, analysis of the problem (and identification of root-cause), and rationale for your chosen solution; later when you get to the SRU you'll also need a test-case and discussion of regression potential.

For the rationale for your chosen solutions, in the case of the increased timeout it sounds like you have good justification in considering the average / maximum values seen in the wild. You mentioned the maximal reasonable time was 2:42 min, were there many cases where the time was greater than that? (I vaguely recall once seeing one that was closer to 10 min, but I could be wrong).

For the case of the socket permissions, the rationale for the solution should probably explain what ownership / permissions the code will set the files to, and discuss the security implications / safety. E.g. if we're already setting other mysql socket files to these permissions, and/or if they're the same permissions that mariadb, postgres, or other services also use, then that'd suggest they're safe here too.

Also, I notice you're setting the sleep value from 1 to 0.9. I don't see a problem doing so, but you may want to explain the rationale for that change, such as if it resolves some problem or makes debugging easier.

Finally, thanks for putting in the good detailed steps to reproduce the problem and solution. That looks like it'll make a great [test case] section for the SRU bug reports. And again I'm really glad to see this work done on the apport reported mysql bugs, this should help quell a lot of our incoming mysql bugs. :-)

review: Needs Fixing

« Back to merge proposal