1296354 - Properly handle etags generated by multipart s3 uploads

Status: RESOLVED DUPLICATE of bug 1398803

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

Description

[Rail Aliiev [:rail]]

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.

Comment 1

[Rail Aliiev [:rail]]

Attachment: PR

Comment 2

[Rail Aliiev [:rail]]

Comment on attachment 8782531 [details] [review]
PR

Merged

Comment 3

[Rail Aliiev [:rail]]

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

Comment 4

[Rail Aliiev [:rail]]

Better to make the push to mirrors script handle multipart etags.

Comment 5

[Rail Aliiev [:rail]]

Attachment: Bug 1296354 - Use actual content hash to compare fiels if ETags are different. a=release DONTBUILD

Review commit: https://reviewboard.mozilla.org/r/73988/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/73988/

Comment 6

[Nick Thomas [:nthomas] (UTC+12)]

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

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/90a6705c14cf
Properly handle etags generated by multipart s3 uploads r=nthomas a=release DONTBUILD

Comment 8

[Rail Aliiev [:rail]]

Comment on attachment 8784653 [details]
Bug 1296354 - Use actual content hash to compare fiels if ETags are different.  a=release DONTBUILD

https://hg.mozilla.org/integration/mozilla-inbound/rev/90a6705c14cf
https://hg.mozilla.org/releases/mozilla-aurora/rev/1d42160deab3
https://hg.mozilla.org/releases/mozilla-beta/rev/a37eba7b5746

Comment 9

[Rail Aliiev [:rail]]

Still need to uplift this to release and esr45.

Comment 10

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/90a6705c14cf

Comment 11

[Rail Aliiev [:rail]]

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

Comment 12

[Rail Aliiev [:rail]]

Comment on attachment 8784653 [details]
Bug 1296354 - Use actual content hash to compare fiels if ETags are different.  a=release DONTBUILD

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/73988/diff/1-2/

Comment 13

[Nick Thomas [:nthomas] (UTC+12)]

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 ?

Comment 14

[Rail Aliiev [:rail]]

Let me come with something better. Probably we can set custom metadata in beetmover and check it instead.

Comment 15

[Rail Aliiev [:rail]]

Attachment: checksums.diff

Maybe something like this?

Comment 16

[Nick Thomas [:nthomas] (UTC+12)]

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.

Comment 17

[Rail Aliiev [:rail]]

back to the pool for now

Comment 18

[Firefox Bug Husbandry Bot]

Bulk change of QA Contact to :jlund, per https://bugzilla.mozilla.org/show_bug.cgi?id=1428483

Comment 19

[Aki Sasaki (not active)]

I think we're going to wontfix this in favor of beetmover scriptworker pushing partner repacks in bug 1398803.

Comment 20

[Firefox Bug Husbandry Bot]

Removing leave-open keyword from resolved bugs, per :sylvestre.