Address pipe remove on reload and add pipe reuse

[ericcarlschwartz]

this addresses issue #4635

if you'd like to test and let me know if you like this @ema feel free

also reuses existing pipes so we don't just create a new one for every reload as discussed with @ema and @SolidWallOfCode at the summit

[ericcarlschwartz]

let me know what i can do to address build issues if anything

[bryancall]

How can a file be a regular file and a fifo?

[zwoop]

This needs to go to 9.0.0 presumably ?

[SolidWallOfCode]

Eric's moved on, we'll probably need to close this out since it's not going to get fixed. @bneradt is working on this now.

[ericcarlschwartz]

hey sorry for the delay but i can get brian’s comment addressed so y’all can get this in

…

On Tue, Oct 22, 2019 at 9:06 AM Alan M. Carroll ***@***.***> wrote: Eric's moved on, we'll probably need to close this out since it's not going to get fixed. @bneradt <https://github.com/bneradt> is working on this now. — You are receiving this because you were assigned. Reply to this email directly, view it on GitHub <#6017?email_source=notifications&email_token=AAMM5W52BWY4VEFOBX3WE6LQP4QH5A5CNFSM4I74RHVKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEB6JMMQ#issuecomment-545035826>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAMM5WZHL2GYPK2I5XBGBCTQP4QH5ANCNFSM4I74RHVA> .

[bneradt]

I've reproduced what @ema described with his excellent description in issue 4635.

I applied @ericcarlschwartz 's change from this PR (using '||' instead of '&&' per Bryan's comment), and indeed it addresses the majority of Emanuele's issues. However, I notice that traffic_server still seems to be leaking the file descriptors with each -HUP against traffic_manager:

$ lsof test_all_headers.pipe 
COMMAND     PID    USER   FD   TYPE DEVICE SIZE/OFF      NODE NAME
[TS_MAIN] 26124 bneradt   53w  FIFO  252,2      0t0 314977783 test_all_headers.pipe
[TS_MAIN] 26124 bneradt   55w  FIFO  252,2      0t0 314977783 test_all_headers.pipe
[TS_MAIN] 26124 bneradt   56w  FIFO  252,2      0t0 314977783 test_all_headers.pipe
cat       27085 bneradt    3r  FIFO  252,2      0t0 314977783 test_all_headers.pipe

I'll look into what can be tweaked to address the leak.

[bneradt]

Digging through LogObject.cc, this patch (added on top of Eric's) cleans up the leaked file descriptors:

$ git diff
diff --git a/proxy/logging/LogObject.cc b/proxy/logging/LogObject.cc
index 0a0bc9f..8752561 100644
--- a/proxy/logging/LogObject.cc
+++ b/proxy/logging/LogObject.cc
@@ -181,6 +181,8 @@ LogObject::~LogObject()
     if (m_host_list.count()) {
       m_host_list.clear();
     }
+  } else {
+    m_logFile->close_file();
   }
   ats_free(m_basename);
   ats_free(m_filename);

[bneradt]

Testing this out locally, removing the unlink seems to get the behavior that addresses the concerns @ema raised.

However, do we have to worry about the comment at line 974:
https://github.com/apache/trafficserver/pull/6017/files#diff-75bc20fd8082b64381dec2cf402275feR974

It seems to indicate that the unlink is intentional so that the metadata is correct. Maybe the comment is incorrect though?

[bryancall]

what does the meta data files say about the fifo in your testing?

[bneradt]

Reading through the code, I see that the meta file is only used for log rolling which is not applicable to pipes. We don't even instantiate a BaseLogFile (which creates and maintains the meta file) for pipes:

LogFile::LogFile(const char *name, const char *header, LogFileFormat format, uint64_t signature, size_t ascii_buffer_size,
                 size_t max_line_size)
  : m_file_format(format),
    m_name(ats_strdup(name)),
    m_header(ats_strdup(header)),
    m_signature(signature),
    m_max_line_size(max_line_size)
{
  if (m_file_format != LOG_FILE_PIPE) {    <<<<<<<< Note created for pipes.
    m_log = new BaseLogFile(name, m_signature);
    m_log->set_hostname(Machine::instance()->hostname);
  } else {
    m_log = nullptr;
  }

In any case, the comment about unlinking the pipe to maintain the metadata for it seems to be a mistaken comment (assuming by metadata the author meant the meta file).

@ericcarlschwartz : I'll create a new PR with the close() call and manage the PR from there.

Thanks!
Brian

[shinrich]

Issue was addressed in PR #6076. Closing this PR.

[zwoop]

Remember to remove Project / Milestone etc. on closed PRs.