Skip to main content

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

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

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/

ed

On Sun, Aug 04, 2013 at 07:26:41PM -0700, Shawn Walker wrote:
> On 07/26/13 18:28, Edward Pilatowicz wrote:
> >thanks for the quick review bart.
> >replies are inline below.
> >
> >a new webrev is available at:
> >     https://ips.java.net/webrev/edp/15743323.2/
>
> doc/client_api_versions.txt:
>   general: none of the sentences following the first
>     in each paragraph are indented as the other
>     entries in this file are
>
>   line 16: s/so we're/we're/
>
>   line 19: s/./;/
>
>

all fixed.

> 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.

>   line 4850: s/"values"/"value"/
>

fixed.

> src/man/pkg.1:
>   line 1739: What made you choose to have it list all facets,
>     instead of making it list only the masked ones or having
>     it be an option that combined with '-a' or '-i' or the
>     default output?  I'm not saying this approach is wrong,
>     I'm just looking for reasoning and thoughts.
>
>

oops.  I don't actually list all facets.  the -m option simply adds
masked facets to the output, it can safely be combined with '-a' or
'-i'.  i've updated this to read:

    Include masked facets in the output. Include a column indicating
    which (if any) facets are masked.

> src/man/pkg.5:
>   line 1203: s/.  /. /  (man page convention is to use one space)
>
>

fixed.

> 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."""

>   line 629: s/that actions has/those actions have/
>
>

fixed.

> 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.

> src/modules/client/imageplan.py:
>   line 352: I'd feel safer with range(2); we'd be guaranteed to never
>     infinite loop.  Then you could put an assert after the loop to
>     ensure we never retried more than we expected.
>

i realized that the while() loop part is unnecessary, so i've changed
this to be:

        try:
                return solver_cb()
        except api_errors.PlanCreationException, e:
                ...
                ignore_inst_parent_deps = True
                return solver_cb()

>   line 398: extra newline
>
>   line 462: s/Solve./Solve;/
>

fixed.

>   lines 572-601:
>     What about pkg change-facet for a masked facet?
>
>     In other words, if I make a local facet change, but all
>     affected facets are already masked, and --reject wasn't
>     used, then that should always be the fastpath.  That
>     doesn't seem to be accounted for here.
>
>     Likewise, if I make a local facet change, but I've already
>     inherited that effective setting, I see no need to invoke
>     the solver or load package data.
>
>     If that looks too complicated (wildcards could be a problem?)
>     then file an RFE for that.
>

masked facets don't have any impact on an image's package contents, so a
facet change that only affect masked facets is really just an update to
the pkg5.image file.

the case you're referring to is handled correctly, but not by invoking
__facet_change_fastpath() (since facet_change is False).

if we're only changing masked facets, then in plan_change_varcets()
we'll fall through to __plan_install_solver() (because use_solver
defaults to true), and in __plan_install_solver() we'll take the fast
path ("the solver is not necessary") out.

>   line 654: Is this another bug fix?  If so, oops.  Thanks for that.
>     A separate bugdb entry might be nice to spotlight that.
>

yep.  i filed:
    17272481 pkg change-{variant|facet} fast path doesn't account for --reject

>   general: thanks for the fastpath cleanup; that's better now;
>     wouldn't have thought of arranging it that way
>

np.

>   line 1906: s/=/ = /
>

fixed.

>   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.

> src/modules/client/linkedimage/common.py:
>   line 3213: s/package wide/package-wide/
>

fixed.

>   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'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.

>   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.

> src/modules/client/plandesc.py:
>   line 547: No _() concerns?
>
>   line 750: s/this the/this is the/ ?
>
>

all fixed.

> 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.

>   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.

>
> src/tests/cli/t_pkg_linked.py:
>   line 1302: s/childred/children/ ?
>
>   lines 1634, 1688, 1698, 1711, 1763, 1773, 2324, 2394, 2403:
>     s/we/when we/ ?
>
>   lines 1245, 1312, 2532, 2869, 2871, 3131: s/it's/its/
>

all fixed.

>   general: I did not look at every test in detail, I only looked
>     to see if you had sufficient coverage.
>

totally understandable.  thanks for looking all this over.

>   Also, what happens in the situation that an admin disables the
>   version-lock facet for the cacao-incorporation and then updates
>   entire with a specific version?  I'm guessing the install holds
>   don't get relaxed.  If so, that's ok; same thing happens now.
>   We should talk to Bart about whether install-hold relationships
>   should always apply even if the incorporate dependency is faceted
>   out.
>

since the cacao-incorporation package doesn't have a parent dependency
on itself, we won't relax its install hold.  the change i introduced
for relaxing install holds only affects packages in child images which
have a parent dependency on themselves.

> src/tests/cli/t_pkg_varcet.py:
>   line 624: s/uninstalled/uninstall/ ?
>

fixed.


[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