Upload artifacts with public_read ACL to start with. #195
Conversation
| @@ -480,23 +480,20 @@ release::gcs::copy_release_artifacts() { | |||
|
|
|||
| # Copy the main set from staging to destination | |||
| logecho -n "- Copying release artifacts to $gcs_destination: " | |||
There was a problem hiding this comment.
Let's pull in some of the lost messaging
"Copying public release artifacts..."
|
|
||
| logecho -n "- Listing final contents to log file: " | ||
| logrun -s $GSUTIL ls -lhr "$gcs_destination" || return 1 | ||
|
|
||
| # Push to mirror if set | ||
| if [[ -n "$bucket_mirror" && "$bucket" != "$bucket_mirror" ]]; then | ||
| logecho -n "- Mirroring build to $gcs_mirror: " | ||
| logrun -s $GSUTIL -q -m rsync -d -r "$gcs_destination" "$gcs_mirror" \ | ||
| logrun -s $GSUTIL -q -m rsync -a public_read -d -r "$gcs_destination" "$gcs_mirror" \ |
There was a problem hiding this comment.
Same here. Let's pull in some of the lost messaging
"Mirroring public build..."
| "$gcs_destination" || return 1 | ||
| # This small sleep gives the eventually consistent GCS bucket listing a chance | ||
| # to stabilize. | ||
| sleep 5 |
There was a problem hiding this comment.
Eww. There are no consistency checks available in gsutil? I know the answer is probably no. :)
There was a problem hiding this comment.
maybe gsutil ls the gcs destination and make sure the number of artifacts matches the number uploaded?
There was a problem hiding this comment.
That's pretty gross. Would it would be better to re-upload to the gcs_mirror instead of trying to copy between gcs_destination and gcs_mirror?
There was a problem hiding this comment.
I'm not sure anything is even still using the --bucket-mirror codepath.
There was a problem hiding this comment.
Great! Let's delete code instead!
There was a problem hiding this comment.
Neat, I didn't find any mentions of "Mirroring build" in PR or CI jobs.
Should I leave the sleep in, accept that the "listing final contents" step might lie, or remove it?
There was a problem hiding this comment.
maybe do gsutil cp -L upload.log ... && cat upload.log?
There was a problem hiding this comment.
That's pretty different from what gsutil ls -lhr outputs. I'll just leave the sleep in with a note that it's just an attempt.
This fixes kubernetes/test-infra#990 where a recursive acl change immediately after an upload might miss objects, since GCS bucket listing is eventually consistent.
|
Thanks for plumbing through all of this. More red (deleted) than green (added). Woot! |
| @@ -139,7 +133,7 @@ if ! common::set_cloud_binaries; then | |||
| common::exit 1 | |||
| fi | |||
|
|
|||
| for bucket in $RELEASE_BUCKET $RELEASE_BUCKET_MIRROR; do | |||
| for bucket in $RELEASE_BUCKET; do | |||
There was a problem hiding this comment.
Yeah that was really my only one too.
|
Removed the last unnecessary for-loop. |
| logecho -n "- Copying release artifacts to $gcs_destination: " | ||
| logrun -s $GSUTIL -qm cp -r $gcs_stage/* $gcs_destination/ || return 1 | ||
| logecho -n "- Copying public release artifacts to $gcs_destination: " | ||
| logrun -s $GSUTIL -qm cp -a public_read -r $gcs_stage/* $gcs_destination/ || return 1 |
|
This PR broke the background release copying because it explicitly set an ACL, which avoided the |
| # to stabilize before the diagnostic listing. There's no way to directly | ||
| # query for consistency, but it's OK if something is dropped from the | ||
| # debugging output. | ||
| sleep 5 | ||
|
|
There was a problem hiding this comment.
I think we should just revert this PR, then change this not to bother with the all:R change. This should be relying on the defacl on the bucket and nothing else.
There was a problem hiding this comment.
it'd probably be worth adding a comment explicitly noting that, too.
There was a problem hiding this comment.
That sounds like a better solution-- I didn't know about defacl.
There was a problem hiding this comment.
I'm sorry I didn't think about it when this PR went by. :/
Add release theme: A Long-Expected Release
This fixes kubernetes/test-infra#990 where a recursive acl change
immediately after an upload might miss objects, since GCS bucket listing
is eventually consistent.