Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Very simple log rotation #3961

Merged
merged 6 commits into from Aug 28, 2013
Merged

Very simple log rotation #3961

merged 6 commits into from Aug 28, 2013

Conversation

@bartv2
Copy link
Member

bartv2 commented Jul 5, 2013

@karlitschek @DeepDiver1975

should prevent extreme growd like #1304, #2699 and #3402.

class Rotate extends \OC\BackgroundJob\Job {
const LOG_SIZE_LIMIT = 104857600; // 100 MB
public function run($logFile) {
$filesize = filesize($logFile);

This comment has been minimized.

Copy link
@VicDeo

VicDeo Jul 5, 2013

Member

@bartv2 supress warning may be?

protected function rotate($logfile) {
$rotated_logfile = $logfile.'.1';
rename($logfile, $rotated_logfile);

This comment has been minimized.

Copy link
@VicDeo

VicDeo Jul 5, 2013

Member

camelCase varable please :) 🐫

@bantu

This comment has been minimized.

Copy link
Member

bantu commented Jul 5, 2013

So you are limitting the total logfile size to about 200 MB. This isn't very clear from the code and commit message. Please add class documentation.

@bantu

This comment has been minimized.

Copy link
Member

bantu commented Jul 5, 2013

Thanks.

@VicDeo

This comment has been minimized.

Copy link
Member

VicDeo commented Jul 6, 2013

👍 tested

@VicDeo

This comment has been minimized.

Copy link
Member

VicDeo commented Jul 6, 2013

@owncloud-bot retest this please

@karlitschek

This comment has been minimized.

Copy link
Contributor

karlitschek commented Jul 6, 2013

👍

@bantu

This comment has been minimized.

Copy link
Member

bantu commented Jul 6, 2013

Jenkins errors seem to be related to this patch. https://ci.tmit.eu/job/pull-request-analyser/2706/console

@bantu

This comment has been minimized.

Copy link
Member

bantu commented Jul 8, 2013

Tests (still) complain about no such table: oc_jobs.

@bantu

This comment has been minimized.

Copy link
Member

bantu commented Jul 8, 2013

Not sure whether/why commit f42754d is required in order to achieve "Very simple log rotation". Commit message does not explain this.

@bartv2

This comment has been minimized.

Copy link
Member Author

bartv2 commented Jul 9, 2013

That commit is not needed for this pr, but to get a better description of the error. Also the new jenkins is messing with the build status.

@@ -95,13 +95,13 @@ public function getAll() {
public function getNext() {
$lastId = $this->getLastJob();
$query = \OC_DB::prepare('SELECT `id`, `class`, `last_run`, `argument` FROM `*PREFIX*jobs` WHERE `id` > ? ORDER BY `id` ASC', 1);

This comment has been minimized.

Copy link
@MorrisJobke

MorrisJobke Jul 10, 2013

Contributor

OC_DB::prepare seems unneeded because this gets called inside of executeAudited https://github.com/owncloud/core/blob/master/lib/db.php#L445

(You already changed that in the above sql statements)

This comment has been minimized.

Copy link
@bartv2

bartv2 Jul 10, 2013

Author Member

executeAudited doesn't have the ability to limit the query, and that is needed here

This comment has been minimized.

Copy link
@MorrisJobke

MorrisJobke Jul 10, 2013

Contributor

It has: just pass in array('sql' => $sql, 'limit' => 123) instead of just $sql ...just read the code I linked

This comment has been minimized.

Copy link
@MorrisJobke

MorrisJobke Jul 10, 2013

Contributor

it's the if clause checking is_array($stmt)

* location and manage that with your own tools.
*/
class Rotate extends \OC\BackgroundJob\Job {
const LOG_SIZE_LIMIT = 104857600; // 100 MB

This comment has been minimized.

Copy link
@MorrisJobke

MorrisJobke Jul 10, 2013

Contributor

100 MiB or 104.8576 MB ;)

This comment has been minimized.

Copy link
@bartv2

bartv2 Jul 10, 2013

Author Member

pedantic ;-)

@MorrisJobke

This comment has been minimized.

Copy link
Contributor

MorrisJobke commented Jul 10, 2013

@DeepDiver1975 Weird jenkins/github issue: green checkmark but failed test

@bartv2

This comment has been minimized.

Copy link
Member Author

bartv2 commented Jul 10, 2013

@kabum that is the new jenkins, it's messing with the status

@bartv2

This comment has been minimized.

Copy link
Member Author

bartv2 commented Jul 10, 2013

fixed the jenkins problem and removed commit f42754d, the commit is saved in branch executeAudited-Backgroundjobs-Joblist for now

class Rotate extends \OC\BackgroundJob\Job {
const LOG_SIZE_LIMIT = 104857600; // 100 MiB
public function run($logFile) {
$filesize = @filesize($logFile);

This comment has been minimized.

Copy link
@DeepDiver1975

DeepDiver1975 Jul 14, 2013

Member

We potentially have the 2GB barrier issue on 32-bit systems.
Possible solution: https://github.com/owncloud/core/blob/master/lib/files/storage/local.php#L100
Or rotate in case of negative size as well.

This comment has been minimized.

Copy link
@bantu

bantu Jul 15, 2013

Member

Interpret it as an unsigned integer and convert it to a float. Loss of precision is not an issue in this case (but there shouldn't be any anyway).
$filesize = (float) sprintf('%u', @filesize($logFile));
This will increase the limit to 4GB on 32-bit systems and you have to no longer consider negative sizes.

This comment has been minimized.

Copy link
@bantu

bantu Jul 15, 2013

Member

@DeepDiver1975 That solution does not look too good to me. It looks like the filesize of a 5GiB file would likely be returned as 1GiB because in 32-bit signed integer world 0 until 2GiB is positive, 2 GiB until 4 GiB is negative and due to wrapping/overflow, 4 GiB until 6 GiB is positive again (and so on). You would probably have to call getFileSizeFromOS() unconditionally if you want to get the exact filesize on 32-bit systems.

This comment has been minimized.

Copy link
@DeepDiver1975

DeepDiver1975 Jul 15, 2013

Member

Interesting finding - THX.
Can you prove that and submit a pull request? THX

This comment has been minimized.

Copy link
@bantu

bantu Jul 16, 2013

Member

@DeepDiver1975 We are getting kind of off-topic (which is probably caused by me), but anyway:

$filename = 'filesize.test';

// 80 * 64 MiB = 5 GiB
$chunk = str_repeat("\1", 67108864);
$chunks = 80;

$fp = fopen($filename, 'wb');
while ($chunks)
{
    fwrite($fp, $chunk);
    --$chunks;
}
fclose($fp);

var_dump(filesize($filename));

On 64-bit the result is int(5368709120), on 32-bit the output should be int(1073741824) (sorry, couldn't test).

Edit: Scratch that, on 32-bit it should not be possible create a 5GiB file using PHP. You have to create it in another way.

This comment has been minimized.

Copy link
@bantu

bantu Jul 19, 2013

Member
dd if=/dev/zero of=filesize.test bs=67108864 count=80
php -r "var_dump(filesize('filesize.test'));"
uname -m
int(5368709120)
x86_64
Warning: filesize(): stat failed for filesize.test in Command line code on line 1
bool(false)
i686
}
protected function rotate($logfile) {
$rotatedLogfile = $logfile.'.1';

This comment has been minimized.

Copy link
@DeepDiver1975

DeepDiver1975 Jul 14, 2013

Member

On the second rotation the .1 file will be overwritten - not that nice

This comment has been minimized.

Copy link
@MorrisJobke

MorrisJobke Jul 14, 2013

Contributor

But otherwise the disk usage will raise. At some time it the older log has to be overwritten.

This comment has been minimized.

Copy link
@DeepDiver1975

DeepDiver1975 Jul 14, 2013

Member

But we shall never delete data without user interaction - that's bad habit.
If you fear running out of storage we better add some notifications for the admin

This comment has been minimized.

Copy link
@bantu

bantu Jul 15, 2013

Member

@DeepDiver1975 Hehe. At least the overwriting is documented in the class description.

If you do not like this behaviour, compression could probably be used like so:

rename($logfile, $logfile.'.1');
$src = fopen($logfile.'.1', 'r');
$dest = fopen("compress.zlib://$logfile.1.gz", 'wb');
stream_copy_to_stream($src, $dest);

Should then probably use a timestamp instead of the hardcoded "1" and increase the log filesize to 256 MiB or so.

This comment has been minimized.

Copy link
@DeepDiver1975

DeepDiver1975 Jul 15, 2013

Member

At least the overwriting is documented in the class description.

If documented or not - we shall not do this.

This comment has been minimized.

Copy link
@bartv2

bartv2 Jul 15, 2013

Author Member

then we should just disable this until configured, that way the admin can decide that they want this. Also admins that want to save there owncloud log data could use the syslog output.

try { //if this is executed before the upgrade to the new backgroundjob system is completed it will throw an exception
\OCP\BackgroundJob::registerJob('OC\Log\Rotate', OC_Config::getValue("datadirectory", OC::$SERVERROOT.'/data').'/owncloud.log');
} catch (Exception $e) {

This comment has been minimized.

Copy link
@Niduroki

Niduroki Jul 23, 2013

Member

Shouldn't something be in here?

This comment has been minimized.

Copy link
@bartv2

bartv2 Jul 23, 2013

Author Member

no see comment on line 566

* location and manage that with your own tools.
*/
class Rotate extends \OC\BackgroundJob\Job {
const LOG_SIZE_LIMIT = 104857600; // 100 MiB

This comment has been minimized.

Copy link
@Niduroki

Niduroki Jul 23, 2013

Member

It would be nice if you could specify a size after which rotation should happen.
100 MiB as a default is okay though.

@Niduroki

This comment has been minimized.

Copy link
Member

Niduroki commented Jul 23, 2013

Besides the already mentioned problems: 👍

@MorrisJobke

This comment has been minimized.

Copy link
Contributor

MorrisJobke commented Aug 26, 2013

@DeepDiver1975 @karlitschek We need a decision on this? Not overwrite growing logs? Default syslog also overwrites itself.

@bartv2 Maybe add a option to the admin backend: "Rotate and overwrite log data (needs up to two times MAX_SIZE of space)" which is enabled by default.

@jancborchardt Opinion on this?

@karlitschek

This comment has been minimized.

Copy link
Contributor

karlitschek commented Aug 26, 2013

I think a config.php option to enable/disable logrotate would be good incl. an option to configure the maximum logfile size. Otherwise let's merge.

@bartv2

This comment has been minimized.

Copy link
Member Author

bartv2 commented Aug 28, 2013

config option added

class Rotate extends \OC\BackgroundJob\Job {
private $max_log_size;
public function run($logFile) {
$this->max_log_size = OC_Config::getValue('log_rotate_size', false);

This comment has been minimized.

Copy link
@MorrisJobke

MorrisJobke Aug 28, 2013

Contributor

\OC_Config <- missing \

@MorrisJobke

This comment has been minimized.

Copy link
Contributor

MorrisJobke commented Aug 28, 2013

If the mentioned issue is fixed everything works fine 👍

@MorrisJobke

This comment has been minimized.

Copy link
Contributor

MorrisJobke commented Aug 28, 2013

@bartv2 I've fixed it, because it was already done locally ;)

@Niduroki

This comment has been minimized.

Copy link
Member

Niduroki commented Aug 28, 2013

@owncloud-bot retest this please

@ghost

This comment has been minimized.

Copy link

ghost commented Aug 28, 2013

Test passed.
Refer to this link for build results: https://ci.owncloud.org/job/pull-request-analyser/746/

MorrisJobke added a commit that referenced this pull request Aug 28, 2013
@MorrisJobke MorrisJobke merged commit 125824b into master Aug 28, 2013
1 check passed
1 check passed
default Merged build finished.
Details
@MorrisJobke MorrisJobke deleted the logrotate branch Aug 28, 2013
@lock lock bot locked as resolved and limited conversation to collaborators Aug 27, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
7 participants
You can’t perform that action at this time.