Skip to content
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.

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

added newsyslog rules for FreeBSD #1724

Open
wants to merge 4 commits into
base: develop
Choose a base branch
from

Conversation

felixjogris
Copy link

@felixjogris felixjogris commented Nov 28, 2019

Description

waagent logs to /var/log/waagent.log. On FreeBSD, this logfile happens to grow. In contrast to Linux, there is no logrotate. Instead, every FreeBSD installation comes with newsyslog, which is similar to logrotate. The configuration of newsyslog is modular, i.e. the main configuration file /etc/newsyslog.conf also reads every file in the directory /etc/newsyslog.conf.d. My patch places a newsyslog config file for waagent in /etc/newsyslog.conf.d, which rotates the log file every midnight, and keeps up to 7 historical, compressed logfiles. setup.py was modified to copy over this configuration file on FreeBSD.


PR information

  • The title of the PR is clear and informative.
  • There are a small number of commits, each of which has an informative message. This means that previously merged commits do not appear in the history of the PR. For information on cleaning up the commits in your pull request, see this page.
  • Except for special cases involving multiple contributors, the PR is started from a fork of the main repository, not a branch.
  • If applicable, the PR references the bug/issue that it fixes in the description.
  • New Unit tests were added for the changes made and Travis.CI is passing.

Quality of Code and Contribution Guidelines


This change is Reviewable

@codecov
Copy link

codecov bot commented Nov 28, 2019

Codecov Report

Merging #1724 into develop will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff            @@
##           develop    #1724   +/-   ##
========================================
  Coverage    68.66%   68.66%           
========================================
  Files           82       82           
  Lines        11737    11737           
  Branches      1645     1645           
========================================
  Hits          8059     8059           
  Misses        3326     3326           
  Partials       352      352           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update dd531d6...078dd89. Read the comment docs.

pgombar
pgombar previously approved these changes Dec 4, 2019
Copy link
Contributor

@pgombar pgombar left a comment

Choose a reason for hiding this comment

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

LGTM. @narrieta can you take a look?

@@ -0,0 +1 @@
/var/log/waagent.log 644 7 * @T00 J /var/run/waagent.pid
Copy link
Member

@narrieta narrieta Dec 5, 2019

Choose a reason for hiding this comment

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

A couple of questions:

  • If I'm reading the documentation correctly, by adding waagent.pid, the agent would be sent a SIGHUP on rotation... how does the agent handle this signal?

  • From the documentation it was not clear to me how optional fields are specified. I see that you skipped owner (2nd field). Are the TABs between "waagent.log" and "644" significant? I also see a TAB between "*" and "@t00"

Thanks

Copy link
Author

Choose a reason for hiding this comment

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

Hi, man page of newsyslog.conf reads as: "Each line has five mandatory fields and four optional fields, separated with whitespace". Any number of TABs or SPACEs count as whitespace.

Copy link
Author

Choose a reason for hiding this comment

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

Hi,

SIGHUP is not needed as FileAppender.write in azurelinuxagent/common/logger.py closes the logfile after each write. I removed the pid file from the newsyslog config.

BR, Felix

@vrdmr vrdmr removed their assignment Apr 19, 2020
@vrdmr vrdmr removed their request for review April 19, 2020 21:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants