Refactor downloading archives in archive_info.py

Move the download out of initialization so we can correctly throw errors while
still supporting UserStories using local data.

Just log warnings when trying to download archives without a specified bucket
or where the user doesn't have permission to access the archive's bucket but
has a local copy of the archive. Raises cloud storage errors when there is no
local copy of the archive and the user doesn't have permission to access the
archive's bucket.

Also removes the ignore_archive parameter since it is only used in the smoke
test and doesn't provide any information that can't be gleaned from the
UserStory not having a bucket.

BUG=

Committed: https://crrev.com/0934df74b30f1b6eb5036dbc2a01b4bf4b668c28
Cr-Commit-Position: refs/heads/master@{#310542}

[aiolos (Not reviewing), 2014-12-19 22:33:09]

aiolos@chromium.org changed reviewers:
+ dtu@chromium.org

[aiolos (Not reviewing), 2014-12-23 18:45:39]

aiolos@chromium.org changed reviewers:
+ nednguyen@google.com

[nednguyen, 2014-12-23 18:51:04]

On 2014/12/23 18:45:51, aiolos (OOO Dec 22-28th) wrote:

Seems like there are many different refactorings in one patch. Can you split
them into different patches for the ease of reviewing?

[nednguyen, 2014-12-23 18:56:23]

https://codereview.chromium.org/794493004/diff/1/tools/telemetry/telemetry/us...
File tools/telemetry/telemetry/user_story/user_story_set.py (right):

https://codereview.chromium.org/794493004/diff/1/tools/telemetry/telemetry/us...
tools/telemetry/telemetry/user_story/user_story_set.py:135:
cloud_storage.GetIfChanged(path, self.bucket)
I am not convinced about moving this functionality from user_story_runner to
user story set. UserStory & UserStorySet has one single responsibility: contains
data sufficient for running a user story test. 

Invoking network & updating the data is not the responsibility of the data
container.

[aiolos (Not reviewing), 2014-12-23 19:05:19]

On 2014/12/23 18:56:23, nednguyen wrote:
>
https://codereview.chromium.org/794493004/diff/1/tools/telemetry/telemetry/us...
> File tools/telemetry/telemetry/user_story/user_story_set.py (right):
> 
>
https://codereview.chromium.org/794493004/diff/1/tools/telemetry/telemetry/us...
> tools/telemetry/telemetry/user_story/user_story_set.py:135:
> cloud_storage.GetIfChanged(path, self.bucket)
> I am not convinced about moving this functionality from user_story_runner to
> user story set. UserStory & UserStorySet has one single responsibility:
contains
> data sufficient for running a user story test. 
> 
> Invoking network & updating the data is not the responsibility of the data
> container.

But that data stored in the serving_dirs is required for running a user story
test. It ssems like overkill, but If you want me to write a new class to handle
downloading the contents of that directory from cloud storage, I can. Or if you
have a better suggestion for where it should be, I'm open to suggestion. But it
really doesn't belong in user_story_runner. We need to download two sets of
information from cloud storage to run the user story test, one is done in
archive_info (the wpr archives), and the other is currently done in
user_story_runner (the, usually binary, data with hash files stored in the
serving_dirs.)

[aiolos (Not reviewing), 2014-12-23 19:07:04]

On 2014/12/23 18:51:04, nednguyen wrote:
> On 2014/12/23 18:45:51, aiolos (OOO Dec 22-28th) wrote:
> 
> Seems like there are many different refactorings in one patch. Can you split
> them into different patches for the ease of reviewing?

I could break it into 2 CL's if you'd like. But the wpr archive related
refactors need to stay together and the serving_dirs related refactor needs to
stay together to avoid breaking things.

[nednguyen, 2014-12-23 20:14:48]

On 2014/12/23 19:07:04, aiolos (OOO Dec 22-28th) wrote:
> On 2014/12/23 18:51:04, nednguyen wrote:
> > On 2014/12/23 18:45:51, aiolos (OOO Dec 22-28th) wrote:
> > 
> > Seems like there are many different refactorings in one patch. Can you split
> > them into different patches for the ease of reviewing?
> 
> I could break it into 2 CL's if you'd like. But the wpr archive related
> refactors need to stay together and the serving_dirs related refactor needs to
> stay together to avoid breaking things.

Ok. Please do so.

[aiolos (Not reviewing), 2015-01-06 18:51:04]

On 2014/12/23 20:14:48, nednguyen wrote:
> On 2014/12/23 19:07:04, aiolos (OOO Dec 22-28th) wrote:
> > On 2014/12/23 18:51:04, nednguyen wrote:
> > > On 2014/12/23 18:45:51, aiolos (OOO Dec 22-28th) wrote:
> > > 
> > > Seems like there are many different refactorings in one patch. Can you
split
> > > them into different patches for the ease of reviewing?
> > 
> > I could break it into 2 CL's if you'd like. But the wpr archive related
> > refactors need to stay together and the serving_dirs related refactor needs
to
> > stay together to avoid breaking things.
> 
> Ok. Please do so.

ptal

[nednguyen, 2015-01-06 19:03:22]

Please update the Description to explain how you change the behavior of
wpr_archive updating mechanism.

It looks to me that instead of having the optional bit "will_update" that
callsite has to specify explicitly, this makes it so that it warns when the file
is already there but can't update, and raise exception when the file is missing?

This approach is LGTM. You may want to wait for Dave's approval also.

https://codereview.chromium.org/794493004/diff/20001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/wpr/archive_info.py (right):

https://codereview.chromium.org/794493004/diff/20001/tools/telemetry/telemetr...
tools/telemetry/telemetry/wpr/archive_info.py:61: def
DownloadArchivesIfNeeded(self):
Please add documentation for this method explaining the trickiness of when it
will raise credential error.

[aiolos (Not reviewing), 2015-01-07 01:09:38]

On 2015/01/06 19:03:22, nednguyen wrote:
> Please update the Description to explain how you change the behavior of
> wpr_archive updating mechanism.
> 
Done.

> It looks to me that instead of having the optional bit "will_update" that
> callsite has to specify explicitly, this makes it so that it warns when the
file
> is already there but can't update, and raise exception when the file is
missing?
> 
> This approach is LGTM. You may want to wait for Dave's approval also.

I can wait for him if you want me to, but I trust you to know this code well
enough to give a yea or nay.

https://codereview.chromium.org/794493004/diff/20001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/wpr/archive_info.py (right):

https://codereview.chromium.org/794493004/diff/20001/tools/telemetry/telemetr...
tools/telemetry/telemetry/wpr/archive_info.py:61: def
DownloadArchivesIfNeeded(self):
On 2015/01/06 19:03:22, nednguyen wrote:
> Please add documentation for this method explaining the trickiness of when it
> will raise credential error.

Done. Does the comment cover your concerns? Or do you want to know when the
Permissions vs Credentials errors occur?

[dtu, 2015-01-08 02:07:38]

lgtm

https://codereview.chromium.org/794493004/diff/20001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/user_story/user_story_runner.py (right):

https://codereview.chromium.org/794493004/diff/20001/tools/telemetry/telemetr...
tools/telemetry/telemetry/user_story/user_story_runner.py:307: def
_UpdateAndCheckArchives(archive_data_file, wpr_archive_info,
_CheckAndUpdateArchives?

https://codereview.chromium.org/794493004/diff/20001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/wpr/archive_info.py (right):

https://codereview.chromium.org/794493004/diff/20001/tools/telemetry/telemetr...
tools/telemetry/telemetry/wpr/archive_info.py:24: class ArchiveError(Exception):
nit: spaces before and after

[nednguyen, 2015-01-08 02:13:16]

https://codereview.chromium.org/794493004/diff/20001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/wpr/archive_info.py (right):

https://codereview.chromium.org/794493004/diff/20001/tools/telemetry/telemetr...
tools/telemetry/telemetry/wpr/archive_info.py:61: def
DownloadArchivesIfNeeded(self):
On 2015/01/07 01:09:38, aiolos wrote:
> On 2015/01/06 19:03:22, nednguyen wrote:
> > Please add documentation for this method explaining the trickiness of when
it
> > will raise credential error.
> 
> Done. Does the comment cover your concerns? Or do you want to know when the
> Permissions vs Credentials errors occur?

I mean adding the function's doc: """ .... """
When you see a function named DonwloadArchivesIfNeeded, you will wonder what
"needed" exactly mean here.

[aiolos (Not reviewing), 2015-01-08 02:31:53]

https://codereview.chromium.org/794493004/diff/20001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/wpr/archive_info.py (right):

https://codereview.chromium.org/794493004/diff/20001/tools/telemetry/telemetr...
tools/telemetry/telemetry/wpr/archive_info.py:24: class ArchiveError(Exception):
On 2015/01/08 02:07:38, dtu wrote:
> nit: spaces before and after

Done.

https://codereview.chromium.org/794493004/diff/20001/tools/telemetry/telemetr...
tools/telemetry/telemetry/wpr/archive_info.py:61: def
DownloadArchivesIfNeeded(self):
On 2015/01/08 02:13:16, nednguyen wrote:
> On 2015/01/07 01:09:38, aiolos wrote:
> > On 2015/01/06 19:03:22, nednguyen wrote:
> > > Please add documentation for this method explaining the trickiness of when
> it
> > > will raise credential error.
> > 
> > Done. Does the comment cover your concerns? Or do you want to know when the
> > Permissions vs Credentials errors occur?
> 
> I mean adding the function's doc: """ .... """
> When you see a function named DonwloadArchivesIfNeeded, you will wonder what
> "needed" exactly mean here. 

Ah, that makes more sense. Done.

[nednguyen, 2015-01-08 05:37:36]

The CQ bit was checked by nednguyen@google.com

[nednguyen, 2015-01-08 05:37:36]

lgtm

[commit-bot: I haz the power, 2015-01-08 05:38:48]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/794493004/40001

[commit-bot: I haz the power, 2015-01-08 07:40:16]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-01-08 07:40:18]

Exceeded time limit waiting for builds to trigger.

[aiolos (Not reviewing), 2015-01-08 17:34:52]

The CQ bit was checked by aiolos@chromium.org

[commit-bot: I haz the power, 2015-01-08 17:35:57]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/794493004/40001

[commit-bot: I haz the power, 2015-01-08 18:42:44]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-01-08 18:43:29]

Patchset 3 (id:??) landed as
https://crrev.com/0934df74b30f1b6eb5036dbc2a01b4bf4b668c28
Cr-Commit-Position: refs/heads/master@{#310542}

[aiolos (Not reviewing), 2015-02-19 21:21:08]

Patchset #4 (id:60001) has been deleted