-
Notifications
You must be signed in to change notification settings - Fork 7
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
Feature/dac 300 weaver compose #114
Conversation
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 got some questions, but overall this is a nice PR.
birdhouse/optional-components/testthredds/docker-compose-extra.yml
Outdated
Show resolved
Hide resolved
birdhouse/optional-components/weaver/conf.extra-service.d/weaver.conf.template
Outdated
Show resolved
Hide resolved
birdhouse/optional-components/weaver/config/weaver/data_sources.json.template
Outdated
Show resolved
Hide resolved
birdhouse/optional-components/weaver/config/weaver/weaver.ini.template
Outdated
Show resolved
Hide resolved
birdhouse/optional-components/weaver/config/weaver/wps_processes.yml.template
Outdated
Show resolved
Hide resolved
Jenkins-bot failing due to Ouranosinc/PAVICS-e2e-workflow-tests#58 |
birdhouse/optional-components/testthredds/docker-compose-extra.yml
Outdated
Show resolved
Hide resolved
birdhouse/optional-components/weaver/conf.extra-service.d/weaver.conf.template
Outdated
Show resolved
Hide resolved
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 volume mapping needs to be reviewed.
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.
Just starting review. Not done yet. 1 change request: move Weaver to birdhouse/components/ instead.
I thought the whole point was to make it optional? I refactored everything as optional for this... |
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.
Finished first pass of my review. Still need to deploy it and play with it. Will probably have more feedbacks once I actually try to use it.
Big component, great work plugging into the stack. I think you got the hang of how the pluggable architecture work. As one of my comment, does weaver allow other birds to plug themselves into weaver config?
birdhouse/components/weaver/config/weaver/data_sources.json.template
Outdated
Show resolved
Hide resolved
birdhouse/components/weaver/config/weaver/wps_processes.yml.template
Outdated
Show resolved
Hide resolved
' | ||
# extend the original 'VARS' from 'birdhouse/pavics-compose.sh' to employ them for template substitution | ||
# adding them to 'VARS', they will also be validated in case of override of 'default.env' using 'env.local' | ||
VARS="$VARS $EXTRA_VARS" |
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.
You sure you want to add to VARS
and not OPTIONAL_VARS
? I have not checked but if all these have default values, OPTIONAL_VARS
is much more appropriate. See https://github.com/bird-house/birdhouse-deploy/blob/e10a2ef9bd77498f1af22267cad5d181d0da65ef/birdhouse/components/monitoring/default.env for example
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.
All variables are mandatory in the sense that if they are overridden by an empty string, compose won't work at all.
So although they are optional in terms of "no need any override", they must be validated.
My understanding of OPTIONAL_VARS
was that empty value will still pass.
Was I wrong? If so, I don't understand why there are 2 variables.
birdhouse/optional-components/wps-healthchecks/docker-compose-extra.yml
Outdated
Show resolved
Hide resolved
…otes the requirement
…eeded, default to original hardcoded values
…ners + remove default open db ports in favor of optional-component to be explicitly defined when needed
New test triggered with latest master merged & added changes. Results should appear eventually... |
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.
LGTM with latest changes for canarie-api.
Exact. As long as Weaver is not activated, nothing should break. That's why I can accept this PR as-is. But once it is activated, we know it can start the first time but we have no idea how it integrate itself into the upgrade process (autodeploy) because that scenario is not tested by the |
E2E Test ResultsDACCS-iac Pipeline ResultsBuild URL : http://daccs-jenkins.crim.ca:80/job/DACCS-iac-birdhouse/720/Result : failure BIRDHOUSE_DEPLOY_BRANCH : feature/DAC-300-weaver-compose DACCS_CONFIGS_BRANCH : master PAVICS_E2E_WORKFLOW_TESTS_BRANCH : master PAVICS_SDI_BRANCH : master DESTROY_INFRA_ON_EXIT : true PAVICS_HOST : https://host-140-8.rdext.crim.ca PAVICS-e2e-workflow-tests Pipeline ResultsTests URL : http://daccs-jenkins.crim.ca:80/job/PAVICS-e2e-workflow-tests/job/master/637/NOTEBOOK TEST RESULTS |
E2E Test ResultsDACCS-iac Pipeline ResultsBuild URL : http://daccs-jenkins.crim.ca:80/job/DACCS-iac-birdhouse/721/Result : failure BIRDHOUSE_DEPLOY_BRANCH : feature/DAC-300-weaver-compose DACCS_CONFIGS_BRANCH : master PAVICS_E2E_WORKFLOW_TESTS_BRANCH : master PAVICS_SDI_BRANCH : master DESTROY_INFRA_ON_EXIT : true PAVICS_HOST : https://host-140-8.rdext.crim.ca PAVICS-e2e-workflow-tests Pipeline ResultsTests URL : http://daccs-jenkins.crim.ca:80/job/PAVICS-e2e-workflow-tests/job/master/638/NOTEBOOK TEST RESULTS |
E2E Test ResultsDACCS-iac Pipeline ResultsBuild URL : http://daccs-jenkins.crim.ca:80/job/DACCS-iac-birdhouse/722/Result : failure BIRDHOUSE_DEPLOY_BRANCH : feature/DAC-300-weaver-compose DACCS_CONFIGS_BRANCH : master PAVICS_E2E_WORKFLOW_TESTS_BRANCH : master PAVICS_SDI_BRANCH : master DESTROY_INFRA_ON_EXIT : true PAVICS_HOST : https://host-140-8.rdext.crim.ca PAVICS-e2e-workflow-tests Pipeline ResultsTests URL : http://daccs-jenkins.crim.ca:80/job/PAVICS-e2e-workflow-tests/job/master/639/NOTEBOOK TEST RESULTS |
E2E Test ResultsDACCS-iac Pipeline ResultsBuild URL : http://daccs-jenkins.crim.ca:80/job/DACCS-iac-birdhouse/724/Result : failure BIRDHOUSE_DEPLOY_BRANCH : feature/DAC-300-weaver-compose DACCS_CONFIGS_BRANCH : master PAVICS_E2E_WORKFLOW_TESTS_BRANCH : master PAVICS_SDI_BRANCH : master DESTROY_INFRA_ON_EXIT : true PAVICS_HOST : https://host-140-8.rdext.crim.ca PAVICS-e2e-workflow-tests Pipeline ResultsTests URL : http://daccs-jenkins.crim.ca:80/job/PAVICS-e2e-workflow-tests/job/master/641/NOTEBOOK TEST RESULTS |
E2E Test ResultsDACCS-iac Pipeline ResultsBuild URL : http://daccs-jenkins.crim.ca:80/job/DACCS-iac-birdhouse/725/Result : failure BIRDHOUSE_DEPLOY_BRANCH : feature/DAC-300-weaver-compose DACCS_CONFIGS_BRANCH : master PAVICS_E2E_WORKFLOW_TESTS_BRANCH : master PAVICS_SDI_BRANCH : master DESTROY_INFRA_ON_EXIT : false PAVICS_HOST : https://host-140-8.rdext.crim.ca PAVICS-e2e-workflow-tests Pipeline ResultsTests URL : http://daccs-jenkins.crim.ca:80/job/PAVICS-e2e-workflow-tests/job/master/642/NOTEBOOK TEST RESULTS |
…s just on the edge of completion in some cases
E2E Test ResultsDACCS-iac Pipeline ResultsBuild URL : http://daccs-jenkins.crim.ca:80/job/DACCS-iac-birdhouse/727/Result : failure BIRDHOUSE_DEPLOY_BRANCH : feature/DAC-300-weaver-compose DACCS_CONFIGS_BRANCH : master PAVICS_E2E_WORKFLOW_TESTS_BRANCH : master PAVICS_SDI_BRANCH : master DESTROY_INFRA_ON_EXIT : true PAVICS_HOST : https://host-140-4.rdext.crim.ca PAVICS-e2e-workflow-tests Pipeline ResultsTests URL : http://daccs-jenkins.crim.ca:80/job/PAVICS-e2e-workflow-tests/job/master/643/NOTEBOOK TEST RESULTS |
…rce restart weaver/worker
E2E Test ResultsDACCS-iac Pipeline ResultsBuild URL : http://daccs-jenkins.crim.ca:80/job/DACCS-iac-birdhouse/733/Result : failure BIRDHOUSE_DEPLOY_BRANCH : feature/DAC-300-weaver-compose 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/645/NOTEBOOK TEST RESULTS |
E2E Test ResultsDACCS-iac Pipeline ResultsBuild URL : http://daccs-jenkins.crim.ca:80/job/DACCS-iac-birdhouse/734/Result : failure BIRDHOUSE_DEPLOY_BRANCH : feature/DAC-300-weaver-compose DACCS_CONFIGS_BRANCH : master PAVICS_E2E_WORKFLOW_TESTS_BRANCH : master PAVICS_SDI_BRANCH : master DESTROY_INFRA_ON_EXIT : false 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/646/NOTEBOOK TEST RESULTS |
# Overview Add any variation of sub-dir notebooks for artifact collection in Jenkins. For instance, following were omitted: https://github.com/Ouranosinc/pavics-sdi/tree/master/docs/source/notebook-components At the same time, ensure following are not matched: https://github.com/Ouranosinc/pavics-sdi/tree/master/docs/source/deprecated ## Related Issue / Discussion Useful for collecting results from bird-house/birdhouse-deploy#114
@tlvu just so you know, indeed this process is not tested by the IAC, but it is tested by our staging environment that will be upgraded as soon as the PR is merged. So if Hirondelle goes well after the merge we will know everything is OK. I thought that we all agreed on that procedure. A PR is accepted based on the IAC tests. An organization deploys on prod using their fork after they check their own staging with master. An organization is not required to validate the upgrade on other organizations staging but can support them in case of documented issues. |
@dbyrns Just to be sure, there are 2 aspects to an upgrade during the autodeploy: 1) the code, 2) the exiting data that might need migration. So I understand for number 2, each org with each different production data will be in a better position to test their own data. However for number 1, that code can be tested by the feature owner, weather it can run successfully inside the autodeploy process, at least without any errors for the most basic case where no data migration is required. Both the autodeploy and weaver are optional components so it's not a blocking issue so I accepted this PR. But it could still introduce a regression for autodeploy and I think we all agree regression is bad. So that's why I mentioned in comment #114 (comment) "Just indicate in the changelog clearly that autodeploy is not tested, in case another org (PCIC) wants to activate this." This is to prevent another org that already has autodeploy currently activated to be careful with activating weaver at the same time. And my comment has been ignored. |
For the changelog, you are right, a message could have been added, surely an honest mistake. |
## Overview Move `mongodb` network to `mongodb` image directly and update `phoenix`/`weaver` images accordingly. ## Changes **Non-breaking changes** - Apply ``mongodb`` network to ``mongodb`` image in order to allow ``phoenix`` to properly reference it. - Remove ``mongodb`` definition from ``./components/weaver`` since the extended ``mongodb`` network is already provided. **Breaking changes** - n/a ## Related Issue / Discussion - Resolves #114 (comment)
@@ -0,0 +1,9 @@ | |||
SERVICES['node']['monitoring'].update({ |
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.
@fmigneault another problem detected after go-live in prod yesterday:
This weaver_config.py
should be added to .gitignore
, else autodeploy is broken because it would detect an unclean checkout.
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.
Do you want a separate PR for this fix only (with version bump and all?).
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.
Absolutely. A hotfix also deserve a proper release to have an identifier to communicate to your users. You would do the same for Magpie I figure.
Live instance (at CRIM with daccs-iac):
https://host-140-36.rdext.crim.ca/twitcher/ows/proxy/weaver
(OpenAPI doc endpoint for info: https://host-140-36.rdext.crim.ca/weaver/api)
Example of
flyingpigeon
's WPS-1subset_bbox
process converted to WPS-REST via Weaver:https://host-140-36.rdext.crim.ca/weaver/providers/flyingpigeon/processes/subset_bbox
Dependencies
DATA_PERSIST_ROOT
and server-related docs)note: until they are merged, changes of this PR will continue to contain all of them
preview of simplified modifications: changes
all above PRs are combined in branch tmp-everything-except-weaver to generate the this diff changes
Changes
x-logging
config to reuse definition across compose. (fixes Free disk space problem due to docker images logging #10)DATA_PERSIST_ROOT
to allow changing default/data
where persistence dirs are generated.Extra
wps-healthchecks
(developed while testing Weaver integration, but not directly needed by it any more).database-external-ports
, to allow exposing port for dev instances, but that should definitely not be open by default.Resolves https://www.crim.ca/jira/browse/DAC-25
Resolves https://www.crim.ca/jira/browse/DAC-300
Resolves crim-ca/weaver#13
daccs-iac config : https://www.crim.ca/stash/projects/DACCS/repos/daccs-configs/pull-requests/1