Add simple update test to pfq.

Add simple update test to pfq.

This CL also fixes the outstanding issue where to won't report progress in
image_to_live on updates.

Change-Id: I1efaf17f0fd5ebb367ae0872377b4d5d0bf5dbf6

BUG=chromium-os:8680, chromium-os:8901
TEST=Ran it with --debug on the x86 pfq locally.

Committed: http://chrome-svn/viewvc/chromeos?view=rev&revision=efbce1e

[petkov, 2011-01-07 01:10:42]

The core change seems OK -- a couple of nits/question.

But you risk an infinite loop I think in the piggy-back change. Maybe either
fix, or split the CL.

http://codereview.chromium.org/6135002/diff/1/bin/cbuildbot.py
File bin/cbuildbot.py (right):

http://codereview.chromium.org/6135002/diff/1/bin/cbuildbot.py#newcode428
bin/cbuildbot.py:428: '--base_image=%s' % image_path,
SimpleTest hardcodes target_image as the source image. Is base_image really
useful then? If not, can you replace with an empty string or /dev/null or
something? Or add a comment.

http://codereview.chromium.org/6135002/diff/1/bin/cbuildbot.py#newcode429
bin/cbuildbot.py:429: '--target_image=%s' % image_path,
does anybody check that the device really updated into the new image? if it's
done by the version, the test may report success on failures. it could be done
by checking rootdev before/after...

http://codereview.chromium.org/6135002/diff/1/image_to_live.sh
File image_to_live.sh (right):

http://codereview.chromium.org/6135002/diff/1/image_to_live.sh#newcode267
image_to_live.sh:267: while [ $(get_update_var CURRENT_OP) !=
${UPDATER_DOWNLOADING} ]; do
you should catch UPDATER_IDLE in this loop otherwise you risk going into an
infinite loop under some error conditions

[petkov, 2011-01-07 01:15:09]

http://codereview.chromium.org/6135002/diff/1/image_to_live.sh
File image_to_live.sh (right):

http://codereview.chromium.org/6135002/diff/1/image_to_live.sh#newcode267
image_to_live.sh:267: while [ $(get_update_var CURRENT_OP) !=
${UPDATER_DOWNLOADING} ]; do
On 2011/01/07 01:10:42, petkov wrote:
> you should catch UPDATER_IDLE in this loop otherwise you risk going into an
> infinite loop under some error conditions

In fact, theoretically, if the update goes really fast, you may end up in
UPDATED_REBOOT_NEEDED state. So, just to be on the safe side, maybe check for
that too.

[sosa, 2011-01-07 22:10:00]

PTAL.  Addressed all issues including cleaning up the status thread and
re-tested.

http://codereview.chromium.org/6135002/diff/1/bin/cbuildbot.py
File bin/cbuildbot.py (right):

http://codereview.chromium.org/6135002/diff/1/bin/cbuildbot.py#newcode428
bin/cbuildbot.py:428: '--base_image=%s' % image_path,
Changed test to just use base_image to target_image. Makes the test a little
more generic as well

On 2011/01/07 01:10:42, petkov wrote:
> SimpleTest hardcodes target_image as the source image. Is base_image really
> useful then? If not, can you replace with an empty string or /dev/null or
> something? Or add a comment.
>

http://codereview.chromium.org/6135002/diff/1/bin/cbuildbot.py#newcode429
bin/cbuildbot.py:429: '--target_image=%s' % image_path,
image_to_live does the following.

1)  Verifies the version string
2)  Verifies that we have changed the root partition.

I think these two would catch a correct update


On 2011/01/07 01:10:42, petkov wrote:
> does anybody check that the device really updated into the new image? if it's
> done by the version, the test may report success on failures. it could be done
> by checking rootdev before/after...

[petkov, 2011-01-07 22:17:29]

http://codereview.chromium.org/6135002/diff/7001/bin/cbuildbot.py
File bin/cbuildbot.py (right):

http://codereview.chromium.org/6135002/diff/7001/bin/cbuildbot.py#newcode703
bin/cbuildbot.py:703: #_UprevPackages(buildroot, tracking_branch, revisionfile,
why is this code commented out? remove? there's more commented out code below...

http://codereview.chromium.org/6135002/diff/7001/image_to_live.sh
File image_to_live.sh (right):

http://codereview.chromium.org/6135002/diff/7001/image_to_live.sh#newcode271
image_to_live.sh:271: echo "Download progress $(get_update_var PROGRESS)"
so you see ".Download progress " now? intentional?

http://codereview.chromium.org/6135002/diff/7001/image_to_live.sh#newcode273
image_to_live.sh:273: sleep ${timeout}
so, you sleep 2*timeout -- intentional?

[sosa, 2011-01-07 23:28:08]

PTAL.  Sorry some debug #'s got leftover in last review

[sosa, 2011-01-07 23:28:44]

http://codereview.chromium.org/6135002/diff/7001/bin/cbuildbot.py
File bin/cbuildbot.py (right):

http://codereview.chromium.org/6135002/diff/7001/bin/cbuildbot.py#newcode703
bin/cbuildbot.py:703: #_UprevPackages(buildroot, tracking_branch, revisionfile,
On 2011/01/07 22:17:29, petkov wrote:
> why is this code commented out? remove? there's more commented out code
below...

Done.

http://codereview.chromium.org/6135002/diff/7001/image_to_live.sh
File image_to_live.sh (right):

http://codereview.chromium.org/6135002/diff/7001/image_to_live.sh#newcode271
image_to_live.sh:271: echo "Download progress $(get_update_var PROGRESS)"
On 2011/01/07 22:17:29, petkov wrote:
> so you see ".Download progress " now? intentional?

Done.

http://codereview.chromium.org/6135002/diff/7001/image_to_live.sh#newcode273
image_to_live.sh:273: sleep ${timeout}
On 2011/01/07 22:17:29, petkov wrote:
> so, you sleep 2*timeout -- intentional?

Done.

[petkov, 2011-01-07 23:41:05]

LGTM. Added a comment to a potential bug unrelated to this CL.

http://codereview.chromium.org/6135002/diff/13001/bin/cros_au_test_harness.py
File bin/cros_au_test_harness.py (right):

http://codereview.chromium.org/6135002/diff/13001/bin/cros_au_test_harness.py...
bin/cros_au_test_harness.py:244: self.PerformUpdate(self.base_image_path,
self.target_image_path)
the comment above seems inconsistent -- here and 3 instances below

[sosa, 2011-01-07 23:41:47]

PTAL properly rebased

[petkov, 2011-01-07 23:43:10]

LGTM