final verification should use GET with short Range instead of HEAD

RESOLVED FIXED

Status

RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: nthomas, Assigned: pmoore)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Reporter)

Description

4 years ago
Akamai caches HEAD and GET requests separately, and when we ask IT to purge CDNs we only act on the GET cache, so final_verification can lie about the current state.
(Reporter)

Comment 1

4 years ago
The relevant code is at 
 http://hg.mozilla.org/build/tools/file/default/release/test-mar-url.sh#l7
and duplicated at line 15. We're using curl 7.19.7 at the moment.

It's not as simple as replacing '-I' with '-r 0-8 -o /dev/null', because that suppresses the headers. We could use mar_headers_debug_file instead of mar_headers_file. It would also need to be taught to parse responses like
   Content-Range: bytes 0-8/45572566
(In reply to Nick Thomas [:nthomas] from comment #1)
> It's not as simple as replacing '-I' with '-r 0-8 -o /dev/null', because
> that suppresses the headers. We could use mar_headers_debug_file instead of
> mar_headers_file. It would also need to be taught to parse responses like
>    Content-Range: bytes 0-8/45572566

fwiw, I tried replacing '-I' with '-iv -o /dev/null' and still got the headers. And, the "-r" option is a nice touch. Script for example:
 https://hg.mozilla.org/build/braindump/rev/841157f4bc99
(Assignee)

Comment 3

4 years ago
Also we'll get back
HTTP/1.1 206 Partial Content
instead of
HTTP/1.1 200 OK
Assignee: nobody → pmoore
Status: NEW → ASSIGNED
(Assignee)

Comment 4

4 years ago
(In reply to Hal Wine [:hwine] (use needinfo) from comment #2)
> (In reply to Nick Thomas [:nthomas] from comment #1)
> > It's not as simple as replacing '-I' with '-r 0-8 -o /dev/null', because
> > that suppresses the headers. We could use mar_headers_debug_file instead of
> > mar_headers_file. It would also need to be taught to parse responses like
> >    Content-Range: bytes 0-8/45572566
> 
> fwiw, I tried replacing '-I' with '-iv -o /dev/null' and still got the
> headers. And, the "-r" option is a nice touch. Script for example:
>  https://hg.mozilla.org/build/braindump/rev/841157f4bc99

I think you and Nick are aligned on this - e.g. "We could use mar_headers_debug_file instead of mar_headers_file" also means parsing the standard error instead of the standard out (which the braindump script is also doing in this example).

However, I played around using cat -v for escaping binary characters, so that we could pull down a couple of bytes, and escape them, and not need to send output to /dev/null (also we'd get these binary characters also in standard out, which we also display in the logs). During the process of experimenting, I noticed that the mar files all start with the same 3 bytes: x4D x41 x52. This translates to the string "MAR" in ASCII. In other words, if we grab the first three bytes (-r 0-2) then we should have no binary content in our output. We can also use cat -v instead of cat when we display in the logs, in case there is a corrupt file, if really needed.
(Assignee)

Comment 5

4 years ago
we'd get these binary characters also in standard out

=>

we'd get these binary characters also in standard error
(Assignee)

Comment 6

4 years ago
Created attachment 8525261 [details] [diff] [review]
bug1100179_tools_v1.patch

Tested locally, and positive case works (i.e. when there are no failures).

This tested the parsing of the Content-Range header, the parsing of the HTTP response code.

I also tested the updated help option (final-verification.sh -h) and the updated output to warn the user there will be no output for some time.

I also manually tested that the curl command produces readable content for the mar header file and the mar header debug file. Technically, I guess the header file is no longer a header file, since it also includes three characters ('MAR') of http body, but I think we might be picking hairs there.

Any questions, let me know!

Pete
Attachment #8525261 - Flags: review?(nthomas)
A couple of comments on the patch:
 - style: I'd prefer the usage output be shorter, as that will scroll off screen. The new content would be great in --help output, or just as comments in the file with a one line pointer in the usage output

 - function: based on my experience last Friday, I'd like to see the "last-modified" header logged on error at the least. That's an easy tell for which file is old and which is new. (Ideally, it could be checked for equality, but that's feature enhancements)

(Note that could be in there now - I'm not familiar with the script)
(Reporter)

Comment 8

4 years ago
FYI, I probably won't have a chance to look at this before next week.
(Assignee)

Comment 9

4 years ago
(In reply to Hal Wine [:hwine] (use needinfo) from comment #7)
> A couple of comments on the patch:
>  - style: I'd prefer the usage output be shorter, as that will scroll off
> screen. The new content would be great in --help output, or just as comments
> in the file with a one line pointer in the usage output

Thanks Hal for the feedback. At the moment, the usage and help are one-of-the-same-thing, in that to see the usage, you run final-verification.sh -h.

What I think helps here, is that the examples are on the last couple of lines of the help, so a user should not need to scroll their terminal if they don't want to read the full help.

I think showing the usage without the supporting help text might be detrimental, as it contains useful and relevant information, and therefore I would be quite keen to keep it if possible.

> 
>  - function: based on my experience last Friday, I'd like to see the
> "last-modified" header logged on error at the least. That's an easy tell for
> which file is old and which is new. (Ideally, it could be checked for
> equality, but that's feature enhancements)
> 

We are currently only retrieving one header per mar file - can you clarify which two files we would be comparing for old vs new?

> (Note that could be in there now - I'm not familiar with the script)

No worries. If you want to give it a spin locally, it should be fine to run the examples from the help, for example. It will generates a report without impacting any remote systems other than nominally querying some download urls. It is also compatible with Mac and Linux, so should work out-of-the-box on your Mac Book Pro. =)
(Assignee)

Comment 10

4 years ago
(In reply to Nick Thomas [:nthomas] from comment #8)
> FYI, I probably won't have a chance to look at this before next week.

No worries!
(In reply to Pete Moore [:pete][:pmoore] from comment #9)
> I think showing the usage without the supporting help text might be
> detrimental, as it contains useful and relevant information, and therefore I
> would be quite keen to keep it if possible.

np - up to the reviewer :)

> >  - function: based on my experience last Friday, I'd like to see the
> > "last-modified" header logged on error at the least. That's an easy tell for
> > which file is old and which is new. (Ideally, it could be checked for
> > equality, but that's feature enhancements)
> > 
> 
> We are currently only retrieving one header per mar file - can you clarify
> which two files we would be comparing for old vs new?

We actually do log the last-modified header on failure in the existing automation, so nothing needs to be done here.

Any checking of last-modified status would be a separate bug/enhancement request which we're currently debating in relduty meetings. I'll put any details on old/new in that bug, if we file it.

> 
> > (Note that could be in there now - I'm not familiar with the script)

Even easier - I found the error report and looked at the failed run!
(Reporter)

Comment 12

4 years ago
Comment on attachment 8525261 [details] [diff] [review]
bug1100179_tools_v1.patch

The usage is quite long, but not excessively much compared to other unix tools, so I'm inclined to allow it. git -h vs git --help we shall not repeat.
Attachment #8525261 - Flags: review?(nthomas) → review+
(Assignee)

Updated

4 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.