-
Notifications
You must be signed in to change notification settings - Fork 7
Remove proxy component's dependency on scheduler and scheduler-job-logrotate-nginx
#631
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
Changes from 2 commits
6e16c8d
7395451
37eb208
fef573b
9b271e5
32a65f7
9391419
78fb779
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,12 +1,10 @@ | ||
| services: | ||
| proxy: | ||
| volumes: | ||
| # Note: proxy-logs volume is defined in the proxy-logs-volume component | ||
| - proxy-logs:${PROXY_LOG_DIR} | ||
| prometheus-log-parser: | ||
| volumes: | ||
| - proxy-logs:/var/log/proxy | ||
| environment: | ||
| - PROXY_LOG_FILE=${PROXY_LOG_FILE} | ||
|
|
||
| volumes: | ||
| proxy-logs: |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| config/proxy/conf.extra-directives.d/proxy-log.conf |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,2 @@ | ||
| access_log ${PROXY_LOG_PATH} main; | ||
mishaschwartz marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| access_log /var/log/nginx/access.log main; | ||
mishaschwartz marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,8 @@ | ||
| services: | ||
| proxy: | ||
| volumes: | ||
| - ./optional-components/proxy-log-volume/config/proxy/conf.extra-directives.d:/etc/nginx/conf.extra-directives.d/proxy-log-volume:ro | ||
| - proxy-logs:${PROXY_LOG_DIR} | ||
|
|
||
| volumes: | ||
| proxy-logs: |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,24 @@ | ||
| # PROXY_LOG_DIR is set to a different directory than the other nginx logs (/var/log/nginx) because the other | ||
| # nginx logs are written to files that are actually symlinks to the stdout and stderr streams. We do not want | ||
| # to share these symlinks in a volume because they will necessarily not point to the correct target once they are | ||
| # mounted in a different container. To avoid potential confusion if trying to read from these symlinked log files | ||
| # in a different container, it is better to just not make them shareable (as a volume) in the first place. | ||
| export PROXY_LOG_DIR="/logs/" | ||
| export PROXY_LOG_FILE="access_file.log" | ||
mishaschwartz marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| export PROXY_LOG_PATH='${PROXY_LOG_DIR}/${PROXY_LOG_FILE}' | ||
|
|
||
| export DELAYED_EVAL=" | ||
| $DELAYED_EVAL | ||
| PROXY_LOG_PATH | ||
| " | ||
|
|
||
| export OPTIONAL_VARS=" | ||
| $OPTIONAL_VARS | ||
| \$PROXY_LOG_FILE | ||
| \$PROXY_LOG_PATH | ||
| " | ||
|
|
||
| COMPONENT_DEPENDENCIES=" | ||
| ./components/scheduler | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For completeness, this The 2 other components that depend on
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The other two depend on proxy because if proxy isn't also included those components will fail entirely. This one won't fail if proxy isn't enabled, it just won't do much.
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Its
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The The code assumes that
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
So with this assumption, why do we need to explicitly add
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not blocking, just curious, with this assumption, why do we need to explicitly add
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It shouldn't be there, you're right. I've removed it. |
||
| ./optional-components/scheduler-job-logrotate-nginx | ||
mishaschwartz marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| " | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,3 @@ | ||
| if ! echo "${BIRDHOUSE_EXTRA_CONF_DIRS}" | grep -q 'scheduler-job-logrotate-nginx[[:space:]]*$'; then | ||
| log WARN 'Access logs for the proxy component are being written to a regular file but no log rotation is enabled. You may want to enable the scheduler and scheduler-job-logrotate-nginx components.' | ||
| fi |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,13 +1,10 @@ | ||
| export SCHEDULER_JOB_LOGROTATE_NGINX_JOB_SCHEDULE="* * * * *" # UTC | ||
| export SCHEDULER_JOB_LOGROTATE_NGINX_ROTATE=150 | ||
| export SCHEDULER_JOB_LOGROTATE_NGINX_SIZE=1M | ||
| export SCHEDULER_JOB_LOGROTATE_NGINX_JOB_SCHEDULE="0,20,40 * * * *" # UTC | ||
| export SCHEDULER_JOB_LOGROTATE_NGINX_ROTATE=50 | ||
| export SCHEDULER_JOB_LOGROTATE_NGINX_SIZE=20M | ||
|
|
||
| VARS=" | ||
| $VARS | ||
| \$SCHEDULER_JOB_LOGROTATE_NGINX_JOB_SCHEDULE | ||
| \$SCHEDULER_JOB_LOGROTATE_NGINX_ROTATE | ||
| \$SCHEDULER_JOB_LOGROTATE_NGINX_SIZE | ||
| " | ||
|
|
||
| # TODO: deprecate this component once https://github.com/bird-house/birdhouse-deploy/issues/618 is resolved | ||
| # remember that deprecating involves adding an entry to optional-components/.gitignore |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,3 @@ | ||
| if ! echo "${ALL_CONF_DIRS}" | grep -q 'proxy-log-volume[[:space:]]*$'; then | ||
| log WARN 'The scheduler-job-logrotate-nginx scheduler job is enabled but proxy access logs are not being written to a regular file that needs rotation. This WILL cause problems! Please disable the scheduler-job-logrotate-nginx job.' | ||
| fi |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By default, the access_log will now go to the
proxycontainer stdout, which can be a lot. We probably should bump the log retention for theproxycontainer herebirdhouse-deploy/birdhouse/components/proxy/docker-compose-extra.yml
Lines 6 to 7 in 26d2c90
max-filefrom 10 to 30? 40?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm actually inclined to leave it as the default since we have the ability to backup logs with the backup scheduler job if you want to retain them for longer.
Increasing
max-filewould just mean that you have more files to backup and a higher chance that they'd contain overlapping logs which just means you're keeping duplicate log data in the backups.Of course that entirely depends on how often you're backing up the logs. If you're waiting weeks between backups then you'll definitely lose log data.
Maybe we should just make this configurable? What do you think @tlvu and @fmigneault
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, configurable is good.
Our server does not have as much traffic as PAVICS, nor acts like a prod, so it is not that critical if some logs are lost or not backed up. I prefer to have sufficient cleanup to avoid disk space problems by default.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See #636