[stage] personas cannot be moved during approval

RESOLVED WONTFIX

Status

--
blocker
RESOLVED WONTFIX
8 years ago
5 years ago

People

(Reporter: vish_moz, Unassigned)

Tracking

Details

(URL)

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

8 years ago
STR:
1. User uploads a persona
2. Admin/Approver approves (accepts) the persona
3. User gets an 'Approval' email with the link to the persona
4. User clicks on the link to his/her approved persona

Actual result:
The persona image is different from the one that user submitted

Expected result:
The persona image should the same as you uploaded
Umm, this sounds important.  MattN, can you look at this?
This wasn't the case for the test persona I uploaded yesterday, but I'll test again shortly.
I'll be busy for a few hours but I'll look into it after if someone else hasn't already.  Bug 571914 has been around for a while but it wasn't easily reproducible. 

Vish, can you reproduce this problem every time?  If so, I think it may be related to the change referenced in Bug 614012 Comment 19.
(Reporter)

Comment 4

8 years ago
(In reply to comment #3)
> Vish, can you reproduce this problem every time?  If so, I think it may be
> related to the change referenced in Bug 614012 Comment 19.

Yes, I was able to reproduce it 3/3 times using two different accounts (admin & non-admin). stephend was also able to reproduce this.
(In reply to comment #3)
> Vish, can you reproduce this problem every time?  If so, I think it may be
> related to the change referenced in Bug 614012 Comment 19.

I believe that comment was referencing Bug 630333 which (I believe) talks about
having the pending images on a separate share from the approved ones.
I just realized that the URL is for staging.  Does this happen on prod as well?
Can you paste the emails in here so we can see the links you got?  Also, what are the links you would expect?
(Reporter)

Comment 8

8 years ago
(In reply to comment #7)
> Can you paste the emails in here so we can see the links you got?  Also, what
> are the links you would expect?

these are the links I got in my "persona approval' email:
https://personas.stage.mozilla.com/en-US/persona/361751
https://personas.stage.mozilla.com/en-US/persona/361749
https://personas.stage.mozilla.com/en-US/persona/361748

Its not about expecting a different link. Its about expecting a different persona image. All the other fields like Persona name, description, created by, added on etc. are correct. Its just the image that is different.
(Reporter)

Comment 9

8 years ago
(In reply to comment #6)
> I just realized that the URL is for staging.  Does this happen on prod as well?

Not happening on prod.

Comment 10

8 years ago
The problem appears to be a 404 while retrieving the data from the CDN

Using https://personas.stage.mozilla.com/en-US/persona/361748
The header thumbnail on the page is stored at 
https://personas.stage.mozilla.com/static/4/8/361748/preview_large.jpg?1297366468

According to the index_1.json header value, the actual header is stored at
http://getpersonas-stage-cdn.mozilla.net/static/4/8/361748/test_persona_header.png?1297366468

The above file doesn't exist. The red color that appears on installing the theme happens to be the accent color #e02a2a


Compared to a persona that does exist on the CDN
https://personas.stage.mozilla.com/en-US/persona/327923
http://getpersonas-stage-cdn.mozilla.net/static/2/3/327923/SwirlsOfBlueGreenHeader.png?1289377557
Looking into this some more, I think it's related to bug 629207.  When I submitted a new persona on stage, it didn't even have the correct image after approval.  Instead, it displayed the image from production that had the same ID.

It seems to me like there's a conflict because of the new prod. data on stage.  I haven't narrowed it down but perhaps the auto-increment ID value is the problem.  It seems like stage is using ID values that have already been used by the production files which were copied. Possibly somewhere in the code there is a failure if the file already exists.

If this is the case then the auto-increment field in all of the tables could be increased to be higher than the latest values from prod.
(In reply to comment #11) 
> I haven't narrowed it down but perhaps the auto-increment ID value is the
> problem.  It seems like stage is using ID values that have already been used by
> the production files which were copied. Possibly somewhere in the code there is
> a failure if the file already exists.
> 
> If this is the case then the auto-increment field in all of the tables could be
> increased to be higher than the latest values from prod.

bkero helped me with some investigation:
We changed the auto-increment value to 1000000 to prevent collisions with the existing production files which were copied over.  This didn't actually fix the problem though and just makes sure that an image from prod with the same ID isn't shown instead (which is very confusing).

When I approved a new persona (id=1000001), the directory was created for it (static/0/1/1000001) but it was empty. This leads me to believe that the 'accept' case is being hit in server/admin/pending.php:88 and make_persona_storage_path($id) is calling mkdir to create the directory but that the rename is failing (and we aren't checking the return value).  All of the files were still in pending/0/1/1000001.

I'm not sure what would have caused this to stop working?  The only idea I had was if the 'static' directory for approved personas was now over NFS and was running into the bug mentioned here: http://www.php.net/manual/en/function.rename.php#90025

Jeremy, do you know if you made any configuration changes in bug 629207 that could cause this?
Created attachment 519823 [details] [diff] [review]
v.1 Patch to check for this problem at approval time. (Not a fix)

This is a patch to check the return value of "rename" so that this problem will be detected immediately by reviewers and the database state will not be corrupted.
Attachment #519823 - Flags: review?(chenba)
Summary: persona after approval is different from what the user uploaded → [stage] persona after approval is different from what the user uploaded

Updated

8 years ago
Attachment #519823 - Flags: review?(chenba) → review+
Created attachment 519985 [details] [diff] [review]
v.1 Potential workaround - remove dest. before rename

This is a patch to potentially workaround the issue on staging.  It basically deletes the destination path if a file or empty directory already exists where the persona folder will be moved to after approval. I don't know for sure that this will fix the issue, but I feel it's worth trying as it shouldn't cause any new issues.
Assignee: nobody → mmn100+bmo
Status: NEW → ASSIGNED

Comment 15

8 years ago
Comment on attachment 519985 [details] [diff] [review]
v.1 Potential workaround - remove dest. before rename

rmdir() would complain if $dest_path is not an empty directory right?  I don't see a check for that.
Attachment #519985 - Flags: review?(chenba) → review-
(In reply to comment #15)
> Comment on attachment 519985 [details] [diff] [review]
> v.1 Potential workaround - remove dest. before rename
> 
> rmdir() would complain if $dest_path is not an empty directory right?  I don't
> see a check for that.

Yes, rmdir would return false but I intentionally didn't check for that since I didn't want to prevent the 'rename' from taking place in the event that the rename overwrote the destination like it's supposed to.

What do you think I should do in that case?  I suppose I could at least error_log(...) for debugging purposes.

Comment 17

8 years ago
I don't understand why there would be a directory in that path, but if our goal is to remove it, then we could empty out the dir first (or we could use exec() :-/ ).

rename() threw an error for me when the new path is an existing directory.  (On ubuntu with PHP 5.2.x.)
Created attachment 521113 [details] [diff] [review]
v.2 Potential workaround - remove or rename dest. before rename

(In reply to comment #17)
> I don't understand why there would be a directory in that path, but if our goal
> is to remove it, then we could empty out the dir first (or we could use exec()
> :-/ ).

The reason there would be an (empty) directory there is because make_persona_path (called by make_persona_storage_path) actually makes the directory if it doesn't exist. 

On my machine with PHP 5.3.5 on Mac OS 10.6:
1) rename non-empty directory to empty directory (this is the common case in our approval code) => SUCCESS
2) rename non-empty directory to non-empty directory (shouldn't happen normally) => PHP gives a warning "PHP Warning:  rename(a,b): Directory not empty" 

Since option 2 should never happen because of our code, I wasn't checking for this case and I didn't want to blindly delete a directory of stuff that shouldn't be there. Instead of deleting it, I'm now logging an error and renaming the existing directory first.

> rename() threw an error for me when the new path is an existing directory.  (On
> ubuntu with PHP 5.2.x.)

Just curious, does it matter whether the destination is empty or not?
Attachment #519985 - Attachment is obsolete: true
Attachment #521113 - Flags: review?(chenba)

Comment 19

8 years ago
Comment on attachment 521113 [details] [diff] [review]
v.2 Potential workaround - remove or rename dest. before rename

Code looks fine.  I know that on production the error reporting is low so the errors are never displayed.  I'm guessing it's the same for stage.  So I don't know where these errors, if they ever occur, are logged.
Attachment #521113 - Flags: review?(chenba) → review+

Comment 20

8 years ago
(In reply to comment #18)
> 
> Since option 2 should never happen because of our code

Sorry, you are right.

> 
> > rename() threw an error for me when the new path is an existing directory.  (On
> > ubuntu with PHP 5.2.x.)
> 
> Just curious, does it matter whether the destination is empty or not?

Sorry again, yeah, it had a file in it.
Committed as r86197 but it didn't seem to fix the issue on stage. When approving a persona it still got to the "There was an error relocating the persona" state.  Looking at the PHP error log is going to be necessary since the problem is specific to the staging environment (it doesn't happen on production).

Jeremy, are PHP errors and warnings logged on personas.stage.mozilla.com? If so, could you grep them for "pending.php" and/or all warnings and errors and let us know what you find? Thanks.
Getting this fixed ASAP would be great since this makes it pretty hard to actually test things on stage.
(In reply to comment #21)
> Jeremy, are PHP errors and warnings logged on personas.stage.mozilla.com? If
> so, could you grep them for "pending.php" and/or all warnings and errors and
> let us know what you find? Thanks.

Jeremy, could you (or someone else) take a look at why the persona directory on stage cannot be moved from the pending directory to the destination for approved personas?  It's possibly file owner/permission issues and the PHP error log should provide some info. This process works fine on production.
Assignee: mmn100+bmo → jeremy.orem+bugs
Looks like a few of the pending files and some of the static/{0..9} files were not owned by apache. Does everything work now?
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
(Reporter)

Comment 25

8 years ago
Now , approving personas gives error:
There was an error relocating the persona. Please contact personas@mozilla.com

Reopening.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Reporter)

Comment 26

8 years ago
Sorry, marking this Resolved again and reopening Bug 644992.
Status: REOPENED → RESOLVED
Last Resolved: 8 years ago8 years ago
Depends on: 644992
Resolution: --- → FIXED
(In reply to comment #24)
> Looks like a few of the pending files and some of the static/{0..9} files were
> not owned by apache. Does everything work now?

No, I don't notice any change. I tried to review persona 361741 which I believe would be in 4/1/361741.  Are the permissions for that directory and its file correct?  What about the directory where the pending images are? 

( https://personas.stage.mozilla.com/pending/4/1/361741/win7lhvinis13.jpg?1296750670 )

(In reply to comment #25)
> Now , approving personas gives error:
> There was an error relocating the persona. Please contact personas@mozilla.com

This is the same problem but the patch's in this bug give an error instead of ignoring the fact that the image could not be relocated.

(In reply to comment #26)
> Sorry, marking this Resolved again and reopening Bug 644992.

That bug is about production and is for a different case.  I've renamed the summary here to make it a little more clear since the symptoms are different after the patches were committed.
Status: RESOLVED → REOPENED
No longer depends on: 644992
Resolution: FIXED → ---
Summary: [stage] persona after approval is different from what the user uploaded → [stage] personas cannot be moved during approval
[root@sm-personas01 personas]# ll pending/4/1/361741
total 116
-rw-r--r-- 1 apache apache  1339 Feb  3 08:31 index_1.json
-rw-r--r-- 1 apache apache  2434 Feb  3 08:31 preview_featured.jpg
-rw-r--r-- 1 apache apache   312 Feb  3 08:31 preview_icon.jpg
-rw-r--r-- 1 apache apache  1509 Feb  3 08:31 preview.jpg
-rw-r--r-- 1 apache apache  8130 Feb  3 08:31 preview_large.jpg
-rw-r--r-- 1 apache apache   469 Feb  3 08:31 preview_popular.jpg
-rw-r--r-- 1 apache apache   358 Feb  3 08:31 preview_small.jpg
-rw-r--r-- 1 apache apache 17123 Feb  3 08:31 win7lhrodapevinis13.jpg
-rw-r--r-- 1 apache apache 58516 Feb  3 08:31 win7lhvinis13.jpg

static only has static/4/1/361741_trash_1300858963.997/

[root@sm-personas01 personas]# ll static/4/1/361741_trash_1300858963.997/
total 96
-rw-r--r-- 1 apache apache 32613 Jan 31 21:20 gradient2.jpg
-rw-r--r-- 1 apache apache 32621 Jan 31 21:20 gradient.jpg
-rw-r--r-- 1 apache apache   861 Jan 31 21:20 index_1.json
-rw-r--r-- 1 apache apache  1995 Jan 31 21:20 preview_featured.jpg
-rw-r--r-- 1 apache apache   291 Jan 31 21:20 preview_icon.jpg
-rw-r--r-- 1 apache apache  1161 Jan 31 21:20 preview.jpg
-rw-r--r-- 1 apache apache  4296 Jan 31 21:20 preview_large.jpg
-rw-r--r-- 1 apache apache   355 Jan 31 21:20 preview_popular.jpg
-rw-r--r-- 1 apache apache   300 Jan 31 21:20 preview_small.jpg



Any ideas? Can someone up the debug logging around these operations?
(Reporter)

Updated

8 years ago
Blocks: 632821

Updated

8 years ago
Assignee: jeremy.orem+bugs → mmn100+bmo
Assignee: mmn100+bmo → nobody
getpersonas.com has been retired.  More information at https://blog.mozilla.org/addons/2013/04/11/background-themes-have-moved-to-amo/
Status: REOPENED → RESOLVED
Last Resolved: 8 years ago6 years ago
Resolution: --- → WONTFIX
(Assignee)

Updated

5 years ago
Component: getpersonas.com → getpersonas.com
Product: Websites → Websites Graveyard
You need to log in before you can comment on or make changes to this bug.