sudo delete_files not working

[schach]

In clean_data.sh following calls don't work. sudo delete_files "cache" sudo delete_files "$backupsdir/*" Looks like sudoing a bash function is not possible.

Replaced "delete_files" with "rm -rf" and removed the quotes to make it work: sudo rm -rf cache sudo rm -rf $backupsdir/*

[rajeshtaneja]

Thanks for spotting this Schach, will this soon.

[rajeshtaneja]

I was looking at this, and using sudo seems wrong, as it will halt the script and request for sudo password. In addition to that cache and backupdir permission is set to 777, so no need to add sudo.

Patch resolving this: https://github.com/rajeshtaneja/moodle-performance-comparison/commit/07f5146cfef52b00d880db2b72b063c160723a7b

Will request Eloy or David to review this before pulling it.

[dmonllao]

Thanks @rajeshtaneja, it looks good, I'm just running a test to ensure there are no problems with it's subdirs

[dmonllao]

cache/ dir contains subdirectories which are not 777 so they could not be deleted if you remove the sudo, these subdirectories and it's child files are created in produce_page_graph (webapp/lib.php) you could also set it's permissions to 777 so there will be no problems when deleting them without sudo

In case you want to test it, all these cache/ files are created when you access the details.php page through the web interface

[rajeshtaneja]

Thanks David,

Here is a patch fixing all permissions. https://github.com/rajeshtaneja/moodle-performance-comparison/commit/ab22037f8d957c109ccf007881c3e1b521b4b5df

[dmonllao]

Sweet Rajesh, the only issue I can think about is that, even after pulling the patch, if somebody runs clean_data.sh script the previous cache/SUBDIR dirs will remain there as they have the old permissions.

Pity that we don't have an upgrade.php, we could chmod 777 -R... up to you.

[rajeshtaneja]

Thanks David,

Unfortunately this can't be done from shell script, as owner is www-data and not the user executing the script. cache directory is created by test_runner.sh, so the owner of cache directory is the the user running compare script, but the dir/files created within cache is done by www-data, so adding php code to recursively change mode won't work. I think it would be nice to just add this information in README to delete old cache files.

[dmonllao]

Ok, then we could sudo rm -rf cache instead of _sudo deletefiles "cache", we would not need webapp/lib.php modifications in that case.

[dmonllao]

Hi @rajeshtaneja , I was just looking at the project issues, do you think of the rm -rf cache approach?

[rajeshtaneja]

Thanks for looking at this David, this got lost somehow.

I still feel we should try avoid sudo where possible, as these scripts are normally used by jobs. IMHO we should fix proper permission and add information about removing old files in update.

[stronk7]

Yeah, I think we should avoid any sudo within any script. If the execution generate files owned by "apache/www", then it will need to be run by "apache/www" or by an user having perms to perform the clean (say, group.. or whatever).

Perhaps this is instead about to add, in instructions, the "detail" of which user should execute every script?

Note that, perhaps, an alternative would be to make this to be a local_xxx plugin, and to have some php scripts on it, able to be run from the browser, so they will be run with the "correct" user. Although I'm not 100% sold to this idea either. I think I prefer clearer specification, or error message if something was not deleted, saying "are you sure that you ran this with the xxxx user" or so.

Just my 2 cents. Ciao :)

[dmonllao]

I am not sure about the CLI scripts running by www-data user, the CLI scripts runs by a normal user, privileges over jmeter... and www-data runs only the web app, the issue here is that the web app fills a images cache dir, as it is a cache dir I thought that it should also be cleaned if we have a clean_data.sh script. About local_xxx, would be hard to make it possible as this projects needs full control over an empty clean moodle site, it needs to switch to other branches...

IMO given the size of the cache directory contents we could just leave the cache there or create another clean_webapp_data.sh

[dmonllao]

Proposal adding a new clean_webapp_data.sh

[rajeshtaneja]

I think it is nice solution, but surely would be good to fix permissions in webapp/lib.php https://github.com/rajeshtaneja/moodle-performance-comparison/commit/ab22037f8d957c109ccf007881c3e1b521b4b5df

[dmonllao]

As commented in https://github.com/moodlehq/moodle-performance-comparison/issues/47#issuecomment-62337645 that would not be enough; up to you, whatever you decide I'm fine with it.

[rajeshtaneja]

Sure David, I am saying with your patch we should include fix for webapp/lib.php That will not need clean_webapp_data.sh to be executed as sudo for later runs.

[dmonllao]

As discussed in HQ, I've added 3 branches (master, 28 and 27) including the new script and moving permissions to 775.

The new script, run by www-data, deletes cache/ (and if in future there are more temp dirs created by the web app) contents, also the ones created before this patch gets integrated.

In our nightly CI server we will:

• Run ./clean_webapp_data.sh manually using www-data user to clean all cache/ 755 contents
• Include jenkins and www-data in the same group (Rajesh is confident about this solution)
• In monthly basis, run ./clean_data.sh and ./clean_webapp_data.sh using jenkins user

[dmonllao]

The branches are

• git://github.com/dmonllao/moodle-performance-comparison issue-47-master
• git://github.com/dmonllao/moodle-performance-comparison issue-47-28
• git://github.com/dmonllao/moodle-performance-comparison issue-47-27

[rajeshtaneja]

Thanks David,

Branches merged :)