Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1008013003:
Add Copy to cloud storage to allow copying from one remote path to another. (Closed)
|
Created:
5 years, 9 months ago by aiolos (Not reviewing) Modified:
5 years, 9 months ago CC:
chromium-reviews, telemetry-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd Copy to cloud storage to allow copying from one remote path to another.
This is needed so that we can pull the needed binaries for
chrome reference builds from their current location in cloud
storage to a bucket that is accessible from the perf bots and
anyone with Google internal access running the tests manually.
BUG=467260
Committed: https://crrev.com/ed35b76057e5ce137e1114a8c563c9d80f0108d4
Cr-Commit-Position: refs/heads/master@{#321209}
Patch Set 1 #
Total comments: 3
Patch Set 2 : Added py doc. #
Total comments: 7
Patch Set 3 : #Patch Set 4 : Rebase #
Messages
Total messages: 22 (6 generated)
aiolos@chromium.org changed reviewers: + dtu@chromium.org, nednguyen@google.com
https://codereview.chromium.org/1008013003/diff/1/tools/telemetry/telemetry/u... File tools/telemetry/telemetry/util/cloud_storage.py (right): https://codereview.chromium.org/1008013003/diff/1/tools/telemetry/telemetry/u... tools/telemetry/telemetry/util/cloud_storage.py:181: def Copy(bucket1, bucket2, remote_path, remote_path2=None): Should I name this something that makes it more obvious that this is copying between two remote paths/buckets, or are the parameter names enough to provide clarity?
On 2015/03/14 01:43:10, aiolos wrote: > https://codereview.chromium.org/1008013003/diff/1/tools/telemetry/telemetry/u... > File tools/telemetry/telemetry/util/cloud_storage.py (right): > > https://codereview.chromium.org/1008013003/diff/1/tools/telemetry/telemetry/u... > tools/telemetry/telemetry/util/cloud_storage.py:181: def Copy(bucket1, bucket2, > remote_path, remote_path2=None): > Should I name this something that makes it more obvious that this is copying > between two remote paths/buckets, or are the parameter names enough to provide > clarity? Can you explain why do we need this in the Description?
On 2015/03/14 16:51:59, nednguyen wrote: > On 2015/03/14 01:43:10, aiolos wrote: > > > https://codereview.chromium.org/1008013003/diff/1/tools/telemetry/telemetry/u... > > File tools/telemetry/telemetry/util/cloud_storage.py (right): > > > > > https://codereview.chromium.org/1008013003/diff/1/tools/telemetry/telemetry/u... > > tools/telemetry/telemetry/util/cloud_storage.py:181: def Copy(bucket1, > bucket2, > > remote_path, remote_path2=None): > > Should I name this something that makes it more obvious that this is copying > > between two remote paths/buckets, or are the parameter names enough to provide > > clarity? > > Can you explain why do we need this in the Description? Ping.
On 2015/03/16 22:07:50, aiolos wrote: > On 2015/03/14 16:51:59, nednguyen wrote: > > On 2015/03/14 01:43:10, aiolos wrote: > > > > > > https://codereview.chromium.org/1008013003/diff/1/tools/telemetry/telemetry/u... > > > File tools/telemetry/telemetry/util/cloud_storage.py (right): > > > > > > > > > https://codereview.chromium.org/1008013003/diff/1/tools/telemetry/telemetry/u... > > > tools/telemetry/telemetry/util/cloud_storage.py:181: def Copy(bucket1, > > bucket2, > > > remote_path, remote_path2=None): > > > Should I name this something that makes it more obvious that this is copying > > > between two remote paths/buckets, or are the parameter names enough to > provide > > > clarity? > > > > Can you explain why do we need this in the Description? > > Ping. From your description, it seems like the chrome binaries is already in cloud storage. Why can't perf bot and people with internal access access it directly?
On 2015/03/16 22:11:05, nednguyen wrote: > On 2015/03/16 22:07:50, aiolos wrote: > > On 2015/03/14 16:51:59, nednguyen wrote: > > > On 2015/03/14 01:43:10, aiolos wrote: > > > > > > > > > > https://codereview.chromium.org/1008013003/diff/1/tools/telemetry/telemetry/u... > > > > File tools/telemetry/telemetry/util/cloud_storage.py (right): > > > > > > > > > > > > > > https://codereview.chromium.org/1008013003/diff/1/tools/telemetry/telemetry/u... > > > > tools/telemetry/telemetry/util/cloud_storage.py:181: def Copy(bucket1, > > > bucket2, > > > > remote_path, remote_path2=None): > > > > Should I name this something that makes it more obvious that this is > copying > > > > between two remote paths/buckets, or are the parameter names enough to > > provide > > > > clarity? > > > > > > Can you explain why do we need this in the Description? > > > > Ping. > > From your description, it seems like the chrome binaries is already in cloud > storage. Why can't perf bot and people with internal access access it directly? Some of the bots don't have access to the bucket they are stored in, and we need to be careful about not leaking the url/bucket secret name. See comment #16 and later (notably 19 and 20) from this internal-only viewable bug: https://code.google.com/p/chromium/issues/detail?id=244450#c16
On 2015/03/16 22:25:24, aiolos wrote: > On 2015/03/16 22:11:05, nednguyen wrote: > > On 2015/03/16 22:07:50, aiolos wrote: > > > On 2015/03/14 16:51:59, nednguyen wrote: > > > > On 2015/03/14 01:43:10, aiolos wrote: > > > > > > > > > > > > > > > https://codereview.chromium.org/1008013003/diff/1/tools/telemetry/telemetry/u... > > > > > File tools/telemetry/telemetry/util/cloud_storage.py (right): > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/1008013003/diff/1/tools/telemetry/telemetry/u... > > > > > tools/telemetry/telemetry/util/cloud_storage.py:181: def Copy(bucket1, > > > > bucket2, > > > > > remote_path, remote_path2=None): > > > > > Should I name this something that makes it more obvious that this is > > copying > > > > > between two remote paths/buckets, or are the parameter names enough to > > > provide > > > > > clarity? > > > > > > > > Can you explain why do we need this in the Description? > > > > > > Ping. > > > > From your description, it seems like the chrome binaries is already in cloud > > storage. Why can't perf bot and people with internal access access it > directly? > > Some of the bots don't have access to the bucket they are stored in, and we need > to be careful about not leaking the url/bucket secret name. See comment #16 and > later (notably 19 and 20) from this internal-only viewable bug: > https://code.google.com/p/chromium/issues/detail?id=244450#c16 The wildcards that we will be using won't work on bots without directory listing permission on the bucket. We also would have to scrub any urls from that bucket to avoid leaking the secret. I don't trust our ability to do so well enough to risk it when the solution is to run a cron job that will move the ref builds to a bucket we don't care about leaking. We should only have the most updated ref build for each platform accessible from the telemetry bucket.
lgtm https://codereview.chromium.org/1008013003/diff/1/tools/telemetry/telemetry/u... File tools/telemetry/telemetry/util/cloud_storage.py (right): https://codereview.chromium.org/1008013003/diff/1/tools/telemetry/telemetry/u... tools/telemetry/telemetry/util/cloud_storage.py:181: def Copy(bucket1, bucket2, remote_path, remote_path2=None): On 2015/03/14 01:43:09, aiolos wrote: > Should I name this something that makes it more obvious that this is copying > between two remote paths/buckets, or are the parameter names enough to provide > clarity? Can you add pydoc?
https://codereview.chromium.org/1008013003/diff/1/tools/telemetry/telemetry/u... File tools/telemetry/telemetry/util/cloud_storage.py (right): https://codereview.chromium.org/1008013003/diff/1/tools/telemetry/telemetry/u... tools/telemetry/telemetry/util/cloud_storage.py:181: def Copy(bucket1, bucket2, remote_path, remote_path2=None): On 2015/03/16 23:12:32, nednguyen wrote: > On 2015/03/14 01:43:09, aiolos wrote: > > Should I name this something that makes it more obvious that this is copying > > between two remote paths/buckets, or are the parameter names enough to provide > > clarity? > > Can you add pydoc? Done.
I forgot to ask what would happen if a file already exist in (bucket2, remote_path_2)? https://codereview.chromium.org/1008013003/diff/20001/tools/telemetry/telemet... File tools/telemetry/telemetry/util/cloud_storage.py (right): https://codereview.chromium.org/1008013003/diff/20001/tools/telemetry/telemet... tools/telemetry/telemetry/util/cloud_storage.py:182: """This should be used to copy files between buckets and/or remote paths. The heading line should tell what this does (https://google-styleguide.googlecode.com/svn/trunk/pyguide.html#Comments). How about: "Copy file from |remote_path_1| with |bucket_1| to |remote_path_2| with |bucket_2|." https://codereview.chromium.org/1008013003/diff/20001/tools/telemetry/telemet... tools/telemetry/telemetry/util/cloud_storage.py:186: url2 = 'gs://%s/%s' % (bucket2, remote_path2 or remote_path) I don't think this "remote_path2 or remote_path" helps saving anything but it makes the parameters list harder to read. Can we simplify this to Copy(bucket1, remote_path1, bucket2, remote_path2)? https://codereview.chromium.org/1008013003/diff/20001/tools/telemetry/telemet... tools/telemetry/telemetry/util/cloud_storage.py:187: logging.info('Moving %s to %s' % (url1, url2)) s/Moving/Copying
> I forgot to ask what would happen if a file already exist in (bucket2, > remote_path_2)? It should be overwritten (which is the desired behavior so that we can smoothly replace the reference build.) I added this to the pydoc for clarity. https://codereview.chromium.org/1008013003/diff/20001/tools/telemetry/telemet... File tools/telemetry/telemetry/util/cloud_storage.py (right): https://codereview.chromium.org/1008013003/diff/20001/tools/telemetry/telemet... tools/telemetry/telemetry/util/cloud_storage.py:182: """This should be used to copy files between buckets and/or remote paths. On 2015/03/17 03:05:59, nednguyen wrote: > The heading line should tell what this does > (https://google-styleguide.googlecode.com/svn/trunk/pyguide.html#Comments). > > How about: "Copy file from |remote_path_1| with |bucket_1| to |remote_path_2| > with |bucket_2|." That doesn't all fit on one line, which is what the the style doc requires. How about this instead? https://codereview.chromium.org/1008013003/diff/20001/tools/telemetry/telemet... tools/telemetry/telemetry/util/cloud_storage.py:186: url2 = 'gs://%s/%s' % (bucket2, remote_path2 or remote_path) On 2015/03/17 03:05:59, nednguyen wrote: > I don't think this "remote_path2 or remote_path" helps saving anything but it > makes the parameters list harder to read. > Can we simplify this to Copy(bucket1, remote_path1, bucket2, remote_path2)? Done. https://codereview.chromium.org/1008013003/diff/20001/tools/telemetry/telemet... tools/telemetry/telemetry/util/cloud_storage.py:187: logging.info('Moving %s to %s' % (url1, url2)) On 2015/03/17 03:05:59, nednguyen wrote: > s/Moving/Copying Done.
lgtm again https://codereview.chromium.org/1008013003/diff/20001/tools/telemetry/telemet... File tools/telemetry/telemetry/util/cloud_storage.py (right): https://codereview.chromium.org/1008013003/diff/20001/tools/telemetry/telemet... tools/telemetry/telemetry/util/cloud_storage.py:182: """This should be used to copy files between buckets and/or remote paths. On 2015/03/17 20:35:26, aiolos wrote: > On 2015/03/17 03:05:59, nednguyen wrote: > > The heading line should tell what this does > > (https://google-styleguide.googlecode.com/svn/trunk/pyguide.html#Comments). > > > > How about: "Copy file from |remote_path_1| with |bucket_1| to |remote_path_2| > > with |bucket_2|." > > That doesn't all fit on one line, which is what the the style doc requires. How > about this instead? looks great now!
The CQ bit was checked by aiolos@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1008013003/40001
The CQ bit was unchecked by commit-bot@chromium.org
Failed to apply patch for
tools/telemetry/telemetry/util/cloud_storage_unittest.py:
While running git apply --index -3 -p1;
error: patch failed:
tools/telemetry/telemetry/util/cloud_storage_unittest.py:47
Falling back to three-way merge...
Applied patch to 'tools/telemetry/telemetry/util/cloud_storage_unittest.py'
with conflicts.
U tools/telemetry/telemetry/util/cloud_storage_unittest.py
Patch: tools/telemetry/telemetry/util/cloud_storage_unittest.py
Index: tools/telemetry/telemetry/util/cloud_storage_unittest.py
diff --git a/tools/telemetry/telemetry/util/cloud_storage_unittest.py
b/tools/telemetry/telemetry/util/cloud_storage_unittest.py
index
da3d9d5e3349ec1a97c42a5a722a714c38d1d693..8f52e5e17f8a5cf6cbf7c0ba4e9bff83caf57f75
100644
--- a/tools/telemetry/telemetry/util/cloud_storage_unittest.py
+++ b/tools/telemetry/telemetry/util/cloud_storage_unittest.py
@@ -47,3 +47,16 @@ class CloudStorageUnitTest(unittest.TestCase):
finally:
stubs.Restore()
cloud_storage.FindGsutil = orig_find_gs_util
+
+
+ def testCopy(self):
+ orig_run_command = cloud_storage._RunCommand
+ def AssertCorrectRunCommandArgs(args):
+ self.assertEqual(expected_args, args)
+ cloud_storage._RunCommand = AssertCorrectRunCommandArgs
+ expected_args = ['cp', 'gs://bucket1/remote_path1',
+ 'gs://bucket2/remote_path2']
+ try:
+ cloud_storage.Copy('bucket1', 'bucket2', 'remote_path1', 'remote_path2')
+ finally:
+ cloud_storage._RunCommand = orig_run_command
The CQ bit was unchecked by commit-bot@chromium.org
The CQ bit was checked by aiolos@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from nednguyen@google.com Link to the patchset: https://codereview.chromium.org/1008013003/#ps60001 (title: "Rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1008013003/60001
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/ed35b76057e5ce137e1114a8c563c9d80f0108d4 Cr-Commit-Position: refs/heads/master@{#321209} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
