hi Shawn,
On 12/12/12 10:18 AM, Shawn Walker wrote:
On 11/29/12 19:08, Tim Foster wrote:
I've got a webrev of the http-depot work ready for review, if anyone has
bandwidth:
https://cr.opensolaris.org/action/browse/pkg/timf/http-depot-webrev-0/http-depot-webrev/
Thanks for the review. I've made some changes and have a new webrev and
incremental at:
https://cr.opensolaris.org/action/browse/pkg/timf/http-depot-webrev-1
The incremental webrev didn't cope well with the file renaming going on
in this webrev, so I hand-generated the incremental webrev entries for
the python script that does Apache config generation.
Other than the comments here, I've made the service method script more
robust when we drop to maintenance mode[1] and done more work to deal
with IPv6 properly.
So I'm not in love with the new executable name; it implies that
/usr/lib/pkg.depotd doesn't support HTTP; yes, it's bikeshedding.
Perhaps 'pkg.depot-multi' or 'pkg.depot-scalable'? That at least seems
less misleading and makes it slightly more obvious what the difference is.
Ok. I'm going with:
/usr/lib/pkg.depot-config and
svc:/application/pkg/depot
I considered pkg.depotcfg and pkg.depot-gen-config (and a few others)
but felt that pkg.depot-config best described what the executable does,
and that taking 'pkg.depotcfg' might be problematic if we ever feel we
need a better non-SMF configuration interface in the future.
I don't expect most users to ever need to interact with the executable -
they're more likely to use the SMF service directly, and even then, will
probably only ever enable/disable it.
A side-effect of this, is that for consistency, the SMF service method
script should also be called svc-pkg-depot, which clashes with the
script we've been using for the existing pkg.depotd-based service.
So this updated webrev does the following renames (omitting module
renames in src/tests here):
src/http-depot.py -> src/depot-config.py
src/svc/pkg-http-depot.xml -> src/svc/pkg-depot.xml
src/svc/svc-pkg-http-depot -> src/svc/svc-pkg-depot
src/pkg/manifests/package:pkg:http-depot.p5m
-> src/pkg/manifests/package:pkg:depot.p5m
src/svc/svc-pkg-depot -> src/svc/svc-pkg-server
and we now deliver
/usr/lib/pkg.depot-config
/lib/svc/method/svc-pkg-depot
/lib/svc/manifest/application/pkg/pkg-depot.xml
/lib/svc/method/svc-pkg-server
(and continue delivering
/lib/svc/manifest/application/pkg/pkg-server.xml )
pkg:/package/pkg/depot
I've made changes throughout svc-pkg-server to disambiguate 'depot',
using 'pkg.depotd' when that's what we really mean.
--------------------------------------------------
src/http-depot.py:
general: s/os.makedirs/misc.makedirs/ seems preferable, and that
should allow dropping the try/except for each case
That doesn't help everywhere, as I'm often looking to provide a little
more information about what part of the depot-config creation failed.
Still, I've done it for consistency.
general: All references to svc:/application/pkg/server should
probably be through a constant.
Yep - turns out we only referenced it once, the second time was a thinko
on my part, 'pkg:/application/pkg/server'. Fixed.
lines 35, 37, 47, 48: unused import
Thanks - that was debug/development code. I really wish RichTraceback
was the default for MakoExceptions.
line 93: nit: make consistent with 83-90 (same multi-line style)
Sure.
lines 105-108: unused? if so, don't need import on line 27
Yep.
line 141: '-a' for "use SMF" seems an odd letter to use. Also, I'm
not claiming it's the best option, but did you consider making the
options more consistent with pkg.depotd to lessen potential confusion?
For example, -d instead of -R? We typically have only used -R for image
roots, whereas this is a repository root. I realise that perhaps we
shouldn't have done that, but we've already established a pattern of usage.
Sure. -a was supposed to mean "All SMF Instances", perhaps recalling
'svcs -a' but that's a tenuous link, I admit. I'm fine with -d, though
it should beL
-d prefix=/path_to_repo
which does make it differ slightly from pkg.depotd
(I'm also changing from ':', which you bring up later, to '=')
but we do complain when incorrect syntax is used, and -d is more in the
same ballpark.
So I'm changing this from:
pkg.http-depot ( -R repository_dir | -a ) -r runtime_dir
[-c cache_dir] [-s cache_size] [-p port] [-h hostname]
[-l logs_dir] [-T template_dir] [-A]
[-t server_type] ( [-F] [-S server_prefix] )
to:
pkg.depot-config ( -d repository_dir | -S ) -r runtime_dir
[-c cache_dir] [-s cache_size] [-p port] [-h hostname]
[-l logs_dir] [-T template_dir] [-A]
[-t server_type] ( [-F] [-P server_prefix] )
line 163: s/return a list of publishers available here/
the list of available publishers/
line 204: s/ie./i.e./
line 239: s/0755/misc.PKG_DIR_MODE/
Yep fixed those. Using os.makedirs uses misc.PKG_DIR_MODE anyway.
line 247: When I did this in the past for pkg.depotd, Danek suggested
I shouldn't. (Enforce port range.) I'm inclined to agree as Apache
should already be handling this.
The difference here is that when you don't enforce port range in
pkg.depotd, you get an immediate error that the port is invalid. Since
we've got a separation here between the time we set the property and the
time its actually used (for example, if the user isn't using SMF they
would generate an Apache configuration to be used, then perhaps copy it
to some other system), so reporting the error early is better imho.
line 269-270: Can you elucidate a bit on this? Keep in mind that we
officially do not support the use of any unicode within the repository
for filenames / paths. The only part of the path that could be unicode
is the parent of the repository.
... and that's the problem. We need to make sure that we can write
directory paths leading to the repository and not have Mako garble them
en-route. The system-repository uses the same chunk of code (and has
tests for non-ascii paths, see t_sysrepo..test_15_unicode which I
haven't included in the tests here, admittedly)
lines 301-303: This should really be:
with file(conf_path, "w") as conf_file:
conf_file.write(conf_text)
Yep.
line 309: redundant
Thanks.
lines 319, 343, 353: the call to os.path.sep.join seems unnecessary;
os.path.join accepts multiple arguments
Thanks.
lines 322-329: more simply written as:
with file(os.path.join(versions_path, "index.html"), "w") as
versions_path:
versions_file.write(fragment and DEPOT_VERSIONS_STR or
DEPOT_FRAGMENT_VERSIONS_STR)
lines 346-349, 355-357: use with() pattern
line 375: extra newline
Yep, thanks, though that should be:
versions_file.write(fragment and DEPOT_FRAGMENT_VERSIONS_STR or
DEPOT_FRAGMENT_VERSIONS_STR)
lines 398-399: Should it query all of the repositories and accumulate
the errors together instead of stopping on the first bad one?
Good idea.
line 434: s/Written/Wrote/ or s/Written/Created/ ?
Yes, 'Created' sounds better.
line 442: The docstring "marked as pkg/standalone" implies something
different than 455 "standalone == False".
line 465: s/root, and prefixes supplied/root and supplied prefixes/
Fixed both of those.
lines 473-474, 478-479: seems like these should be accumulated and
raised after processing all of them
Good idea.
line 486: prefixes also shouldn't contain unicode; I'm fairly certain
transport will explode in pretty colours if any of the URI contains
them. Also, aren't there other chars Apache doesn't like or that hold
special meaning? Such as '?' and '%' ? You could possibly just re-use
misc.valid_pub_prefix here to test for validity.
Good idea - valid_pub_prefix seems as restrictive as SMF instance names,
so that works.
line 492: 0.0.0.0 may be more appropriate; it means INADDR_ANY, which
means all active interfaces. While 127.0.0.1 may not be externally
routable for the system or hostname resolution may not work properly for
it. See http://defect.opensolaris.org/bz/show_bug.cgi?id=8145#c4
I've changed that, thanks. This gets used as the ServerName and I'm now
using it in the Apache Listen directive as well.
--------------------------------------------------
src/man/pkg.http-depot.1m:
general: Sometimes there are two spaces after the end of sentences,
and sometimes not.
Yep, I will be filing a docs bug so that Alta can start cleaning this up
- the man page is mostly a draft to better document the feature.
line 22: XXX?
I'll remove that, and open a bug to require that support.
line 45: We generally use ',' not ':' for multiple values. I suspect
you felt ':' was less likely in a path. Just a note on consistency.
It's not a multiple value, it's a key/value pair. As noted above,
perhaps changing it to '=' makes that more clear.
line 89: s/ie./i.e./
line 100: s/to./on./ is the general nomenclature I believe
line 111: See earlier note about 0.0.0.0. I would also suggest to
the user that they use '::' if they want IPv6. I'd also check that
actually works :-)
There's some grief with IPv6 addresses, in that our version of Apache
chokes with IPv6 addresses which I've worked around, letting the system
choose its own ServerName if we're using IPv6. I also needed a few
changes to the method script: it pings the $host:$port to make sure the
service is running, and that needed square brackets for IPv6 addresses.
line 155: The wording implies the default value is 'apache2' but
that is not explicitly specified.
line 166: s/for clients./for clients using an existing web service./
line 166: s/dropped into/placed in/ ?
line 179: The prefix prefix?
line 186: s/ directives/ directives/
lines 196, 202: XXX
line 256: I believe we now use 'Uncommitted'
Fixed all of these.
--------------------------------------------------
src/modules/server/face.py:
lines 142-149: leftover debug code?
Yep, thanks. I still wish RichTraceback was the default.
--------------------------------------------------
src/pkg/manifests/package%3Apkg%3Ahttp-depot.p5m:
line 27: s/http/HTTP/
line 27: s/server/package repository server/ ?
I'm going with "A service that provides a scalable web-based pkg(5) server."
lines 58, 59: why 'renamenew' instead of 'true'?
I can't remember. I think I originally was going to store some sort of
boilerplate text in this file, and wanted users to get new versions of
that boilerplate, but I can't think of a reason why we really need that.
Changing to 'true'
--------------------------------------------------
src/svc/svc-pkg-depot:
general: lots of error messages aren't line wrapped;
also seems like error messages should be a constant
like we do in zone scripts instead of repeated
Fixed.
line 53: s/is is/it is/
I can't find that?
line 77: surely there's a better option than eval?
I'm all ears, but this was the best I could come up with. I'm trying to
avoid a storm of svcprop commands when starting the service - this was
the best I could come up with (borrowed from when I last used it in the
zfs-auto-snapshot) If you're uncomfortable with it, I could switch back
to the way we do this in the system-repository.
line 114: s/ a //
Yep
lines 134-135: erp?
Egads - thanks!
--------------------------------------------------
src/tests/cli/t_http-depot.py:
line 36: I think this is unnecessary due to line 35
Yes.
lines 332-337: I think fmri.get_link_path() or fmri.get_url_path()
will do what you want for the most part
Thanks.
line 2623: the default sleeptime seems excessive given that's seconds
(~277 hours?)
I've typically been using this as a convenient way to put the brakes on
the entire test execution for a given method, keeping all the state
we've built up. After investigating breakage, I tend to Ctrl-C the test
run, remove the snooze() and carry on.
I'll set it to 3h (i.e. enough time to have a really good lunch and pick
up where you left off afterwards :-) which might mean that if snooze()
somehow makes it into the test gate, it won't massively affect testing,
though perhaps it becomes less noticeable as a result.
--------------------------------------------------
src/util/apache2/depot/depot.conf.mako:
line 27: 'it be used' ?
Yep.
--------------------------------------------------
src/util/apache2/depot/depot_index.py:
general: I see lots of places here that should really be using the
httplib constants instead of '503', etc.
Fixed.
lines 361-405: drop the 'else:' and de-indent since
everything above returns or raises an exception
Sure. (though I think you're referring to 356-398 in the webrev)
line 597: This should be '(object)' instead of '()' at least.
Otherwise it's considered an "old-style" class I believe.
Sure.
lines 679-680: The server should not expose general tracebacks.
(In short, don't send them to clients, just log them.) Also,
we historically avoided raising 500 errors as that could cause
Apache to stop proxying the server. I don't know it matters here
since this doesn't appear to be a reverseproxy scenario.
(more weird line numbers here) It's not a reverse proxy.
> The HTTP
spec says that servers are permitted to return a 404 error which
is my preference since it makes less obvious to would-be attackers
when they've potentially found an error case.
but yes, that's a reasonable point, I've changed that.
--------------------------------------------------
src/util/apache2/depot/repos.shtml:
line 46: s/following/following package/
line 48: s/the/the package/
line 48: s/on/from/ to be consistent?
All fixed.
line 53: If this page is accessible from multiple hostnames, things
could be confusing if the links here are absolute instead of relative.
Just be certain that the URI reflects the hostname things are being
accessed from. In the event that a server can be accessed from multiple
hostnames, the user probably configured it with an IP address (for
HTTP). Only in the case of HTTPS would the actual name matter (due to
the certificate, obviously).
Yes, I believe it's correct - accessing my desktop using *.oracle.com or
localhost results in the correct CSS being used for each case at least.
--------------------------------------------------
src/web/en/base.shtml:
We could just kill the stats page. It's mostly useless anyway.
Yeah, though not this changeset.
line 34: It'd be nice if we had something less ambiguous than
'http_depot' :-( Yay bikeshedding.
Right, see up at the top.
--------------------------------------------------
src/web/en/catalog.shtml:
lines 247-249: I think this can use get_url_path() too; might| Gr
allow simplifying this whole section
--------------------------------------------------
src/web/en/search.shtml:
lines 360-370: See note about catalog.shtml.
Yep, that works, thanks.
That's all I can think of for now.
Excellent, thanks again for the review.
cheers,
tim
[1] In particular, when a user disables the last remaining pkg/server
instance, which causes our depot method script to refresh, that service
correctly drops to maintenance (no remaining pkg/servers to base our
configuration off) However, it needs to do so leaving no remaining
Apache processes, which otherwise get in our way when we enable a
pkg/server instance, then clear the depot.
_______________________________________________
pkg-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
