On 06/21/12 03:50 PM, Brock Pytlik wrote:
https://cr.opensolaris.org/action/browse/pkg/timf/granular-proxies-4/
Thanks for taking a look, much appreciated. Comments are below, and an
updated webrev and incremental webrev with the changes discussed below
is at:
https://cr.opensolaris.org/action/browse/pkg/timf/granular-proxies-5
(that's still based off the gate just after Ed's push: I have also
merged with Dan's progress-tracking bits locally, but the merge was
straightforward, so it's not included in this webrev)
Couple of UI questions.
If I want to remove a proxy for an origin, how do I do that? For that
matter, how do I change what a proxy for an origin is?
You need to remove the origin completely and add it with a different (or
absent) --proxy. There is no user-interface for adding or removing
proxies from an existing origin or mirror at present (it seems like an
uncommon-enough operation that just removing and adding a replacement
origin is sufficient for now)
I think we discussed this offline, but to confirm, this means I can't
have the same origin configured with and without a proxy, correct?
Yep, correct.
This means that an NGZ can no longer configure a direct connection to any
origin that the sysrepo is providing for it. Is that correct? I'm not
thrilled with losing this functionality but it seems like it was forced
upon you, and I'm not going to block the putback over that. I'd
appreciate an RFE filed (if there's not one already) to restore this
ability in the future.
Correct, I'll file an RFE - we can't do this for the same reason we
don't allow multiple proxies, because the client isn't correctly
compositing multiple routes to the same URI (eg. proxied and
non-proxied) though I can't see a bug or RFE for that.
As such, I had to do work to temporarily remove existing user-configured
origins in the image that match the URIs of system publisher origins.
When the "use-system-repo" image property is turned off, those
user-origins become visible again.
When using the system publisher, user-configured origins that do not
conflict with system publisher-provided origins are used as normal.
client.py:
Some of this code looks like there can only be a single proxy per URI,
but other bits of code makes it look like we support multiple proxies.
Is the multiple proxy code just laying track for future work, or have I
not gotten my head wrapped around something?
Yes, it's laying track (uh, rope? :-), the API
(pkg.client.publisher.RepositoryURI) restricts us to at most one proxy
per origin or mirror.
api.py:
4558, 4563: You may want to sort, or otherwise order these so you don't
get spurious differences (unless the ordering of the proxies is significant)
Fixed.
imageconfig.py:
617-642: A comment here explaining that the reason we're duplicating
data (the uri's in both the key and the value) is to make inserting the
dictionaries into plist easier.
I think in general, I'm finding this code a bit confusing. Perhaps a
block comment describing what data structure we're creating and why we
want to create it?
Ok, this is in the write-path - we do have an explanation of the data
structure down at line 803ish when we go to read it.
I'm struggling to understand why we're creating a
dictionary of lists of dictionaries. Especially when the lists of
dictionaries all get merged together at the end anyway.
The dictionaries don't get merged - we end up writing a list of
dictionaries to the backing configuration file (so for example, we would
always keep a ssl_key and ssl_cert pair together in a single dictionary)
> Why not just
build plist from the start? I see the comment on line 607, but that's
not covering *why* we need this 3 layers of structure.
The original intent was to try to store as basic an object structure as
possible in the pkg5.image file so that file remains easy for humans to
read. The rationale for using a list of dictionaries was discussed at
http://mail.opensolaris.org/pipermail/pkg-discuss/2012-March/028882.html
That said, I've expanded the comment the comment both here, and in the
read_publisher(..) method.
898-899: Could you reformat these lines to make it clearer a list
comprehension is happening?
Sure.
1223: How could we get here w/out proxy_url being set?
Any time we're not using the system repository in an image we can have
no proxy_url. That is, a BlendedConfig object with 'use-system-repo'
set to False calls __merge_publishers with a NullSystemPublisher sys_cfg.
[ the next question ought to be: why are we calling __merge_publishers
in that case - I'm not sure ]
1231: I'm concerned about this line given that, generally, this code
looks like it supports multiple proxies (even if we're not exercising
that functionality yet). Specifically, I think we want to check whether
each proxy in the list is a "system" proxy, rather than just checking
the list contains exactly one item with the "system" proxy in it.
That's reasonable, though there should only ever be a single system
proxy. Either way, I've changed it so that it looks in the list for a
single system-proxy and replaces it with the actual system-repository
netloc and port. Attempts to use an 'system' ProxyURI that hadn't been
fixed would show themselves pretty quickly (since '<sysrepo>' isn't a
valid URL)
publisher.py:
line 88: Given bug 7140770, do we really support https proxies? (or are
you fixing that bug as part of the wad?)
Good catch, I'd forgotten about that. I'll defer that fix for now.
line 147: indent looks too far
294: I feel like this should be comparing against self.__uri ... but I'm
not sure why I think that. So, take it or leave it as a suggestion.
Sure.
301-304,314-319: Rather than doing isinstance(self,ProxyURI), why not
set "supported_schemes" (or pick a better name) as a class property
that's set to SUPPORTED_SCHEMES in RepositoryURI and
SUPPORTED_PROXY_SCHEMES in ProxyURI. Then you can just check if
self.__scheme not in self.supported_schemes. If the issue is with
raising the right exception, there's no reason that the exception
couldn't also be a class property (__unsupported_scheme_exception for
example).
That's nicer, but doesn't get me past the misc.valid_pub_uri(..) check
which needs to be more relaxed when checking for proxies, I've left it
as is for now, but I'd be happy to clean this up later.
343: Instead of have _override_uri, why not just store the uri in
self._uri, instead of self.__uri?
Just safety - I wanted to be more strict about the ways in which the
__uri member could be modified.
382: inserted extra white space
423: Why do we first set __system to None?
No reason, fixed.
483: nit, """ at end of previous line please. 536, 568 too if it fits.
Yep.
487: could you expand the comment to explain why? Also, it seems strange
(at least needing a comment) that we take proxy as an argument to init,
then just ignore it.
I can't see any reason for that. Fixed.
engine.py
138, 535, 1129: nit about """ again
(this is in repo.py now)
127-131: This seems to be defined identically in many (every?)
subclasses ... why not just define it once here?
No reason, the same is true of get_url()
2267, 2285: typically there'd be a blank line between the docstring and
the first semantic line
Fixed.
stats.py
59: nit about """ again
91, 111: given that key() is being called below, I think these are now
TransportRepoUris, so the docstring should be updated. Ideally the
variable names would be changed as well to reflect that, but I don't
have to see that changed.
Thanks, I thought I'd got all of those - fixed. I'll leave the variable
names alone though.
transport.py:
490: nit about """ again
Fixed.
485-506: I admit I'm not thrilled by this factoring of the code. I think
I'd be more comfortable with fromrepouri knowing what info a
TransportRepoURI cares about, and have it return a list of
TransportRepoURIs. This function could then just take care of flattening
the lists it got back into a single list. Is that a change that could be
made easily? I'm concerned that this isn't the obvious place to look
when we want to add more properties.
Yes, that's straightforward, and I agree with you - here's not the right
place. Making TransportRepoURI.from_repouri() take a RepositoryURI
object and return a list of TransportRepoURIs is better. Fixed.
2831, 2846: could we have an assert of some kind to make sure that we're
only dealing w/ file repos in these methods?
I guess we could - that makes me uneasy though, for now these are the
only consumers, I'm not sure we'll always have that guarantee.
misc.py:
2361+: If I'm reading this right, then if I set 'http_proxy' in my
environment, that's only going to be applied to uri's that are already
being proxied, just with a different proxy. Do I have that right? That
seems sorta busted to me...
Nope, it's only important to override an existing proxy set for a given
origin - otherwise, the default behaviour of libcurl result in it using
the proxy environment variable (as it does in the gate today).
However, get_runtime_proxy also gets used when dumping transport stats,
and that was being impacted by what's here (in that if a proxy isn't set
on the origin, we weren't correctly printing that a proxy was in fact used)
t_pkg_sysrepo:
568-576: Not a big deal, but these might be easier to debug if they're
failing if these were assertEqual's to a specific line in the output,
which would also ensure the publishers were properly ordered.
Good idea - I've added that.
t_sysrepo:
2: Just curious, where'd that come from? (It seems to be in a selection
of other files in the gate, so I guess it's not a big deal, but it's a
bit strange it's only in some of them...)
It's because there's some non-ascii text at line 701, introduced to
ensure the sysrepo_p5p support can cope with unicode paths so we need to
declare the encoding of the python source.
305: line wrap?
Yep, fixed.
933: Why do another image create here?
Good question, probably something left over from debugging when I was
running into:
7177991 system-repository could be more graceful in the face of broken
remote proxies
I've removed it that image-create.
Thanks again for the comments - I believe I've covered all of them but
I'll wait till Friday evening my time before putting back in case anyone
else has thoughts on the updated webrev.
cheers,
tim
_______________________________________________
pkg-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
