Skip to main content

[pkg-discuss] Re: code review request: pkg dehydrate/rehydrate

  • From: Yiteng Zhang < >
  • To: Shawn Walker < >
  • Cc: , Danek Duvall < >, Bart Smaalders < >, Erik Trauschke < >
  • Subject: [pkg-discuss] Re: code review request: pkg dehydrate/rehydrate
  • Date: Wed, 25 Jun 2014 17:45:43 -0700

On 06/25/14 05:04 PM, Shawn Walker wrote:
On 06/24/14 15:00, Yiteng Zhang wrote:
On 06/23/14 04:08 PM, Danek Duvall wrote:
Yiteng Zhang wrote:

webrev: https://ips.java.net/webrev/yitezhan/pkg_hydrate-2/
client.py:

   - line 242: doesn't fix now get all the usual BE management flags
     (--no-backup-be, etc)?  If not, shouldn't it?

   - line 1627: if _op not in (PKG_OP_REVERT, PKG_OP_FIX, ...):

- line 1851ff: this seems like the wrong place for this; it should be
     done in the dehydrate/rehydrate API calls, or possibly deeper.

Worry that users/machines will interrupt the execution of dehydrate/
rehydrate. If so, the property of "dehydrated" should not be saved
because rehydrate depends on this property. So this is probably
a good place to put the code -- test the return code of __api_execute_plan
and save the property.

pkg.1:

   - There are a lot of changes here which are unrelated to hydration;
how
     and why did they get combined?

Alta answered this: these changes went into 11.2 but not into 12. I agree
to push these changes to 12 via a different changeset first.

pkg.5:

   - line 404: a couple of formatting and wording changes:
         This attribute is used to prevent a file from being removed
during
         a \fBdehydrate\fR operation.  The value of the \fBdehydrate\fR
         attribute can be \fBtrue\fR (the default) or \fBfalse\fR. If
it is
         \fBfalse\fR, \fBpkg dehydrate\fR will not remove the file.

api.py:

   - line 2221: "... to dehydrate on" -> "... to dehydrate"

api_errors.py:

- line 524, 697, 702: self.uncofigured_pubs -> self.unconfigured_pubs

   - line 700: " on." -> "."

image.py:

   Frankly, I'd rather see a boolean dehydrated property on each
publisher.
   I think that it makes more sense -- when listing the properties of a
   publisher, this is logically one of them, and having to make the
code go
   look in one place (the publisher object) for most of the properties
and
in another (the image) for the last one seems like bad architecture to
   me.

When do we want to mark the publisher, at the time we check the publisher
object before pkgplan creation of dehydrate/rehydrate, or after api
execution of dehydrate/rehydrate and create the publisher object again?
I think
the latter one makes more sense, and it also relates to the above problem:
where to put the code of saving property.

   That said, I still have comments on the code here.

- line 2371: using a mutable object as a default argument is bad form,
since once you change it, it's changed, even for future calls. Use
     None -- check_publishers() already does a boolean test on it, so
that
     should just work (unless check_publishers() can return its input
     instead of an empty list).

   - line 2374: "'publisher'" -> "'publishers'".  Also, this doesn't
     actually perform the dehydrate/rehydrate operation, so you should
say
     that the publishers are being marked, not that that's the action
you're
     performing.

   - line 2380: I'd do this with a try/except as well.

- line 2383: I'd use ast.literal_eval() here, instead; I think it'd be
     clearer.

   - line 2384ff: I'd use a generator expression or sets here:

         pubs.extend((p for p in publishers if p not in pubs))

     or

         pubs = list(set(pubs) + set(publishers))

     Similarly for the removal, but you'd have to use sets there.

   - line 2400: I don't understand -- "manifest files"? Perhaps just
     "remove the property entirely"?

   - line 2407: "yes" -> "so"; "exceptions" -> "exception"

   - line 2409: strike "added".

   - line 2412: Use a multi-line form:

         blah = [
             var
             for var in iter
             if test
         ]

   - line 2415ff: I'd probably do something like this:

         if not publishers:
             return configured_pubs

         unconf_pubs = [ pub for pub in publishers if pub not in
configured_pubs ]
         if unconf_pubs:
             raise ....

         return publishers

     I'm not sure I really like the way these two functions are
structured
     more generally, but I'm also not certain I have a better way of
doing
     it.

   - line 4490, 4491: why is this check here?

I knew api will have this check in execute(), but the old pkg fix had
this check at the
very beginning of method __repair() and __repair() also invoked
execute(). I inclined
to remove this check here though.

imageplan.py:

   - line 1324: do we want to count the number of installed packages,
or the
     number of installed packages that will get dehydrated?
- line 1331: use iterkeys() instead of keys(); it'll take less memory.

   - line 1332: is this a fast enough loop that it doesn't need any
progress
     tracking while we're in it?
     I wonder if __gen_hydrate_actions() should be a generator, that
yields
     its dictionary entries, rather than returninga dict. That way,
we can
     save some memory by not having that entire dict in memory at the
same
     time as the list of package plans; and we can update the progress
     tracker once for each manifest generation / pkgplan creation step.

-line 1324, line 1332

I will use a generator to yield fmri, manifest and its dictionary entries.

To count the number of installed packages that will get dehydrated at
the first place,
we have to call __gen_hydrate_actions to set the value of "goal" for
pt.plan_start.
And we will call __gen_hydrate_actions again for pkgplan creation.

So I think it is better to: count the number of all installed packages and
update the progress tracker once for each package's fmri retrieval;
also add progress tracking without incrementing tracker's nitems in the
loop
of pkgplan creation.

   - line 1389: I'd rename "pt" to "tracker" or "progtrack" or
something a
     bit more mnemonic.

- line 1364ff: this is another place that could stand to be de-looped:

         pubs = [pub for pub in pubs if pub not in dehydrated_pubs]

And the idiom for copying a list is to use a slice -- l[:] -- not a
     copy constructor.

   - line 1390: need a space before the open parenthesis.

   - line 1391: quotes around "pubs".  Is the mention of zone config
files
     (here and on line 1406) relevant anymore?  You're not checking for
     zones config files, you're checking for actions with
dehydrate=false,
     which may be more than just those.

   - line 1416, 1428: why do you need this test for file existence?
Seems
racy. Is it faster to do it this way than to just try to remove the
file during plan execution and ignore failure if it's already gone?

Without this test for file existence, dehydrate will always propose actions
to add and return an exit code 0. But if all the files are gone, dehydrate
should not add any action to the dictionary and should return an exit
code 4,
which stands for "nothing to do".


I realized another point you guys raised in the meeting: if a publisher is
dehydrated, installation of future packages should be installed dehydrated.
Please note that I implemented and prefer that installation of future
packages
are installed normally and its publisher is still dehydrated.

As we said in the meeting, the "principle of least surprise" applies here. All packaging operations for a dehydrated publisher should also be automatically dehydrated. We really need to change the behaviour here.

-Shawn

OK. I will change the behavior here.
I have two approaches in my mind but not sure which is better?
Take "pkg install" for example, one way is after "pkg install" installs
packages from a dehydrated publisher, we automatically call the
dehydrate api to dehydrate the packages; the other way is we do
the check in "pkg install" to see if it will operate on a dehydrated publisher,
and if so, we only install the files that will not be removed in dehydrate,
so the newly installed packages look like they have been dehydrated.

Yiteng


[pkg-discuss] Re: code review request: pkg dehydrate/rehydrate

Bart Smaalders 06/05/2014

[pkg-discuss] Re: code review request: pkg dehydrate/rehydrate

Yiteng Zhang 06/05/2014

[pkg-discuss] Re: code review request: pkg dehydrate/rehydrate

Yiteng Zhang 06/09/2014

[pkg-discuss] Re: code review request: pkg dehydrate/rehydrate

Yiteng Zhang 06/13/2014

[pkg-discuss] Re: code review request: pkg dehydrate/rehydrate

Yiteng Zhang 06/19/2014

[pkg-discuss] Re: code review request: pkg dehydrate/rehydrate

Danek Duvall 06/23/2014

[pkg-discuss] Re: code review request: pkg dehydrate/rehydrate

Yiteng Zhang 06/24/2014

[pkg-discuss] Re: code review request: pkg dehydrate/rehydrate

Shawn Walker 06/26/2014

[pkg-discuss] Re: code review request: pkg dehydrate/rehydrate

Yiteng Zhang 06/26/2014

[pkg-discuss] Re: code review request: pkg dehydrate/rehydrate

Shawn Walker 06/26/2014

[pkg-discuss] Re: code review request: pkg dehydrate/rehydrate

Danek Duvall 06/26/2014

[pkg-discuss] Re: code review request: pkg dehydrate/rehydrate

Shawn Walker 06/26/2014

[pkg-discuss] Re: code review request: pkg dehydrate/rehydrate

Danek Duvall 06/26/2014

[pkg-discuss] Re: code review request: pkg dehydrate/rehydrate

Shawn Walker 06/26/2014

Message not available

Message not available

[pkg-discuss] Re: code review request: pkg dehydrate/rehydrate

Yiteng Zhang 06/25/2014

[pkg-discuss] Re: code review request: pkg dehydrate/rehydrate

Shawn Walker 06/25/2014

[pkg-discuss] Re: code review request: pkg dehydrate/rehydrate

Shawn Walker 06/25/2014

[pkg-discuss] Re: code review request: pkg dehydrate/rehydrate

Danek Duvall 06/27/2014

[pkg-discuss] Re: code review request: pkg dehydrate/rehydrate

Shawn Walker 06/26/2014
 
 
Close
loading
Please Confirm
Close