On Thu, 2011-04-14 at 19:40 -0700, Edward Pilatowicz wrote:
> On Wed, Mar 30, 2011 at 09:36:05PM -0700, Brock Pytlik wrote:
> > Here's the system repository work that Tim and I have been working
> > on for quite a while now.
> > http://cr.opensolaris.org/~bpytlik/ips-sysrepo-v1/
> >
>
> more comments below.
Thanks for taking a look Ed,
> ----------
> src/pkg/manifests/package%2Fsysrepo.p5m
> src/util/apache2/sysrepo/logs/access_log
> src/util/apache2/sysrepo/logs/error_log
>
> - hm. it seems weird to me to deliver empty log files that will get
> appended to. (that said, i don't know if this is something that we
> recommend to folks packaging up other services.)
Right - Apache wants any configured log files to exist before it'll run.
I could simply touch them as part of the start method script, instead of
delivering empty stubs, but felt that having some means for an
administrator to 'pkg search' for a location for sysrepo logs would be
neat.
> ----------
> src/svc/svc-pkg-sysrepo
> src/svc/pkg-sysrepo.xml
>
> - we no longer support 32-bit systems, so you can assume all systems
> support 64 bit operation and always start the 64 bit server.
Ok.
> - why are we bothering to support prefork and worker type apache
> deployments. can't we just decide on one and use that?
That's in there now, because I haven't yet investigated which is
preferable for the sysrepo service :-(
I'm not sure how to answer questions like "What size should our cache
be?", "Disk proxy cache or memory cache?", "How many workers/threads
should we use?"
Those are things that are really going to depend on the number of
clients using the system publisher per-site, and how much pkg traffic is
going through it.
[ I'm delivering our mako http.conf template with a preserve attribute
for this reason ]
Hopefully over time, we'll work out what the best default values are - I
tried to come up with sensible defaults, but if there are any apache
experts in the room, I'd welcome their input.
> ----------
> src/svc/svc-pkg-sysrepo
>
> - i'm confused as to the relationship between APACHE_ETC_ROOT,
> SYSREPO_TEMPLATE_DIR, and the smf template_dir property. it seems
> that we can't actually change the smf template_dir property since we
> deliver template files to that location? if so do we really need to
> support this as a configurable property in smf? why not just hard
> code it in the start method.
I'm being lazy - it was there originally to leave as much of the
standard apache2 method-script logic in place (switching between prefork
and worker) and also to leave the service as flexible as possible for
the test suite.
We then added a -t option to sysrepo.py, which the test suite calls
directly, so I think I can torch APACHE_ETC_ROOT now. I'll try to clean
this up.
> - the comment example refers to apache22:
> svccfg -s apache22 ...
I'll remove that.
> - i really don't understand why you have to save and restore the
> httpd.pid file.
apachectl needs to use the http.pid file when refreshing the apache
instance, however pkg.sysrepo(1M) obliterates the /system/volatile dir
it uses to save it's configuration/runtime state each time that script
runs (guaranteeing we write a fresh configuration each time)
> (you save and restore it during refresh|stop, but
> this script will never run any of the code inbetween the save and
> restore in that case.
But yes, you're right - this looks bizzare - I'll investigate.
> - this seems kinda weird:
> /usr/bin/su pkg5srv
>
> why isn't this whole service running as the pkg5srv user? (also, if
> the sysrepo httpd process is writing to the cache dir doesn't it need
> to be running as the same user as the htcacheclean process?
That's due to the way apache starts. We start as root because we're
looking to use a low-numbered port (1008) Apache then changes ownership
and runs the httpd processes as the user specified in the
sysrepo_httpd.conf file, with any proxy cache files being owned by that
user too.
I felt that starting htcacheclean as that user was preferable to it
being a process that runs as root.
> - given that we're using apache as an embedded server i don't think we
> should be sourcing ${APACHE_ETC_ROOT}/envvars (since it in turn
> sources /etc/apache2/2.2/envvars which presumably exists to configure
> the the general apache server, not our embedded one.)
We can't avoid that if we're using the stock copy of apachectl, which
sources /usr/apache2.2/bin/envvars which in turn
sources /etc/apache2/2.2/envvars, unless we ship our own (essentially
duplicate) version of the apachectl script.
But you do bring up a good point: elsewhere in the sysrepo_httpd.conf,
we share configuration with /etc/apache2.2 by looking
at /etc/apache2/2.2/conf.d/modules-64.load, so that's something I need
to fix too. Driving a bigger wedge between /etc/apache2 and the system
repository is needed.
> ----------
> src/util/apache2/sysrepo/envvars
>
> - this file doesn't deliver any content and we always update this file
> from the smf service start service. so why bother delivering this
> file? why not just always overwrite it from the smf start scrit?
The intent was to allow system administrators more flexibility in
modifying the apache configuration. The start method script modifies
any existing file, rather than clobbering it entirely, mirroring the way
the current apache22 service works.
> ----------
> src/util/apache2/sysrepo/sysrepo_publisher_response.mako
>
> - wow. this is cryptic. could it get some comments? (i have no idea
> what "mako" even is.) do we need a copyright in here?
Mako is Python templating language, pkg(5) uses it as part of the
pkg.depotd web infrastructure and it felt like a good fit for the system
publisher. I've got modifications already that add a copyright, and a
few more comments to this file.
> - the XXX comment says we might not want to expose the source URIs, but
> if i access syspub/0 i can see them there anyway as the "proxied_uris"
> and "origins". so it seems we're already exposing this information
> via additional interfaces? if we have to expose this information then
> remove the XXX comment.
That's not the same thing - in particular, this is for file://
repositories being proxied by the system publisher.
For example, in the syspub/0 response you would see a URI like:
http:///test12/a7999d8ab1883a309202cf3fa8393025b292ae42
but the system publisher "publisher/0" response for that repository
exposes that file:/// URI from the global zone:
----
{
"packages": [],
"publishers": [
{
"alias": null,
"intermediate_certs": [],
"name": "test12",
"packages": [],
"repositories": [
{
"collection_type": "core",
"description": "This is an automatic response. This publisher is
generated automatically by the IPS system repository, and serves content from
the file-based repository file:///tmp/ips.test.14490/repo_contents2",
"legal_uris": [],
"mirrors": [],
"name": "IPS System Repository: test12",
"origins": [],
"refresh_seconds": null,
"registration_uri": "",
"related_uris": []
}
],
"signing_ca_certs": []
}
],
"version": 1
}
----
The reference to the file:// repo needs to be removed if we're to avoid
exposing global-zone paths to non-global zones.
It turns out, that this template is actually unnecessary for standard
system-publisher client access: the pkg(1) client, when when using the
system publisher, obtains all of its publisher configuration from the
"syspub/0" response now.
This code is there because I was testing the sysrepo server-side before
I had Brock's client side changes, so was using the sysrepo as a
standard pkg(5) server and needed the publisher/0 response to work for
all proxied URLs, be they file:// or http*:// ones in order to install
images.
[ I'd like to keep it around in the meantime if nobody objects, as it
makes testing the server code a little more straightforward, and has
some interesting possibilities for the future (like using the system
publisher as one-stop proxy for common groups of pkg publishers) ]
> ----------
> etc/pkg/sysrepo/sysrepo_httpd.conf.mako
>
> - this file seems derived from a stock apache template config file but
> since we don't have a copy of the original template that you used,
> syncing your changes to a future version of apache will be difficult.
> do you think that the delta to this file could be factored as a patch
> to a reference/unmodified version of the original template file and
> then check both in? or just check in a reference version of the
> unmodified file?
That's a very good point - I'm a bit wary of having to potentially do
code reviews of patches (been there with xen before) but perhaps
checking the reference version of the file in would be worthwhile.
> ----------
> src/modules/misc.py
>
> - recursive_chown_dir() do you need any special handling for symlinks?
> (right now you'll chown them as well but that will actually chown the
> target not the link itself.)
Yep, I'll investigate that.
Thanks for the code review, much appreciated.
cheers,
tim
_______________________________________________
pkg-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
