Enforce the load order of components defined in env.local#304
Merged
mishaschwartz merged 15 commits intomasterfrom Apr 12, 2023
Merged
Enforce the load order of components defined in env.local#304mishaschwartz merged 15 commits intomasterfrom
mishaschwartz merged 15 commits intomasterfrom
Conversation
Collaborator
E2E Test ResultsDACCS-iac Pipeline ResultsBuild URL : http://daccs-jenkins.crim.ca:80/job/DACCS-iac-birdhouse/1378/Result : failure BIRDHOUSE_DEPLOY_BRANCH : load-components-in-defined-order DACCS_CONFIGS_BRANCH : master PAVICS_E2E_WORKFLOW_TESTS_BRANCH : master PAVICS_SDI_BRANCH : master DESTROY_INFRA_ON_EXIT : true PAVICS_HOST : https://host-140-69.rdext.crim.ca PAVICS-e2e-workflow-tests Pipeline ResultsTests URL : http://daccs-jenkins.crim.ca:80/job/PAVICS-e2e-workflow-tests/job/master/1033/NOTEBOOK TEST RESULTS |
fmigneault
requested changes
Mar 25, 2023
Member
There was a problem hiding this comment.
I'm happy to see that explicit $COMPONENT_DEPENDENCIES references are not needed anymore. That is much easier to maintain and less error-prone for new users!
I think dependencies should probably be loaded before the component itself.
By this, I refer to the similar behaviour when importing modules in Python or other language.
For example:
# file a.py
from b import B# file b.py
from c import C# main
from a import AThis will start loading a, but in order to complete loading A, b will have to be loaded completely first.
When starting to load b to get B, C will be loaded until the end of c module.
Import of b then completes, then a, and finally main finishes doing its thing.
This is to avoid an issue where let's say I use weaver variables to define other cowbird variables relevant to weaver. Naturally, weaver parameters must be loaded first (and all its dependencies that can themselves be needed by weaver) before cowbird can use them.
It would make sense to have docker-compose.yml files loaded in the same order that reflects that logic, such that what is reported as COMPOSE_CONF_LIST follows the same dependency resolution order.
I also think that EXTRA_CONF_DIRS should be loaded completely at the end, after all the COMPONENT_DEPENDENCIES, to define "anything left" that wasn't already loaded by other components. By that point, most of the time only explicit overrides will remain, or components that are not a dependency of anything, so it's fine to load last.
I tried running pavics-compose with a merge of #284 + this PR + my custom overrides.
I got the following loading order.
COMPOSE_CONF_LIST=
-f docker-compose.yml
-f ./config/proxy/docker-compose-extra.yml
-f ./config/phoenix/docker-compose-extra.yml
-f ./config/malleefowl/docker-compose-extra.yml
-f ./config/postgres/docker-compose-extra.yml
-f ./config/wps_outputs-volume/docker-compose-extra.yml
-f ./config/data-volume/docker-compose-extra.yml
-f ./config/flyingpigeon/docker-compose-extra.yml
-f ./config/catalog/docker-compose-extra.yml
-f ./config/mongodb/docker-compose-extra.yml
-f ./config/geoserver/docker-compose-extra.yml
-f ./config/finch/docker-compose-extra.yml
-f ./config/raven/docker-compose-extra.yml
-f ./config/hummingbird/docker-compose-extra.yml
-f ./config/thredds/docker-compose-extra.yml
-f ./config/portainer/docker-compose-extra.yml
-f ./config/magpie/docker-compose-extra.yml
-f ./config/twitcher/docker-compose-extra.yml
-f ./config/jupyterhub/docker-compose-extra.yml
-f ./config/frontend/docker-compose-extra.yml
-f ./config/ncwms2/docker-compose-extra.yml
-f ./config/project-api/docker-compose-extra.yml
-f ./config/solr/docker-compose-extra.yml
-f ./components/monitoring/docker-compose-extra.yml
-f ./optional-components/testthredds/docker-compose-extra.yml
-f ./optional-components/test-weaver/docker-compose-extra.yml
-f ./components/weaver/docker-compose-extra.yml
-f ./components/cowbird/docker-compose-extra.yml
-f ../../daccs-env/docker-compose-extra.yml
-f ./config/canarie-api/config/proxy/docker-compose-extra.yml
-f ./config/phoenix/config/proxy/docker-compose-extra.yml
-f ./config/malleefowl/config/data-volume/docker-compose-extra.yml
-f ./config/malleefowl/config/magpie/docker-compose-extra.yml
-f ./config/malleefowl/config/proxy/docker-compose-extra.yml
-f ./config/malleefowl/config/wps_outputs-volume/docker-compose-extra.yml
-f ./config/wps_outputs-volume/config/proxy/docker-compose-extra.yml
-f ./config/flyingpigeon/config/magpie/docker-compose-extra.yml
-f ./config/flyingpigeon/config/wps_outputs-volume/docker-compose-extra.yml
-f ./config/catalog/config/magpie/docker-compose-extra.yml
-f ./config/catalog/config/proxy/docker-compose-extra.yml
-f ./config/geoserver/config/magpie/docker-compose-extra.yml
-f ./config/geoserver/config/proxy/docker-compose-extra.yml
-f ./config/finch/config/magpie/docker-compose-extra.yml
-f ./config/finch/config/proxy/docker-compose-extra.yml
-f ./config/finch/config/wps_outputs-volume/docker-compose-extra.yml
-f ./config/raven/config/magpie/docker-compose-extra.yml
-f ./config/raven/config/proxy/docker-compose-extra.yml
-f ./config/raven/config/wps_outputs-volume/docker-compose-extra.yml
-f ./config/hummingbird/config/data-volume/docker-compose-extra.yml
-f ./config/hummingbird/config/magpie/docker-compose-extra.yml
-f ./config/hummingbird/config/wps_outputs-volume/docker-compose-extra.yml
-f ./config/thredds/config/proxy/docker-compose-extra.yml
-f ./config/portainer/config/proxy/docker-compose-extra.yml
-f ./config/magpie/config/proxy/docker-compose-extra.yml
-f ./config/twitcher/config/proxy/docker-compose-extra.yml
-f ./config/jupyterhub/config/magpie/docker-compose-extra.yml
-f ./config/jupyterhub/config/proxy/docker-compose-extra.yml
-f ./config/frontend/config/proxy/docker-compose-extra.yml
-f ./config/ncwms2/config/magpie/docker-compose-extra.yml
-f ./config/ncwms2/config/proxy/docker-compose-extra.yml
-f ./config/ncwms2/config/wps_outputs-volume/docker-compose-extra.yml
-f ./config/project-api/config/proxy/docker-compose-extra.yml
-f ./config/solr/config/proxy/docker-compose-extra.yml
-f ./optional-components/canarie-api-full-monitoring/config/proxy/docker-compose-extra.yml
-f ./optional-components/all-public-access/config/catalog/docker-compose-extra.yml
-f ./optional-components/all-public-access/config/finch/docker-compose-extra.yml
-f ./optional-components/all-public-access/config/flyingpigeon/docker-compose-extra.yml
-f ./optional-components/all-public-access/config/hummingbird/docker-compose-extra.yml
-f ./optional-components/all-public-access/config/malleefowl/docker-compose-extra.yml
-f ./optional-components/all-public-access/config/ncwms2/docker-compose-extra.yml
-f ./optional-components/all-public-access/config/raven/docker-compose-extra.yml
-f ./optional-components/all-public-access/config/secure-data-proxy/docker-compose-extra.yml
-f ./optional-components/all-public-access/config/thredds/docker-compose-extra.yml
-f ./optional-components/wps-healthchecks/config/catalog/docker-compose-extra.yml
-f ./optional-components/wps-healthchecks/config/finch/docker-compose-extra.yml
-f ./optional-components/wps-healthchecks/config/flyingpigeon/docker-compose-extra.yml
-f ./optional-components/wps-healthchecks/config/hummingbird/docker-compose-extra.yml
-f ./optional-components/wps-healthchecks/config/malleefowl/docker-compose-extra.yml
-f ./optional-components/wps-healthchecks/config/raven/docker-compose-extra.yml
-f ./optional-components/secure-thredds/config/magpie/docker-compose-extra.yml
-f ./optional-components/testthredds/config/proxy/docker-compose-extra.yml
-f ./components/weaver/config/magpie/docker-compose-extra.yml
-f ./components/weaver/config/proxy/docker-compose-extra.yml
-f ./components/weaver/config/twitcher/docker-compose-extra.yml
-f ./optional-components/secure-data-proxy/config/magpie/docker-compose-extra.yml
-f ./optional-components/secure-data-proxy/config/proxy/docker-compose-extra.yml
-f ./components/cowbird/config/geoserver/docker-compose-extra.yml
-f ./components/cowbird/config/jupyterhub/docker-compose-extra.yml
-f ./components/cowbird/config/magpie/docker-compose-extra.yml
-f ./components/cowbird/config/proxy/docker-compose-extra.ymlAlthough it works, I think there are a lot of chances that any component after my ../../daccs-env/docker-compose-extra.yml override could break my setup at any time, if they just so happen to modify the set of dependencies or rewrite a docker-compose item I also override.
I would also expect all ./components/weaver/configs/[...] to be loaded somewhat close to the ./components/weaver, since they are themselves only dependencies of that component.
tlvu
requested changes
Mar 25, 2023
Collaborator
tlvu
left a comment
tlvu
left a comment
There was a problem hiding this comment.
I suggested a logic change to restore the original intended override capabilities.
Not tested so I am not sure it does what I think it should do or can be implemented (recursive call is usually more tricky to code). Let me know how it goes.
| current_dependencies=$COMPONENT_DEPENDENCIES | ||
| source_conf_files "$COMPONENT_DEPENDENCIES" 'COMPONENT_DEPENDENCIES' | ||
| done | ||
|
|
Collaborator
There was a problem hiding this comment.
Very nice simplification, much cleaner logic !!! In the previous review #296 (comment) I was lost at this part of the logic too.
birdhouse/read-configs.include.sh
Outdated
| if [ -f "$COMPONENT_DEFAULT_ENV" ]; then | ||
| echo "reading '$COMPONENT_DEFAULT_ENV'" | ||
| . "$COMPONENT_DEFAULT_ENV" | ||
| source_conf_files "$COMPONENT_DEPENDENCIES" "a dependency of $adir" |
Collaborator
There was a problem hiding this comment.
I order to make this config loader order respect the intended override precedence, I would suggest the following logic change.
Current logic in source_conf_files():
for adir in list_of_conf_dirs:
add adir to ALL_CONF_DIRS
read adir/default.env # get value of COMPONENT_DEPENDENCIES
recursive call source_conf_files on COMPONENT_DEPENDENCIES
add COMPONENT_DEPENDENCIES to ALL_CONF_DIRS
read defaut.env of COMPONENT_DEPENDENCIES # get value of COMPONENT_DEPENDENCIES and turtle down
If comp2 depends on comp1, and only comp2 is enabled, ALL_CONF_DIRS will list comp2 before comp1 so if comp2 has a fragment to override comp1 compose file, the fragment might not work since comp1 compose file has not been read yet.
Furthermore, if comp2 default.env has override for comp1 default.env, given comp2 default.env is read before comp1 default.env, it will not be able to override it properly.
Also, since the ordering of ALL_CONF_DIRS is reversed, we will also executing all the pre/post docker-compose-up scripts in the reverse order.
To be safe, maybe I would change the logic to something like below (untested, I might be wrong).
Proposed logic in source_conf_files():
for adir in list_of_conf_dirs:
read adir/default.env # get value of COMPONENT_DEPENDENCIES
recursive call source_conf_files on COMPONENT_DEPENDENCIES
read adir of COMPONENT_DEPENDENCIES default.env # COMPONENT_DEPENDENCIES variable name clashing here, might need to save previous value
recursive call source_conf_files on COMPONENT_DEPENDENCIES # and turtle down
read adir COMPONENT_DEPENDENCIES default.env # read again to be able to override dependent default.env
append adir COMPONENT_DEPENDENCIES to ALL_CONF_DIRS
read adir/default.env # again to override dependent default.env
append adir to ALL_CONF_DIRS
So the suggested logic above might fix the loading order for "hard" dependencies (listed in COMPONENT_DEPENDENCIES).
For "soft" dependencies (ex: all-public-access has magpie config to make all the birds public but do not list all the birds in its COMPONENT_DEPENDENCIES), the user still need to ensure all-public-access is listed after all the birds (all its soft dependencies) in EXTRA_CONF_DIRS.
Collaborator
|
To further preserve the loading order from before, this section of code that generate birdhouse-deploy/birdhouse/pavics-compose.sh Lines 107 to 130 in a91b7dc Should also be changed to this so all extra fragments modifying other components will load immediately after its parent component: This change, coupled with my suggested change in #304 (comment), should hopefully result in this kind of load order: |
Collaborator
Author
|
@fmigneault @tlvu Ok I believe I've updated the load order as expected. See the updated description of this PR (or the CHANGES.md file) for an explanation of the current behaviour |
Collaborator
E2E Test ResultsDACCS-iac Pipeline ResultsBuild URL : http://daccs-jenkins.crim.ca:80/job/DACCS-iac-birdhouse/1392/Result : failure BIRDHOUSE_DEPLOY_BRANCH : load-components-in-defined-order 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 PAVICS-e2e-workflow-tests Pipeline ResultsTests URL : http://daccs-jenkins.crim.ca:80/job/PAVICS-e2e-workflow-tests/job/master/1039/NOTEBOOK TEST RESULTS |
tlvu
reviewed
Mar 28, 2023
Collaborator
tlvu
left a comment
tlvu
left a comment
There was a problem hiding this comment.
Nice load ordering fix. Minor comments for extra robustness.
birdhouse/read-configs.include.sh
Outdated
| ALL_CONF_DIRS="$ALL_CONF_DIRS | ||
| $adir | ||
| " | ||
| COMPONENT_DEFAULT_ENV=$_OLD_COMPONENT_DEFAULT_ENV |
Collaborator
There was a problem hiding this comment.
Say we have 3 or more levels of dependencies, example: magpie <== twitcher <== weaver <== cowbird (not 100% sure this dependency graph is actually true, just for illustration).
_OLD_COMPONENT_DEFAULT_ENV will be local to each invocation of source_conf_files?
Collaborator
Author
There was a problem hiding this comment.
No, it isn't actually, this is one of the edge cases that the test helped uncover 😄 . A better solution is to use a stack and push/pop the adir to it.
birdhouse/read-configs.include.sh
Outdated
| if [ -f "$COMPONENT_DEFAULT_ENV" ]; then | ||
| # Source config settings of dependencies first if they haven't been sourced previously. | ||
| # Note that this will also define the order that docker-compose-extra.yml files will be loaded. | ||
| source_conf_files "$(. "$COMPONENT_DEFAULT_ENV" && echo "$COMPONENT_DEPENDENCIES")" "a dependency of $adir" |
Collaborator
There was a problem hiding this comment.
Woo fancy, sourcing and echo in same line. This helps for variable scoping or just to make the code more compact?
Collaborator
Author
There was a problem hiding this comment.
It's for variable scoping. Since $(...) runs in a subshell then we can get the value of $COMPONENT_DEPENDENCIES without populating the environment of the main process with the variables sourced in COMPONENT_DEFAULT_ENV (since we want to source the dependencies first).
birdhouse/read-configs.include.sh
Outdated
| source_conf_files() { | ||
| dirs=$1 | ||
| conf_locations=$2 | ||
| _old_adir=$adir # a posix-compliant way to make this a local variable' |
Collaborator
There was a problem hiding this comment.
adir is a very non specific var. If it is used in another function before calling this one, would it mess this one up?
Collaborator
Author
There was a problem hiding this comment.
It shouldn't matter... but this has been changed anyway with the new stack implimentation
birdhouse/pavics-compose.sh
Outdated
| extra_compose="$conf_dir/docker-compose-extra.yml" | ||
| if [ -f "$extra_compose" ]; then | ||
| if echo "$CONFIGURED_COMPONENTS" | grep -q "$service_name"; then | ||
| if printf '%b' "${LOADED_COMPONENTS}" | grep -q "^$override_service_name$"; then |
Collaborator
Author
There was a problem hiding this comment.
Thanks! I never remember how to use printf so I'm always looking it up
| ./config/proxy/docker-compose-extra.yml), the following additional load order rules apply: | ||
|
|
||
| - if the component that is being overridden has already been loaded, the override file is loaded immediately | ||
| - otherwise, the override files will be loaded immediately after the component that is being overridden has been loaded |
Collaborator
There was a problem hiding this comment.
Very nice delayed load ! Components do not have to know the load ordering amount themselves.
Collaborator
E2E Test ResultsDACCS-iac Pipeline ResultsBuild URL : http://daccs-jenkins.crim.ca:80/job/DACCS-iac-birdhouse/1395/Result : failure BIRDHOUSE_DEPLOY_BRANCH : load-components-in-defined-order 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 Infrastructure deployment failed. Instance has not been destroyed. @matprov |
Collaborator
E2E Test ResultsDACCS-iac Pipeline ResultsBuild URL : http://daccs-jenkins.crim.ca:80/job/DACCS-iac-birdhouse/1396/Result : failure BIRDHOUSE_DEPLOY_BRANCH : load-components-in-defined-order 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 |
Collaborator
These last two builds have simply been cancelled as they were stuck for one day. |
Collaborator
Author
|
@matprov yeah I noticed that they seemed to be stuck waiting for a worker process to start the job. Are there available workers on your jenkins running? Let me know if there's anything I can do on my end to help debug the issue. |
Collaborator
|
@mishaschwartz This is the second time it happens in a few months. I haven't seen this before. For now I have cleaned up some resources and restarted Jenkins to let jobs finish, but further investigation will be done when problem arise as I don't have other information for now. Ref.: #308 (comment) |
Collaborator
E2E Test ResultsDACCS-iac Pipeline ResultsBuild URL : http://daccs-jenkins.crim.ca:80/job/DACCS-iac-birdhouse/1419/Result : success BIRDHOUSE_DEPLOY_BRANCH : load-components-in-defined-order 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/1050/NOTEBOOK TEST RESULTS |
github-actions
bot
added
the
documentation
mishaschwartz
force-pushed
the
load-components-in-defined-order
branch
from
7df01ba to
11d0279
Compare
Collaborator
E2E Test ResultsDACCS-iac Pipeline ResultsBuild URL : http://daccs-jenkins.crim.ca:80/job/DACCS-iac-birdhouse/1430/Result : success BIRDHOUSE_DEPLOY_BRANCH : load-components-in-defined-order 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/1060/NOTEBOOK TEST RESULTS |
Collaborator
E2E Test ResultsDACCS-iac Pipeline ResultsBuild URL : http://daccs-jenkins.crim.ca:80/job/DACCS-iac-birdhouse/1431/Result : success BIRDHOUSE_DEPLOY_BRANCH : load-components-in-defined-order 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 PAVICS-e2e-workflow-tests Pipeline ResultsTests URL : http://daccs-jenkins.crim.ca:80/job/PAVICS-e2e-workflow-tests/job/master/1061/NOTEBOOK TEST RESULTS |
Collaborator
E2E Test ResultsDACCS-iac Pipeline ResultsBuild URL : http://daccs-jenkins.crim.ca:80/job/DACCS-iac-birdhouse/1433/Result : failure BIRDHOUSE_DEPLOY_BRANCH : load-components-in-defined-order DACCS_CONFIGS_BRANCH : master PAVICS_E2E_WORKFLOW_TESTS_BRANCH : master PAVICS_SDI_BRANCH : master DESTROY_INFRA_ON_EXIT : true PAVICS_HOST : https://host-140-92.rdext.crim.ca PAVICS-e2e-workflow-tests Pipeline ResultsTests URL : http://daccs-jenkins.crim.ca:80/job/PAVICS-e2e-workflow-tests/job/master/1062/NOTEBOOK TEST RESULTS |
Collaborator
E2E Test ResultsDACCS-iac Pipeline ResultsBuild URL : http://daccs-jenkins.crim.ca:80/job/DACCS-iac-birdhouse/1434/Result : failure BIRDHOUSE_DEPLOY_BRANCH : load-components-in-defined-order 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/1063/NOTEBOOK TEST RESULTS |
Collaborator
E2E Test ResultsDACCS-iac Pipeline ResultsBuild URL : http://daccs-jenkins.crim.ca:80/job/DACCS-iac-birdhouse/1436/Result : success BIRDHOUSE_DEPLOY_BRANCH : load-components-in-defined-order 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/1064/NOTEBOOK TEST RESULTS |
Collaborator
Author
|
@tlvu @fmigneault I know you guys are busy with other work but this PR is ready to go and I need at least 1 approved review to merge it in. If you're ok with me merging this in can I get an approval so that github will let me proceed. Thanks! |
tlvu
approved these changes
Apr 11, 2023
Collaborator
tlvu
left a comment
tlvu
left a comment
There was a problem hiding this comment.
LGTM, minor comments, sorry for the delay.
| # we don't use usual .env filename, because docker-compose uses it | ||
|
|
||
| if [ -e "$COMPOSE_DIR/env.local" ]; then | ||
| echo "Using local environment file at: ${BIRDHOUSE_LOCAL_ENV:="$COMPOSE_DIR/env.local"}" |
Collaborator
There was a problem hiding this comment.
This is a new feature that env.local location is not hardcoded anymore but can be set by env var BIRDHOUSE_LOCAL_ENV. I think this is worth a mention in the CHANGE.md and even birdhouse/README.rst, unless you intend this feature to stay hidden for testing only.
Collaborator
Author
There was a problem hiding this comment.
Good point. I intended it as a hidden feature and I think I'd like to keep it hidden for now.
Collaborator
Author
|
@fmigneault I lied that I only needed one approval to merge, sorry... since you requested changes in your last review I still need you to approve this in order to move forward |
fmigneault
reviewed
Apr 11, 2023
Member
fmigneault
left a comment
fmigneault
left a comment
There was a problem hiding this comment.
Just docs to validate. Behaviour seems ok based on tests.
Comment on lines
+93
to
+95
| - config/finch/docker-compose-extra.yml | ||
| - config/proxy/docker-compose-extra.yml | ||
| - config/finch/config/proxy/docker-compose-extra.yml |
Member
There was a problem hiding this comment.
This seems to contradict what the test evaluates, or there is something I missed?
I'm expecting proxy to be first as finch depends on it to provide its extra compose YAML.
Collaborator
Author
There was a problem hiding this comment.
Finch doesn't have proxy as an explicit dependency (it is not included in the COMPONENT_DEPENDENCIES variable defined in config/finch/default.env) so proxy is not loaded before finch. Because of this, the order that the components appear in the DEFAULT_CONF_DIRS list is respected (finch is before proxy). However, the file that overrides proxy settings (config/finch/config/proxy/docker-compose-extra.yml) can't be loaded until after both finch and proxy are loaded, since it could potentially override settings in either one, so it is loaded as soon as both of finch an proxy are loaded.
From the CHANGES.md file, here is the relevant explanation:
This load order also applies to the order that docker-compose-extra.yml files are specified. If a component also
includes an override file for another component (eg: ./config/finch/config/proxy/docker-compose-extra.yml overrides
./config/proxy/docker-compose-extra.yml), the following additional load order rules apply:
- if the component that is being overridden has already been loaded, the override file is loaded immediately
- otherwise, the override files will be loaded immediately after the component that is being overridden has been loaded
Member
There was a problem hiding this comment.
I see. So in the tests, proxy is loaded before finch because of some other component that defined it needed proxy as dependency, but considering only proxy/finch by themselves in this example, they have no particular order. Only the extra should be last, as it needs both.
fmigneault
approved these changes
Apr 12, 2023
Comment on lines
+93
to
+95
| - config/finch/docker-compose-extra.yml | ||
| - config/proxy/docker-compose-extra.yml | ||
| - config/finch/config/proxy/docker-compose-extra.yml |
Member
There was a problem hiding this comment.
I see. So in the tests, proxy is loaded before finch because of some other component that defined it needed proxy as dependency, but considering only proxy/finch by themselves in this example, they have no particular order. Only the extra should be last, as it needs both.
Collaborator
Author
|
@fmigneault yes that is a good way to think about it. In this case the example with just proxy and finch was meant to be a minimal example for explanatory purposes. I don't think that you'd ever have a case where only finch and proxy are running on your stack 🤷 |
Member
I don't think anyone would do that, but actually thinking about it, the |
Collaborator
E2E Test ResultsDACCS-iac Pipeline ResultsBuild URL : http://daccs-jenkins.crim.ca:80/job/DACCS-iac-birdhouse/1444/Result : failure BIRDHOUSE_DEPLOY_BRANCH : load-components-in-defined-order DACCS_CONFIGS_BRANCH : master PAVICS_E2E_WORKFLOW_TESTS_BRANCH : master PAVICS_SDI_BRANCH : master DESTROY_INFRA_ON_EXIT : true PAVICS_HOST : https://host-140-133.rdext.crim.ca PAVICS-e2e-workflow-tests Pipeline ResultsTests URL : http://daccs-jenkins.crim.ca:80/job/PAVICS-e2e-workflow-tests/job/master/1066/NOTEBOOK TEST RESULTS |
Collaborator
Author
Yes that will eventually be the case where there will be some components that are always required (at least in production) and proxy will be one of them, that's for the next PR though. |
Collaborator
E2E Test ResultsDACCS-iac Pipeline ResultsBuild URL : http://daccs-jenkins.crim.ca:80/job/DACCS-iac-birdhouse/1446/Result : failure BIRDHOUSE_DEPLOY_BRANCH : load-components-in-defined-order DACCS_CONFIGS_BRANCH : master PAVICS_E2E_WORKFLOW_TESTS_BRANCH : master PAVICS_SDI_BRANCH : master DESTROY_INFRA_ON_EXIT : true PAVICS_HOST : https://host-140-133.rdext.crim.ca PAVICS-e2e-workflow-tests Pipeline ResultsTests URL : http://daccs-jenkins.crim.ca:80/job/PAVICS-e2e-workflow-tests/job/master/1067/NOTEBOOK TEST RESULTS |
Collaborator
E2E Test ResultsDACCS-iac Pipeline ResultsBuild URL : http://daccs-jenkins.crim.ca:80/job/DACCS-iac-birdhouse/1447/Result : success BIRDHOUSE_DEPLOY_BRANCH : load-components-in-defined-order DACCS_CONFIGS_BRANCH : master PAVICS_E2E_WORKFLOW_TESTS_BRANCH : master PAVICS_SDI_BRANCH : master DESTROY_INFRA_ON_EXIT : true PAVICS_HOST : https://host-140-46.rdext.crim.ca PAVICS-e2e-workflow-tests Pipeline ResultsTests URL : http://daccs-jenkins.crim.ca:80/job/PAVICS-e2e-workflow-tests/job/master/1068/NOTEBOOK TEST RESULTS |
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
5 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
Extra components defined in the
EXTRA_CONF_DIRSvariables were being loaded before the dependant componentsdefined in the
COMPONENT_DEPENDENCIESvariables in each default.env file. This meant that if an extra componentwas meant to override some setting defined in a dependant component, the setting would not be overridden by the
extra component.
This change enforces the following load order rules:
DEFAULT_CONF_DIRSare loaded before those inEXTRA_CONF_DIRSDEFAULT_CONF_DIRSorEXTRA_CONF_DIRSCOMPONENT_DEPENDENCIESvariable are immediately loaded unless they have already beenloaded
For example, with the following files in place:
the load order is:
This load order also applies to the order that docker-compose-extra.yml files are specified. If a component also
includes an override file for another component (eg: ./config/finch/config/proxy/docker-compose-extra.yml overrides
./config/proxy/docker-compose-extra.yml), the following additional load order rules apply:
For example, with the following files in place:
the docker compose files will be loaded in the following order:
Changes
Non-breaking changes
Breaking changes
Related Issue / Discussion
EXTRA_CONF_DIRSnot respected byCOMPOSE_CONF_LIST#303Additional Information
Note that commit remove unnecessary variable extension is not strictly necessary for this fix. It simplifies the env files a bit though.