Closed Bug 394500 Opened 12 years ago Closed 12 years ago

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

Categories

(Release Engineering :: General, defect, P2)

defect

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: joduinn, Assigned: nthomas)

References

()

Details

Attachments

(5 files, 1 obsolete file)

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?
OS: Mac OS X → All
Priority: -- → P3
Hardware: PC → All
Assignee: build → nrthomas
Priority: P3 → P2
* 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
Attachment #279259 - Flags: review?(ccooper)
Attachment #279259 - Attachment is patch: true
Attachment #279259 - Attachment mime type: application/octet-stream → text/plain
Attachment #279259 - Flags: review?(ccooper) → review+
Attachment #279259 - Attachment description: Update staging fixes → Update staging fixes [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.
Attachment #279739 - Flags: review?(ccooper)
Comment on attachment 279739 [details] [diff] [review]
Follow up to 279259 [checked in]

Good catch.
Attachment #279739 - Flags: review?(ccooper) → review+
Attachment #279739 - Attachment description: Follow up to 279259 → Follow up to 279259 [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.
Attachment #279923 - Flags: review?(rhelmer)
> - (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.]
Assignee: nrthomas → build
Is this similar to that permissions problem we were seeing with the Breakpad symbol uploads?
Attachment #279923 - Flags: review?(rhelmer) → review+
Attachment #279923 - Attachment description: Fix permissions on buildID → Fix permissions on buildID [checked in]
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.
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.
Assignee: build → nrthomas
Status: NEW → ASSIGNED
Attachment #283732 - Flags: review?(joduinn)
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.
Attachment #283732 - Flags: review?(rhelmer)
Status: ASSIGNED → NEW
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 on attachment 283732 [details] [diff] [review]
Fix permissions on partial mars and snippets

Uh yeah, I should do it properly.
Attachment #283732 - Attachment is obsolete: true
Attachment #283732 - Flags: review?(rhelmer)
Attachment #283732 - Flags: review?(joduinn)
This patch addresses Rob's comment.
Attachment #283932 - Flags: review?(joduinn)
Attachment #283932 - Flags: review?(rhelmer)
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.
Attachment #283932 - Flags: review?(joduinn) → review-
(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 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.
Attachment #283932 - Flags: review- → review+
(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.

Attachment #283932 - Flags: review?(rhelmer) → review+
Landed.
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
We need to ensure that stage-merged/ is 755 too, since that permission ends up on the $app/$version/.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
This should fix batch-skel/stage/, which is rsync'd to stage-merged/. Also removes the unused $mergeDir variable.
Attachment #284043 - Flags: review?(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?
Attachment #284043 - Flags: review?(rhelmer) → review+
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
Attachment #284043 - Attachment description: Fix permissions of stage-merged/ by setting 0755 on progenitor → Fix permissions of stage-merged/ by setting 0755 on progenitor [checked in]
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.
Attachment #283932 - Attachment description: Fix permissions on partial mars and snippets - v2 → Fix permissions on partial mars and snippets - v2 [checked in]
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.
All the issues hit during 2.0.0.8 are already fixed. Lets use follow-ups for anything else that's discovered.
Status: REOPENED → RESOLVED
Closed: 12 years ago12 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.