Remove proxy component's dependency on scheduler and scheduler-job-logrotate-nginx
#631
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
github-actions
bot
added
documentation
Collaborator
E2E Test ResultsDACCS-iac Pipeline ResultsBuild URL : http://daccs-jenkins.crim.ca:80/job/DACCS-iac-birdhouse/3948/Result 🆘 ABORTEDBIRDHOUSE_DEPLOY_BRANCH : remove-proxy-dependency DACCS_IAC_BRANCH : master DACCS_CONFIGS_BRANCH : master PAVICS_E2E_WORKFLOW_TESTS_BRANCH : master PAVICS_SDI_BRANCH : master DESTROY_INFRA_ON_EXIT : true PAVICS_HOST : https://host-140-216.rdext.crim.ca
|
fmigneault
approved these changes
Jan 12, 2026
Member
fmigneault
left a comment
fmigneault
left a comment
There was a problem hiding this comment.
I have not tested it on a server, but it looks fine from static analysis.
tlvu
requested changes
Jan 15, 2026
Collaborator
tlvu
left a comment
tlvu
left a comment
There was a problem hiding this comment.
Thanks for making proxy "independent" again !
Below are some changes requested, but it's great a PR overall.
| '$status $body_bytes_sent "$http_referer" ' | ||
| '"$http_user_agent" "$http_x_forwarded_for"'; | ||
|
|
||
| access_log ${PROXY_LOG_PATH} main; |
Collaborator
There was a problem hiding this comment.
By default, the access_log will now go to the proxy container stdout, which can be a lot. We probably should bump the log retention for the proxy container here
max-file from 10 to 30? 40?
Collaborator
Author
There was a problem hiding this comment.
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-file would 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
Member
There was a problem hiding this comment.
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.
| " | ||
|
|
||
| COMPONENT_DEPENDENCIES=" | ||
| ./components/scheduler |
Collaborator
There was a problem hiding this comment.
For completeness, this proxy-log-volume component also depend on proxy !
The 2 other components that depend on proxy-log-volume also explicitly depend on proxy, they won't need that anymore, although I think our component dependency calculation will handle duplicate dependencies fine.
Collaborator
Author
There was a problem hiding this comment.
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.
Its docker-compose-extra.yml reference the proxy container so if it is not there, I think compose up will error out, not sure, can you test?
Collaborator
Author
There was a problem hiding this comment.
The proxy component is in the BIRDHOUSE_DEFAULT_CONF_DIRS list so if it's missing then we have a whole lot of other problems than just this.
The code assumes that BIRDHOUSE_DEFAULT_CONF_DIRS is always there (which is also why we shouldn't need to explicitly add any of these components to COMPONENT_DEPENDENCIES).
Collaborator
There was a problem hiding this comment.
The
proxycomponent is in theBIRDHOUSE_DEFAULT_CONF_DIRSlist so if it's missing then we have a whole lot of other problems than just this.
So with this assumption, why do we need to explicitly add proxy as dependency in the other 2 components then?
Collaborator
There was a problem hiding this comment.
Not blocking, just curious, with this assumption, why do we need to explicitly add proxy as dependency in the other 2 components (canarie-api and prometheus-log-parser) then?
Collaborator
Author
There was a problem hiding this comment.
It shouldn't be there, you're right. I've removed it.
...nal-components/proxy-log-volume/config/proxy/conf.extra-directives.d/proxy-log.conf.template
Show resolved
Hide resolved
Collaborator
|
On the topic of productizing Proposed: export SCHEDULER_JOB_LOGROTATE_NGINX_JOB_SCHEDULE="*/20 * * * *" # UTC
export SCHEDULER_JOB_LOGROTATE_NGINX_ROTATE=50
export SCHEDULER_JOB_LOGROTATE_NGINX_SIZE=20M But I wonder if increasing the file size, would the We should also not hardcode the docker image in |
Collaborator
Author
|
@tlvu Are you suggesting we run it every 20 minutes or once per hour at minute 20?
It would take longer for canarie-api but I think that 20 minutes worth of logs isn't that bad. I also have a proposed change to canarie-api (keep an eye out for that later) that would mean that this would only be an issue on restart instead of requiring canarie-api to read through the entire file every minute. This is not a problem at all for prometheus-log-parser. |
Collaborator
E2E Test ResultsDACCS-iac Pipeline ResultsBuild URL : http://daccs-jenkins.crim.ca:80/job/DACCS-iac-birdhouse/3965/Result 🆘 ABORTEDBIRDHOUSE_DEPLOY_BRANCH : remove-proxy-dependency DACCS_IAC_BRANCH : master DACCS_CONFIGS_BRANCH : master PAVICS_E2E_WORKFLOW_TESTS_BRANCH : master PAVICS_SDI_BRANCH : master DESTROY_INFRA_ON_EXIT : true PAVICS_HOST : https://host-140-91.rdext.crim.ca
|
Collaborator
Yes I meant every 20 minute and But thinking about this again, maybe every 10 mins is safer for high traffic production server. Let's rather be safe than sorry.
Superb ! |
tlvu
reviewed
Jan 21, 2026
...nal-components/proxy-log-volume/config/proxy/conf.extra-directives.d/proxy-log.conf.template
Outdated
Show resolved
Hide resolved
birdhouse/optional-components/scheduler-job-logrotate-nginx/config.yml.template
Outdated
Show resolved
Hide resolved
Collaborator
E2E Test ResultsDACCS-iac Pipeline ResultsBuild URL : http://daccs-jenkins.crim.ca:80/job/DACCS-iac-birdhouse/3982/Result ✅ SUCCESSBIRDHOUSE_DEPLOY_BRANCH : remove-proxy-dependency DACCS_IAC_BRANCH : master DACCS_CONFIGS_BRANCH : master PAVICS_E2E_WORKFLOW_TESTS_BRANCH : master PAVICS_SDI_BRANCH : master DESTROY_INFRA_ON_EXIT : true PAVICS_HOST : https://host-140-91.rdext.crim.ca PAVICS-e2e-workflow-tests Pipeline ResultsTests URL : http://daccs-jenkins.crim.ca:80/job/PAVICS-e2e-workflow-tests/job/master/616/NOTEBOOK TEST RESULTS |
Collaborator
E2E Test ResultsDACCS-iac Pipeline ResultsBuild URL : http://daccs-jenkins.crim.ca:80/job/DACCS-iac-birdhouse/3984/Result 🆘 ABORTEDBIRDHOUSE_DEPLOY_BRANCH : remove-proxy-dependency DACCS_IAC_BRANCH : master DACCS_CONFIGS_BRANCH : master PAVICS_E2E_WORKFLOW_TESTS_BRANCH : master PAVICS_SDI_BRANCH : master DESTROY_INFRA_ON_EXIT : true PAVICS_HOST : https://host-140-91.rdext.crim.ca
|
Collaborator
|
You also forgot to remove the hardcode of the logrotate docker image in |
Collaborator
E2E Test ResultsDACCS-iac Pipeline ResultsBuild URL : http://daccs-jenkins.crim.ca:80/job/DACCS-iac-birdhouse/3991/Result ❌ FAILUREBIRDHOUSE_DEPLOY_BRANCH : remove-proxy-dependency DACCS_IAC_BRANCH : master DACCS_CONFIGS_BRANCH : master PAVICS_E2E_WORKFLOW_TESTS_BRANCH : master PAVICS_SDI_BRANCH : master DESTROY_INFRA_ON_EXIT : true PAVICS_HOST : https://host-140-154.rdext.crim.ca PAVICS-e2e-workflow-tests Pipeline ResultsTests URL : http://daccs-jenkins.crim.ca:80/job/PAVICS-e2e-workflow-tests/job/master/622/NOTEBOOK TEST RESULTS |
tlvu
approved these changes
Jan 26, 2026
Collaborator
tlvu
left a comment
tlvu
left a comment
There was a problem hiding this comment.
All good for me now, just missing the 2>&1 thanks !
birdhouse/optional-components/scheduler-job-logrotate-nginx/config.yml.template
Outdated
Show resolved
Hide resolved
birdhouse/optional-components/scheduler-job-logrotate-nginx/config.yml.template
Outdated
Show resolved
Hide resolved
| " | ||
|
|
||
| COMPONENT_DEPENDENCIES=" | ||
| ./components/scheduler |
Collaborator
There was a problem hiding this comment.
Not blocking, just curious, with this assumption, why do we need to explicitly add proxy as dependency in the other 2 components (canarie-api and prometheus-log-parser) then?
Collaborator
E2E Test ResultsDACCS-iac Pipeline ResultsBuild URL : http://daccs-jenkins.crim.ca:80/job/DACCS-iac-birdhouse/3996/Result ✅ SUCCESSBIRDHOUSE_DEPLOY_BRANCH : remove-proxy-dependency DACCS_IAC_BRANCH : master DACCS_CONFIGS_BRANCH : master PAVICS_E2E_WORKFLOW_TESTS_BRANCH : master PAVICS_SDI_BRANCH : master DESTROY_INFRA_ON_EXIT : true PAVICS_HOST : https://host-140-91.rdext.crim.ca PAVICS-e2e-workflow-tests Pipeline ResultsTests URL : http://daccs-jenkins.crim.ca:80/job/PAVICS-e2e-workflow-tests/job/master/625/NOTEBOOK TEST RESULTS |
mishaschwartz
added a commit
that referenced
this pull request
Jan 27, 2026
## Overview Introduce a new component `components/logging` that sets default logging options for all docker compose services started by `birdhouse-deploy`. This component is enabled by default. The default value is set by the `BIRDHOUSE_LOGGING_DEFAULT` environment variable. To change the default value, set the `BIRDHOUSE_LOGGING_DEFAULT` to a JSON string in the local environment file that contains a valid [docker compose logging configuration](https://docs.docker.com/reference/compose-file/services/#logging). For example, to set the default driver to "local" set the following in your local environment file: ```sh export BIRDHOUSE_LOGGING_DEFAULT='{"driver": "local"}' ``` You can also override logging options for a single service using environment variables using a variable `BIRDHOUSE_LOGGING_<service_name>` where `<service_name>` is the uppercase name of the docker compose service with hyphens replaced with underscores. For example, to set the default driver to "local" only for the `weaver-worker` service: ```sh export BIRDHOUSE_LOGGING_WEAVER_WORKER='{"driver": "local"}' ``` Logging options can can also be set directly in a component's ``docker-compose-extra.yml`` file. The order of precedence for logging options are as follows: 1. logging options specified by `BIRDHOUSE_LOGGING_<service_name>` environment variable 2. logging options set in a `docker-compose-extra.yml` file 3. logging options specified by `BIRDHOUSE_LOGGING_DEFAULT` environment variable ## Changes **Non-breaking changes** - Adds new required component **Breaking changes** - None ## Related Issue / Discussion - Initially from: #631 (comment) ## Additional Information ## CI Operations <!-- The test suite can be run using a different DACCS config with ``birdhouse_daccs_configs_branch: branch_name`` in the PR description. To globally skip the test suite regardless of the commit message use ``birdhouse_skip_ci`` set to ``true`` in the PR description. Using ``[<cmd>]`` (with the brackets) where ``<cmd> = skip ci`` in the commit message will override ``birdhouse_skip_ci`` from the PR description. Such commit command can be used to override the PR description behavior for a specific commit update. However, a commit message cannot 'force run' a PR which the description turns off the CI. To run the CI, the PR should instead be updated with a ``true`` value, and a running message can be posted in following PR comments to trigger tests once again. --> birdhouse_daccs_configs_branch: master birdhouse_skip_ci: false
Collaborator
E2E Test ResultsDACCS-iac Pipeline ResultsBuild URL : http://daccs-jenkins.crim.ca:80/job/DACCS-iac-birdhouse/3999/Result ❌ FAILUREBIRDHOUSE_DEPLOY_BRANCH : remove-proxy-dependency DACCS_IAC_BRANCH : master DACCS_CONFIGS_BRANCH : master PAVICS_E2E_WORKFLOW_TESTS_BRANCH : master PAVICS_SDI_BRANCH : master DESTROY_INFRA_ON_EXIT : true PAVICS_HOST : https://host-140-154.rdext.crim.ca PAVICS-e2e-workflow-tests Pipeline ResultsTests URL : http://daccs-jenkins.crim.ca:80/job/PAVICS-e2e-workflow-tests/job/master/627/NOTEBOOK TEST RESULTS |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Overview
Creates new settings in
optional-components/proxy-log-volumethat create theproxy-logsdocker volume as well as instructing Nginx to write access logs to an additional log file (specified byPROXY_LOG_PATH). These settings are included as aCOMPONENT_DEPENDENCYin components that require access to the theproxyaccess logs as a regular file. If no components require access to these logs as a regular file then theproxycomponent will only write access logs to the stdout stream for that container.Right now, the only components that require access to logs in this way are
components/canarie-apiandoptional-components/prometheus-log-parser. Both of these now includeoptional-components/proxy-log-volumeas aCOMPONENT_DEPENDENCY.Note: this means that if no optional components require
optional-components/proxy-log-volumeas a dependency then logs from theproxycontainer will only be written to stdout/stderr. This means that there is no need for any additional custom log rotation handling since the logs are handled directly by docker. This means that theproxyservice itself no longer need to be dependant on theschedulerandscheduler-job-logrotate-nginxcomponents.Note: a previous discussion suggested that logs could be parsed directly from the stdout stream of the
proxycontainer. However, there is no way to do so that doesn't require very hacky workarounds. Possible solutions that were explored and rejected include:proxycontainer from the host to the relevant containers.Rejected because this practice is highly discouraged by docker as the actual storage location of log files is not standardized and may be changed in future versions.
proxycontainer.Rejected because this is very difficult to set up and is untested when then mounted to other containers.
Also, a different named pipe would be required for each consumer which is currently very difficult to set up using birdhouse's deployment tools.
Breaking Change: if a custom component (not included in this repository) uses the
proxy-logsnamed volume. It must now includeoptional-components/proxy-log-volumeas aCOMPONENT_DEPENDENCYfor that custom component.Breaking Change: if
SCHEDULER_JOB_BACKUP_ARGSspecifies-l proxyexplicitly (not-l '*') then this should be changed to-l proxy-log-volumesince the backup script has been moved. Note that it is not necessary to specify-l proxy-log-volumeif--birdhouse-logsis also specified because the log data is identical in both.Changes
Non-breaking changes
Breaking changes
Related Issue / Discussion
Additional Information
CI Operations
birdhouse_daccs_configs_branch: master
birdhouse_skip_ci: false