Skip to main content

[pkg-discuss] Re: review request: linked image facet inheritance

  • From: Shawn Walker < >
  • To:
  • Cc: Edward Pilatowicz < >
  • Subject: [pkg-discuss] Re: review request: linked image facet inheritance
  • Date: Tue, 06 Aug 2013 17:00:17 -0700

On 08/06/13 09:33, Edward Pilatowicz wrote:
replies are inline below.

a new webrev is available at:
        https://ips.java.net/webrev/edp/15743323.3/

a diff against the previous webrev (145 loc) is at:
        https://ips.java.net/webrev/edp/15743323.3.diff/

My replies are against the diff.

Be sure to file a separate bug just for the man page changes for Alta.

ed

On Sun, Aug 04, 2013 at 07:26:41PM -0700, Shawn Walker wrote:
On 07/26/13 18:28, Edward Pilatowicz wrote:
...
src/client.py:
   line 4745: I see you accounted for list_masked here, but
     we don't actually have any command line options for it
     or do anything with it.  Any need to keep this?


list_facet() and list_variant() both use the common options processing
and both use the same options table definition, client.py`opts_list_varcet.
so i couldn't add list_masked to just list_facet() without creating
separate opts_list_facet() and opts_list_variant() options tables.

But 'opts_list_varcet' is just a list. It seems like you could easily just do this and then drop line 5846 and the list_masked from list_variant():

opts_list_facet = \
    [opts_cb_varcet] + \
    opts_list_varcet + \
    [
    ("list_masked",  False),
]

Right?

...
src/modules/actions/generic.py:
   line 628: They're really unnecessary because we don't need them
     for conflict checking, right?  Perhaps it would be useful to
     state that as currently you might be left wondering why they're
     unnecessary.


i know the stripped action cache is used for conflict checking but i
wasn't sure if that was its only purpose, or it was used for anything
else (like pkg verify, fix, etc).

i updated the comment to read:

                 """Strip actions of attributes which are unnecessary once
                 those actions have been installed in an image.  Stripped
                 actions are saved in an images stripped action cache and used
                 for conflicting actions checks during image planning
                 operations."""

That's fine.

...
src/modules/client/api.py:
   line 894: A *brief* comment explaining why it's ok to ignore wildcard
     matches and fallback to _match_src might be helpful.



i've added the following comment:

                                 # this facet is not set (directly or 
indirectly
                                 # via wildcards) so the system will assign it
                                 # a default value.

Here's why that's confusing to me -- the condition that's being checked for is "if name not in facets"; but doesn't that only return True for exact entries and not wildcard matches?

...
   line 3545: Oh...how did this manifest itself?



this was one of those things that really annoyed me.  ;)

if you use the solver to evaluate facet changes, we'll list every
package in the image as changing, even if they are not.  (your fast path
optimization allow us to avoid this problem if we avoid the solver, but
that doesn't always happen.)

so here's an example of what happens without this change when we do a
change-facet operation on a machine with 4 zones (when we recurse we do
a sync-linked):

---8<---
# pkg change-facet -n \
     facet.version-lock.consolidation/osnet/osnet-incorporation=false 2>&1 | \
     grep "Packages to change"
             Packages to change:   1
|        Packages to change: 244
|        Packages to change: 244
|        Packages to change: 281
|        Packages to change: 281
---8<---

with this optimization the "Packages to change" for each zone becomes 1.

also, having lots of empty pkg_plan objects unnecessarily bloats the
serialized plan description object.  by stripping out empty pkg_plan
object for a small-server zone the serialized plan description went from
about 175 Kb to 98 Kb.

Cool; I was a little leary of pkg fix or pkg revert cases being accidentally zapped by this, but on further inspection, it seems safe enough.

...
   lines 3219-3220: Using the 'known' catalog from the image here
     should be faster and use less memory than using the manifest.

i don't understand what you mean.  are your suggesting using different
interfaces to iterate over the manifests and actions in place of
get_manifest() and gen_actions*()?

I was suggesting using the catalog interfaces instead of the Manifest interfaces; for an example, see lines 4521-4528 in image.py.

But this is really just potential optimisation work, so you could file an ER for that to avoid delaying pushing your changes.

     I'd check; also, you really care about faceted actions not for
     the current variant?  Or was that just to avoid variant change
     issues?


i care about all facet values regardless of what they evaluate to, and
since "excludes" evaluates facets and variants at the same time i'm
forced to ignore variants (because i can't ignore things based on
facets.)

i've thought about it a bit, and technically i probably should be
filtering based on variants, but given that (as i stated above) variant
and facet filtering are combined, i'd need to jump through some extra
hoops to do variant filtering (and then i'd need to write some test
cases for it, etc).  given our current use of faceted dependencies i
don't think this will cause any problems, but let me know if you want me
to fix it and i will.

That was my conclusion as well, which is why I didn't object specifically :-)

   line 3268: Same as above; I'd expect it to be better to use catalog
     instead.  Also not thrilled about adding is_constrained() to
     Manifest; seems awfully specific to solver / linked image stuff.
     And the name is fairly generic for something specific to parent
     dependencies.  I'd suggest "is_own_grandfather()" but that would
     be silly :-)



i changed is_constrained() to try and make it more generic.  i renamed
it to:

     has_dependency(self, fmri, dtype, excludes=EmptyI)

it returns true if the manifest has a dependency of the type specified
that on the specified fmri.

Ok, I'll live with it ;-)

...
src/modules/facet.py:
   lines 46-50: I want to make 'facet.debug' False by default in the
     future; do you foresee any issues with the changes made here
     with that?


i don't think so.  the tricky bit with doing this would be deciding if
you actually want facet.debug.* to be present within the facet
dictionary.  (in which case what happens when a user un-sets it, etc.)
my guess is that this would best be implemented via hard coded behavior
in the Facet object and _allow_facet().

in either case, i'm assuming we'd still report SYSTEM as the source for
any facets which match the facet.debug.* pattern and don't match any
other user specified patterns.

   lines 278-280: You saw this right?  No concerns?


yes, i saw this and i looked at _allow_facet() and i have no concerns.

essentially, i haven't changed the behavior of __getitem__() directly.
what i did change was how we manage __keylist, since inherited facets
are now prioritized over local facets.  but this is ok because both
__getitem__() and _allow_facet() use __keylist to do facet matching, so
the change affects both of them equally.

   lines 357-380: Since this is only being used for incorporate
dependency actions, this being written in Python is probably fine.
But if we were to ever apply this to more than just those, I'd
rather this be rewritten in CPython just as the existing facet
matching is done today.  It really does make a big difference.


yeah, i was trying to avoid doing this in C.  ;)

my desktop has 1501 packages installed and 4305 installed incorporate
dependency actions.  i'm guessing i won't see a large perf boost with
that limited number of actions, but i agree if this function is ever
used on larger sets of actions then we would want to re-write it.

   lines 395, 408: No way to raise the exception directly instead of
generating on synthetically?

   line 491: s/read only/read-only/

   line 497: Insert newline above please.



all fixed.

src/modules/manifest.py:
   lines 859-864: This is a hot path; it's unfortunate that we've had
     to add attribute filtering here.  To mitigate that, setting
     attr_filter=None and then doing a boolean check on attr_filter
     might be faster.  To test, you could add something to
     tests/perf/actionbench.py or create a new 'manbench.py'.


i didn't realize this was a hot path.  we don't have to do the filtering
here.  to avoid slowing this down i've backed out my changes to
gen_actions_by_type() and created a new function
gen_filtered_actions_by_type(), that simply calls gen_actions_by_type()
and filters the results.  so only new callers of new function
gen_filtered_actions_by_type() will incur any extra filtering overhead.

Cool. I know it's a bit of code duplication, but I feel much better about this implementation.

   line 870:   Do we really need to check for programmer error here?
     Wasn't this primarily useful for debugging?


it would have saved me some debugging time so i added it.  i assume
you're concerned about performance here since this is a hot path?
i've gone ahead and removed it.

Otherwise, everything seems fine.

-Shawn



[pkg-discuss] Re: review request: linked image facet inheritance

Shawn Walker 08/05/2013

[pkg-discuss] Re: review request: linked image facet inheritance

Edward Pilatowicz 08/06/2013

[pkg-discuss] Re: review request: linked image facet inheritance

Shawn Walker 08/06/2013

[pkg-discuss] Re: review request: linked image facet inheritance

Edward Pilatowicz 08/07/2013

[pkg-discuss] Re: review request: linked image facet inheritance

Shawn Walker 08/07/2013

[pkg-discuss] Re: review request: linked image facet inheritance

Edward Pilatowicz 08/08/2013
 
 
Close
loading
Please Confirm
Close