Closed Bug 409449 Opened 14 years ago Closed 11 years ago

automatically verify test channel contents match the corresponding live channel contents

Categories

(Release Engineering :: General, defect, P3)

defect

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: joduinn, Assigned: bhearsum)

References

Details

(Whiteboard: [automation][updates][simple])

Attachments

(3 files, 3 obsolete files)

Could we automate this? 

Maybe do this verification earlier; could this be done when snippets are pushed to staging area?

For now, we do the following manually:

cd /opt/aus2/snippets/staging/20071218-Firefox-3.0b2
find -type d -iregex '.*beta.*' | perl -nle '$a = $_; $a =~ s/beta/releasetest/; system("diff -r -u $_ ../20071218-Firefox-3.0b2-test/$a");' 2>&1 | tee ~/fx-3.0b2.diff

...but there is likely a cleaner/nicer/legible way to do this?
Priority: -- → P3
Component: Release Engineering → Release Engineering: Future
QA Contact: build → release
Blocks: 409493
Summary: Verify test channel contents match the corresponding live channel contents → automatically verify test channel contents match the corresponding live channel contents
Mass move of bugs from Release Engineering:Future -> Release Engineering. See
http://coop.deadsquid.com/2010/02/kiss-the-future-goodbye/ for more details.
Component: Release Engineering: Future → Release Engineering
Whiteboard: [automation][updates][simple]
Priority: P3 → P5
Shouldn't be too hard
Assignee: nobody → bhearsum
Priority: P5 → P3
Attached patch channel comparison script (obsolete) — Splinter Review
This is a more robust version of what we used to do to verify channel contents. It compares every file found for existence in the other directory (going both ways), and diffs all files that exist in both.
Attachment #471616 - Flags: review?(nrthomas)
This factory is becoming quite the behemoth, but it's tough to do this stuff anywhere else. It'll probably end up looking better when we move it to a ScriptFactory.

The main thing here is the refactoring and movement of the top bit of uploadSnippets. I think it's easier to read this way, but if you disagree or want more comments please say so!

I moved the sleep step because it's annoyed for awhile that we sleep in between uploading test snippets and other snippets.

I'll attach output from a test run w/ purposely different snippets.
Attachment #471622 - Flags: review?(nrthomas)
Attached file output
Comment on attachment 471622 [details] [diff] [review]
integrate into ReleaseUpdatesFactory

>diff --git a/process/factory.py b/process/factory.py
>         self.uploadSnippets()
>+        self.verifySnippets()

Do you want to verify after transfer to catch errors in transmission ? I'm thinking we should verify before upload because it will be faster if NFS is not involved, and we don't have to maintain a tools checkout on aus2.

>+            self.channels['beta']['compareTo'] = 'betatest'

I think this will fail because we use stage-old for betatest and ftp for beta. If you want we can make that difference go away and just use ftp, but I'm not sure how overloaded that variable is.

>             # We only push test channel snippets from automation.
>-            if type == 'test':
>+            if localDir.find('test') > -1:

.endswith('test') is a stronger condition ?
Comment on attachment 471616 [details] [diff] [review]
channel comparison script

>diff --git a/release/compare-channel-snippets.sh b/release/compare-channel-snippets.sh
>+for asnippet in `find . -type f -iregex ".*$achannel.*"`; do

Even though the separate directories saves us, shouldn't this match on /$achannel/ ? eg look for beta and get betatest. 

Could you cut off the leading './' from find ? 

>+    bsnippet=`echo $asnippet | sed -e "s/$achannel/$bchannel/"`
>+    if [[ ! -e "$bdir/$bsnippet" ]]; then
>+        echo "WARN: $adir/$asnippet exists"
>+        echo "  but $bdir/$bsnippet doesn't"

Using `basename $adir` (and bdir) would make the logs less verbose. Same again when comparing the other way.

>+    diff -Naur $adir/$asnippet $bdir/$bsnippet

diff -au would be enough wouldn't it ? Given the earlier existence check and comparing files rather than dirs.

>+for bsnippet in `find . -type f -iregex ".*$bchannel.*"`; do
>+    asnippet=`echo $bsnippet | sed -e "s/$bchannel/$achannel/"`
>+    if [[ ! -e "$adir/$asnippet" ]]; then

Would be worth a comment here along the lines of "if the file exists in adir we already diffed them so no point doing it again".

r+ with changes.
Attachment #471616 - Flags: review?(nrthomas) → review+
(In reply to comment #6)
> Comment on attachment 471622 [details] [diff] [review]
> integrate into ReleaseUpdatesFactory
> 
> >diff --git a/process/factory.py b/process/factory.py
> >         self.uploadSnippets()
> >+        self.verifySnippets()
> 
> Do you want to verify after transfer to catch errors in transmission ? I'm
> thinking we should verify before upload because it will be faster if NFS is not
> involved, and we don't have to maintain a tools checkout on aus2.

Yeah, that's a good point. I'll make that change. Except under very exceptional circumstances, I think it'll be functionally equivalent since rsync is good about catching errors.

> >+            self.channels['beta']['compareTo'] = 'betatest'
> 
> I think this will fail because we use stage-old for betatest and ftp for beta.
> If you want we can make that difference go away and just use ftp, but I'm not
> sure how overloaded that variable is.

Another good point. I'm not prepared to make a change like that for the purpose of this bug, so I'll just remove the comparison.

> 
> >             # We only push test channel snippets from automation.
> >-            if type == 'test':
> >+            if localDir.find('test') > -1:
> 
> .endswith('test') is a stronger condition ?

I'll make that change.
Attached patch address comments in script (obsolete) — Splinter Review
Attachment #471616 - Attachment is obsolete: true
Attachment #471987 - Flags: review?(nrthomas)
Nick, I've addressed all of your comments. I decided to keep the snippet verification post-upload, but done on the slave because I couldn't find any benefit to doing it before uploading.

I also noticed that we were going to be perma-orange because of missing snippets on the release channel for all alphas/betas, so I added some log verification. It does a few things:
- ensure we stay green when the only missing files are from alphas/betas
- raise all other missing files into a special 'Warnings' log
- raise all files that are different between channels into a special log

I changed the behaviour of the script slightly to make this easier. Specifically, it now returns 0 on warnings instead of 1. Arguably, that's valid.
Attachment #471622 - Attachment is obsolete: true
Attachment #473690 - Flags: review?(nrthomas)
Attachment #471622 - Flags: review?(nrthomas)
Attachment #471987 - Attachment is obsolete: true
Attachment #473691 - Flags: review?(nrthomas)
Attachment #471987 - Flags: review?(nrthomas)
Attachment #473690 - Flags: review?(nrthomas) → review+
Attachment #473691 - Flags: review?(nrthomas) → review+
Comment on attachment 473690 [details] [diff] [review]
updated buildbotcustom patch

changeset:   1001:31fe6f81d7cd
Comment on attachment 473691 [details] [diff] [review]
updated tools patch

changeset:   861:72ee172ce22f
Attachment #473691 - Flags: checked-in+
Comment on attachment 473690 [details] [diff] [review]
updated buildbotcustom patch

changeset:   1001:31fe6f81d7cd
Attachment #473690 - Flags: checked-in+
Fully landed, documentation updated: https://wiki.mozilla.org/index.php?title=Release%3ARelease_Automation_on_Mercurial%3ADocumentation&action=historysubmit&diff=252312&oldid=247611
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Product: mozilla.org → Release Engineering
You need to log in before you can comment on or make changes to this bug.