394500 - FTP area keeps getting set to read-only?!?

Status: RESOLVED FIXED

(Release Engineering :: General, defect, P2)

Description

[John O'Duinn [:joduinn] (please use "needinfo?" flag)]

The FTP area keeps getting set to read-only. This has happened a few times now, so filing tracking bug.

Some examples are:
- (build) permissions on all ${os}_info.txt files were read-only user
- (updates) permissions on AUS config (snippets) and partial MARs were wrong
- (stage) permissions problems for non-stage-merged dirs; 
- - manually did 775 for dirs an 664 for files. group perms seem ok, and batch-skel/stage-merged ok as well. 

Maybe a bug in the rsync from the build machines? Maybe a bug in the initial staging FTP area setup? Or something else?

Comment 1

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

Attachment: Update staging fixes [checked in]

* Removes an old call to the shell version groom-files, which produced empty logs and doesn't seem to be doing anything

* Fixes typo that leaves behind .../batch1/mar/windows-xpi/ etc

Comment 2

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

Attachment: Follow up to 279259 [checked in]

The previous patch (attachment 279259 [details] [diff] [review]) lead to removal ../batch1/mar (so we got no mars at all) rather than just the *-xpi dirs. I've tested this fix on the staging setup.

Comment 3

[Chris Cooper [:coop] (he/him)]

Comment on attachment 279739 [details] [diff] [review]
Follow up to 279259 [checked in]

Good catch.

Comment 4

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

Attachment: Fix permissions on buildID [checked in]

> - (build) permissions on all ${os}_info.txt files were read-only user

Should fix the above issue on file creation and transfer. For some reason perl's tempfile() ignores the 0002 umask on the tinderboxes.

Comment 5

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

> - (stage) permissions problems for non-stage-merged dirs; 
>...
> - manually did 775 for dirs an 664 for files. group perms seem ok, and
>   batch-skel/stage-merged ok as well. 

AIUI the problem here is the permissions are ok in prestage-trimmed, but get munged in stage-unsigned, stage-signed and mar, which points the finger at Stage:GroomFiles(). I mapped out the permissions on the staging setup:
  http://spreadsheets.google.com/pub?key=pZm1Hv_8CTFZ0xsrhG22LUg&gid=0

For dir creation, GroomFiles() calls Mozbuild::Util:MkdirWithPath() but doesn't pass the optional mask. It gives dirs which are cltbld:cltbld 0600 despite a umask on staging-build-console is 0002. That's at
http://mxr.mozilla.org/mozilla/source/tools/release/Bootstrap/Step/Stage.pm#689
Alternatively, we could try setting the mode on prestage-trimmed to 2775.

For files, it's a copy operation at
  http://mxr.mozilla.org/mozilla/source/tools/release/Bootstrap/Step/Stage.pm#693
Could use move here to maintain the already correct permissions.

Alternatively, we could just fix this all at the end of the Stage step in the same way we've been manually working around this.

[Back to the pool so that other can have a crack at this if they want.]

Comment 6

[(not currently active) Ted Mielczarek]

Is this similar to that permissions problem we were seeing with the Breakpad symbol uploads?

Comment 7

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

Paul fixed all the staging problems over in bug 387970, build we did here, so that just leaves the updates problem.

The partial mars and update snippets are created 0600, dirs 700, with cltbld:cltbld ownership - this is on staging-build-console. Should be an easy fix in the Push step.

Comment 8

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

Attachment: Fix permissions on partial mars and snippets

This should fix it up. I'm setting different modes on files because on stage.m.o they get group mozilla, while on AUS they hold on to cltbld.

Comment 9

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

Comment on attachment 283732 [details] [diff] [review]
Fix permissions on partial mars and snippets

Haven't had a chance to test this beyond pushing test files around.

Comment 10

[Robert Helmer [:rhelmer]]

Comment on attachment 283732 [details] [diff] [review]
Fix permissions on partial mars and snippets

Looks like it'll do the trick. 

What is the reason to use shell "find" not the File::Find callback stuff (like in Stage.pm)?

Comment 11

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

Comment on attachment 283732 [details] [diff] [review]
Fix permissions on partial mars and snippets

Uh yeah, I should do it properly.

Comment 12

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

Attachment: Fix permissions on partial mars and snippets - v2 [checked in]

This patch addresses Rob's comment.

Comment 13

[John O'Duinn [:joduinn] (please use "needinfo?" flag)]

Comment on attachment 283932 [details] [diff] [review]
Fix permissions on partial mars and snippets - v2 [checked in]

Looks good, except for a typo with 664 vs 644 below. 

"chmod(0664, $dir) or die("Couldn't chmod $dir to 644: $!");
"

The rest looks good, so happy to r+ after the typo is fixed.

Comment 14

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

(In reply to comment #13)
> "chmod(0664, $dir) or die("Couldn't chmod $dir to 644: $!");

Ah, that should be 664 in both cases (files are cltbld:cltbld on the AUS staging box). I can correct it prior to checkin to save a further review.

Comment 15

[John O'Duinn [:joduinn] (please use "needinfo?" flag)]

Comment on attachment 283932 [details] [diff] [review]
Fix permissions on partial mars and snippets - v2 [checked in]

per offline IRC, happy to r+, assuming the one typo is fixed before landing.

Comment 16

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

(In reply to comment #14)
> (In reply to comment #13)
> > "chmod(0664, $dir) or die("Couldn't chmod $dir to 644: $!");
> 
> Ah, that should be 664 in both cases (files are cltbld:cltbld on the AUS
> staging box). I can correct it prior to checkin to save a further review.

John suggests using 644 for both, which will also work fine and is slightly safer if the groups ever change on the staging box. Rob, please imagine you read chmod(0644) in the callback function.

Comment 17

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

Landed.

Comment 18

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

We need to ensure that stage-merged/ is 755 too, since that permission ends up on the $app/$version/.

Comment 19

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

Attachment: Fix permissions of stage-merged/ by setting 0755 on progenitor [checked in]

This should fix batch-skel/stage/, which is rsync'd to stage-merged/. Also removes the unused $mergeDir variable.

Comment 20

[Robert Helmer [:rhelmer]]

Comment on attachment 284043 [details] [diff] [review]
Fix permissions of stage-merged/ by setting 0755 on progenitor [checked in]

yeah that mergeDir is pretty useless :)

Just to make sure I'm folliwng - this'll fix it on skelDir, and then it'll be preserved when that's rsync'd to create stage-merged, right?

Comment 21

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

Comment on attachment 284043 [details] [diff] [review]
Fix permissions of stage-merged/ by setting 0755 on progenitor [checked in]

(In reply to comment #20)
> (From update of attachment 284043 [details] [diff] [review])
> yeah that mergeDir is pretty useless :)
> 
> Just to make sure I'm folliwng - this'll fix it on skelDir, and then it'll be
> preserved when that's rsync'd to create stage-merged, right?

That's the one - I've verified it works locally, but a staging run will be the best test.

We could explicitly set the group on stage-merged, but stage:pub/firefox/releases/ has the sticky group bit so I don't think it's necessary.


Checking in Bootstrap/Step/Stage.pm;
/cvsroot/mozilla/tools/release/Bootstrap/Step/Stage.pm,v  <--  Stage.pm
new revision: 1.22; previous revision: 1.21
done

Comment 22

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

Comment on attachment 283932 [details] [diff] [review]
Fix permissions on partial mars and snippets - v2 [checked in]

(In reply to comment #17)
> Landed.

Belatedly adding [checked in] to attachment description.

Leaving the bug open for the moment to see if we missed anything.

Comment 23

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

We discovered that Buildbot processes run with umask 077 (thanks to twistd), unless you override it - bug 399628.

Leaving this open until 2.0.0.8 goes out, but I think we're in good shape now.

Comment 24

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

All the issues hit during 2.0.0.8 are already fixed. Lets use follow-ups for anything else that's discovered.