Bug 227229 - databases/mysql56-server: Adapt ${name}_limits to the new resource limiting mechanics for services
Summary: databases/mysql56-server: Adapt ${name}_limits to the new resource limiting m...
Status: Closed FIXED
Alias: None
Product: Ports & Packages
Classification: Unclassified
Component: Individual Port(s) (show other bugs)
Version: Latest
Hardware: Any Any
: --- Affects Only Me
Assignee: Mahdi Mokhtari
URL:
Keywords: regression
Depends on:
Blocks: 227205
  Show dependency treegraph
 
Reported: 2018-04-03 00:50 UTC by Mateusz Piotrowski
Modified: 2018-04-11 00:32 UTC (History)
0 users

See Also:
mmokhi: maintainer-feedback+
mmokhi: maintainer-feedback+


Attachments
Patch adding support for the new ${name}_limits mechanics (databases/mysql56-server) (2.80 KB, patch)
2018-04-03 00:50 UTC, Mateusz Piotrowski
no flags Details | Diff
Patch adding support for the new ${name}_limits mechanics (databases/mysql56-server, revision: 2) (2.77 KB, patch)
2018-04-03 00:58 UTC, Mateusz Piotrowski
no flags Details | Diff
Patch adding support for the new ${name}_limits mechanics (databases/mysql56-server, revision: 3) (2.77 KB, patch)
2018-04-04 18:12 UTC, Mateusz Piotrowski
0mp: maintainer-approval?
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mateusz Piotrowski freebsd_committer freebsd_triage 2018-04-03 00:50:07 UTC
Created attachment 192144 [details]
Patch adding support for the new ${name}_limits mechanics (databases/mysql56-server)

Thanks to the recent change to rc.subr(8) it is no longer necessary to
define a resource limiting logic in service scripts. limits(1) is now run
by default and its configuration is handled via the "${name}_limits"
variable.

Unfortunately, there is a collision of variable names. The service
scripts are broken on FreeBSD version with that new change (12.0-CURRENT,
11-STABLE and soon 11.2-RELEASE). It has been reported and diagnosed here.[2]

I'm submitting a patch fixing the service script for this port. A similar patch
has already been accepted by a maintainer of another database port.[2]

This port was previously fixed to unbreak the service on newer systems. The submitted patch removes support for the old behavior from newer systems. 

The patch preserves the old behavior on the systems without the
modification.

It would be great if you could review the patch and test it. :)

[1]: https://svnweb.freebsd.org/base?view=revision&revision=r328331
[2]: https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=226907
Comment 1 Mateusz Piotrowski freebsd_committer freebsd_triage 2018-04-03 00:58:32 UTC
Created attachment 192145 [details]
Patch adding support for the new ${name}_limits mechanics (databases/mysql56-server, revision: 2)

I forgot to remove the previous fix entirely in the previous patch. Here's a new one.
Comment 2 Mahdi Mokhtari freebsd_committer freebsd_triage 2018-04-03 06:05:59 UTC
Hi,
Thanks for nice work, reduced for me A LOT :D
I'll test it and feedback to you by afternoon (my local time ``:D).
Comment 3 Mahdi Mokhtari freebsd_committer freebsd_triage 2018-04-04 16:06:14 UTC
Hi again.
Tested the patch, my 12 poudriere jail was 1200056 and it failed, so I changed the Makefile condition to include it and it came fixed.
Everything else was fine so far, are you ok if I commit it with my change or I'm missing something about 1200056 vs 1200057 thing?
Comment 4 Mateusz Piotrowski freebsd_committer freebsd_triage 2018-04-04 17:50:01 UTC
(In reply to Mahdi Mokhtari from comment #3)

Ah. So two things:

 * 1200056 and 1200057 is not exactly precise. The change was introduced in between those two versions: 1200056 is r327952, 1200057 is r329033 and the change was introduced in r328331. I guess it is not really solvable easily. I'd argue that the best idea is to update CURRENT machines to at least 1200057.
 * The bigger problem is with the "${OSVERSION} < 1101515" as I made an off-by-one error. It should be 1101514 which will be the next value of __FreeBSD_version: 1101513 is r331838, the change was backported in r331880 and it will be included it the next version (1101514).

Thank you for spotting it. I'll submit fixed patches today.
Comment 5 Mateusz Piotrowski freebsd_committer freebsd_triage 2018-04-04 18:12:03 UTC
Created attachment 192226 [details]
Patch adding support for the new ${name}_limits mechanics (databases/mysql56-server, revision: 3)

The previous patch would fail to trigger the new ${name}_limits semantics for __FreeBSD_version 1101514. The new patch fixes it (thanks to mmokhi@).
Comment 6 commit-hook freebsd_committer freebsd_triage 2018-04-04 20:45:38 UTC
A commit references this bug:

Author: mmokhi
Date: Wed Apr  4 20:45:15 UTC 2018
New revision: 466505
URL: https://svnweb.freebsd.org/changeset/ports/466505

Log:
  databases/mysql56-server: Adapt ${name}_limits to the new mechanism
  Regarding to the changes to rc.subr(8) it is no longer necessary to
  define a resource limiting logic in service scripts.
  limits(1) is now run by default and its configuration is handled
  via the "${name}_limits" variable.
  This however causes collision of variable names and also is not
  compatible with the old mechanism.
  This fixes the rc-script of mysql56 for the bases with both
  old and new mechanism.

  PR:		227229
  Submitted by:	0mp
  Reported by:	0mp
  Sponsored by:	Netzkommune GmbH

Changes:
  head/databases/mysql56-server/Makefile
  head/databases/mysql56-server/files/mysql-server.in
  head/databases/mysql56-server/pkg-plist
Comment 7 Mahdi Mokhtari freebsd_committer freebsd_triage 2018-04-04 20:51:17 UTC
Committed, thanks a lot for your help :)
Comment 8 Mateusz Piotrowski freebsd_committer freebsd_triage 2018-04-04 21:08:34 UTC
(In reply to Mahdi Mokhtari from comment #7)

Great! Thank you for actually devoting some time to test out this patch and catching that little bug. :)
Comment 9 Mahdi Mokhtari freebsd_committer freebsd_triage 2018-04-04 21:17:52 UTC
(In reply to Mateusz Piotrowski from comment #8)
Oh sure thing __/||\__
Thanks again for revising it 3 times :)