Done and retested with the change.
Should the be final version. If I can get a couple of final sign-offs, I
will RTI this change.
https://cr.opensolaris.org/action/browse/caiman/ghart/IPS_Install_Progress_V4/
Thanks,
Geoffrey
On 06/08/12 10:37, Ethan Quach wrote:
On 06/08/12 04:30, Darren Kenny wrote:
Hi Geoffrey,
On 07/06/2012 23:12, Geoffrey Hart wrote:
Karen, Darren:
I know that both of you are now happy with the code change (assuming
Darren likes the latest webrev listed below). But I do have to ask a
question that might result in a slight change to this. Darren requested
that I make the logging INFO so it will appear on the console, but I'm
having second thoughts about this. Currently the console shows progress
with text like:
Install Phase 20958/86138
This allows the user in a clear fashion to see it progressing so we
know
it is not hung and we can see how fast it is progressing. By
allowing my
new logging to go to the console we see the output become more
cluttered:
Install Phase 23252/86138 14:59:12
Install
Phase .
.. 26% complete.
Install Phase 29649/86138 15:00:12
Install
Phase .
.. 34% complete.
Install Phase 33658/86138 15:01:12
Install
Phase .
.. 39% complete.
Install Phase 34629/86138 15:02:12
Install
Phase .
.. 40% complete.
Install Phase 35574/86138 15:03:12
Install
Phase .
.. 41% complete.
This is where you are seeing the mixed content of the
show_stdout=True and
the new logging you're doing.
I think that if there is no tty associated with the output, then it
shouldn't be generating the numbers x/X. Certainly we should handle this
case and only have one of them depending on the circumstance.
This is automatically handled by the AbstractIPS() class. It detects
if we have a tty or not and picks the appropriate progress tracking
class. The issue is that you've added some lines in
InstallFancyProgressTracker to output to stdout, which is adverse to
what it's used for. See it's comment:
class InstallFancyProgressTracker(progress.FancyUNIXProgressTracker):
''' Subclass of the IPS api's FancyUNIXProgressTracker; we leverage
that class's progress reporting, allowing it to output
straight to
stdout. The overridden methods we define allow us to capture the
parts of the progress that we want recorded to the install log.
Is there a good reason to have this extra logging to the console? Or
does it make more sense to just have it go to the log file and keep the
current look to the console?
If you are seeing the x/X type output on an AI console, where you're not
logged in, then I think that is strange, since I wouldn't expect it.
You need to ensure only one of these is output, depending on whether
there
is a tty - since without a tty, the x/X type output really doesn't work
correctly.
See above, and my other mail. Just revert the lines in
InstallFancyProgressTracker back to debug and things will be fine again.
-ethan
Thanks,
Darren.
Your thoughts?
Latest webrev:
https://cr.opensolaris.org/action/browse/caiman/ghart/IPS_Install_Progress_V3/
- Geoffrey
On 06/07/12 16:06, Karen Tung wrote:
The updated info on the bug looks OK.
I have no further comments.
--Karen
On 06/07/12 12:12, Geoffrey Hart wrote:
Sorry, looks like I didn't hit save on the bug. Updated now.
- Geoffrey
On 06/07/12 12:47, Karen Tung wrote:
Hi Geoffrey,
Sorry I overlooked your email too. I just looked at the updated
webrev.
The code changes look OK to me now.
However, the bug is still not updated. Can you update the bug?
Thanks,
--Karen
On 06/05/12 08:44, Geoffrey Hart wrote:
Updated webrev with suggested changes:
https://cr.opensolaris.org/action/browse/caiman/ghart/IPS_Install_Progress_V2/webrev/
On 06/05/12 02:01, Darren Kenny wrote:
On 01/06/2012 22:56, Geoffrey Hart wrote:
Yes we have everything downloaded already. I didn;t add
anything for
progress on the download phase since it already shows the
progress for
download on the console. The motivation for this fix is the 10
or so
minutes it takes for install where we do no logging to the
console or
log file.
OK, thanks, just wanted to be sure... :)
As for as displaying to the console, very good point. I will
correct
that to show status to the console.
Thanks.
So with this in mind, what are your thoughts on logging every
minute vs
logging after every 5 or 10 %?
Every minute is fine - so even if there is some long install task
(which I
hope there isn't anything longer than 1 minute TBH) it would still
show
some output so the user doesn't think things have stalled...
Thanks,
Darren.
Thanks,
Geoffrey
On 06/01/12 10:25, Darren Kenny wrote:
On Fri Jun 1 17:11:44 2012, Geoffrey Hart wrote:
Thanks Darren.
I will run this through pep8 as I make the other corrections
you mentioned.
I selected to log every minute rather than every 5% so there
would be
regular log entries. With network problems, it could be a long
delay
between entries if I logged every 5% of the way. Your thoughts?
I can see where you're coming from, but at this point - install
- we've
already downloaded everything, haven't we?
Or are you adding the progress at the download phase too -
I'm not
totally
sure of of the phases based on the names in the webrev...
BTW, does any of this appear on the console during an AI
install?
Ideally we want it to, so people will see progress without
logging in,
and I'm thinking that it would need to be at the INFO
level rather than DEBUG level for that to be the case.
Thanks,
Darren.
- Geoffrey
On 06/01/12 04:39, Darren Kenny wrote:
Hi Geoffrey,
Thanks for looking at this...
In general I'm happy with the approach, but have some minor
comments in
addition to Karen's:
- You could probably omit the need for the percent_complete
variable
totally, if you changed the lines to look like:
if time_diff.seconds>= 60:
self._logger_output("%s%% complete." %
(100 * self.act_cur_nactions) /
self.act_goal_nactions)
- Another approach, rather than using time, would have been to
just
remember the last percentage calculated, and just report
every 5%,
or similar.
Either works for me - and in someways 60 seconds is
better, just
wondering if you considered this approach and ruled
it out?
- The indentation around the percentage calculation is
confusing, and
possible not PEP8 clean - have you run PEP8 against the
code?
An example of this is:
if time_diff.seconds>= 60:
percent_complete = (100 *
self.act_cur_nactions)/self.act_goal_nactions
self._logger_output("%s%% complete." %
percent_complete)
this would be more readable, if calculation was indented
differently,
(and also PEP8 cleaner),
if time_diff.seconds>= 60:
percent_complete = (100 *
self.act_cur_nactions) / self.act_goal_nactions
self._logger_output("%s%% complete." %
percent_complete)
or even:
if time_diff.seconds>= 60:
percent_complete = \
(100 * self.act_cur_nactions) /
self.act_goal_nactions
self._logger_output("%s%% complete." %
percent_complete)
My preference would be the latter...
Thanks,
Darren.
Afternoon all!
Could I please have a code review for:
BUG: 7171973 IPS transfer code should show more logging
during install
of packages
CODE REVIEW:
https://cr.opensolaris.org/action/browse/caiman/ghart/IPS_Install_Progress/
DESCRIPTION: Adding progress logging during the install of
IPS packages
so during the 10+ minutes of install time there are now log
entries to
show the install is proceeding and not hung.
OLD LOG ENTRY:
2012-05-29 16:19:30,799
InstallationLogger.generated-transfer-1653-1
DEBUG Install Phase ... Started.
2012-05-29 16:31:40,035
InstallationLogger.generated-transfer-1653-1
DEBUG Install Phase ... Done.
NEW LOG ENTRY:
2012-05-29 16:19:30,799
InstallationLogger.generated-transfer-1653-1
DEBUG Install Phase ... Started.
2012-05-29 16:20:30,803
InstallationLogger.generated-transfer-1653-1
DEBUG 24% complete.
2012-05-29 16:21:30,804
InstallationLogger.generated-transfer-1653-1
DEBUG 32% complete.
2012-05-29 16:22:32,195
InstallationLogger.generated-transfer-1653-1
DEBUG 40% complete.
2012-05-29 16:23:32,239
InstallationLogger.generated-transfer-1653-1
DEBUG 47% complete.
2012-05-29 16:24:32,241
InstallationLogger.generated-transfer-1653-1
DEBUG 55% complete.
2012-05-29 16:25:32,264
InstallationLogger.generated-transfer-1653-1
DEBUG 61% complete.
2012-05-29 16:26:32,347
InstallationLogger.generated-transfer-1653-1
DEBUG 64% complete.
2012-05-29 16:27:32,365
InstallationLogger.generated-transfer-1653-1
DEBUG 72% complete.
2012-05-29 16:28:34,229
InstallationLogger.generated-transfer-1653-1
DEBUG 75% complete.
2012-05-29 16:29:34,291
InstallationLogger.generated-transfer-1653-1
DEBUG 81% complete.
2012-05-29 16:30:36,185
InstallationLogger.generated-transfer-1653-1
DEBUG 85% complete.
2012-05-29 16:31:36,189
InstallationLogger.generated-transfer-1653-1
DEBUG 98% complete.
2012-05-29 16:31:40,035
InstallationLogger.generated-transfer-1653-1
DEBUG Install Phase ... Done.
TESTING:
- Confirmed that new logging works and appears in this new
format.
- Verified that the install still succeeds and the OS boots
correctly
after install
Thanks,
Geoffrey
_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
