compress leak test / codesighs logs prior to uploading

RESOLVED FIXED

Status

P3
normal
RESOLVED FIXED
8 years ago
5 years ago

People

(Reporter: bhearsum, Assigned: edransch)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [automation][simple])

Attachments

(2 attachments, 5 obsolete attachments)

(Reporter)

Description

8 years ago
We should start compressing the leak test and codesighs logs before uploading them to stage. For each check-in we currently upload around 30MB per platform. These files all compress well (24MB -> 3.4MB in a single test -- 88% compression rate). At 10 checkins a day across 5 platforms that's over 1GB, which could be reduced to about 150MB.

This isn't a massive win, but would be prudent to do at some point.
Whiteboard: [automation]

Updated

7 years ago
Whiteboard: [automation] → [automation][simple]
(Reporter)

Comment 1

7 years ago
To do this, we'll need to:
* Uncompress the old malloc and leak logs after this step: http://hg.mozilla.org/build/buildbotcustom/file/default/process/factory.py#l1258
* Compress the new malloc and leak logs before this step: http://hg.mozilla.org/build/buildbotcustom/file/default/process/factory.py#l1346
* Uncompress the old codesighs log after this step: http://hg.mozilla.org/build/buildbotcustom/file/default/process/factory.py#l1636
* Compress the new codesighs log before this step: http://hg.mozilla.org/build/buildbotcustom/file/default/process/factory.py#l1670

We should use gzip for this.
(Assignee)

Updated

7 years ago
Assignee: nobody → edransch
(Assignee)

Comment 2

7 years ago
Created attachment 589003 [details] [diff] [review]
diff for buildbotcustom (modifications to factory.py)
Attachment #589003 - Flags: review?(bhearsum)
(Assignee)

Comment 3

7 years ago
Created attachment 589004 [details] [diff] [review]
diff to build-tools (2 new bash scripts)
Attachment #589004 - Flags: review?(bhearsum)
(Reporter)

Comment 4

7 years ago
Comment on attachment 589003 [details] [diff] [review]
diff for buildbotcustom (modifications to factory.py)

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

This looks fine with the name changes mentioned below.

::: process/factory.py
@@ +1258,2 @@
>          self.addStep(RetryingShellCommand(
> +            name='get_unpack_logs',

Nit: let's just call this get_logs.

@@ +1355,2 @@
>              self.addStep(RetryingShellCommand(
> +                name='pack_upload_logs',

And this one just 'upload_logs'

@@ +1645,2 @@
>          self.addStep(RetryingShellCommand(
> +            name='get_unpack_codesize_logs',

Same thing here - drop the 'unpack_' part.

@@ +1689,2 @@
>          self.addStep(RetryingShellCommand(
> +            name='pack_upload_logs',

And here.
Attachment #589003 - Flags: review?(bhearsum) → review+
(Reporter)

Comment 5

7 years ago
Comment on attachment 589004 [details] [diff] [review]
diff to build-tools (2 new bash scripts)

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

The logic looks fine overall, there's a few small improvements I'd like to see though:

::: buildfarm/utils/pack_scp.sh
@@ +5,5 @@
> +# $3 : files to compress
> +# $4 : username
> +# $5 : ssh key file
> +# $6 : target location
> +

I'd like to see some basic argument checking here. Nothing fancy - just that you get the right number of args and that $2 exists and is a directory.

@@ +12,5 @@
> +echo "tar -cz -C ${2} -f ${1} ${3}"
> +tar -cz -C $2 -f $1 $3
> +
> +echo "scp -o User=$4 -o IdentityFile=\"${HOME}/.ssh/$5\" $1 $6"
> +scp -o User=$4 -o IdentityFile="\"${HOME}/.ssh/$5\"" $1 $6

Should use retry.py here, too.

::: buildfarm/utils/wget_unpack.sh
@@ +3,5 @@
> +# $1 : url where files are located
> +# $2 : compressed file expected
> +# $3 - $N : list of files expected in case compressed files doesn't exist
> +
> +set +e

Can you add a comment explaining why +e needs to be used here? It's not obvious at a glance.

@@ +5,5 @@
> +# $3 - $N : list of files expected in case compressed files doesn't exist
> +
> +set +e
> +
> +wget -O wget_unpack.tar.gz ${1}/${2}

It would be good to use retry.py here. You can see an example of using it in a shell script here: http://hg.mozilla.org/build/tools/file/default/release/updates/verify.sh#l16, http://hg.mozilla.org/build/tools/file/default/release/updates/verify.sh#l125. Same goes for the wget commands further down. You can even use the --stdout-regexp and --stderr-regexp to have it look for specific HTTP codes.

@@ +22,5 @@
> +if [ $getResult -eq 0 ] && [ $unpackResult -eq 0 ]; then
> +    echo "Got the packed files"
> +    for ((i=3 ; i <= $# ; i++ )); do
> +        echo "mv ./${!i} ./${!i}.old"
> +        mv ./${!i} ./${!i}.old

Rather than baking in the leak test/codesighs logic, let's make this more generic and specify these files with both their remote name, and desired locale name. For example, rather than arg3 being malloc.log, have it be malloc.log:malloc.log.old. This should make the script more generic.
Attachment #589004 - Flags: review?(bhearsum) → review-
(Assignee)

Comment 6

7 years ago
Created attachment 590233 [details] [diff] [review]
patch to build-tools
Attachment #589004 - Attachment is obsolete: true
Attachment #590233 - Flags: review?(bhearsum)
(Assignee)

Comment 7

7 years ago
Created attachment 590234 [details] [diff] [review]
patch to buildbotcustom
Attachment #589003 - Attachment is obsolete: true
Attachment #590234 - Flags: review?(bhearsum)
(Reporter)

Comment 8

7 years ago
Comment on attachment 590233 [details] [diff] [review]
patch to build-tools

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

r+ with the FILES args fixed. Sorry for not noticing that in the first review!

::: buildfarm/utils/pack_scp.sh
@@ +17,5 @@
> +{
> +    echo "Usage: pack_scp.sh COMP_FILE DIR FILES USERNAME SSH_KEY TARGET"
> +    echo  "        COMP_FILE: name of compressed file"
> +    echo  "        DIR:       directory to move to before compression"
> +    echo  "        FILES:     files to compress"

It's a bit weird to have FILES as not the last arg, since there can be multiple of them. Rather than requiring quoting on them let's make this the last arg, so it can be passed multiple times easily. You can either use 'shift' to get rid of the earlier args and then $@ to access them, or loop over them and put them in another variable.
Attachment #590233 - Flags: review?(bhearsum) → review-
(Reporter)

Comment 9

7 years ago
Comment on attachment 590234 [details] [diff] [review]
patch to buildbotcustom

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

This patch is OK, except the comment mentioned below, and it will need to be adjusted for the pack_scp.sh arg change.

::: process/factory.py
@@ +1359,5 @@
> +                        'logs.tar.gz ' + ' .. ' +
> +                        '\'malloc.log sdleak.tree\' ' +
> +                        '%s ' % self.stageUsername +
> +                        '%s ' % self.stageSshKey +
> +                        '%s:/%s/%s' % (self.stageServer, self.stageBasePath,

Please add a comment here about why we need the extra slash - it took awhile to debug this and we don't want to lose that knowledge!
(Assignee)

Comment 10

7 years ago
Created attachment 590340 [details] [diff] [review]
patch to build-tools

move list of files to end of arguments
Attachment #590233 - Attachment is obsolete: true
Attachment #590340 - Flags: review?(bhearsum)
(Assignee)

Comment 11

7 years ago
Created attachment 590341 [details] [diff] [review]
patch to buildbotcustom

move list of files to end of arguments
Attachment #590234 - Attachment is obsolete: true
Attachment #590341 - Flags: review?(bhearsum)
Attachment #590234 - Flags: review?(bhearsum)
(Reporter)

Comment 12

7 years ago
Comment on attachment 590340 [details] [diff] [review]
patch to build-tools

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

Looks good! I like that the other arguments have named variables now, too.
Attachment #590340 - Flags: review?(bhearsum) → review+
(Assignee)

Comment 13

7 years ago
Created attachment 590347 [details] [diff] [review]
patch to buildbotcustom

Ooops, wrong patch.
Adjust so that file names are last in the argument list
Attachment #590341 - Attachment is obsolete: true
Attachment #590347 - Flags: review?(bhearsum)
Attachment #590341 - Flags: review?(bhearsum)
(Reporter)

Updated

7 years ago
Attachment #590347 - Flags: review?(bhearsum) → review+
(Reporter)

Updated

7 years ago
Attachment #590340 - Flags: checked-in+
(Reporter)

Updated

7 years ago
Attachment #590347 - Flags: checked-in+
(Reporter)

Comment 14

7 years ago
Merged to production today.
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED

Updated

7 years ago
Depends on: 722940
Depends on: 750513
Depends on: 798238
Product: mozilla.org → Release Engineering
You need to log in before you can comment on or make changes to this bug.