Reworked devserver so that update images generated are cached in directories named after

Reworked devserver so that update images generated are cached in directories named after
the md5 hashes of the images used to generate the updates. Only the 12 most recent deltas
are preserved. All deltas generated (outside of the factory codepath) are modified. 

The purpose of this is to speed up delta generation during automated tests, especially when
running autotest and/or ctest.

autoupdate.py has been refactored in a number of small ways.

All of the Generate methods now return the name of the update generated (or retrieved from
cache), instead of the update file being assumed to be static/update.gz.

Update files are now created directly in the cache directories instead of in the image
directory and then copied.

--clear_cache has been added to devserver.py to force clearing of all cached update values.

--use_cached no longer does anything, but has not yet been removed. It appears to be
intended to force the use of static/update.gz, but I can't find any cases where it's used.

TEST=The autoupdate unit test has been udpated and is passing.
./image_to_live.sh has been tested by hand with both full image and delta updates.
./ctest can run successfully.

BUG=chromium-os:8207
(early prep)

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

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

[ericli, 2010-11-13 00:49:49]

Please also include dalecurtis as the reviewer for this cl. We had been affected
so many times because of devserver changes.

Thanks.

[petkov, 2010-11-13 00:55:17]

You'd also need to make sure you're not breaking the factory flow.

http://codereview.chromium.org/4906001/diff/2001/autoupdate.py
File autoupdate.py (right):

http://codereview.chromium.org/4906001/diff/2001/autoupdate.py#newcode212
autoupdate.py:212: update_path = os.path.join(static_image_dir, 'update.gz')
Since you're updating this stuff, maybe we should rename "update.gz" to
something that doesn't incorrectly imply this is a gzip file. Maybe just
update.bin or something like that.

http://codereview.chromium.org/4906001/diff/2001/autoupdate.py#newcode232
autoupdate.py:232: def GenerateStatefulFile(self, image_path, static_image_dir):
Why are you not calling ./cros_generate_stateful_update_payload any more? There
was intentional effort to channel stateful update generation through a common
script because there're other clients of this -- test lab, network testing lab,
etc.

http://codereview.chromium.org/4906001/diff/2001/autoupdate.py#newcode257
autoupdate.py:257: """Given one, or two images for an update, this finds which
I think Python style is one summary line. Then followed by a blank line and a
detailed description if necessary.

http://codereview.chromium.org/4906001/diff/2001/autoupdate.py#newcode258
autoupdate.py:258: cache directory should hold the udpate files, even if they
don't exist yet.
80 chars

http://codereview.chromium.org/4906001/diff/2001/autoupdate.py#newcode335
autoupdate.py:335: if os.path.exists(os.path.join(static_image_dir,
cache_sub_dir, 'update.gz')):
80 chars

http://codereview.chromium.org/4906001/diff/2001/autoupdate.py#newcode403
autoupdate.py:403: # XXX Work this into new path
Remove this comment along with the commented code? Or change to a TODO.

http://codereview.chromium.org/4906001/diff/2001/autoupdate.py#newcode510
autoupdate.py:510: """Generates an update for non-factory and returns file name
relative to
One line summary. Then description.

http://codereview.chromium.org/4906001/diff/2001/devserver.py
File devserver.py (right):

http://codereview.chromium.org/4906001/diff/2001/devserver.py#newcode136
devserver.py:136: parser.add_option('--clear_cache', action="store_true",
default=False,
Keep options sorted.

http://codereview.chromium.org/4906001/diff/2001/devserver.py#newcode161
devserver.py:161: else:
No need for "else:" here...

http://codereview.chromium.org/4906001/diff/2001/devserver.py#newcode164
devserver.py:164: cmd = 'cd %s; ls -1tr | head --lines=-12 | xargs rm -rf' %
cache_dir
ls -ltr returns more than just filenames, right?

I've seen this done through find (e.g., see
src/platform/init/chromeos-cleanup-logs).

[DaleCurtis, 2010-11-13 01:53:44]

I'll take a more thorough look at this on Monday, but just from your summary: If
update.gz and stateful.image.gz/stateful.tgz exist in the requested directory,
you should serve them instead of using your new md5 workflow.

Also, +1 on petkov's comment above: You should not remove
cros_generate_stateful_update_payload. sosa spent a lot of time making the
stateful update process more space efficient.

On 2010/11/13 00:55:17, petkov wrote:
> You'd also need to make sure you're not breaking the factory flow.
> 
> http://codereview.chromium.org/4906001/diff/2001/autoupdate.py
> File autoupdate.py (right):
> 
> http://codereview.chromium.org/4906001/diff/2001/autoupdate.py#newcode212
> autoupdate.py:212: update_path = os.path.join(static_image_dir, 'update.gz')
> Since you're updating this stuff, maybe we should rename "update.gz" to
> something that doesn't incorrectly imply this is a gzip file. Maybe just
> update.bin or something like that.
> 
> http://codereview.chromium.org/4906001/diff/2001/autoupdate.py#newcode232
> autoupdate.py:232: def GenerateStatefulFile(self, image_path,
static_image_dir):
> Why are you not calling ./cros_generate_stateful_update_payload any more?
There
> was intentional effort to channel stateful update generation through a common
> script because there're other clients of this -- test lab, network testing
lab,
> etc.
> 
> http://codereview.chromium.org/4906001/diff/2001/autoupdate.py#newcode257
> autoupdate.py:257: """Given one, or two images for an update, this finds which
> I think Python style is one summary line. Then followed by a blank line and a
> detailed description if necessary.
> 
> http://codereview.chromium.org/4906001/diff/2001/autoupdate.py#newcode258
> autoupdate.py:258: cache directory should hold the udpate files, even if they
> don't exist yet.
> 80 chars
> 
> http://codereview.chromium.org/4906001/diff/2001/autoupdate.py#newcode335
> autoupdate.py:335: if os.path.exists(os.path.join(static_image_dir,
> cache_sub_dir, 'update.gz')):
> 80 chars
> 
> http://codereview.chromium.org/4906001/diff/2001/autoupdate.py#newcode403
> autoupdate.py:403: # XXX Work this into new path
> Remove this comment along with the commented code? Or change to a TODO.
> 
> http://codereview.chromium.org/4906001/diff/2001/autoupdate.py#newcode510
> autoupdate.py:510: """Generates an update for non-factory and returns file
name
> relative to
> One line summary. Then description.
> 
> http://codereview.chromium.org/4906001/diff/2001/devserver.py
> File devserver.py (right):
> 
> http://codereview.chromium.org/4906001/diff/2001/devserver.py#newcode136
> devserver.py:136: parser.add_option('--clear_cache', action="store_true",
> default=False,
> Keep options sorted.
> 
> http://codereview.chromium.org/4906001/diff/2001/devserver.py#newcode161
> devserver.py:161: else:
> No need for "else:" here...
> 
> http://codereview.chromium.org/4906001/diff/2001/devserver.py#newcode164
> devserver.py:164: cmd = 'cd %s; ls -1tr | head --lines=-12 | xargs rm -rf' %
> cache_dir
> ls -ltr returns more than just filenames, right?
> 
> I've seen this done through find (e.g., see
> src/platform/init/chromeos-cleanup-logs).

[dgarrett, 2010-11-15 21:50:33]

Hopefully, everything has now been addressed.

http://codereview.chromium.org/4906001/diff/2001/autoupdate.py
File autoupdate.py (right):

http://codereview.chromium.org/4906001/diff/2001/autoupdate.py#newcode212
autoupdate.py:212: update_path = os.path.join(static_image_dir, 'update.gz')
This sounds good to me, but I'm not certain what external code may depend on
that name.

On 2010/11/13 00:55:18, petkov wrote:
> Since you're updating this stuff, maybe we should rename "update.gz" to
> something that doesn't incorrectly imply this is a gzip file. Maybe just
> update.bin or something like that.

http://codereview.chromium.org/4906001/diff/2001/autoupdate.py#newcode232
autoupdate.py:232: def GenerateStatefulFile(self, image_path, static_image_dir):
Because I was reading the wrong side of a merge as the external changes. I'll
put it back the way it should be.

On 2010/11/13 00:55:18, petkov wrote:
> Why are you not calling ./cros_generate_stateful_update_payload any more?
There
> was intentional effort to channel stateful update generation through a common
> script because there're other clients of this -- test lab, network testing
lab,
> etc.

http://codereview.chromium.org/4906001/diff/2001/devserver.py
File devserver.py (right):

http://codereview.chromium.org/4906001/diff/2001/devserver.py#newcode161
devserver.py:161: else:
On 2010/11/13 00:55:18, petkov wrote:
> No need for "else:" here...

Done.

http://codereview.chromium.org/4906001/diff/2001/devserver.py#newcode164
devserver.py:164: cmd = 'cd %s; ls -1tr | head --lines=-12 | xargs rm -rf' %
cache_dir
On 2010/11/13 00:55:18, petkov wrote:
> ls -ltr returns more than just filenames, right?
> 
> I've seen this done through find (e.g., see
> src/platform/init/chromeos-cleanup-logs).

I specifically want it to return more than files. Each entry is normally a
directory containing update.gz, and stateful_image.gz.

[sosa, 2010-11-15 21:59:17]

Periods at end of comments.

I noticed there is some code paths you don't use ... some extra src_images, etc.

Also, why did you change the input params in a lot of places including
src_image?  Did you not like using self.vars?

http://codereview.chromium.org/4906001/diff/10001/autoupdate.py
File autoupdate.py (right):

http://codereview.chromium.org/4906001/diff/10001/autoupdate.py#newcode238
autoupdate.py:238: def FindCachedUpdateImageSubDir(self, src_image, dest_image):
docstring

http://codereview.chromium.org/4906001/diff/10001/autoupdate.py#newcode240
autoupdate.py:240: cache directory should hold the udpate files, even if they
don't exist yet.
spelling

http://codereview.chromium.org/4906001/diff/10001/autoupdate.py#newcode292
autoupdate.py:292: if stateful_update_file and
os.path.exists(stateful_update_file):
This case isn't possible.

http://codereview.chromium.org/4906001/diff/10001/autoupdate.py#newcode305
autoupdate.py:305: update filename (not directory) relative to static_image_dir
on success,
Use path rather than filename .. relative path

http://codereview.chromium.org/4906001/diff/10001/autoupdate.py#newcode317
autoupdate.py:317: if os.path.exists(os.path.join(static_image_dir,
cache_sub_dir, 'update.gz')):
80 char

http://codereview.chromium.org/4906001/diff/10001/autoupdate.py#newcode321
autoupdate.py:321: src_image = ""
Use '' not "" to be consistent unless it is a docstring

http://codereview.chromium.org/4906001/diff/10001/autoupdate.py#newcode321
autoupdate.py:321: src_image = ""
You never use src_image

http://codereview.chromium.org/4906001/diff/10001/autoupdate.py#newcode322
autoupdate.py:322: dest_image = os.path.join(full_cache_dir, 'dest_image.bin')
What's the point of this var?

http://codereview.chromium.org/4906001/diff/10001/autoupdate.py#newcode385
autoupdate.py:385: # XXX Work this into new path
What's this here for?

http://codereview.chromium.org/4906001/diff/10001/autoupdate.py#newcode492
autoupdate.py:492: """Generates an update for non-factory and returns file name
relative to
One line for first line in doc string

http://codereview.chromium.org/4906001/diff/10001/autoupdate.py#newcode495
autoupdate.py:495: Returns the update file relative to the static_image_dir on
success."""
""" on new line

http://codereview.chromium.org/4906001/diff/10001/autoupdate.py#newcode495
autoupdate.py:495: Returns the update file relative to the static_image_dir on
success."""
file relative?  or just the base name?

http://codereview.chromium.org/4906001/diff/10001/autoupdate.py#newcode581
autoupdate.py:581: update_name =
self.GenerateUpdatePayloadForNonFactory(board_id,
update name seems a little unclear.  How about payload_name?

http://codereview.chromium.org/4906001/diff/10001/devserver.py
File devserver.py (right):

http://codereview.chromium.org/4906001/diff/10001/devserver.py#newcode137
devserver.py:137: help='Clear out all cached deltas and exit')
Just deltas?

http://codereview.chromium.org/4906001/diff/10001/devserver.py#newcode160
devserver.py:160: sys.exit(os.system('sudo rm -rf %s' % cache_dir))
Why not just clean and then startup?

http://codereview.chromium.org/4906001/diff/10001/devserver.py#newcode163
devserver.py:163: # Clear all but the last 12 cached deltas
Set as a module constant i.e. CACHED_ENTRIES=12

http://codereview.chromium.org/4906001/diff/10001/devserver.py#newcode165
devserver.py:165: if os.system(cmd) != 0:
Return return code?

[DaleCurtis, 2010-11-15 22:38:04]

Mostly style nits. I've patched in your CL to our test farm and was able to
serve updates after moving our pre-generated update.gz and stateful.tgz into the
appropriate cache folder for a given build. A bit round about, but you can't
know which stateful.tgz / update.gz in the old directory layout goes with which
upgrade. So a backwards compatibility hack is out of the question.

http://codereview.chromium.org/4906001/diff/14001/autoupdate.py
File autoupdate.py (left):

http://codereview.chromium.org/4906001/diff/14001/autoupdate.py#oldcode13
autoupdate.py:13: 
Why remove? Standard is two lines between top level definitions.

http://codereview.chromium.org/4906001/diff/14001/autoupdate.py
File autoupdate.py (right):

http://codereview.chromium.org/4906001/diff/14001/autoupdate.py#newcode140
autoupdate.py:140: cmd = ("md5sum %s | awk '{print $1}'" % update_path)
Is there a reason you didn't use openssl like above? Seems more consistent.

http://codereview.chromium.org/4906001/diff/14001/autoupdate.py#newcode197
autoupdate.py:197: image_dir = os.path.dirname(image_path)
Dead variable.

http://codereview.chromium.org/4906001/diff/14001/autoupdate.py#newcode229
autoupdate.py:229: 
Extra blank line here and in most other methods. Style is no line between
docstring and first line of code. gpylint is a good test for these things. Even
if it's not 100% with CrOS style.

http://codereview.chromium.org/4906001/diff/14001/autoupdate.py#newcode297
autoupdate.py:297: return None
No need to explicitly return None, method will automatically return None.

http://codereview.chromium.org/4906001/diff/14001/autoupdate.py#newcode339
autoupdate.py:339: else:
No return for None.

http://codereview.chromium.org/4906001/diff/14001/autoupdate.py#newcode368
autoupdate.py:368: return None
Rework to avoid return None.

http://codereview.chromium.org/4906001/diff/14001/autoupdate.py#newcode395
autoupdate.py:395: return None
Rework to:

if unpack zip:
  return GeneateUpdateImageWithCache(...)

_Log..(Fail)

So return None is unnecessary.

http://codereview.chromium.org/4906001/diff/14001/autoupdate.py#newcode514
autoupdate.py:514: return None
No None.

[DaleCurtis, 2010-11-15 22:43:30]

Whoops, I meant I moved just update.gz in the cache folder. stateful.tgz /
stateful.image.gz stayed where they are.

On 2010/11/15 22:38:04, dalec wrote:
> Mostly style nits. I've patched in your CL to our test farm and was able to
> serve updates after moving our pre-generated update.gz and stateful.tgz into
the
> appropriate cache folder for a given build. A bit round about, but you can't
> know which stateful.tgz / update.gz in the old directory layout goes with
which
> upgrade. So a backwards compatibility hack is out of the question.
> 
> http://codereview.chromium.org/4906001/diff/14001/autoupdate.py
> File autoupdate.py (left):
> 
> http://codereview.chromium.org/4906001/diff/14001/autoupdate.py#oldcode13
> autoupdate.py:13: 
> Why remove? Standard is two lines between top level definitions.
> 
> http://codereview.chromium.org/4906001/diff/14001/autoupdate.py
> File autoupdate.py (right):
> 
> http://codereview.chromium.org/4906001/diff/14001/autoupdate.py#newcode140
> autoupdate.py:140: cmd = ("md5sum %s | awk '{print $1}'" % update_path)
> Is there a reason you didn't use openssl like above? Seems more consistent.
> 
> http://codereview.chromium.org/4906001/diff/14001/autoupdate.py#newcode197
> autoupdate.py:197: image_dir = os.path.dirname(image_path)
> Dead variable.
> 
> http://codereview.chromium.org/4906001/diff/14001/autoupdate.py#newcode229
> autoupdate.py:229: 
> Extra blank line here and in most other methods. Style is no line between
> docstring and first line of code. gpylint is a good test for these things.
Even
> if it's not 100% with CrOS style.
> 
> http://codereview.chromium.org/4906001/diff/14001/autoupdate.py#newcode297
> autoupdate.py:297: return None
> No need to explicitly return None, method will automatically return None.
> 
> http://codereview.chromium.org/4906001/diff/14001/autoupdate.py#newcode339
> autoupdate.py:339: else:
> No return for None.
> 
> http://codereview.chromium.org/4906001/diff/14001/autoupdate.py#newcode368
> autoupdate.py:368: return None
> Rework to avoid return None.
> 
> http://codereview.chromium.org/4906001/diff/14001/autoupdate.py#newcode395
> autoupdate.py:395: return None
> Rework to:
> 
> if unpack zip:
>   return GeneateUpdateImageWithCache(...)
> 
> _Log..(Fail)
> 
> So return None is unnecessary.
> 
> http://codereview.chromium.org/4906001/diff/14001/autoupdate.py#newcode514
> autoupdate.py:514: return None
> No None.

[dgarrett, 2010-11-15 23:08:30]

On 2010/11/15 21:59:17, sosa wrote:
> Periods at end of comments.

That's foreign to me, and I suspect I'll be forgetting to do it for a long time.

> 
> I noticed there is some code paths you don't use ... some extra src_images,
etc.

There was a little noise left over from the work around for images being
modified by mistake.

> 
> Also, why did you change the input params in a lot of places including
> src_image?  Did you not like using self.vars?

  I have no problems with the self.vars, though I did find it a bit confusing
that we mixed the two (self.src_image, and passing in image_path). However, part
way down the call stack, static_image_dir actually gets modified to the cached
output dir, instead of the base static directory. I modified the call arguments
to make this possible.

[dgarrett, 2010-11-15 23:32:39]

Re-uploaded to address more code review issues.

http://codereview.chromium.org/4906001/diff/10001/autoupdate.py
File autoupdate.py (right):

http://codereview.chromium.org/4906001/diff/10001/autoupdate.py#newcode238
autoupdate.py:238: def FindCachedUpdateImageSubDir(self, src_image, dest_image):
On 2010/11/15 21:59:17, sosa wrote:
> docstring

Done.

http://codereview.chromium.org/4906001/diff/10001/autoupdate.py#newcode240
autoupdate.py:240: cache directory should hold the udpate files, even if they
don't exist yet.
On 2010/11/15 21:59:17, sosa wrote:
> spelling

Done.

http://codereview.chromium.org/4906001/diff/10001/autoupdate.py#newcode322
autoupdate.py:322: dest_image = os.path.join(full_cache_dir, 'dest_image.bin')
Both src_iamge and dest_image were left over for the work around for images
being modified by mistake.

On 2010/11/15 21:59:17, sosa wrote:
> What's the point of this var?

http://codereview.chromium.org/4906001/diff/10001/autoupdate.py#newcode495
autoupdate.py:495: Returns the update file relative to the static_image_dir on
success."""
On 2010/11/15 21:59:17, sosa wrote:
> file relative?  or just the base name?

File relative.

http://codereview.chromium.org/4906001/diff/10001/autoupdate.py#newcode495
autoupdate.py:495: Returns the update file relative to the static_image_dir on
success."""
On 2010/11/15 21:59:17, sosa wrote:
> """ on new line

Done.

http://codereview.chromium.org/4906001/diff/10001/autoupdate.py#newcode581
autoupdate.py:581: update_name =
self.GenerateUpdatePayloadForNonFactory(board_id,
On 2010/11/15 21:59:17, sosa wrote:
> update name seems a little unclear.  How about payload_name?

Done.

http://codereview.chromium.org/4906001/diff/10001/devserver.py
File devserver.py (right):

http://codereview.chromium.org/4906001/diff/10001/devserver.py#newcode160
devserver.py:160: sys.exit(os.system('sudo rm -rf %s' % cache_dir))
On 2010/11/15 21:59:17, sosa wrote:
> Why not just clean and then startup?

I wanted a way to clean and exit as well as clean and start. However, I didn't
want two different command line options. Since devserver will start with no
options, clean and no other option would mean there was no way to clear and
exit.

http://codereview.chromium.org/4906001/diff/10001/devserver.py#newcode165
devserver.py:165: if os.system(cmd) != 0:
You can't since you aren't in a function. I wrote it that way the first time
though.

On 2010/11/15 21:59:17, sosa wrote:
> Return return code?

http://codereview.chromium.org/4906001/diff/14001/autoupdate.py
File autoupdate.py (left):

http://codereview.chromium.org/4906001/diff/14001/autoupdate.py#oldcode13
autoupdate.py:13: 
On 2010/11/15 22:38:04, dalec wrote:
> Why remove? Standard is two lines between top level definitions.

Done.

http://codereview.chromium.org/4906001/diff/14001/autoupdate.py
File autoupdate.py (right):

http://codereview.chromium.org/4906001/diff/14001/autoupdate.py#newcode140
autoupdate.py:140: cmd = ("md5sum %s | awk '{print $1}'" % update_path)
Yes. Open SSL as used above generates binary values, not ascii strings. I needed
the ascii strings for the directory names. The first pass was written using
_GetHash().

On 2010/11/15 22:38:04, dalec wrote:
> Is there a reason you didn't use openssl like above? Seems more consistent.

http://codereview.chromium.org/4906001/diff/14001/autoupdate.py#newcode197
autoupdate.py:197: image_dir = os.path.dirname(image_path)
On 2010/11/15 22:38:04, dalec wrote:
> Dead variable.

Done.

http://codereview.chromium.org/4906001/diff/14001/autoupdate.py#newcode229
autoupdate.py:229: 
On 2010/11/15 22:38:04, dalec wrote:
> Extra blank line here and in most other methods. Style is no line between
> docstring and first line of code. gpylint is a good test for these things.
Even
> if it's not 100% with CrOS style.

Done.

http://codereview.chromium.org/4906001/diff/14001/autoupdate.py#newcode297
autoupdate.py:297: return None
On 2010/11/15 22:38:04, dalec wrote:
> No need to explicitly return None, method will automatically return None.

True, I just considered it easier to understand, given that it returns a non
None value in the non-error case.

http://codereview.chromium.org/4906001/diff/14001/autoupdate.py#newcode339
autoupdate.py:339: else:
On 2010/11/15 22:38:04, dalec wrote:
> No return for None.

Same reasoning as above.

http://codereview.chromium.org/4906001/diff/14001/autoupdate.py#newcode368
autoupdate.py:368: return None
On 2010/11/15 22:38:04, dalec wrote:
> Rework to avoid return None.

Again, I'm considering it better style that if you return an explicit value part
of the time, you should explicitly return None in the other cases to show that
you're doing it on purpose.

I can certainly understand why you might feel differently though.

[petkov, 2010-11-15 23:51:03]

LGTM but wait for the other reviewers.

[sosa, 2010-11-15 23:59:08]

Few nits.  You can add a TODO to remove use_cached in a separate CL if you like
but with your changes it is no longer used.

http://codereview.chromium.org/4906001/diff/17002/autoupdate.py
File autoupdate.py (right):

http://codereview.chromium.org/4906001/diff/17002/autoupdate.py#newcode48
autoupdate.py:48: self.use_cached = use_cached
No longer is being used remove.

http://codereview.chromium.org/4906001/diff/17002/autoupdate.py#newcode220
autoupdate.py:220: """Generates a stateful update gz given a full path to an
image.
payload

http://codereview.chromium.org/4906001/diff/17002/autoupdate.py#newcode242
autoupdate.py:242: cache directory should hold the udpate files, even if they
don't exist
update*

http://codereview.chromium.org/4906001/diff/17002/autoupdate.py#newcode382
autoupdate.py:382: # TODO: Either work caching into this path before we unpack,
TODO(->tentative who<-):

http://codereview.chromium.org/4906001/diff/17002/autoupdate.py#newcode578
autoupdate.py:578: payload_name =
self.GenerateUpdatePayloadForNonFactory(board_id,
Sorry now that Ive re-read your code a bit .. payload_path*.  I'm trying to be
consistent w/ name / path in this code.

http://codereview.chromium.org/4906001/diff/17002/devserver.py
File devserver.py (right):

http://codereview.chromium.org/4906001/diff/17002/devserver.py#newcode138
devserver.py:138: parser.add_option('--use_cached', action="store_true",
default=False,
Remove this option ... it's no longer being used.

[dgarrett, 2010-11-16 00:41:39]

http://codereview.chromium.org/4906001/diff/17002/autoupdate.py
File autoupdate.py (right):

http://codereview.chromium.org/4906001/diff/17002/autoupdate.py#newcode220
autoupdate.py:220: """Generates a stateful update gz given a full path to an
image.
On 2010/11/15 23:59:08, sosa wrote:
> payload

Done.

http://codereview.chromium.org/4906001/diff/17002/autoupdate.py#newcode242
autoupdate.py:242: cache directory should hold the udpate files, even if they
don't exist
On 2010/11/15 23:59:08, sosa wrote:
> update*

Done.

http://codereview.chromium.org/4906001/diff/17002/autoupdate.py#newcode382
autoupdate.py:382: # TODO: Either work caching into this path before we unpack,
On 2010/11/15 23:59:08, sosa wrote:
> TODO(->tentative who<-):

Done.

http://codereview.chromium.org/4906001/diff/17002/autoupdate.py#newcode578
autoupdate.py:578: payload_name =
self.GenerateUpdatePayloadForNonFactory(board_id,
On 2010/11/15 23:59:08, sosa wrote:
> Sorry now that Ive re-read your code a bit .. payload_path*.  I'm trying to be
> consistent w/ name / path in this code.

Done.

http://codereview.chromium.org/4906001/diff/17002/devserver.py
File devserver.py (right):

http://codereview.chromium.org/4906001/diff/17002/devserver.py#newcode138
devserver.py:138: parser.add_option('--use_cached', action="store_true",
default=False,
On 2010/11/15 23:59:08, sosa wrote:
> Remove this option ... it's no longer being used.

Done.

[sosa, 2010-11-16 01:16:39]

LGTM w/ nit though dale may wanna dbl check

http://codereview.chromium.org/4906001/diff/31001/autoupdate.py
File autoupdate.py (right):

http://codereview.chromium.org/4906001/diff/31001/autoupdate.py#newcode381
autoupdate.py:381: # TODO (dgarrett): Either work caching into this path before
Nit:  No space after TODO - TODO(person) like other todo's.

[DaleCurtis, 2010-11-16 01:30:05]

Retested with all fixes, still works. LGTM.

On 2010/11/16 01:16:39, sosa wrote:
> LGTM w/ nit though dale may wanna dbl check
> 
> http://codereview.chromium.org/4906001/diff/31001/autoupdate.py
> File autoupdate.py (right):
> 
> http://codereview.chromium.org/4906001/diff/31001/autoupdate.py#newcode381
> autoupdate.py:381: # TODO (dgarrett): Either work caching into this path
before
> Nit:  No space after TODO - TODO(person) like other todo's.

[sosa, 2010-11-16 17:51:13]

Looks like this commit broke stateful updates for ctest on the official builders
(they can't find the stateful.tgz file on the dev server).

Don can you take a look and revert if you need to?  I'll file an issue

[sosa, 2010-11-16 17:53:14]

Filed http://code.google.com/p/chromium-os/issues/detail?id=9215

On Tue, Nov 16, 2010 at 9:51 AM,  <sosa@chromium.org> wrote:
> Looks like this commit broke stateful updates for ctest on the official
> builders
> (they can't find the stateful.tgz file on the dev server).
>
> Don can you take a look and revert if you need to?  I'll file an issue
>
> http://codereview.chromium.org/4906001/
>

[dgarrett, 2010-11-16 22:32:38]

Because of the problem in which stateful.tgz wasn't being found, cached values
are now symlinked from the old/known locations.

Also renamed a few variables and update a few comments to be a little clearer
about the intentions.

I'm doing more testing locally while this review is out.

[dgarrett, 2010-11-16 23:45:51]

On 2010/11/16 22:32:38, dgarrett wrote:
> Because of the problem in which stateful.tgz wasn't being found, cached values
> are now symlinked from the old/known locations.
> 
> Also renamed a few variables and update a few comments to be a little clearer
> about the intentions.
> 
> I'm doing more testing locally while this review is out.

So, is this good?

[sosa, 2010-11-17 00:07:40]

Just a few comments

http://codereview.chromium.org/4906001/diff/40001/autoupdate.py
File autoupdate.py (right):

http://codereview.chromium.org/4906001/diff/40001/autoupdate.py#newcode147
autoupdate.py:147: os.remove(dest)
unlink better?

http://codereview.chromium.org/4906001/diff/40001/autoupdate.py#newcode273
autoupdate.py:273: update and stateful payload with full file names
tuple

http://codereview.chromium.org/4906001/diff/40001/autoupdate.py#newcode331
autoupdate.py:331: if not os.path.exists(cache_update_payload):
You have the possibility of this weird case where you might have created the
cache but not symlinked it correctly.

http://codereview.chromium.org/4906001/diff/40001/autoupdate.py#newcode357
autoupdate.py:357: # return just the filename which is symlink in
static_image_dir
This might break archive_dir i.e. the test automation workflow since they have
relative paths.

[Dale Curtis, 2010-11-17 00:33:51]

I'm confused why you want to put the stateful update in the cache directory.
Unlike update.gz it doesn't change based on a source image. Unless I'm missing
something you'll regenerate it every time you get a new update.

Also, now that you're generating sym links. What happens when multiple dev
server requests come in at the same time for different src/dest images? I
haven't looked closely, but is there a chance for a race condition/clobbering
here?


On 2010/11/17 00:07:40, sosa wrote:
> Just a few comments
> 
> http://codereview.chromium.org/4906001/diff/40001/autoupdate.py
> File autoupdate.py (right):
> 
> http://codereview.chromium.org/4906001/diff/40001/autoupdate.py#newcode147
> autoupdate.py:147: os.remove(dest)
> unlink better?
> 
> http://codereview.chromium.org/4906001/diff/40001/autoupdate.py#newcode273
> autoupdate.py:273: update and stateful payload with full file names
> tuple
> 
> http://codereview.chromium.org/4906001/diff/40001/autoupdate.py#newcode331
> autoupdate.py:331: if not os.path.exists(cache_update_payload):
> You have the possibility of this weird case where you might have created the
> cache but not symlinked it correctly.
> 
> http://codereview.chromium.org/4906001/diff/40001/autoupdate.py#newcode357
> autoupdate.py:357: # return just the filename which is symlink in
> static_image_dir
> This might break archive_dir i.e. the test automation workflow since they have
> relative paths.

[dgarrett, 2010-11-17 00:35:27]

http://codereview.chromium.org/4906001/diff/40001/autoupdate.py
File autoupdate.py (right):

http://codereview.chromium.org/4906001/diff/40001/autoupdate.py#newcode147
autoupdate.py:147: os.remove(dest)
On 2010/11/17 00:07:40, sosa wrote:
> unlink better?

Either way, they are synonyms.

http://codereview.chromium.org/4906001/diff/40001/autoupdate.py#newcode273
autoupdate.py:273: update and stateful payload with full file names
On 2010/11/17 00:07:40, sosa wrote:
> tuple

Done.

http://codereview.chromium.org/4906001/diff/40001/autoupdate.py#newcode331
autoupdate.py:331: if not os.path.exists(cache_update_payload):
You might have created the cache directory, but not populated it with payloads
yet. It should work as is, but it does take up cache slot going forward. I'll
clean it up.

On 2010/11/17 00:07:40, sosa wrote:
> You have the possibility of this weird case where you might have created the
> cache but not symlinked it correctly.

http://codereview.chromium.org/4906001/diff/40001/autoupdate.py#newcode357
autoupdate.py:357: # return just the filename which is symlink in
static_image_dir
On 2010/11/17 00:07:40, sosa wrote:
> This might break archive_dir i.e. the test automation workflow since they have
> relative paths.

The names returned from here are relative to static_image_dir. The symlink is
always in that directory, so it's always just the basename of the path.

[sosa, 2010-11-17 00:39:41]

LGTM w/ nit after you address dale's comments.

http://codereview.chromium.org/4906001/diff/40001/autoupdate.py
File autoupdate.py (right):

http://codereview.chromium.org/4906001/diff/40001/autoupdate.py#newcode353
autoupdate.py:353: # If the generation worked, create symlinks
Change comment ... more like ... Cache directory exists, create Symlinks to it

[dgarrett, 2010-11-17 00:41:03]

On 2010/11/17 00:33:51, Dale Curtis wrote:
> I'm confused why you want to put the stateful update in the cache directory.
> Unlike update.gz it doesn't change based on a source image. Unless I'm missing
> something you'll regenerate it every time you get a new update.

You will. Does it never change? Or just infrequently? This isn't something I
understand very well.

> 
> Also, now that you're generating sym links. What happens when multiple dev
> server requests come in at the same time for different src/dest images? I
> haven't looked closely, but is there a chance for a race condition/clobbering
> here?

Does that ever happen? My understanding is that devserver is only run with a
single
client out there, at least for Non Factory requests which are the only ones
affected
by this change.

I think I can back up my understanding by pointing out that the server was
putting
the real files right where I'm not putting symlinks before I started making
changes,
which means it would have had the same issue before.

[sosa, 2010-11-17 00:41:09]

stateful image doesn't differ on cached deltas w/ same target image,
but does differ on cached fulls.

On Tue, Nov 16, 2010 at 4:33 PM,  <dalecurtis@google.com> wrote:
> I'm confused why you want to put the stateful update in the cache directory.
> Unlike update.gz it doesn't change based on a source image. Unless I'm
> missing
> something you'll regenerate it every time you get a new update.
>
> Also, now that you're generating sym links. What happens when multiple dev
> server requests come in at the same time for different src/dest images? I
> haven't looked closely, but is there a chance for a race
> condition/clobbering
> here?
>
>
> On 2010/11/17 00:07:40, sosa wrote:
>>
>> Just a few comments
>
>> http://codereview.chromium.org/4906001/diff/40001/autoupdate.py
>> File autoupdate.py (right):
>
>> http://codereview.chromium.org/4906001/diff/40001/autoupdate.py#newcode147
>> autoupdate.py:147: os.remove(dest)
>> unlink better?
>
>> http://codereview.chromium.org/4906001/diff/40001/autoupdate.py#newcode273
>> autoupdate.py:273: update and stateful payload with full file names
>> tuple
>
>> http://codereview.chromium.org/4906001/diff/40001/autoupdate.py#newcode331
>> autoupdate.py:331: if not os.path.exists(cache_update_payload):
>> You have the possibility of this weird case where you might have created
>> the
>> cache but not symlinked it correctly.
>
>> http://codereview.chromium.org/4906001/diff/40001/autoupdate.py#newcode357
>> autoupdate.py:357: # return just the filename which is symlink in
>> static_image_dir
>> This might break archive_dir i.e. the test automation workflow since they
>> have
>> relative paths.
>
>
>
> http://codereview.chromium.org/4906001/
>

[dgarrett, 2010-11-17 00:56:02]

http://codereview.chromium.org/4906001/diff/40001/autoupdate.py
File autoupdate.py (right):

http://codereview.chromium.org/4906001/diff/40001/autoupdate.py#newcode353
autoupdate.py:353: # If the generation worked, create symlinks
On 2010/11/17 00:39:41, sosa wrote:
> Change comment ... more like ... Cache directory exists, create Symlinks to it

No, you won't get here if the generation fails. There is a return out above for
the error case. "if not payloads".

[Dale Curtis, 2010-11-17 01:21:14]

On 2010/11/17 00:41:03, dgarrett wrote:
> On 2010/11/17 00:33:51, Dale Curtis wrote:
> > I'm confused why you want to put the stateful update in the cache directory.
> > Unlike update.gz it doesn't change based on a source image. Unless I'm
missing
> > something you'll regenerate it every time you get a new update.
> 
> You will. Does it never change? Or just infrequently? This isn't something I
> understand very well.

I forgot there are users who reuse the same image directory. We use
--archive_dir plus subdirectories to avoid the problem this solves. For someone
who keeps dropping a new base image in the dev server, the stateful partition
will keep getting clobbered otherwise.

> 
> > 
> > Also, now that you're generating sym links. What happens when multiple dev
> > server requests come in at the same time for different src/dest images? I
> > haven't looked closely, but is there a chance for a race
condition/clobbering
> > here?
> 
> Does that ever happen? My understanding is that devserver is only run with a
> single
> client out there, at least for Non Factory requests which are the only ones
> affected
> by this change.
> 
> I think I can back up my understanding by pointing out that the server was
> putting
> the real files right where I'm not putting symlinks before I started making
> changes,
> which means it would have had the same issue before.

Good point. We pre-generate components in the lab here, so it wouldn't affect
us. And I doubt many dev servers see the bandwidth to hit this issue.

Some issues came up in testing. E-mail sent.

[Dale Curtis, 2010-11-17 01:30:25]

After using the right URL :| , testing worked fine.

LGTM

On 2010/11/17 01:21:14, Dale Curtis wrote:
> On 2010/11/17 00:41:03, dgarrett wrote:
> > On 2010/11/17 00:33:51, Dale Curtis wrote:
> > > I'm confused why you want to put the stateful update in the cache
directory.
> > > Unlike update.gz it doesn't change based on a source image. Unless I'm
> missing
> > > something you'll regenerate it every time you get a new update.
> > 
> > You will. Does it never change? Or just infrequently? This isn't something I
> > understand very well.
> 
> I forgot there are users who reuse the same image directory. We use
> --archive_dir plus subdirectories to avoid the problem this solves. For
someone
> who keeps dropping a new base image in the dev server, the stateful partition
> will keep getting clobbered otherwise.
> 
> > 
> > > 
> > > Also, now that you're generating sym links. What happens when multiple dev
> > > server requests come in at the same time for different src/dest images? I
> > > haven't looked closely, but is there a chance for a race
> condition/clobbering
> > > here?
> > 
> > Does that ever happen? My understanding is that devserver is only run with a
> > single
> > client out there, at least for Non Factory requests which are the only ones
> > affected
> > by this change.
> > 
> > I think I can back up my understanding by pointing out that the server was
> > putting
> > the real files right where I'm not putting symlinks before I started making
> > changes,
> > which means it would have had the same issue before.
> 
> Good point. We pre-generate components in the lab here, so it wouldn't affect
> us. And I doubt many dev servers see the bandwidth to hit this issue.
> 
> Some issues came up in testing. E-mail sent.