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

[Martin Konrad]

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

[Andrew Johnson (anj)]

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)]

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

[Andrew Johnson (anj)]

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)]

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)]

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)]

@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)]

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

[Andrew Johnson (anj)]

@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)]

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

[mdavidsaver (mdavidsaver)]

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

I was misreading.

[Andrew Johnson (anj)]

Core Group review at ESS: Approve for 3.14 branch.

review: Approve

diff --git a/src/util/iocLogServer.c b/src/util/iocLogServer.c
index f5694fc..f42aa45 100644
--- a/src/util/iocLogServer.c
+++ b/src/util/iocLogServer.c
@@ -347,11 +347,6 @@ static int openLogFile (struct ioc_log_server *pserver)
 {
 	enum TF_RETURN ret;
 
-	if (ioc_log_file_limit==0u) {
-		pserver->poutfile = stderr;
-		return IOCLS_ERROR;
-	}
-
 	if (pserver->poutfile && pserver->poutfile != stderr){
 		fclose (pserver->poutfile);
 		pserver->poutfile = NULL;
@@ -627,7 +622,7 @@ static void writeMessagesToLog (struct iocLogClient *pclient)
 				strlen(pclient->ascii_time) + nchar + 3u;
 		assert (nTotChar <= INT_MAX);
 		ntci = (int) nTotChar;
-		if ( pclient->pserver->filePos+ntci >= pclient->pserver->max_file_size ) {
+		if ( pclient->pserver->max_file_size && pclient->pserver->filePos+ntci >= pclient->pserver->max_file_size ) {
 			if ( pclient->pserver->max_file_size >= pclient->pserver->filePos ) {
 				unsigned nPadChar;
 				/*
@@ -771,7 +766,7 @@ static int getConfig(void)
 			&EPICS_IOC_LOG_FILE_LIMIT, 
 			&ioc_log_file_limit);
 	if(status>=0){
-		if (ioc_log_file_limit<=0) {
+		if (ioc_log_file_limit < 0) {
 			envFailureNotify (&EPICS_IOC_LOG_FILE_LIMIT);
 			return IOCLS_ERROR;
 		}