On 01/21/12 00:26, Edward Pilatowicz wrote:
hey brock,
thanks for looking at this. an updated webrev is at:
https://cr.opensolaris.org/action/browse/pkg/edp/pkgplan_cleanup2/webrev
19137 package plan object should be easier to serialize
more comments are inline below.
ed
On Fri, Jan 20, 2012 at 07:17:47PM -0800, Brock Pytlik wrote:
On 01/20/12 15:06, Edward Pilatowicz wrote:
hey all,
could i get a review for:
https://cr.opensolaris.org/action/browse/pkg/edp/pkgplan_cleanup/
19137 package plan object should be easier to serialize
the changes should be pretty strait forward and boring. the main
interesting points are:
- stop caching "image", "check_cancelation", and "progtrack" within
pkgplan objects, instead we now pass pointer to these objects to
member functions that need them.
- switch from using pickle to json for pkgplan action serialization
- the license.py actions loses it's preinstall() callback since we
dropped filelist support ages ago.
- add a test case that verifies action serialization during staged
execution in child images.
i pulled these changes out of my parallel linked images gate in an
effort to reduce the delta in that gate and make it easier to review.
(it should also have the added benefit of making it easier for me to
sync up with changes in the pkg gate. ;)
thanks,
ed
_______________________________________________
pkg-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
General comments:
[snip]
I don't understand why "image" needed to be removed from
PkgPlan.__init__ nor why it's better to pass it in to a bunch of
functions instead of having it set once. Just because the "image" is
set in the PkgPlan doesn't mean it has to be part of what's
serialized. To load a PkgPlan, you'd create one with the image you
want it to operate on (or whatever image you're going to pass into
evaluate or install or ...) and use setstate on it.
i don't think i understand the benefits to keeping an image pointer in
the PkgPlan. the disadvantages to keeping it are that it makes it more
difficult to load serialized PkgPlan objects and pass around PkgPlan
objects.
to elaborate on these two points. first, in my parallel linked image
gate i've added the ability to save an entire PlanDescription object to
disk (including PkgPlan objects) and then load them independently of
having an image. loading becomes more difficult if the PkgPlan wants to
cache an image pointer.
second, one of the eventual goals for gen_plan_*() api functions was to
have them return PlanDescription objects for all child linked images.
if PlanDescription objects reference PkgPlan objects then this becomes
difficult since the parent doesn't have an Image object for those child
images associated with the PlanDescription (and associating them with
any current Image object doesn't make any sense).
I'll cover this below.
Since progtrack and check_cancel are only used in download (from
what I can see), I don't feel strongly about whether that should be
treated like "image" or only passed into the download function.
pkgplan:
111: Why is something being compared to the type of None? What else
can be in state[i] other than a string or None? Also, while it might
be ok in this one case, I'd prefer that isinstance was used instead
of comparing the types directly since if something was ever a
subclass of NoneType, an object of that type would fail this test
(which sort of brings me back to asking why the type is being used
at all).
151: Similar question, why isn't this just 'if state[i] is not
None:'? What else can be in the origin or destination fmri other
than None or a PkgFmri?
159, 161: Same questions about using type(None).
i did this because if you have an Action object and you try doing:
if src != None:
then you'll get an exception:
---8<---
File
"/export/ws/pkg.pkgplan_cleanup/proto/root_i386/usr/lib/python2.6/vendor-packages/pkg/actions/generic.py",
line 431, in __ne__
if self.name == other.name and \
AttributeError: 'NoneType' object has no attribute 'name'
---8<---
because the __ne__() function for action objects assumes your comparing
the current action against another action (and not some other type of
object).
so i changed all getstate() and setstate() code to use:
type(XXX) != type(None)
because that makes the comparison independent of any __ne__()
implementation in the class your comparing against (and for consistency,
i did this for all the "None" comparisons in the *state()). if you'd
like me to do this some other way then just let me know what's
preferable.
Seems like you and Danek worked out something reasonable here.
pkg5unittest:
2800-2809: Shouldn't this also happen on update? I think it's
so any pkg operation that can install or update an fmri could use this
functionality (sync, update, change facet, and change variant), but
there are no other tests that would use this functionality so i didn't
add it.
probably better located in _api_finish since update also shares
this, and I think potentially change-variant/facet could as well
(though that would be one weird package). It's probably also worth
i've gone ahead and moved some of the functionality into _api_finish().
Thanks
filing a test suite bug to remind us that there are instances in the
test suite where we used self.pkg("... --accept ...") instead of the
api because the api interface was lacking and that those should be
changed to use the api interface now.
i'm not sure i understand. afaik the pkg client "api" never lacked any
functionality for updating license viewed/accepted status. is this not
the case? or are you saying that because the "pkg5unittest api" lacked
this functionality and other folks used pkg.1 interfaces instead?
Yep, the latter.
While the rest of the changes seem fine, I'd like to see another
webrev once image gets put back into PkgPlan as that caused a lot of
noise which made it hard to see the real changes here.
i think that the removal of image from the PkgPlan is beneficial for the
reasons i mentioned above.
i'm also failing to understand the benefits of keeping the image pointer
in PkgPlan. as best as i can tell the main objection to the removal of
image from the PkgPlan is that it's extra delta. is the concern over
this delta related to risk management in an update? would it help if i
isolated the image removal change into it's own wad? is there some
other reason for keeping it that i'm missing?
If it appeared that my main objection to removing the image as a member
of the PkgPlan is that it created too much delta, you have my apologies
for not being clearer. One of the standards I use when deciding what
kinds of changes to make to existing code is how much mostly spurious
change am I causing. If there's a good reason to make the change it
still happens, but it does make me stop and reconsider what I have in
mind. That was what my first email probably reflected.
If agreed with your goal of removing the image from the pkgplan object,
then I wouldn't have an issue with the amount of delta it caused,
unfortunately, I don't think that's the best way to achieve what you
want in the code.
So I saw several issues presented as to why the image needs to be
removed from pkgplan: a) it makes serialization easier b) you eventually
want to store PkgPlan's as part of the PlanDescription and pass them
around independent of images.
I think I've already addressed point a in my initial email. In the
webrev as it is today, you've replaced PkgPlan(image) with PkgPlan()
then two lines later pp.somefunction_that_takes_an_image_as_an_argument.
So I assert my original suggestion still works.
Now that I understand the larger picture though, I don't think the right
thing to do is to include PkgPlan's in the PlanDescription. For
starters, that would make the PkgPlan objects a part of the API just
like the publisher object is. In addition, it would mean removing the
image from the PkgPlan and passing it as a separate argument to at least
10 different functions. To me, when something is being used as an
argument to that many functions, it's an indication that it's probably
something that should be part of the object rather than a parameter to
functions. Also, if you pass an image to functions independently, then
that suggests that one could pass a different image to evaluate than to
download, which would only cause headaches.
Instead of removing the image parameter and making the changes you've
proposed, I suggest that we create a PkgPlanDescription object, whose
sole purpose is to describe a plan, but never to carry out actions on an
image. This allows clean separation of the description of a plan for
outside consumers (like the parent image or displaying to the user) from
the objects needed to carry out the plan (which must have access and
knowledge of the image to manipulate). This is identical to the
separation we made between the ImagePlan and PlanDescription classes and
I think the reasons we made that split hold equally well for PkgPlan's
and PkgPlanDescriptions.
Thanks,
Brock
_______________________________________________
pkg-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
