Skip to content

Conversation

@blizzz
Copy link
Member

@blizzz blizzz commented Jun 29, 2018

was #9760 – rebased

jernst added 3 commits June 29, 2018 10:37
Added a unit test

Signed-off-by: Johannes Ernst <jernst@indiecomputing.com>
Changed name of default system (not systemd) logger from ownCloud to Nextcloud, to be consistent
Fixed license per #9760 (comment)
Pulled upstream updates

Signed-off-by: Johannes Ernst <jernst@indiecomputing.com>
Signed-off-by: Johannes Ernst <jernst@indiecomputing.com>
Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de>
@blizzz blizzz added this to the Nextcloud 14 milestone Jun 29, 2018

public function __construct(IConfig $config) {
openlog($config->getSystemValue('syslog_tag', 'ownCloud'), LOG_PID | LOG_CONS, LOG_USER);
openlog($config->getSystemValue('syslog_tag', 'Nextcloud'), LOG_PID | LOG_CONS, LOG_USER);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Chaning of this tag should be in the release notes.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will add it to the crucial changes document for now.

…n the web server error log

Signed-off-by: Morris Jobke <hey@morrisjobke.de>
@MorrisJobke
Copy link
Member

I also tested a bit the error cases and it caused a white page of death. So I tweaked a bit the error handling to not fail super hard if the logger throws an exception itself. So now we have the "black on white - there happened an internal server error" as well as a log to the web servers error log about an unhandled exception containing the hint and the message. 👍 from my side

@MorrisJobke
Copy link
Member

@rullzer @blizzz Please have a final look at my last commit.

@blizzz
Copy link
Member Author

blizzz commented Jun 29, 2018

@MorrisJobke 👍 (i cannot review here)

@MorrisJobke MorrisJobke added 4. to release Ready to be released and/or waiting for tests to finish and removed 3. to review Waiting for reviews labels Jun 29, 2018
@rullzer
Copy link
Member

rullzer commented Jun 29, 2018

sure 👍

@blizzz
Copy link
Member Author

blizzz commented Jun 29, 2018

The same issue popped up again, just with a different file (PHP Fatal error: Uncaught RuntimeException: opendir(/dev/shm/data-autotest/owncloud.db-shm). Perhaps somewhere cleaning up fails? I pimped the method now to also deal with unwhitelisted files in that dir and unlink them. Let's see. cc @nickvergessen as you were working on tearDownAfterClassCleanStrayDataFiles couple years ago :D

@blizzz
Copy link
Member Author

blizzz commented Jun 29, 2018

^ that made things only worse 😩

\OC::$server->getLogger()->logException($ex, array('app' => 'index'));
\OC::$server->getLogger()->logException($ex2, array('app' => 'index'));
} catch (Throwable $e) {
// no way to log it properly - but to avoid a white page of death we try harder and ignore this one here
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe fall back to error_loging the exception message so there is at least an error message somewhere

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is one, because it is logged in the print already. If there happen so many errors, then it is likely to be the same root cause. And logging it twice (both times while creating the logger) doesn't make it more valuable.

print("More details can be found in the server log.\n");

// and then throw it again to log it at least to the web server error log
throw $e;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@icewind1991 This one logs it then in the web server error log, but allows to print something before things break apart.

@blizzz blizzz force-pushed the feature/9760/systemd-logger branch from bc0f2f8 to 0dd2621 Compare June 29, 2018 13:00
@blizzz
Copy link
Member Author

blizzz commented Jun 29, 2018

Next attempt: the db-shm file is now white listed (y no complain about db-wal file?) and the FileLog tests delete the log files it created.

UPDATE:

(y no complain about db-wal file

now it does. didn't do it locally (i could reproduce otherwise).

@blizzz blizzz force-pushed the feature/9760/systemd-logger branch 2 times, most recently from aaab568 to 40eb104 Compare June 29, 2018 13:25
@blizzz
Copy link
Member Author

blizzz commented Jun 29, 2018

Nah. hydra.

Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de>
@blizzz blizzz force-pushed the feature/9760/systemd-logger branch from 40eb104 to 83339ae Compare June 29, 2018 22:30
@blizzz
Copy link
Member Author

blizzz commented Jun 29, 2018

The image for "nodb-php7.2" was pushed to nextcloudci/php7.2:php7.2-12, which in itself only added the systemd module. But apparently it also updated php-redis, which behaves differently when an Redis connection cannot be established: it fires Exceptions now instead of emitting warnings. The failing test configuration has Redis disabled, but because of this they are not skipped. Further the exception we receive is "Connection refused" which I consider too general to just safely ignore it. For now it reverted the image back to 11 for that configuration and it seems to work again. The drama around the stray files… weird symptom leading to wrong directions. Does not happen anymore.

@blizzz
Copy link
Member Author

blizzz commented Jun 29, 2018

For the record, in -12 we have php-redis 4.0.0-1+0~20180412074203.5+jessie~1.gbp24a357,
in -11 it is 3.1.6-1+0~20180104152238.4+jessie~1.gbp3cf38a

@blizzz blizzz merged commit 2f8ebe2 into master Jun 29, 2018
@blizzz blizzz deleted the feature/9760/systemd-logger branch June 29, 2018 22:57
@blizzz
Copy link
Member Author

blizzz commented Jun 29, 2018

thanks @jernst :)

@jernst
Copy link
Contributor

jernst commented Jun 29, 2018

No problem. I need this!!

@MorrisJobke
Copy link
Member

There was a regression: fixed by #10080

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

4. to release Ready to be released and/or waiting for tests to finish enhancement feature: logging

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants