Code review request for pkg set/unset publisher bug fix.

Hi there,

Post by Thejaswini
15771543 set-publisher -p error message difficult to understand when publish
- The fix is that the error message is made more clear.
15769004 pkg unset-publisher could use a progress tracker
- Here I have added progress tracker logs which would appear
while deleting the publisher metadata.
https://cr.opensolaris.org/action/browse/pkg/tk241774/15771543/

src/client.py

line 4504, it's ok to delete this blank line I think.

src/modules/client/progress.py

line 1205, why are we calling a method "_load_metadata_output" when
we're actually removing metadata. I think it would make more sense to
call the method on line 1960, 2355 "_remove_metadata_output"?

line 1962, you don't need the continuation '\' character because you're
using them within parentheses.

General: how long does metadata removal usually take? Rather than a single:

"Removing metadata ... done"

prompt, would it be useful to have a rolling progress, showing how we're
doing when removing bits of metadata instead? eg.

"Removing metadata 1/100"

In the case of the example in the bug report, 16 minutes is a very long
time to be waiting with no feedback other than, effectively "I'm busy".

cheers,
tim

Shawn Walker

2013-02-08 19:12:35 UTC

On 02/07/13 19:15, Tim Foster wrote:
...

"Removing metadata ... done"
prompt, would it be useful to have a rolling progress, showing how we're
doing when removing bits of metadata instead? eg.
"Removing metadata 1/100"
In the case of the example in the bug report, 16 minutes is a very long
time to be waiting with no feedback other than, effectively "I'm busy".

The problem with that idea is that we don't know how much metadata there
is to remove and since we're not actually removing the metadata
piece-by-piece (we use rmtree) we can't provide meaningful progress for
each metadata item as it is removed.

The progress tracker should have a spinner that outputs from
time-to-time for cases like these (I thought it did?).

-Shawn

Tim Foster

2013-02-10 21:15:40 UTC

"Removing metadata 1/100"

We do have some visibility into what we're removing, in some parts of
the code[1] we're iterating over several directories looking for
candidate directories to remove, calling shutil.rmtree when we need to.

Some shutil.rmtree calls will take longer than others, I'd rather a
counter telling me how many directory-trees of content to consider
removing are left, than just a random spinning '|/-\|-\'.

You're right in that when we're removing an entire publisher (line
3422), we've no visibility into the metadata removal, but not with the
rest of the code (perhaps in that case, metadata removal is so fast it's
not worth considering emitting progress?)

cheers
tim

[1]
http://src.opensolaris.org/source/xref/pkg/gate/src/modules/client/image.py#3424
ish

Thejaswini

2013-02-11 11:23:22 UTC

Shawn, Tim, Thanks for looking into this and providing your comments.

In the case of output of the form:
"Removing metadata 1/100"
writing code to compute the goal, i.e. the exact no. of directories
removed, is not worth as it will again add on to the performance.

As per the bug, the no. of metadata to be removed is huge:

***@mcescher$ du -sh /a/var/pkg/publisher/solaris
3.2G /a/var/pkg/publisher/solaris
***@mcescher$ find /a/var/pkg/publisher/solaris -type f | wc -l
107422
***@mcescher$ find /a/var/pkg/publisher/solaris -type d | wc -l
2385

In such cases, as suggested by Shawn something like below would be good
enough.
# pkg unset-publisher solaris
PHASE ITEMS
Updating package cache 1/1

IMO we could add an additional jobitem and use it while calling the
progress tracker which would display "Removing package metadata" instead
of " Updating package cache".
The output would look something like:
# pkg unset-publisher solaris
PHASE ITEMS
Removing package metadata 1/1

Is it fine to add this Job item?

Regarding the changes for 15771543, I would remove the check present and
see if additional changes needs to be done.

Will send the webrev once I have the changes tested.

Thanks,
Thejaswini K

Oracle
Thejaswini K
Revenue Product Engineering (RPE), Systems
Phone: +91 8066927709 | Mobile: +91 9663324594
ORACLE India | Off Langford Road | Bangalore | 560025
Green Oracle <http://www.oracle.com/commitment> Oracle is committed to
developing practices and products that help protect the environment

"Removing metadata 1/100"

We do have some visibility into what we're removing, in some parts of
the code[1] we're iterating over several directories looking for
candidate directories to remove, calling shutil.rmtree when we need to.
Some shutil.rmtree calls will take longer than others, I'd rather a
counter telling me how many directory-trees of content to consider
removing are left, than just a random spinning '|/-\|-\'.
You're right in that when we're removing an entire publisher (line
3422), we've no visibility into the metadata removal, but not with the
rest of the code (perhaps in that case, metadata removal is so fast
it's not worth considering emitting progress?)
cheers
tim
[1]
http://src.opensolaris.org/source/xref/pkg/gate/src/modules/client/image.py#3424
ish

Thejaswini

2013-02-19 08:09:02 UTC

Hi,

I have made the suggested changes and uploaded the webrev
@
https://cr.opensolaris.org/action/browse/pkg/tk241774/15771543-rev02/webrev/.

1. "15769004 pkg unset-publisher could use a progress tracker"
The output now looks like:
***@S12_13:~# pkg unset-publisher solaris
PHASE ITEMS
Updating package cache 1/1

2. 15771543 set-publisher -p error message difficult to understand when
publish
#pkg set-publisher -p /myrepo/repo1 solaris ---- adds the origin.
#pkg set-publisher -p /myrepo/repo1 ---- Also adds the
origin.

I have modified 2 testcases to pass after my changes.

Thanks,
Thejaswini K

Oracle
Thejaswini K
Revenue Product Engineering (RPE), Systems
Phone: +91 8066927709 | Mobile: +91 9663324594
ORACLE India | Off Langford Road | Bangalore | 560025
Green Oracle <http://www.oracle.com/commitment> Oracle is committed to
developing practices and products that help protect the environment

No, it should add the new origin to the publisher, it should not
remove any origins.
--
Shawn

This is with regard to bug "*Bug 15771543 - SUNBT7143562
set-publisher -p should simply update existing publisher sources"
PUBLISHER TYPE STATUS P LOCATION
solaris origin online F
file:///root/myrepo/*repo1*/
And then I issue the command "pkg set-publisher -p /root/myrepo/repo2
solaris"
this should update the location of the publisher configured.
i.e.
PUBLISHER TYPE STATUS P LOCATION
solaris origin online F
file:///root/myrepo/*repo2*/
Please correct me if I am wrong.
Thanks,
Thejaswini K.
<oracle_sig_logo.gif>
Thejaswini K
Revenue Product Engineering (RPE), Systems
Phone: +91 8066927709 | Mobile: +91 9663324594
ORACLE India | Off Langford Road | Bangalore | 560025
<green-for-email-sig_0.gif> <http://www.oracle.com/commitment> Oracle
is committed to developing practices and products that help protect
the environment

Post by Thejaswini
Shawn, Tim, Thanks for looking into this and providing your comments.
"Removing metadata 1/100"
writing code to compute the goal, i.e. the exact no. of directories
removed, is not worth as it will again add on to the performance.
3.2G /a/var/pkg/publisher/solaris
107422
2385
In such cases, as suggested by Shawn something like below would be
good enough.
# pkg unset-publisher solaris
PHASE ITEMS
Updating package cache 1/1
IMO we could add an additional jobitem and use it while calling the
progress tracker which would display "Removing package metadata"
instead of " Updating package cache".
# pkg unset-publisher solaris
PHASE ITEMS
Removing package metadata 1/1
Is it fine to add this Job item?
Regarding the changes for 15771543, I would remove the check present
and see if additional changes needs to be done.
Will send the webrev once I have the changes tested.
Thanks,
Thejaswini K
Oracle
Thejaswini K
Revenue Product Engineering (RPE), Systems
Phone: +91 8066927709 | Mobile: +91 9663324594
ORACLE India | Off Langford Road | Bangalore | 560025
Green Oracle <http://www.oracle.com/commitment> Oracle is committed
to developing practices and products that help protect the environment

"Removing metadata 1/100"

We do have some visibility into what we're removing, in some parts
of the code[1] we're iterating over several directories looking for
candidate directories to remove, calling shutil.rmtree when we need to.
Some shutil.rmtree calls will take longer than others, I'd rather a
counter telling me how many directory-trees of content to consider
removing are left, than just a random spinning '|/-\|-\'.
You're right in that when we're removing an entire publisher (line
3422), we've no visibility into the metadata removal, but not with
the rest of the code (perhaps in that case, metadata removal is so
fast it's not worth considering emitting progress?)
cheers
tim
[1]
http://src.opensolaris.org/source/xref/pkg/gate/src/modules/client/image.py#3424
ish

_______________________________________________
pkg-discuss mailing list
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss

Shawn Walker

2013-02-19 19:41:19 UTC

Post by Thejaswini
Hi,
I have made the suggested changes and uploaded the webrev
@
https://cr.opensolaris.org/action/browse/pkg/tk241774/15771543-rev02/webrev/.
1. "15769004 pkg unset-publisher could use a progress tracker"
PHASE ITEMS
Updating package cache 1/1

So one thing I noticed is that there is a distinct disadvantage to doing
this progress tracking in image.py; notably that simply doing a
'set-publisher' now outputs the progress message for updating the
package cache. That is not something we want.

So instead of placing the progress tracking in modules/client/image.py,
I think we should consider adding it to client.py in publisher_unset.
And instead of settings 'goal=1', we can also set goal=len(args) which
will be nice.

I didn't test this earlier, so I apologise, and because I suggested the
change, I can't "review" the change for modules/client/iamge.py.

I would like someone else's feedback on that change if possible.

-Shawn

Thejaswini

2013-02-21 07:56:32 UTC

The new rev 03 webrev is @
https://cr.opensolaris.org/action/browse/pkg/tk241774/15771543-rev03/

This includes:
For bug "15769004 pkg unset-publisher could use a progress tracker"
- As suggested by Shawn, I have added the progress tracker in client.py

For bug "15771543 set-publisher -p error message difficult to understand
when publish "
#pkg set-publisher -p /myrepo/repo1 solaris ---- adds the origin.
#pkg set-publisher -p /myrepo/repo1 ---- Also adds the
origin.

Please let me know your comments on this.
Thanks
Thejaswini K

So one thing I noticed is that there is a distinct disadvantage to
doing this progress tracking in image.py; notably that simply doing a
'set-publisher' now outputs the progress message for updating the
package cache. That is not something we want.
So instead of placing the progress tracking in
modules/client/image.py, I think we should consider adding it to
client.py in publisher_unset. And instead of settings 'goal=1', we can
also set goal=len(args) which will be nice.
I didn't test this earlier, so I apologise, and because I suggested
the change, I can't "review" the change for modules/client/iamge.py.
I would like someone else's feedback on that change if possible.
-Shawn

Shawn Walker

2013-02-21 16:30:43 UTC

Post by Thejaswini
https://cr.opensolaris.org/action/browse/pkg/tk241774/15771543-rev03/
For bug "15769004 pkg unset-publisher could use a progress tracker"
- As suggested by Shawn, I have added the progress tracker in client.py

Again, I cannot be your final reviewer, but I think you want the call to
job_add_progress *after* the call to remove_publisher() since you
haven't actually made any progress before that point.

-Shawn

Thejaswini

2013-02-22 05:42:20 UTC

Again, I cannot be your final reviewer, but I think you want the call
to job_add_progress *after* the call to remove_publisher() since you
haven't actually made any progress before that point.

remove_publisher() is the function which takes long. If job_add_progress
is called after this function then the progress tracker would not appear
until remove_publisher() returns.
And then again as reported the command would sit displaying nothing.

Is there a way where the tracker could display
/Updating package cache *0/*1 /before calling//remove_publisher() and
/Updating package cache *1/*1/ after remove_publisher() returns.

Thanks
-Thejaswini K.

Post by Shawn Walker
-Shawn

Tim Foster

2013-02-27 21:11:47 UTC

Post by Thejaswini

Again, I cannot be your final reviewer, but I think you want the call
to job_add_progress *after* the call to remove_publisher() since you
haven't actually made any progress before that point.

I agree - that would be a useful change to make.

Post by Thejaswini
remove_publisher() is the function which takes long. If job_add_progress
is called after this function then the progress tracker would not appear
until remove_publisher() returns.
And then again as reported the command would sit displaying nothing.

Yep.

Post by Thejaswini
Is there a way where the tracker could display
/Updating package cache *0/*1 /before calling//remove_publisher() and
/Updating package cache *1/*1/ after remove_publisher() returns.

I'm surprised it doesn't do that already -- I'd have thought that by
calling progtrack.job_start(...) that we'd print the first line above,
then once you've actually made progress, it prints the second.

(in your testing, are you giving the system enough work to do, such that
there's always a long enough time period for this job to complete?)

cheers,
tim

Shawn Walker

2013-02-27 21:19:43 UTC

Post by Thejaswini

Again, I cannot be your final reviewer, but I think you want the call
to job_add_progress *after* the call to remove_publisher() since you
haven't actually made any progress before that point.

I agree - that would be a useful change to make.

Yep.

I suspect this is because the JOB stuff is usually called within the
context of image planning.

But it still seems strange as the JOB interfaces appear to be usable
outside of that context.

My only concern here is that we're able to provide progress output here
without adding new interfaces; that makes this suitable for backport if
desired and keeps us from having to coordinate cross-consolidation.

-Shawn

Thejaswini

2013-02-28 10:24:55 UTC

Thanks Shawn, Tim.
Please find my reply inlined.

Post by Thejaswini

Again, I cannot be your final reviewer, but I think you want the call
to job_add_progress *after* the call to remove_publisher() since you
haven't actually made any progress before that point.

I agree - that would be a useful change to make.

Post by Thejaswini
remove_publisher() is the function which takes long. If
job_add_progress
is called after this function then the progress tracker would not appear
until remove_publisher() returns.
And then again as reported the command would sit displaying nothing.

Yep.

No it does not print anything after calling job_start()

Post by Shawn Walker
I suspect this is because the JOB stuff is usually called within the
context of image planning.
But it still seems strange as the JOB interfaces appear to be usable
outside of that context.
My only concern here is that we're able to provide progress output
here without adding new interfaces; that makes this suitable for
backport if desired and keeps us from having to coordinate
cross-consolidation.

I see in image.py that job_add_progress() is called before the action
is actually performed.
Looking at the definition of job_start(), it does not seem to perform
any printing operations.
I have made changes in job_start() so that it prints 0/1.
Webrev is @
https://cr.opensolaris.org/action/browse/pkg/tk241774/15771543-rev04/webrev/

- Thejaswini K.

Post by Shawn Walker
-Shawn

Shawn Walker

2013-02-19 19:32:30 UTC

The thing is, I don't know that we need to be specific; "updating
package cache" is accurate as the package data we're removing is stored
in the package cache.

-Shawn

Shawn Walker

2013-02-08 19:06:40 UTC

Post by Thejaswini
Hi all,
15771543 set-publisher -p error message difficult to understand when publish
- The fix is that the error message is made more clear.
15769004 pkg unset-publisher could use a progress tracker
- Here I have added progress tracker logs which would appear while
deleting the publisher metadata.
https://cr.opensolaris.org/action/browse/pkg/tk241774/15771543/

I think these should really be separate changesets instead of the same
one; they're not really related changes.

To add to that, I think the changes for 15771543 should not be made, or
rather, are the wrong set of changes to make.

Among the pkg(5) team, we discussed a long time ago whether -p should
simply accept new package sources, and the consensus was that now that
we support composition (and package signing), it should be safe to do
so. I meant to fix this in S11 FCS but failed to file an RFE for it (or
I can't find the RFE I filed).

There is little value in enforcing the current check, so I'd suggest the
right change is to instead delete lines 4489-4496 and 4500-4512 in the
original src/client.py. So, I'd repurpose the bug as "set-publisher -p
should simply update existing publisher sources".

As for the progress changes for 15769004, we shouldn't be adding new
methods to the progress tracker unless absolutely necessary. I'd
suggest using the generic JOB progress mechanism instead; see the
attached patch.

-Shawn

Shawn Walker

2013-02-08 19:10:11 UTC