Skip to content

Address pipe remove on reload and add pipe reuse #6017

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.

Sign up for GitHub

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

Closed
ericcarlschwartz wants to merge 1 commit into from
Closed

Address pipe remove on reload and add pipe reuse #6017

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 18 additions & 7 deletions proxy/logging/LogFile.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -170,14 +170,25 @@ LogFile::open_file()
bool file_exists = LogFile::exists(m_name);

if (m_file_format == LOG_FILE_PIPE) {
// setup pipe
if (mkfifo(m_name, S_IRUSR | S_IWUSR | S_IRGRP) < 0) {
if (errno != EEXIST) {
Error("Could not create named pipe %s for logging: %s", m_name, strerror(errno));
return LOG_FILE_COULD_NOT_CREATE_PIPE;
bool pipe_exists = false;
// reuse existing pipe if available
struct stat stat_p;
stat(m_name, &stat_p);
if (S_ISREG(stat_p.st_mode) && S_ISFIFO(stat_p.st_mode)) {
Copy link
Contributor

@bryancall bryancall Oct 16, 2019

Choose a reason for hiding this comment

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

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

pipe_exists = true; // pipe exists try to reuse
Debug("log-file", "Reusing existing pipe %s for logging", m_name);
}

// setup pipe if not there
if (!pipe_exists) {
if (mkfifo(m_name, S_IRUSR | S_IWUSR | S_IRGRP) < 0) {
if (errno != EEXIST) {
Error("Could not create named pipe %s for logging: %s", m_name, strerror(errno));
return LOG_FILE_COULD_NOT_CREATE_PIPE;
}
} else {
Debug("log-file", "Created named pipe %s for logging", m_name);
}
} else {
Debug("log-file", "Created named pipe %s for logging", m_name);
}

// now open the pipe
Expand Down
3 changes: 1 addition & 2 deletions proxy/logging/LogObject.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -966,7 +966,7 @@ LogObjectManager::_solve_filename_conflicts(LogObject *log_object, int maxConfli
// roll old filename so the new object can use the filename
// it requested (previously we used to rename the NEW file
// but now we roll the OLD file), or if the log object writes
// to a pipe, just remove the file if it was open as a pipe
// to a pipe, just don't roll

bool roll_file = true;

Expand All @@ -988,7 +988,6 @@ LogObjectManager::_solve_filename_conflicts(LogObject *log_object, int maxConfli
roll_file = false;
} else {
if (S_ISFIFO(s.st_mode)) {
unlink(filename);
Copy link
Contributor

@bneradt bneradt Oct 22, 2019

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

@bryancall bryancall Oct 22, 2019

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@bneradt bneradt Oct 25, 2019

Choose a reason for hiding this comment

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

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

roll_file = false;
}
}
Expand Down