Closed Bug 1296354 Opened 3 years ago Closed 2 years ago

Properly handle etags generated by multipart s3 uploads

Categories

(Release Engineering :: Custom Release Requests, defect, P3)

defect

Tracking

(Not tracked)

RESOLVED DUPLICATE of bug 1398803

People

(Reporter: rail, Unassigned)

References

Details

Attachments

(3 files)

According to http://docs.aws.amazon.com/AmazonS3/latest/API/RESTCommonResponseHeaders.html:

If an object is created by either the Multipart Upload or Part Copy operation, the ETag is not an MD5 digest, regardless of the method of encryption.

We use ETag when push files to the releases directory. It works fine if we push only once. If we retry, ETags in the candidates directory (multipart) are different than theones in the releases directory (not multipart).

passing --disable-multipart to s3cmd should solve the issue.
Attached file PR
Attachment #8782531 - Flags: review?(mixedpuppy)
See Also: → 1296356
Attachment #8782531 - Flags: review?(mixedpuppy) → review+
Comment on attachment 8782531 [details] [review]
PR

Merged
Attachment #8782531 - Flags: checked-in+
Comment on attachment 8782531 [details] [review]
PR

I had to revert the patch, because the uploads of big files never succeeded. It worked fine for the signature uploads, but failed for the bigger binaries.
 
https://archive.mozilla.org/pub/firefox/tinderbox-builds/mozilla-beta-macosx64/release-mozilla-beta-firefox-win64_partner_repacks-bm86-build1-build15.txt.gz
Attachment #8782531 - Flags: checked-in+ → checked-in-
Better to make the push to mirrors script handle multipart etags.
Summary: partner-repacks should not use multipart uploads → Properly handle etags generated by multipart s3 uploads
Keywords: leave-open
Comment on attachment 8784653 [details]
Bug 1296354 - Use actual content hash to compare fiels if ETags are different.  a=release DONTBUILD

https://reviewboard.mozilla.org/r/73988/#review71916
Attachment #8784653 - Flags: review?(nthomas) → review+
Pushed by raliiev@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/90a6705c14cf
Properly handle etags generated by multipart s3 uploads r=nthomas a=release DONTBUILD
Still need to uplift this to release and esr45.
Doh, for some reason it doesn't work as expected... 

13:04:59    FATAL - pub/firefox/releases/49.0b7/mac-EME-free/hr/Firefox 49.0b7.dmg already exists with the different content (src ETag: "f64e330b673bf508ccc2c6fe9fec946a-6", dest ETag: "66fb0791ffcd47d18aa77703b5fac93a"), aborting
I'm assuming that bugzilla should have set another r? on me. It looks like we will compute two hashes for every partner repack copied into release (EME-free and sha1). Since we have all locales for those, and a full set of platforms for EME-free, this could take some time.  Do you have any data on that ?  It may be possible to test for '-' and only compute a hash when it's present. The downside is that it's only md5 and the known possibility of collisions there.

Alternatively, could you remind me why we copy into releases twice ? Can we avoid this by adjusting exclusion lists ?
Let me come with something better. Probably we can set custom metadata in beetmover and check it instead.
Attached patch checksums.diffSplinter Review
Maybe something like this?
Attachment #8785965 - Flags: feedback?(nthomas)
Comment on attachment 8785965 [details] [diff] [review]
checksums.diff

Review of attachment 8785965 [details] [diff] [review]:
-----------------------------------------------------------------

Seems like this would work, and doesn't add any requests AFAICT. Possibly opens up the sec situation slightly (eg replace a file, put the old hash in metadata), but that still doesn't allow overwriting the releases copy.

Another approach would be to add 
 --exclude .*linux-i686/.* --exclude .*linux-x86_64/.* --exclude .*mac/.* --exclude .*win32/.* --exclude .*win64/.*
when calling push-candidate-to-releases.py for partner repacks. No more fragile than the partner exclusions we have in the main push.

::: testing/mozharness/scripts/release/beet_mover.py
@@ +325,2 @@
>          else:
>              if not get_hash(key.get_contents_as_string()) == get_hash(open(source).read()):

You could use checksum for rhs of this comparison too.

@@ +326,5 @@
>              if not get_hash(key.get_contents_as_string()) == get_hash(open(source).read()):
>                  # for now, let's halt. If necessary, we can revisit this and allow for overwrites
>                  #  to the same buildnum release with different bits
>                  self.fatal("`{}` already exists with different checksum.".format(s3_key))
>              self.log("`{}` has the same MD5 checksum, not uploading".format(s3_key))

Not MD5 any more.
Attachment #8785965 - Flags: feedback?(nthomas) → feedback+
back to the pool for now
Assignee: rail → nobody
Priority: -- → P3
Bulk change of QA Contact to :jlund, per https://bugzilla.mozilla.org/show_bug.cgi?id=1428483
QA Contact: coop → jlund
I think we're going to wontfix this in favor of beetmover scriptworker pushing partner repacks in bug 1398803.
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 1398803
Removing leave-open keyword from resolved bugs, per :sylvestre.
Keywords: leave-open
You need to log in before you can comment on or make changes to this bug.