Issue 5071748311547904: Issue 1094 - Generate update manifests on the update server (Closed)
|
Created:
July 24, 2014, 2:10 p.m. by Felix Dahlke Modified:
July 28, 2014, 7:08 a.m. Visibility:
Public. |
DescriptionIssue 1094 - Generate update manifests on the update server
Patch Set 1 : #
Total comments: 14
Patch Set 2 : Reduced duplication #
Total comments: 24
Patch Set 3 : Use flock #Patch Set 4 : Addressed comments #Patch Set 5 : Run as user sitescripts #
Total comments: 1
MessagesTotal messages: 16
http://codereview.adblockplus.org/5071748311547904/diff/5649050225344512/modu... File modules/updateserver/manifests/init.pp (right): http://codereview.adblockplus.org/5071748311547904/diff/5649050225344512/modu... modules/updateserver/manifests/init.pp:29: $update_manifest_dirs = ['/var/www/update/adblockplus', Now that /var/www/update is used over and over, I'd prefer using a variable for this prefix, much like $base_dir below. http://codereview.adblockplus.org/5071748311547904/diff/5649050225344512/modu... modules/updateserver/manifests/init.pp:38: file {'/var/www/update/adblockplusie': I assume it's intentional that adblockplusie is not included in the $update_manifest_dirs? (I don't know these parts of our infrastructure, software and requirements well enough yet to answer that myself..) http://codereview.adblockplus.org/5071748311547904/diff/5649050225344512/modu... modules/updateserver/manifests/init.pp:82: path => '/usr/bin', Are you sure that hg(1) is available in /usr/bin? http://codereview.adblockplus.org/5071748311547904/diff/5649050225344512/modu... modules/updateserver/manifests/init.pp:91: $update_update_manifests_script = '/usr/local/bin/update_update_manifests' Is the update_update prefix intentional? http://codereview.adblockplus.org/5071748311547904/diff/5649050225344512/modu... modules/updateserver/manifests/init.pp:98: $update_update_manifests_dependencies = ['python-m2crypto', 'python-jinja2'] Is the update_update prefix intentional? http://codereview.adblockplus.org/5071748311547904/diff/5649050225344512/modu... File modules/updateserver/templates/update_update_manifests.erb (right): http://codereview.adblockplus.org/5071748311547904/diff/5649050225344512/modu... modules/updateserver/templates/update_update_manifests.erb:3: run_file="/var/run/update_update_manifests" At http://mywiki.wooledge.org/BashFAQ/045 please find an example for a better way to implement lock mechanisms in shell scripts - one that is easy, short and not prone to race conditions.
Uploaded a new patch set. http://codereview.adblockplus.org/5071748311547904/diff/5649050225344512/modu... File modules/updateserver/manifests/init.pp (right): http://codereview.adblockplus.org/5071748311547904/diff/5649050225344512/modu... modules/updateserver/manifests/init.pp:29: $update_manifest_dirs = ['/var/www/update/adblockplus', On 2014/07/25 07:04:28, matze wrote: > Now that /var/www/update is used over and over, I'd prefer using a variable for > this prefix, much like $base_dir below. Done. http://codereview.adblockplus.org/5071748311547904/diff/5649050225344512/modu... modules/updateserver/manifests/init.pp:38: file {'/var/www/update/adblockplusie': On 2014/07/25 07:04:28, matze wrote: > I assume it's intentional that adblockplusie is not included in the > $update_manifest_dirs? (I don't know these parts of our infrastructure, software > and requirements well enough yet to answer that myself..) Yup, we can't generate that one yet with how things currently are in sitescripts. Working on that in parallel, will be a separate patch. http://codereview.adblockplus.org/5071748311547904/diff/5649050225344512/modu... modules/updateserver/manifests/init.pp:82: path => '/usr/bin', On 2014/07/25 07:04:28, matze wrote: > Are you sure that hg(1) is available in /usr/bin? Yes. http://codereview.adblockplus.org/5071748311547904/diff/5649050225344512/modu... modules/updateserver/manifests/init.pp:91: $update_update_manifests_script = '/usr/local/bin/update_update_manifests' On 2014/07/25 07:04:28, matze wrote: > Is the update_update prefix intentional? Yes, we're updating update manifests here. http://codereview.adblockplus.org/5071748311547904/diff/5649050225344512/modu... modules/updateserver/manifests/init.pp:98: $update_update_manifests_dependencies = ['python-m2crypto', 'python-jinja2'] On 2014/07/25 07:04:28, matze wrote: > Is the update_update prefix intentional? Yes.
You've neither replied to my comment for the update_update_manifests.erb template nor updated the file. So, what's up with that one?
On 2014/07/25 07:31:53, matze wrote: > You've neither replied to my comment for the update_update_manifests.erb > template nor updated the file. So, what's up with that one? Missed it, sorry. Will address this in a bit.
http://codereview.adblockplus.org/5071748311547904/diff/5657382461898752/modu... File modules/updateserver/files/sitescripts (right): http://codereview.adblockplus.org/5071748311547904/diff/5657382461898752/modu... modules/updateserver/files/sitescripts:2: hg_root=/var/lib/adblockplus We usually keep the repositories under /opt (see discourse, notificationserver and sitescripts) - I guess we should do it here as well for consistency. http://codereview.adblockplus.org/5071748311547904/diff/5657382461898752/modu... modules/updateserver/files/sitescripts:6: abpsafari_key=%(hg_root)s/adblockplussafari.pem Ouch, keeping that key on the update server only for the sake of extracting the extension ID sucks. We need to find a better solution for this. I guess that a long-term solution will be a build server to produce releases. That one could store the relevant metadata for each release in some location where the update server can look it up. This will also make it unnecessary to sync the repositories on the update server. http://codereview.adblockplus.org/5071748311547904/diff/5657382461898752/modu... modules/updateserver/files/sitescripts:16: geckoUpdateManifestPath=%(web_root)s/adblockplus/update.rdf That's a rather misleading path given that this file will be used for multiple extensions including URL Fixer. Maybe /firefox/update.rdf? We could also create individual update manifests for the extensions, that would require more changes to the scripts however. http://codereview.adblockplus.org/5071748311547904/diff/5657382461898752/modu... modules/updateserver/files/sitescripts:20: androidUpdateManifestPath=%(web_root)s/adblockplusandroid/androidupdates.xml updates.xml please - it was only called androidupdates.xml on the main server because of another file called updates.xml. http://codereview.adblockplus.org/5071748311547904/diff/5657382461898752/modu... File modules/updateserver/manifests/init.pp (right): http://codereview.adblockplus.org/5071748311547904/diff/5657382461898752/modu... modules/updateserver/manifests/init.pp:26: file {"${update_dir}": Why put it into quotation marks here? file {$update_dir: http://codereview.adblockplus.org/5071748311547904/diff/5657382461898752/modu... modules/updateserver/manifests/init.pp:117: command => $update_update_manifests_script, I'd rather not have anything from sitescripts run as root unless there is a very good reason for that. Please have some user that will own the manifest directories and run sitescripts. http://codereview.adblockplus.org/5071748311547904/diff/5657382461898752/modu... File modules/updateserver/templates/update_update_manifests.erb (right): http://codereview.adblockplus.org/5071748311547904/diff/5657382461898752/modu... modules/updateserver/templates/update_update_manifests.erb:6: exit 0 What if this script crashes and doesn't remove the file? Please use a standard solution, e.g. the daemon package (http://libslack.org/daemon/manpages/daemon.1.html). http://codereview.adblockplus.org/5071748311547904/diff/5657382461898752/modu... modules/updateserver/templates/update_update_manifests.erb:12: hg pull -q -u -R <%= base_dir %>/<%= repository %> Updating is unnecessary, the scripts work with the repository data directly rather than the working directory. *If* you choose to update, you should do it with -r default and --check, see filtermaster module. http://codereview.adblockplus.org/5071748311547904/diff/5657382461898752/modu... modules/updateserver/templates/update_update_manifests.erb:16: python -m sitescripts.extensions.bin.updateUpdateManifests No need to export the variable if it is only used by a single command: PYTHONPATH=/opt/sitescripts python -m ...
http://codereview.adblockplus.org/5071748311547904/diff/5649050225344512/modu... File modules/updateserver/templates/update_update_manifests.erb (right): http://codereview.adblockplus.org/5071748311547904/diff/5649050225344512/modu... modules/updateserver/templates/update_update_manifests.erb:3: run_file="/var/run/update_update_manifests" On 2014/07/25 07:04:28, matze wrote: > At http://mywiki.wooledge.org/BashFAQ/045 please find an example for a better > way to implement lock mechanisms in shell scripts - one that is easy, short and > not prone to race conditions. Done.
http://codereview.adblockplus.org/5071748311547904/diff/5649050225344512/modu... File modules/updateserver/manifests/init.pp (right): http://codereview.adblockplus.org/5071748311547904/diff/5649050225344512/modu... modules/updateserver/manifests/init.pp:82: path => '/usr/bin', On 2014/07/25 07:17:45, Felix H. Dahlke wrote: > On 2014/07/25 07:04:28, matze wrote: > > Are you sure that hg(1) is available in /usr/bin? > > Yes. Thinking a bit more about it - this does look like a bad assumption. What if we at some point decide to install Mercurial via pip? I'd rather go with a standard search path like '/bin:/usr/bin:/usr/sbin:/usr/local/bin'.
Uploaded a new patch set. http://codereview.adblockplus.org/5071748311547904/diff/5649050225344512/modu... File modules/updateserver/manifests/init.pp (right): http://codereview.adblockplus.org/5071748311547904/diff/5649050225344512/modu... modules/updateserver/manifests/init.pp:82: path => '/usr/bin', On 2014/07/25 09:54:39, Wladimir Palant wrote: > On 2014/07/25 07:17:45, Felix H. Dahlke wrote: > > On 2014/07/25 07:04:28, matze wrote: > > > Are you sure that hg(1) is available in /usr/bin? > > > > Yes. > > Thinking a bit more about it - this does look like a bad assumption. What if we > at some point decide to install Mercurial via pip? I'd rather go with a standard > search path like '/bin:/usr/bin:/usr/sbin:/usr/local/bin'. Done. Left sbin out since I really wouldn't expect hg there. http://codereview.adblockplus.org/5071748311547904/diff/5657382461898752/modu... File modules/updateserver/files/sitescripts (right): http://codereview.adblockplus.org/5071748311547904/diff/5657382461898752/modu... modules/updateserver/files/sitescripts:2: hg_root=/var/lib/adblockplus On 2014/07/25 09:50:11, Wladimir Palant wrote: > We usually keep the repositories under /opt (see discourse, notificationserver > and sitescripts) - I guess we should do it here as well for consistency. Based on the file system hierarchy standard, /var/lib is the best place for application specific changing files. /opt is for applications that don't comply with the FHS basically. http://codereview.adblockplus.org/5071748311547904/diff/5657382461898752/modu... modules/updateserver/files/sitescripts:6: abpsafari_key=%(hg_root)s/adblockplussafari.pem On 2014/07/25 09:50:11, Wladimir Palant wrote: > Ouch, keeping that key on the update server only for the sake of extracting the > extension ID sucks. We need to find a better solution for this. > > I guess that a long-term solution will be a build server to produce releases. > That one could store the relevant metadata for each release in some location > where the update server can look it up. This will also make it unnecessary to > sync the repositories on the update server. Yes, I believe that's the best approach. For now we can live with this how it is I suppose. http://codereview.adblockplus.org/5071748311547904/diff/5657382461898752/modu... modules/updateserver/files/sitescripts:16: geckoUpdateManifestPath=%(web_root)s/adblockplus/update.rdf On 2014/07/25 09:50:11, Wladimir Palant wrote: > That's a rather misleading path given that this file will be used for multiple > extensions including URL Fixer. Maybe /firefox/update.rdf? > > We could also create individual update manifests for the extensions, that would > require more changes to the scripts however. Went even further and called the thing "gecko". But yeah, I guess it makes sense to split this up, filed a follow-up for that: https://issues.adblockplus.org/ticket/1142 http://codereview.adblockplus.org/5071748311547904/diff/5657382461898752/modu... modules/updateserver/files/sitescripts:20: androidUpdateManifestPath=%(web_root)s/adblockplusandroid/androidupdates.xml On 2014/07/25 09:50:11, Wladimir Palant wrote: > updates.xml please - it was only called androidupdates.xml on the main server > because of another file called updates.xml. Done. http://codereview.adblockplus.org/5071748311547904/diff/5657382461898752/modu... File modules/updateserver/manifests/init.pp (right): http://codereview.adblockplus.org/5071748311547904/diff/5657382461898752/modu... modules/updateserver/manifests/init.pp:26: file {"${update_dir}": On 2014/07/25 09:50:11, Wladimir Palant wrote: > Why put it into quotation marks here? > > file {$update_dir: Done. http://codereview.adblockplus.org/5071748311547904/diff/5657382461898752/modu... modules/updateserver/manifests/init.pp:117: command => $update_update_manifests_script, On 2014/07/25 09:50:11, Wladimir Palant wrote: > I'd rather not have anything from sitescripts run as root unless there is a very > good reason for that. Please have some user that will own the manifest > directories and run sitescripts. I actually started out having a dedicated user for this, but it's more hassle, and I didn't really see any reason why it shouldn't run as root, we're not running any externally available services here. We can go with a dedicated user if you really think it's an issue though. One problem I had was that I couldn't think of a good name for one, suggestions? http://codereview.adblockplus.org/5071748311547904/diff/5657382461898752/modu... File modules/updateserver/templates/update_update_manifests.erb (right): http://codereview.adblockplus.org/5071748311547904/diff/5657382461898752/modu... modules/updateserver/templates/update_update_manifests.erb:6: exit 0 On 2014/07/25 09:50:11, Wladimir Palant wrote: > What if this script crashes and doesn't remove the file? > > Please use a standard solution, e.g. the daemon package > (http://libslack.org/daemon/manpages/daemon.1.html). Using flock now, are you happy with that? It'll handle crashes. http://codereview.adblockplus.org/5071748311547904/diff/5657382461898752/modu... modules/updateserver/templates/update_update_manifests.erb:12: hg pull -q -u -R <%= base_dir %>/<%= repository %> On 2014/07/25 09:50:11, Wladimir Palant wrote: > Updating is unnecessary, the scripts work with the repository data directly > rather than the working directory. *If* you choose to update, you should do it > with -r default and --check, see filtermaster module. Done. http://codereview.adblockplus.org/5071748311547904/diff/5657382461898752/modu... modules/updateserver/templates/update_update_manifests.erb:16: python -m sitescripts.extensions.bin.updateUpdateManifests On 2014/07/25 09:50:11, Wladimir Palant wrote: > No need to export the variable if it is only used by a single command: > > PYTHONPATH=/opt/sitescripts python -m ... It's shorter that way, I try to stick to 80 chars.
http://codereview.adblockplus.org/5071748311547904/diff/5657382461898752/modu... File modules/updateserver/files/sitescripts (right): http://codereview.adblockplus.org/5071748311547904/diff/5657382461898752/modu... modules/updateserver/files/sitescripts:16: geckoUpdateManifestPath=%(web_root)s/adblockplus/update.rdf On 2014/07/25 10:31:15, Felix H. Dahlke wrote: > Went even further and called the thing "gecko". But yeah, I guess it makes sense > to split this up, filed a follow-up for that: > > https://issues.adblockplus.org/ticket/1142 Note that splitting up is better done before we redirect the old update manifests, this will get pretty messy otherwise. http://codereview.adblockplus.org/5071748311547904/diff/5657382461898752/modu... File modules/updateserver/manifests/init.pp (right): http://codereview.adblockplus.org/5071748311547904/diff/5657382461898752/modu... modules/updateserver/manifests/init.pp:117: command => $update_update_manifests_script, On 2014/07/25 10:31:15, Felix H. Dahlke wrote: > I actually started out having a dedicated user for this, but it's more hassle, > and I didn't really see any reason why it shouldn't run as root, we're not > running any externally available services here. The reason is that we have less reason to worry about the quality of our sitescripts contents (security-wise and otherwise). > We can go with a dedicated user if you really think it's an issue though. Yes, please. > One > problem I had was that I couldn't think of a good name for one, suggestions? sitescripts? http://codereview.adblockplus.org/5071748311547904/diff/5657382461898752/modu... File modules/updateserver/templates/update_update_manifests.erb (right): http://codereview.adblockplus.org/5071748311547904/diff/5657382461898752/modu... modules/updateserver/templates/update_update_manifests.erb:6: exit 0 On 2014/07/25 10:31:15, Felix H. Dahlke wrote: > Using flock now, are you happy with that? It'll handle crashes. I'd really prefer a standardized solution that would keep the complexity out of our code.
On 2014/07/25 10:31:15, Felix H. Dahlke wrote: > http://codereview.adblockplus.org/5071748311547904/diff/5657382461898752/modu... > modules/updateserver/files/sitescripts:2: hg_root=/var/lib/adblockplus > On 2014/07/25 09:50:11, Wladimir Palant wrote: > > We usually keep the repositories under /opt (see discourse, notificationserver > > and sitescripts) - I guess we should do it here as well for consistency. > > Based on the file system hierarchy standard, /var/lib is the best place for > application specific changing files. /opt is for applications that don't comply > with the FHS basically. You are both right. But when it comes to standards vs. consistency, I'd go for the latter. Ideally we value both, so maybe we can file another ticket for improving the overall FHS compliance in the aforementioned modules later? > http://codereview.adblockplus.org/5071748311547904/diff/5657382461898752/modu... > modules/updateserver/manifests/init.pp:117: command => > $update_update_manifests_script, > On 2014/07/25 09:50:11, Wladimir Palant wrote: > > I'd rather not have anything from sitescripts run as root unless there is a > very > > good reason for that. Please have some user that will own the manifest > > directories and run sitescripts. > > I actually started out having a dedicated user for this, but it's more hassle, > and I didn't really see any reason why it shouldn't run as root, we're not > running any externally available services here. > > We can go with a dedicated user if you really think it's an issue though. One > problem I had was that I couldn't think of a good name for one, suggestions? Why not (re-)using the user the delivering daemon runs as? > http://codereview.adblockplus.org/5071748311547904/diff/5657382461898752/modu... > File modules/updateserver/templates/update_update_manifests.erb (right): > > http://codereview.adblockplus.org/5071748311547904/diff/5657382461898752/modu... > modules/updateserver/templates/update_update_manifests.erb:6: exit 0 > On 2014/07/25 09:50:11, Wladimir Palant wrote: > > What if this script crashes and doesn't remove the file? > > > > Please use a standard solution, e.g. the daemon package > > (http://libslack.org/daemon/manpages/daemon.1.html). > > Using flock now, are you happy with that? It'll handle crashes. I like the choice. Still, the lock file may remain, but with the flock way to go this does not matter. By the way, a clean_up function and traps are also a pretty lightweight and standard-compliant solution if one wants to be very, very "clean" ;-), see also http://linuxcommand.org/wss0160.php#clean_up
Uploaded a new patch set. Matze, please reply inline to comments instead of replying to the whole message, it's easier to follow that way. http://codereview.adblockplus.org/5071748311547904/diff/5657382461898752/modu... File modules/updateserver/files/sitescripts (right): http://codereview.adblockplus.org/5071748311547904/diff/5657382461898752/modu... modules/updateserver/files/sitescripts:16: geckoUpdateManifestPath=%(web_root)s/adblockplus/update.rdf On 2014/07/25 11:11:02, Wladimir Palant wrote: > On 2014/07/25 10:31:15, Felix H. Dahlke wrote: > > Went even further and called the thing "gecko". But yeah, I guess it makes > sense > > to split this up, filed a follow-up for that: > > > > https://issues.adblockplus.org/ticket/1142 > > Note that splitting up is better done before we redirect the old update > manifests, this will get pretty messy otherwise. Messy how? We don't need redirects for the individual update manifests if those are going to the proper update server to begin with. http://codereview.adblockplus.org/5071748311547904/diff/5657382461898752/modu... File modules/updateserver/manifests/init.pp (right): http://codereview.adblockplus.org/5071748311547904/diff/5657382461898752/modu... modules/updateserver/manifests/init.pp:117: command => $update_update_manifests_script, On 2014/07/25 11:11:02, Wladimir Palant wrote: > On 2014/07/25 10:31:15, Felix H. Dahlke wrote: > > I actually started out having a dedicated user for this, but it's more hassle, > > and I didn't really see any reason why it shouldn't run as root, we're not > > running any externally available services here. > > The reason is that we have less reason to worry about the quality of our > sitescripts contents (security-wise and otherwise). > > > We can go with a dedicated user if you really think it's an issue though. > > Yes, please. > > > One > > problem I had was that I couldn't think of a good name for one, suggestions? > > sitescripts? Done. http://codereview.adblockplus.org/5071748311547904/diff/5657382461898752/modu... File modules/updateserver/templates/update_update_manifests.erb (right): http://codereview.adblockplus.org/5071748311547904/diff/5657382461898752/modu... modules/updateserver/templates/update_update_manifests.erb:6: exit 0 On 2014/07/25 11:11:02, Wladimir Palant wrote: > On 2014/07/25 10:31:15, Felix H. Dahlke wrote: > > Using flock now, are you happy with that? It'll handle crashes. > > I'd really prefer a standardized solution that would keep the complexity out of > our code. I've looked into daemon, but I don't like the fact that we have to remember to call the script with it (e.g. when regenerating update manifests manually). If the script guarantees mutual exclusion itself, nothing can go wrong. It seems that flock is the standarised solution for having a script run mutually exclusive - it adds only 3 lines to the script and doesn't require additional packages.
http://codereview.adblockplus.org/5071748311547904/diff/5718998062727168/modu... File modules/updateserver/manifests/init.pp (right): http://codereview.adblockplus.org/5071748311547904/diff/5718998062727168/modu... modules/updateserver/manifests/init.pp:31: $sitescripts_var_dir = '/var/lib/sitescripts' This was /var/lib/adblockplus before. This is now more confusing since we have /opt/sitescripts, which suggests it's a all-in-one package, and then we have these runtime files here in the proper FHS place. I don't see a better way without changing the sitescripts module though. /opt/sitescripts is a hg repository, adding files not under version control there seems worse. I suppose the cleanest way would be to move /opt/sitescripts to /usr/lib/sitescripts.
I'm not really happy with the flock solution - three lines or not, it's non-trivial enough that I would prefer to remove this complexity from our code. Still, LGTM.
LGTM. I understand that the flock(1) solution isn't perfect, especially because it requires knowledge about this technique to understand what's going on (or, at least, an explaining comment). Still, as long as we're not using similar locking in multiple shell scripts, I believe it's better than introducing another dependency. 
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
