On Tue, Jun 21, 2011 at 07:55:02PM -0700, Brock Pytlik wrote:
> On 06/21/11 07:06 PM, Edward Pilatowicz wrote:
> >hey all,
> >
> >i've got another wad of misc zones/li bugfixes:
> >
> >http://mcescher.us.oracle.com/export/ws/pkg.bugfix2/webrev
> >
> >18349 linked image code needs to be more resilient in the face of zoneadm
> >failure
> >18550 zone attach should reject entire if necessary
> >18578 p2v - cannot install s11 images with embedded zones
> >
> >thanks
> >ed
> >_______________________________________________
> >pkg-discuss mailing list
> >[email protected]
> >http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
> General comment:
> I find this code fairly hard to review because parameter names
> without obvious interpretations don't have any comments or
> docstrings explaining them (for example li_md_only). Adding
> docstrings for the parameters would be a nice addition.
>
li_md_only (and other parameters) are documented in the API. for
example:
'li_md_only' don't actually modify any packages in the current
images, only sync the linked image metadata from the parent
image.
i hate cut and paste duplication so instead i've added the following to
the attach_child and sync_child docstrings:
For descriptions of parameters please see the descriptions in
api.py`gen_plan_*"""
> There seems to be a lot of renaming going on for reasons I couldn't
> really infer (f -> ferrout, pkg_errout -> errout).
>
consistency. i added stderr capture to invocations of commands other
that pkg, so pkg_error out no longer seemed appropriate since i wanted
all the subcommand stderr capture file variables to have the same name.
> I had a hard time lining up the changes I was seeing with the bugs
> listed above, but here's what I came up with:
> 18349: zone.py lines 162-177 and the change from SubprocessError to
> LinkedImageException seem related to this, but I'm left wondering
> what happens if /etc/zones exists, and is populated, but populated
> with garbage? From what I'm seeing, it looks like in that situation,
> we'd print a nice error message, but it's not clear to me how these
> changes would allow a 'pkg fix' to be run on the GZ when it couldn't
> before.
>
if /etc/zones exists, but the contents are totally screwed up the zone
commands will fail. (now we'll report it without a stacktrace, which is
nice.) there is really no way that i can verify all the data that zone
commands access to verify that they'll always work. this is a general
best effort.
> 18550: this seems to be covered by the change to attach where
> --reject is optionally provided plus the addition of the reject_list
> parameters in common.py.
>
yep.
> 18578: I think this is the -I change in p2v.sh...
>
yep. along with the index file rm/touch in p2v
> The rest of the changes I have a hard time understanding how they're
> fixing these bugs.
>
> linkedimage/common.py:
> I wasn't sure why lin had been removed from a bunch of functions.
> I'm assuming it's because it's only needed in __detach_child, and it
> can be inferred from lic. Is there ever a situation where we'd want
> to use a different name than a child thought it had?
>
that should never happen.
also, these changes are related to 18550. i had to modify the sync path
through these functions because an attach in a parent image turns into a
sync in a child, and we need to be able to pass reject_list through from
the attach to the recursive sync. previously there was no way to do
this. so i while adding that parameter (by switching to **kwargs) i
removed up the unnecessary parameter.
> t_linked_image:
> Having a test that the -I option (if it's needed) allows fix and
> other commands to work in the face of a broken /etc/zones seems
> useful.
>
agreed, i didn't do this because it would require additional refactoring
linked image initialization. but i've gone ahead and added this in.
previously we would run commands when initialize the Image object, which
the client does before it even parses -I. so i've refactored some of
this stuff and made it more robust in the face of failures.
> I looked over the shell files but I have no feeling that I'd notice
> anything wrong w/them, someone else needs to review those bits.
>
sure.
> p2v.sh:
> A comment explaining why the -I is necessary would be nice. At first
> glance, since we know we're in a ngz, I'm surprised a -I is needed
> (and by extension, if we ever did support images which link to the
> zone root image, like user images might, I would've thought we'd
> want to propagate the change in variant setting.)
>
so all the brand callback scripts run in the global zone. also, when we
initialize the image object on the zone we're p2v'ing, we have no way of
knowing that we're operating on a ngz image because the zone variant may
be set to global (since we haven't changed it yet). hence the need for
-I.
> Just a random thought that popped into my head but is there any
> worry about making sure to change the variant in all the boot
> environments that might have come along from the physical system, or
> is only one specific BE moved during p2v?
>
right now p2v only operates on a single BE.
thanks for slogging through this.
the webrev has been updated.
ed
_______________________________________________
pkg-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
