-
Notifications
You must be signed in to change notification settings - Fork 6
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
Bump canarie api 1.0.0 #452
Conversation
@mishaschwartz |
run tests |
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.
Didn't test it, but looks ok conceptually. Just a few comments to confirm.
Might need to push the component into the CI's EXTRA_CONF_DIRS
to have a valid test run. It monitors the /canarie/
endpoint to check when the instance is ready to perform test.
canarie-api: &canarie-volumes | ||
volumes: | ||
- ./components/twitcher/config/canarie-api/canarie_api_monitoring.py:${CANARIE_MONITORING_EXTRA_CONF_DIR}/twitcher_canarie_api_monitoring.py:ro | ||
canarie-api-cron: *canarie-volumes |
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.
Does this anchor work when all components are combined into one big compose file?
Doesn't it get overridden by whichever definition was loaded first/last/whatever?
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 it works, docker compose parses yaml file individually first and then combines them into one big definition. Yaml anchors are applied at the file level.
nginx && \ | ||
gunicorn -b 0.0.0.0:2000 --workers 1 --log-level=DEBUG --timeout 30 -k gevent canarieapi.wsgi \ | ||
" | ||
exec /bin/sh -c "gunicorn -b 0.0.0.0:2000 --workers 1 --log-level=DEBUG --timeout 30 -k gevent canarieapi.wsgi" |
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.
Can this be left multi-line? It's easier to read diffs.
@@ -1,7 +1,7 @@ | |||
# Folder inside "proxy" container to drop extra monitoring config | |||
export CANARIE_MONITORING_EXTRA_CONF_DIR="/conf.d" | |||
|
|||
export PROXY_IMAGE="pavics/canarieapi:0.7.1" | |||
export CANARIE_IMAGE="pavics/canarieapi:1.0.0" |
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.
Use a CANARIE_VERSION
and other CANARIE_...
similar variables as for other components?
CHANGES.md
Outdated
[//]: # (list changes here, using '-' for each new entry, remove this when items are added) | ||
## Changes | ||
|
||
- bump canarie-api version to 1.0.0 |
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.
use reference URL to the tagged versions in the source repo
- canarie-data:/data/ | ||
- ./components/canarie-api/docker_configuration.py:/config/docker_configuration.py:ro | ||
- ./components/canarie-api/entrypoint-cron:/entrypoint:ro | ||
- proxy-logs:/logs/:ro |
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.
Might be worth adding a note that this path aligns with
birdhouse-deploy/birdhouse/components/canarie-api/docker_configuration.py.template
Line 17 in bd7e9b8
'access_log': '/logs/${PROXY_LOG_FILE}' |
E2E Test ResultsDACCS-iac Pipeline ResultsBuild URL : http://daccs-jenkins.crim.ca:80/job/DACCS-iac-birdhouse/2636/Result ✅ SUCCESS BIRDHOUSE_DEPLOY_BRANCH : bump-canarie-api-1.0.0 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/1596/NOTEBOOK TEST RESULTS |
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.
Minor feedback about data persistance. Not a blocker because currently it's the same behavior already, if the container is delete, we lose proxy logs and canarie-api stats.
|
||
volumes: | ||
canarie-data: | ||
proxy-logs: |
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.
Very much like this PR. It's much cleaner this way that the canarie-api monitoring is separate from the proxy container.
I just wonder why are you using data-volume and not direct persistence on disk. And if you really need to use data-volume, the current way we will lose the data on ./pavics-compose.sh down -v
.
To avoid losing data on ./pavics-compose.sh down -v
, use external data-volume like the monitoring component.
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.
A few reasons...
No other component is storing logs in a persistent manner. I'm not opposed to this but this could be a larger discussion about log retention that isn't related to this PR
the current way we will lose the data on
./pavics-compose.sh down -v
Sure, but are you bringing the stack down with the -v
flag often? Personally, I never ever use the -v
flag in production for that reason, and I'll delete a volume if needed manually.
I actually find it annoying to have to delete the externally created jupyterhub, prometheus, grafana, etc. volumes when I'm testing and want to start with a blank slate. I personally would actually like to see the externally created volumes become non-external (or a bind-mount from disk) for this reason.
Like I mentioned above, we don't currently have a defined log retention mechanism/policy. I'd love to add one, but maybe we can defer the discussion of how to manage logs when we discuss that.
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 think the way it is defined currently is the right way.
If a platform maintainer wants to persist the logs at a specific location/drive/bind-mount, they simply need to add the relevant overrides to define the volume.
The flexibility is left up to the maintainer.
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.
Sure, but are you bringing the stack down with the
-v
flag often?
I actually do use -v
from time to time. Otherwise, all the anonymous volume are not delete and next up
new anonymous volumes will be created and the old anonymous volumes are only deleted with docker volume prune --all
.
Here is an example of anonymous volume:
- /tmp |
@tlvu @fmigneault I'm thinking this needs a minor version bump to 2.3.0. Is that ok with everyone? |
Yes you are technically introducing a new component (canarie-api) so yes, a minor bump. |
Oh sorry, new container, the component already exist. |
Overview
This version of canarie-api permits running the proxy (nginx) container independently of the canarie-api application. This makes it easier to monitor the logs of canarie-api and proxy containers simultaneously and allows for the configuration files for canarie-api to be mapped to the canarie-api containers where appropriate.
Changes
Non-breaking changes
Breaking changes
Related Issue / Discussion
Additional Information
Links to other issues or sources.
CI Operations
birdhouse_daccs_configs_branch: master
birdhouse_skip_ci: false