Bug 227230

Summary: databases/mysql57-server: Adapt ${name}_limits to the new resource limiting mechanics for services
Product: Ports & Packages Reporter: Mateusz Piotrowski <0mp>
Component: Individual Port(s)Assignee: Mahdi Mokhtari <mmokhi>
Status: Closed FIXED    
Severity: Affects Only Me CC: amdmi3, cy, rootservice
Priority: --- Keywords: regression
Version: LatestFlags: mmokhi: maintainer-feedback+
mmokhi: maintainer-feedback+
Hardware: Any   
OS: Any   
See Also: https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=227435
Bug Depends on:    
Bug Blocks: 225657, 227205    
Attachments:
Description Flags
Patch adding support for the new ${name}_limits mechanics (databases/mysql57-server)
none
Patch adding support for the new ${name}_limits mechanics (databases/mysql57-server, revision: 1) 0mp: maintainer-approval?

Description Mateusz Piotrowski freebsd_committer freebsd_triage 2018-04-03 01:00:10 UTC 
Created attachment 192146 [details]
Patch adding support for the new ${name}_limits mechanics (databases/mysql57-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 Mahdi Mokhtari freebsd_committer freebsd_triage 2018-04-03 06:07:44 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).

BTW, you posted an edit for your patch on MySQL56, is the edit already applied on 57's patch as well? Or I can do it while testing :)
Comment 2 Mahdi Mokhtari freebsd_committer freebsd_triage 2018-04-03 06:08:16 UTC 
Yes I see it's already applied ``:)
Comment 3 Mateusz Piotrowski freebsd_committer freebsd_triage 2018-04-04 18:12:46 UTC 
Created attachment 192227 [details]
Patch adding support for the new ${name}_limits mechanics (databases/mysql57-server, revision: 1)

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 4 commit-hook freebsd_committer freebsd_triage 2018-04-04 20:57:50 UTC 
A commit references this bug:

Author: mmokhi
Date: Wed Apr  4 20:57:12 UTC 2018
New revision: 466507
URL: https://svnweb.freebsd.org/changeset/ports/466507

Log:
  databases/mysql57-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 mysql57 for the bases with both
  old and new mechanism.
  (This is port of the r466505 [and r466506])

  PR:		227230
  Submitted by:	0mp
  Reported by:	0mp
  Differential Revision:	Netzkommune GmbH

Changes:
  head/databases/mysql57-server/Makefile
  head/databases/mysql57-server/files/mysql-server.in
Comment 5 Mahdi Mokhtari freebsd_committer freebsd_triage 2018-04-04 21:02:09 UTC 
Committed. Thanks :)
Comment 6 Markus Kohlmeyer 2018-04-06 00:44:33 UTC 
Regression!

The variables %%LEGACY_LIMITS%% and %%MODERN_LIMITS%% in mysql57-server/files/mysql-server.in are not replaced correctly (in fact, they have no effect) and so mysql-server will not (re)start, because mysql_limits will be set to YES/NO and the new limits system let the rc-script error out with a file not found.
Comment 7 Markus Kohlmeyer 2018-04-06 01:27:48 UTC 
Forgot system info:

[root@devnoip:~] # uname -v -m -K -U
FreeBSD 11.1-STABLE #0 r332075: Thu Apr  5 21:17:08 CEST 2018     root@devnoip.rootservice.org:/usr/obj/usr/src/sys/MYKERNEL  amd64 1101513 1101513
Comment 8 Mateusz Piotrowski freebsd_committer freebsd_triage 2018-04-06 01:30:17 UTC 
(In reply to Markus Kohlmeyer from comment #7)

I'm afraid you are right.

I guess that it's because PLIST_SUB is used instead of SUB_LIST. I'm testing it at the moment. Thank you for reporting this.
Comment 9 Mateusz Piotrowski freebsd_committer freebsd_triage 2018-04-06 02:22:04 UTC 
(In reply to Markus Kohlmeyer from comment #6)
Oh, it is not a problem with the patches! They work just as intended (never mind my last comment; I panicked a little bit).

The problem is that you are using the latest 11-STABLE, which has the new limits behavior but its version is still 1101513.

Unfortunately, the condition checking the version of the system is not precise. In order to make it simple it does not cover all the cases. The FreeBSD versions which will not detect the new limits mechanism correctly are:

 * 12-CURRENT between r328331 (the new mechanism was introduced) and r329033 (1200057 version was tagged)
 * 11-STABLE between r331880 (when the change was backported) and version 1101514, which has not been tagged yet.

While on 12-CURRENT the simplest solution is to just update the system, I am not sure what we should do about 11-STABLE. I guess the simplest solution would be to request committers to tag it 1101514 as soon as possible.
Comment 10 Mahdi Mokhtari freebsd_committer freebsd_triage 2018-04-06 08:21:44 UTC 
(In reply to Mateusz Piotrowski from comment #9)
Emailed to the amdmi3@ and cy@ about the request for tag.
I also cc'd them here.