Issue 29362580: Issue 4641 - Append performance fix to trac cgi file (Closed)
|
Created:
Nov. 15, 2016, 6:42 p.m. by f.nicolaisen Modified:
Nov. 21, 2016, 7:20 a.m. CC:
f.lopez Visibility:
Public. |
DescriptionWe’re not moving trac yet. This change improves trac setup fully provisioned by puppet.
Patch Set 1 #
Total comments: 7
Patch Set 2 : Issue 4641 - Performance fix to trac cgi file #
Total comments: 13
Patch Set 3 : Issue 4641 - Performance fix to trac cgi file #Patch Set 4 : Issue 4641 - Performance fix to trac cgi file #
Total comments: 6
Patch Set 5 : Drop arrow aligning, use root, keep under 80 cols #Patch Set 6 : Forgot the py file #MessagesTotal messages: 15
If you don't send mail, nobody will notice that a review is created (hg review sends mail automatically when the review is created but I guess that you didn't get this one working). https://codereview.adblockplus.org/29362580/diff/29362581/modules/trac/manife... File modules/trac/manifests/init.pp (right): https://codereview.adblockplus.org/29362580/diff/29362581/modules/trac/manife... modules/trac/manifests/init.pp:238: $cgi_performance_fix = " Nit: This isn't really related to CGI in any way, it's a Trac performance fix - so maybe this variable should be called $trac_performance_fix. https://codereview.adblockplus.org/29362580/diff/29362581/modules/trac/manife... modules/trac/manifests/init.pp:262: command => "echo \"$cgi_performance_fix\" >> /home/trac/htdocs-$name/cgi-bin/trac.fcgi", Actually, this needs to be added at the top of the file - fcgi_frontend.run() only returns when the server shuts down so the code you added here won't run. Currently this code has been placed after the copyright comment but it could also go directly after the shebang. https://codereview.adblockplus.org/29362580/diff/29362581/modules/trac/manife... modules/trac/manifests/init.pp:264: path => '/usr/bin:/usr/sbin:/bin:/usr/local/bin', The way I see it, this command will run every time you provision the server, so you will end up adding the code multiple times. I guess that something like `unless => 'grep "Local modifications" /home/trac/htdocs-$name/cgi-bin/trac.fcgi'` should work here. And then we still need to consider the scenario that our local modifications code changes in future. So maybe run the modifications unconditionally but replace the existing "Local modifications" section if there is one already? Getting rather complicated, I guess that this logic would be better off in a Python script.
Thanks for the feedback and the directions! One point is still unclear to me (see inline). https://codereview.adblockplus.org/29362580/diff/29362581/modules/trac/manife... File modules/trac/manifests/init.pp (right): https://codereview.adblockplus.org/29362580/diff/29362581/modules/trac/manife... modules/trac/manifests/init.pp:238: $cgi_performance_fix = " On 2016/11/16 09:26:18, Wladimir Palant wrote: > Nit: This isn't really related to CGI in any way, it's a Trac performance fix - > so maybe this variable should be called $trac_performance_fix. Acknowledged. https://codereview.adblockplus.org/29362580/diff/29362581/modules/trac/manife... modules/trac/manifests/init.pp:262: command => "echo \"$cgi_performance_fix\" >> /home/trac/htdocs-$name/cgi-bin/trac.fcgi", On 2016/11/16 09:26:18, Wladimir Palant wrote: > Actually, this needs to be added at the top of the file - fcgi_frontend.run() > only returns when the server shuts down so the code you added here won't run. > Currently this code has been placed after the copyright comment but it could > also go directly after the shebang. Acknowledged. https://codereview.adblockplus.org/29362580/diff/29362581/modules/trac/manife... modules/trac/manifests/init.pp:264: path => '/usr/bin:/usr/sbin:/bin:/usr/local/bin', On 2016/11/16 09:26:18, Wladimir Palant wrote: > The way I see it, this command will run every time you provision the server, so > you will end up adding the code multiple times. Won't it only run after the "deploy_$name" exec is run?
https://codereview.adblockplus.org/29362580/diff/29362581/modules/trac/manife... File modules/trac/manifests/init.pp (right): https://codereview.adblockplus.org/29362580/diff/29362581/modules/trac/manife... modules/trac/manifests/init.pp:264: path => '/usr/bin:/usr/sbin:/bin:/usr/local/bin', On 2016/11/16 15:01:28, f.nicolaisen wrote: > Won't it only run after the "deploy_$name" exec is run? Given that deploy_$name runs every time - sure. Now that you mention it, I guess that deploy_$name overwrites trac.fcgi so that there is nothing wrong with adding our modifications to it after that. Feel free to test this however.
On 2016/11/16 15:16:44, Wladimir Palant wrote: > https://codereview.adblockplus.org/29362580/diff/29362581/modules/trac/manife... > File modules/trac/manifests/init.pp (right): > > https://codereview.adblockplus.org/29362580/diff/29362581/modules/trac/manife... > modules/trac/manifests/init.pp:264: path => > '/usr/bin:/usr/sbin:/bin:/usr/local/bin', > On 2016/11/16 15:01:28, f.nicolaisen wrote: > > Won't it only run after the "deploy_$name" exec is run? > > Given that deploy_$name runs every time - sure. Now that you mention it, I guess > that deploy_$name overwrites trac.fcgi so that there is nothing wrong with > adding our modifications to it after that. Feel free to test this however. OK, I've uploaded a new patch set that injects the extra configuration using sed and a snippet file. Tested provision multiple times, file ends up looking like this: #!/usr/bin/python # -*- coding: utf-8 -*- # # Copyright (C) 2003-2009 Edgewall Software # Copyright (C) 2003-2004 Jonas Borgström <jonas@edgewall.com> # All rights reserved. # # This software is licensed as described in the file COPYING, which # you should have received as part of this distribution. The terms # are also available at http://trac.edgewall.org/wiki/TracLicense. # # This software consists of voluntary contributions made by many # individuals. For the exact contribution history, see the revision # history and logs, available at http://trac.edgewall.org/log/. # # Author: Jonas Borgström <jonas@edgewall.com> ### local modifications to the trac.fcgi file v1 from trac.web.session import session orig_get = session.get def patched_get(self, key, default=none): if key == 'accesskeys': return '1' return orig_get(self, key, default) session.get = patched_get ### end of local modifications try: import os import pkg_resources if 'TRAC_ENV' not in os.environ and \ 'TRAC_ENV_PARENT_DIR' not in os.environ: os.environ['TRAC_ENV'] = '/home/trac/environment-issues' if 'PYTHON_EGG_CACHE' not in os.environ: if 'TRAC_ENV' in os.environ: egg_cache = os.path.join(os.environ['TRAC_ENV'], '.egg-cache') elif 'TRAC_ENV_PARENT_DIR' in os.environ: egg_cache = os.path.join(os.environ['TRAC_ENV_PARENT_DIR'], '.egg-cache') pkg_resources.set_extraction_path(egg_cache) from trac.web import fcgi_frontend fcgi_frontend.run() except SystemExit: raise except Exception, e: print 'Content-Type: text/plain\r\n\r\n', print 'Oops...' print print 'Trac detected an internal error:' print print e print import traceback import StringIO tb = StringIO.StringIO() traceback.print_exc(file=tb) print tb.getvalue()
https://codereview.adblockplus.org/29362580/diff/29362602/modules/trac/manife... File modules/trac/manifests/init.pp (right): https://codereview.adblockplus.org/29362580/diff/29362602/modules/trac/manife... modules/trac/manifests/init.pp:261: require => File["/home/trac/htdocs-$name/cgi-bin/trac_performance_fix.snippet"], This should also require Exec["deploy_$name"], it cannot run earlier than deployment.
https://codereview.adblockplus.org/29362580/diff/29362602/modules/trac/manife... File modules/trac/manifests/init.pp (right): https://codereview.adblockplus.org/29362580/diff/29362602/modules/trac/manife... modules/trac/manifests/init.pp:261: require => File["/home/trac/htdocs-$name/cgi-bin/trac_performance_fix.snippet"], On 2016/11/16 17:02:52, Wladimir Palant wrote: > This should also require Exec["deploy_$name"], it cannot run earlier than > deployment. The File has that require, so this is transitively already the case. Should I add the same require here for the sake of clarity?
LGTM https://codereview.adblockplus.org/29362580/diff/29362602/modules/trac/manife... File modules/trac/manifests/init.pp (right): https://codereview.adblockplus.org/29362580/diff/29362602/modules/trac/manife... modules/trac/manifests/init.pp:261: require => File["/home/trac/htdocs-$name/cgi-bin/trac_performance_fix.snippet"], On 2016/11/16 17:04:55, f.nicolaisen wrote: > The File has that require, so this is transitively already the case. Should I > add the same require here for the sake of clarity? Ah, I didn't notice this.
https://codereview.adblockplus.org/29362580/diff/29362602/modules/trac/files/... File modules/trac/files/trac_performance_fix.snippet (right): https://codereview.adblockplus.org/29362580/diff/29362602/modules/trac/files/... modules/trac/files/trac_performance_fix.snippet:4: def patched_get(self, key, default=none): Typo: "None" vs. "none". Also, I guess it should be "Session" vs. "session" above and below? https://codereview.adblockplus.org/29362580/diff/29362602/modules/trac/manife... File modules/trac/manifests/init.pp (right): https://codereview.adblockplus.org/29362580/diff/29362602/modules/trac/manife... modules/trac/manifests/init.pp:249: file {"/home/trac/htdocs-$name/cgi-bin/trac_performance_fix.snippet": Why not using a target path like /usr/local/lib/python2.7/dist-packages/trac_monkey_patches.py or similar? That would allow for patching using a simple include statement, which can be inserted with a file_line resource (from Puppetlabs stdlib). https://codereview.adblockplus.org/29362580/diff/29362602/modules/trac/manife... modules/trac/manifests/init.pp:256: exec {"append_cgi_config_to_$name": It isn't really appending any more.
https://codereview.adblockplus.org/29362580/diff/29362602/modules/trac/files/... File modules/trac/files/trac_performance_fix.snippet (right): https://codereview.adblockplus.org/29362580/diff/29362602/modules/trac/files/... modules/trac/files/trac_performance_fix.snippet:4: def patched_get(self, key, default=none): On 2016/11/17 07:43:31, mathias wrote: > Typo: "None" vs. "none". Also, I guess it should be "Session" vs. "session" > above and below? I don't know. This snippet was directly copied from the live Trac instance I believe, as described in issue 4641. https://codereview.adblockplus.org/29362580/diff/29362602/modules/trac/manife... File modules/trac/manifests/init.pp (right): https://codereview.adblockplus.org/29362580/diff/29362602/modules/trac/manife... modules/trac/manifests/init.pp:249: file {"/home/trac/htdocs-$name/cgi-bin/trac_performance_fix.snippet": On 2016/11/17 07:43:31, mathias wrote: > Why not using a target path like > /usr/local/lib/python2.7/dist-packages/trac_monkey_patches.py or similar? That > would allow for patching using a simple include statement, which can be inserted > with a file_line resource (from Puppetlabs stdlib). Nice. Yeah, we can try doing that instead of exec/sed. I don't think file_line can read contents from files though, so the snippet will have to be a value inside this file again. https://codereview.adblockplus.org/29362580/diff/29362602/modules/trac/manife... modules/trac/manifests/init.pp:256: exec {"append_cgi_config_to_$name": On 2016/11/17 07:43:31, mathias wrote: > It isn't really appending any more. True that, will fix.
https://codereview.adblockplus.org/29362580/diff/29362602/modules/trac/files/... File modules/trac/files/trac_performance_fix.snippet (right): https://codereview.adblockplus.org/29362580/diff/29362602/modules/trac/files/... modules/trac/files/trac_performance_fix.snippet:4: def patched_get(self, key, default=none): On 2016/11/17 08:23:46, f.nicolaisen wrote: > On 2016/11/17 07:43:31, mathias wrote: > > Typo: "None" vs. "none". Also, I guess it should be "Session" vs. "session" > > above and below? > > I don't know. This snippet was directly copied from the live Trac instance I > believe, as described in issue 4641. Well, I guess somewhere in the process the snippet lost upper-case letters. The version in the ticket does not have the typos mentioned above. https://codereview.adblockplus.org/29362580/diff/29362602/modules/trac/manife... File modules/trac/manifests/init.pp (right): https://codereview.adblockplus.org/29362580/diff/29362602/modules/trac/manife... modules/trac/manifests/init.pp:249: file {"/home/trac/htdocs-$name/cgi-bin/trac_performance_fix.snippet": On 2016/11/17 08:23:46, f.nicolaisen wrote: > On 2016/11/17 07:43:31, mathias wrote: > > Why not using a target path like > > /usr/local/lib/python2.7/dist-packages/trac_monkey_patches.py or similar? That > > would allow for patching using a simple include statement, which can be > inserted > > with a file_line resource (from Puppetlabs stdlib). > > Nice. Yeah, we can try doing that instead of exec/sed. I don't think file_line > can read contents from files though, so the snippet will have to be a value > inside this file again. It should be possible, something like this one: file_line {"patch $name trac.fcgi": after => "^# Author:", line => "import trac_monkey_patches", path => "/home/trac/htdocs-$name/cgi-bin/trac.fcgi", }
https://codereview.adblockplus.org/29362580/diff/29362602/modules/trac/files/... File modules/trac/files/trac_performance_fix.snippet (right): https://codereview.adblockplus.org/29362580/diff/29362602/modules/trac/files/... modules/trac/files/trac_performance_fix.snippet:4: def patched_get(self, key, default=none): On 2016/11/17 10:18:33, mathias wrote: > On 2016/11/17 08:23:46, f.nicolaisen wrote: > > On 2016/11/17 07:43:31, mathias wrote: > > > Typo: "None" vs. "none". Also, I guess it should be "Session" vs. "session" > > > above and below? > > > > I don't know. This snippet was directly copied from the live Trac instance I > > believe, as described in issue 4641. > > Well, I guess somewhere in the process the snippet lost upper-case letters. The > version in the ticket does not have the typos mentioned above. Wtf... OK, good thing you spotted this! I must've accidentally hit ~ while the block was selected in vim or something. Really weird. https://codereview.adblockplus.org/29362580/diff/29362602/modules/trac/manife... File modules/trac/manifests/init.pp (right): https://codereview.adblockplus.org/29362580/diff/29362602/modules/trac/manife... modules/trac/manifests/init.pp:249: file {"/home/trac/htdocs-$name/cgi-bin/trac_performance_fix.snippet": On 2016/11/17 10:18:33, mathias wrote: > On 2016/11/17 08:23:46, f.nicolaisen wrote: > > On 2016/11/17 07:43:31, mathias wrote: > > > Why not using a target path like > > > /usr/local/lib/python2.7/dist-packages/trac_monkey_patches.py or similar? > That > > > would allow for patching using a simple include statement, which can be > > inserted > > > with a file_line resource (from Puppetlabs stdlib). > > > > Nice. Yeah, we can try doing that instead of exec/sed. I don't think file_line > > can read contents from files though, so the snippet will have to be a value > > inside this file again. > > It should be possible, something like this one: > > file_line {"patch $name trac.fcgi": > after => "^# Author:", > line => "import trac_monkey_patches", > path => "/home/trac/htdocs-$name/cgi-bin/trac.fcgi", > } OK, I'll try. Thanks!
New strategy: use file_line to inject a python import statement, as suggested by Matze in previous review.
Almost there ;-) https://codereview.adblockplus.org/29362580/diff/29363527/modules/trac/manife... File modules/trac/manifests/init.pp (right): https://codereview.adblockplus.org/29362580/diff/29363527/modules/trac/manife... modules/trac/manifests/init.pp:152: ensure => present, Please do not align arrows. This is a documented exception from the Puppet Style Guide (https://adblockplus.org/coding-style#puppet), introduced in order to avoid changing unrelated lines in patch-sets. https://codereview.adblockplus.org/29362580/diff/29363527/modules/trac/manife... modules/trac/manifests/init.pp:155: owner => 'trac', The owner should probably be set to 'root' for anything in '/usr/local/lib'. At least as long as there is no necessity to do otherwise. https://codereview.adblockplus.org/29362580/diff/29363527/modules/trac/manife... modules/trac/manifests/init.pp:264: import trac_performance_fix", Wouldn't a "\n" do it here? Or, in order to keep the 80 chars but not over-engineering stuff (i.e. using join() just for that sake), simply dropping the matched line? Or just matching the first empty line?
https://codereview.adblockplus.org/29362580/diff/29363527/modules/trac/manife... File modules/trac/manifests/init.pp (right): https://codereview.adblockplus.org/29362580/diff/29363527/modules/trac/manife... modules/trac/manifests/init.pp:152: ensure => present, On 2016/11/17 22:08:37, mathias wrote: > Please do not align arrows. This is a documented exception from the Puppet Style > Guide (https://adblockplus.org/coding-style#puppet), introduced in order to > avoid changing unrelated lines in patch-sets. Acknowledged. https://codereview.adblockplus.org/29362580/diff/29363527/modules/trac/manife... modules/trac/manifests/init.pp:155: owner => 'trac', On 2016/11/17 22:08:36, mathias wrote: > The owner should probably be set to 'root' for anything in '/usr/local/lib'. At > least as long as there is no necessity to do otherwise. Acknowledged. https://codereview.adblockplus.org/29362580/diff/29363527/modules/trac/manife... modules/trac/manifests/init.pp:264: import trac_performance_fix", On 2016/11/17 22:08:36, mathias wrote: > Wouldn't a "\n" do it here? Or, in order to keep the 80 chars but not > over-engineering stuff (i.e. using join() just for that sake), simply dropping > the matched line? Or just matching the first empty line? Puppet doesn't accept dropping it - the match also has to match the inserted line. I'll try shortening it to 80 chars.
LGTM. 
|
