Canarie-api should not be a mandatory component#310
Merged
mishaschwartz merged 7 commits intomasterfrom Apr 11, 2023
Merged
Conversation
Collaborator
|
Can one of the admins verify this patch? |
Collaborator
E2E Test ResultsDACCS-iac Pipeline ResultsBuild URL : http://daccs-jenkins.crim.ca:80/job/DACCS-iac-birdhouse/1424/Result : success BIRDHOUSE_DEPLOY_BRANCH : proxy-rewrite DACCS_CONFIGS_BRANCH : master PAVICS_E2E_WORKFLOW_TESTS_BRANCH : master PAVICS_SDI_BRANCH : master DESTROY_INFRA_ON_EXIT : true PAVICS_HOST : https://host-140-118.rdext.crim.ca PAVICS-e2e-workflow-tests Pipeline ResultsTests URL : http://daccs-jenkins.crim.ca:80/job/PAVICS-e2e-workflow-tests/job/master/1056/NOTEBOOK TEST RESULTS |
tlvu
approved these changes
Apr 10, 2023
Collaborator
tlvu
left a comment
tlvu
left a comment
There was a problem hiding this comment.
Totally approve the spirit of this PR. However I probably will not be able to perform a proper review of this PR and your 2 other PR as well this week. I have to give a hand for the paper David is alluring about in this comment #300 (comment)
However I do not want to hold you back either. So I propose that as long as your PR is back-compat (no manual intervention required to roll-out, to clean up after roll-out, to ensure subsequent autodeploy works) and Francis is okay with it, then go ahead and merge.
If later we find some problems, ex: the component load order changed and it's a regression, we can simply just fix it in a subsequent PR, just like the component load order. There is no requirement a PR is perfect on first try. Software have bugs all the time.
tlvu
reviewed
Apr 10, 2023
Collaborator
tlvu
left a comment
tlvu
left a comment
There was a problem hiding this comment.
Quick review, just minor comment.
birdhouse/config/proxy/default.env
Outdated
|
|
||
| # Folder inside "proxy" container to drop extra monitoring config | ||
| export CANARIE_MONITORING_EXTRA_CONF_DIR="/conf.d" | ||
| export PROXY_IMAGE="nginx" |
Collaborator
There was a problem hiding this comment.
I would rather pin the specific latest version at this point in time. Because latest version is not reproducible. It changes on each build.
Reproducibility is very important if we want to be able to diagnose problem across multiple different deployments of PAVICS.
For a given PAVICS tag, we should know precisely which version of the binary is being deployed. We do allow image override via env.local but that's mostly for testing or the staging env. For production deployment, I think and hope most users will want to stick to the tested versions coming from the stack.
Collaborator
Author
There was a problem hiding this comment.
Sure and I agree. This is literally just a copy from https://github.com/Ouranosinc/CanarieAPI/blob/master/Dockerfile#L1 to maintain the existing settings but I'll explicitly pin it to whatever is the latest going forward.
| # Folder inside "proxy" container to drop extra monitoring config | ||
| export CANARIE_MONITORING_EXTRA_CONF_DIR="/conf.d" | ||
|
|
||
| export PROXY_IMAGE="pavics/canarieapi:0.3.5" |
Collaborator
There was a problem hiding this comment.
Thanks for keeping this old behavior for backward compat.
Collaborator
Author
Collaborator
E2E Test ResultsDACCS-iac Pipeline ResultsBuild URL : http://daccs-jenkins.crim.ca:80/job/DACCS-iac-birdhouse/1428/Result : success BIRDHOUSE_DEPLOY_BRANCH : proxy-rewrite DACCS_CONFIGS_BRANCH : master PAVICS_E2E_WORKFLOW_TESTS_BRANCH : master PAVICS_SDI_BRANCH : master DESTROY_INFRA_ON_EXIT : true PAVICS_HOST : https://host-140-36.rdext.crim.ca PAVICS-e2e-workflow-tests Pipeline ResultsTests URL : http://daccs-jenkins.crim.ca:80/job/PAVICS-e2e-workflow-tests/job/master/1059/NOTEBOOK TEST RESULTS |
github-actions
bot
added
ci/operations
Collaborator
E2E Test ResultsDACCS-iac Pipeline ResultsBuild URL : http://daccs-jenkins.crim.ca:80/job/DACCS-iac-birdhouse/1432/Result : failure BIRDHOUSE_DEPLOY_BRANCH : proxy-rewrite 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 Infrastructure deployment failed. Instance has not been destroyed. @matprov |
Member
|
@mishaschwartz |
Collaborator
Author
|
@fmigneault of course. I'll do that today. What is the current state for #284 btw? Is it waiting on any more changes or is it almost ready to merge? |
Member
|
@mishaschwartz It only needs some final validation to confirm that the monitoring is working correctly, but I did not get the chance to invest more time into it lately. |
tlvu
reviewed
Apr 12, 2023
| @@ -1,2 +1,5 @@ | |||
| docker_configuration.py | |||
| conf.extra-service.d/canarie-api.conf | |||
Collaborator
There was a problem hiding this comment.
@mishaschwartz Our staging env found this file became "new" file for git and broke the subsequent autodeploy !
Could you re-add this file to .gitignore?
Collaborator
Author
Collaborator
There was a problem hiding this comment.
Completely un-related to Jenkins. I merged #308 yesterday and today I went onto our staging machine and see it still did not autodeploy that merge. So I checked the logs and found out the reason.
mishaschwartz
added a commit
that referenced
this pull request
Apr 12, 2023
## Overview In order to maintain backwards compatibility, old files that are no longer present in the code should be kept in the gitignore files. This adds back one file to the relevant .gitignore file that no longer exists under `conf.extra-service.d/canarie-api.conf`. ## Changes **Non-breaking changes** Add old file to gitignore **Breaking changes** None ## Related Issue / Discussion See: #310 (comment) ## Additional Information
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
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
Canarie-api is currently deployed in the same container as the nginx reverse proxy
service meaning that it is not possible to deploy nginx without including canarie-api.
This means that it is currently not possible to run this deployment without canarie-api
or use a different monitoring application. This change fully separates the configuration
for canarie-api and nginx so that a user can choose to run nginx with or without canarie-api.
Canarie-api has been kept on the DEFAULT_CONF_DIRS list so that canarie-api is included by
default, for backwards-compatibility. In order to run nginx without canarie-api, remove the
./conf/canarie-apiline from the DEFAULT_CONF_DIRS environment variable.A user can also choose a specific version of the nginx docker image to use by specifying
the PROXY_IMAGE environment variable (default is "nginx"). Note that if canarie-api is used
(by including the
./conf/canarie-apiline in DEFAULT_CONF_DIRS), then the PROXY_IMAGEvariable will be ignored.
Changes
Non-breaking changes
Makes it possible to run the stack without canarie-api
Breaking changes
None
Related Issue / Discussion
Related to the various "pluggable component" issues. This makes canarie-api a "pluggable component".
Additional Information