On 06/21/12 15:59, Tim Foster wrote:
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
[snip]
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)
Right, the dictionaries don't get merged together, but the lists do. I
guess I'm wondering why this isn't just a list of dictionaries from the
start, instead of a dictionary of lists of dictionaries. Are origin_info
and mirror_info used after line 650 (I couldn't find anyplace where they
were)? If not, why create them outside of the loop?
> 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.
Sorry, maybe I wasn't clear enough. To be concrete, why not do the
following:
1) move plist = [] from line 646 to line 617.
2) delete lines 612 and 613
3) change every place where the code currently does
info_map.setdefault(r.uri, []).append(<mumble>) to plist.append(<mumble>)
4) remove lines 647 and 648.
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 ]
Ah. Yes... I'm sure I had a very clever reason for doing that at the
time ... I just have no idea what it is now...
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)
Thanks for making the change. I think I was concerned about the future
where we might have a publisher configured in the NGZ to go through the
systemrepo, and also connect through a second proxy directly.
[snip]
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.
Great thanks :)
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.
Well, the comment suggests to me that these methods can only validly be
used on file repos, otherwise ignoring the proxy value set on the origin
wouldn't be ok... Mostly I was suggesting putting what's in the comment
in as an assert. If you're concerned about adding an assert, then I'm
concerned about the comment.
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).
See, I think that's where I'm confused. If I have 2 publishers
configured, one with a proxied origin and one without, and now I do
http_proxy=http://foo.com, I think I'd expect connections for both
publishers, not just the one with a proxied origin, to be routed through
the foo.com proxy. I think that's not how this code works. Is there a
reason I'd only want one of the two publishers to get proxied via foo.com?
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)
Everything else looks good to me.
Thanks,
Brock
_______________________________________________
pkg-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
