Merge ~info-martin-konrad/epics-base:fix-logserver-file-limit into epics-base:3.14

Proposed by Martin Konrad on 2018-08-13
Status: Merged
Approved by: Andrew Johnson on 2018-09-05
Approved revision: 8e42f516b0f6552857c7eba4171b32353f75a807
Merged at revision: d8214a45319ed9a476182d186168bb6a89cc8c3f
Proposed branch: ~info-martin-konrad/epics-base:fix-logserver-file-limit
Merge into: epics-base:3.14
Diff against target: 34 lines (+2/-7)
1 file modified
src/util/iocLogServer.c (+2/-7)
Reviewer Review Type Date Requested Status
Andrew Johnson 2018-08-13 Approve on 2018-09-05
mdavidsaver Approve on 2018-08-13
Review via email: mp+353038@code.launchpad.net

Commit message

iocLogserver: allow log file limit to be disabled

According to the Application Developer's Guide setting the environment variable EPICS_IOC_LOG_FILE_LIMIT=0 should disable the limit on the file size.

This fixes lp:1786858

To post a comment you must log in.
Andrew Johnson (anj) wrote :

Hi Martin, you can't propose a merge into 3.14 or 3.15 if you checked out 3.16, you have to start from a checkout of the branch you're going to propose to merge into, or an earlier version of that branch, or possibly an older branch.

review: Disapprove
Martin Konrad (info-martin-konrad) wrote :

Sorry, I didn't expect anyone to look at it that quickly ;-) I already rebased my fix on 3.14.

Andrew Johnson (anj) wrote :

Ah, this looks much better now.

I was considering proposing that we unbundle the IOC log server completely, since it isn't terribly flexible. I've been playing with Logstash recently, which can be configured to act as an IOC log server (just set it up for tcp input on the appropriate port number) and offers a lot more control over the back-end. Other logging tools should offer similar features, so we probably don't need to maintain our own server nowadays.

mdavidsaver (mdavidsaver) wrote :

This change looks like it does what is intended.

I assume the goal here is to use an external tool like logrotate to bound file size?

> I was considering proposing that we unbundle the IOC log server completely

+1

If we keep it, I think it is reasonable to retain the ability to log to stderr (or better stdout) as this would mesh well with systemd daemon logging.

review: Approve
Martin Konrad (info-martin-konrad) wrote :

I wasn't aware that the network communication used by the IOC log facilities is _that_ simple. In that case I guess we might also want to feed the data directly into Logstash rather than writing it to a file which gets monitored by Filebeat :-)

As soon as we have a standard tool + some instructions for setting it up I would be happy to retire iocLogserver - its code looks like it hasn't been touched in a long time...

Martin Konrad (info-martin-konrad) wrote :

@davidsaver: I don't think it actually wrote any log data to stderr. Note that the next line says "return IOCLS_ERROR" which causes it to print a misleading message like

"File access problems to `ioc.log' because `Success'

before it quits. Maybe this feature got broken over the decades? If we want to do this I would probably prefer to write the data to stdout and iocLogserver's messages to stderr.

Martin Konrad (info-martin-konrad) wrote :

I have created a separate issue for the discussion about retiring iocLogserver: https://bugs.launchpad.net/epics-base/+bug/1786966

Andrew Johnson (anj) wrote :

@mdavidsaver I'm not sure what you mean by "retain the ability to log to stderr".

Are you talking about how (absent a call to eltc(0)) the errlog system copies its message stream to stderr as well as sending it to any registered listeners? I'm not suggesting that we touch the client side of errlog at all, just that we drop modules/libcom/src/log/iocLogServer.c from the 7.0 branch.

@martin: My logstash configuration that completely replaces the above program looks like this:

input {
    tcp {
        host => "${EPICS_IOC_LOG_INET:localhost}"
        port => "${EPICS_IOC_LOG_PORT:7004}"
        id => "iocLog-${EPICS_IOC_LOG_PORT:7004}"
        codec => "line"
    }
}

output {
    file {
        path => "/path/to/logs/ioc-%{+YYYY-MM-dd}.json"
    }
}

That output configuration creates a new file every day, you would probably send the messages straight off to your log database instead.

I'm wondering whether we might want to adjust the errlog code to support the multiline input codec by ensuring that there is a space after any '\n' that appears before the end of a message. I haven't found any multi-line messages which would need that yet so maybe not (and it could be done by hand anyway so it isn't really necessary).

The epicsStackTrace.c code will not work well with the single-line codec but could be easily adjusted to work with the multiline one; there may be other examples that I'm not aware of. Here's the codec configuration I suggest we aim to support:

        codec => multiline {
            pattern => "^\s"
            what => "previous"
        }

review: Approve
Andrew Johnson (anj) wrote :

I will copy my logstash comments to the new issue...

mdavidsaver (mdavidsaver) wrote :

> @mdavidsaver I'm not sure what you mean by "retain the ability to log to stderr".

I was misreading.

Andrew Johnson (anj) wrote :

Core Group review at ESS: Approve for 3.14 branch.

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1diff --git a/src/util/iocLogServer.c b/src/util/iocLogServer.c
2index f5694fc..f42aa45 100644
3--- a/src/util/iocLogServer.c
4+++ b/src/util/iocLogServer.c
5@@ -347,11 +347,6 @@ static int openLogFile (struct ioc_log_server *pserver)
6 {
7 enum TF_RETURN ret;
8
9- if (ioc_log_file_limit==0u) {
10- pserver->poutfile = stderr;
11- return IOCLS_ERROR;
12- }
13-
14 if (pserver->poutfile && pserver->poutfile != stderr){
15 fclose (pserver->poutfile);
16 pserver->poutfile = NULL;
17@@ -627,7 +622,7 @@ static void writeMessagesToLog (struct iocLogClient *pclient)
18 strlen(pclient->ascii_time) + nchar + 3u;
19 assert (nTotChar <= INT_MAX);
20 ntci = (int) nTotChar;
21- if ( pclient->pserver->filePos+ntci >= pclient->pserver->max_file_size ) {
22+ if ( pclient->pserver->max_file_size && pclient->pserver->filePos+ntci >= pclient->pserver->max_file_size ) {
23 if ( pclient->pserver->max_file_size >= pclient->pserver->filePos ) {
24 unsigned nPadChar;
25 /*
26@@ -771,7 +766,7 @@ static int getConfig(void)
27 &EPICS_IOC_LOG_FILE_LIMIT,
28 &ioc_log_file_limit);
29 if(status>=0){
30- if (ioc_log_file_limit<=0) {
31+ if (ioc_log_file_limit < 0) {
32 envFailureNotify (&EPICS_IOC_LOG_FILE_LIMIT);
33 return IOCLS_ERROR;
34 }

Subscribers

People subscribed via source and target branches