-
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
Write build context to a build directory instead of using birdhouse/
#362
base: master
Are you sure you want to change the base?
Conversation
E2E Test ResultsDACCS-iac Pipeline ResultsBuild URL : http://daccs-jenkins.crim.ca:80/job/DACCS-iac-birdhouse/1909/Result : failure BIRDHOUSE_DEPLOY_BRANCH : build-to-build-dir 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 |
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 please have a look at the changes in this file and let me know if I'm missing something.
Because this file is called by pavics-compose.sh we end up building the build directory multiple times. I thought about making the "build" step only trigger when the first argument is "up" but it also looks like just calling these exec
commands with docker is sufficient (since the container already exists).
E2E Test ResultsDACCS-iac Pipeline ResultsBuild URL : http://daccs-jenkins.crim.ca:80/job/DACCS-iac-birdhouse/1910/Result : failure BIRDHOUSE_DEPLOY_BRANCH : build-to-build-dir 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 Infrastructure deployment failed. Instance has not been destroyed. @matprov |
Before doing an in-depth review, I would like to better understand the reasoning, or the exact needs, for these changes.
Why create a copy?
Running
This could be a more personal preference, but I find it much easier to switch between the instantiated file and its template when they are next to each other. Resolving the equivalent file hierarchy between The explicit
I believe having autodeploy break in such cases is intentional. This is to avoid some files to be silently included or generated from incorrectly configured volume mount configs (e.g.: from a typo in the path), which can take longer to debug or identify.
Not sure if I misinterpret this or not, but I have custom files and components loaded from other locations that are not under The @tlvu |
That's understandable, I'm happy to clarify. The main reasons for this change are described in the "Additional Information" section in the PR description above. Please see below for some additional clarifications:
Creating a copy is the simplest way of ensuring that the output of template files are created outside the birdhouse directory and it requires the fewest changes to the pavics-compose.sh code. However there are other options that would allow us to do the same thing with fewer copies.
I'm happy to make the pavics-compose.sh smarter to avoid extra copies.
Correct, that's the strategy we're going for here. The only difference is that we're using that we can then use that generated config file to actually deploy the stack.
Ok that is interesting, I actually have the opposite preference because it makes searching for a term in my IDE easier when I can specify whether I expect it in the build directory or the birdhouse directory. Searching become very cluttered when you have a lot of very similar files side-by-side.
I'm interested why you think that. To me this strategy creates a lot of additional files that are not easy to maintain at all. Especially since we need to keep a reference to files that were generated in old versions that aren't there anymore. In my proposed changes, we no longer need to keep these files at all. The big .gitignore file that is added here is simply so that you don't have to clean up all the old files if you don't want to do it right away. It's a courtesy but is not required.
Right, and do all of those have to specify the paths in the docker-compose-extra.yml files as relative to the birdhouse directory or as absolute paths? Doesn't that make the code less portable and harder to share with others? If you move that directory don't you have to update all the paths in the files?
I'm not sure I understand how this prevents this error:
Do you mean included in a git commit? The changes proposed here also prevent that, without the need to maintain lots of .gitignore files.
I don't understand how this is affected by whether a file is in the .gitignore file or not. Maybe an example would help me understand.
Ok, that makes sense. To continue the analogy, the change that I'm proposing here is similar to installing the python package where the files get written to a site-packages directory. Or the strategy a lot of software employs to take the code from a
I don't think it breaks much at all. In fact it should be very backwards compatible |
E2E Test ResultsDACCS-iac Pipeline ResultsBuild URL : http://daccs-jenkins.crim.ca:80/job/DACCS-iac-birdhouse/1949/Result : failure BIRDHOUSE_DEPLOY_BRANCH : build-to-build-dir DACCS_CONFIGS_BRANCH : master PAVICS_E2E_WORKFLOW_TESTS_BRANCH : master PAVICS_SDI_BRANCH : master DESTROY_INFRA_ON_EXIT : true PAVICS_HOST : https://host-140-67.rdext.crim.ca Infrastructure deployment failed. Instance has not been destroyed. @matprov |
E2E Test ResultsDACCS-iac Pipeline ResultsBuild URL : http://daccs-jenkins.crim.ca:80/job/DACCS-iac-birdhouse/1951/Result : failure BIRDHOUSE_DEPLOY_BRANCH : build-to-build-dir DACCS_CONFIGS_BRANCH : master PAVICS_E2E_WORKFLOW_TESTS_BRANCH : master PAVICS_SDI_BRANCH : master DESTROY_INFRA_ON_EXIT : true PAVICS_HOST : https:// Infrastructure deployment failed. Instance has not been destroyed. @matprov |
4fd4e3f
to
1cf36a3
Compare
E2E Test ResultsDACCS-iac Pipeline ResultsBuild URL : http://daccs-jenkins.crim.ca:80/job/DACCS-iac-birdhouse/1952/Result : failure BIRDHOUSE_DEPLOY_BRANCH : build-to-build-dir DACCS_CONFIGS_BRANCH : master PAVICS_E2E_WORKFLOW_TESTS_BRANCH : master PAVICS_SDI_BRANCH : master DESTROY_INFRA_ON_EXIT : true PAVICS_HOST : https:// Infrastructure deployment failed. Instance has not been destroyed. @matprov |
1cf36a3
to
cb51c48
Compare
E2E Test ResultsDACCS-iac Pipeline ResultsBuild URL : http://daccs-jenkins.crim.ca:80/job/DACCS-iac-birdhouse/1953/Result : failure BIRDHOUSE_DEPLOY_BRANCH : build-to-build-dir DACCS_CONFIGS_BRANCH : master PAVICS_E2E_WORKFLOW_TESTS_BRANCH : master PAVICS_SDI_BRANCH : master DESTROY_INFRA_ON_EXIT : true PAVICS_HOST : https:// Infrastructure deployment failed. Instance has not been destroyed. @matprov |
cb51c48
to
95c6284
Compare
E2E Test ResultsDACCS-iac Pipeline ResultsBuild URL : http://daccs-jenkins.crim.ca:80/job/DACCS-iac-birdhouse/1954/Result : failure BIRDHOUSE_DEPLOY_BRANCH : build-to-build-dir DACCS_CONFIGS_BRANCH : master PAVICS_E2E_WORKFLOW_TESTS_BRANCH : master PAVICS_SDI_BRANCH : master DESTROY_INFRA_ON_EXIT : true PAVICS_HOST : https:// Infrastructure deployment failed. Instance has not been destroyed. @matprov |
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.
Need to revert to keep old docker-compose
, otherwise looks good. Please allow me to test on my end before merging this.
PAVICS_LOG_DIR="${PAVICS_LOG_DIR:-/tmp/pavics-compose}" | ||
CELERY_HEALTHCHECK="/opt/local/bin/weaver/celery-healthcheck" | ||
mkdir -p "${PAVICS_LOG_DIR}" | ||
# note: use 'tee' instead of capturing in variable to allow displaying results directly when running command | ||
${PAVICS_COMPOSE} exec weaver bash "${CELERY_HEALTHCHECK}" | tee "${PAVICS_LOG_DIR}/weaver.log" | ||
docker exec weaver bash "${CELERY_HEALTHCHECK}" | tee "${PAVICS_LOG_DIR}/weaver.log" |
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 curious, we can not use pavics-compose.sh
as a drop-in replacement for docker-compose
after this PR so we have to directly use docker
?
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.
We can still use pavics-compose.sh
, especially now that the build directory is only re-created on "up". But I still think this is a good change here since it's no longer necessary to call pavics-compose.sh
and this is simpler (and more consistent with what we're doing in other post-docker-up
scripts.
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 didn't even realize this change in my previous review, but I think it can actually produce quite different behaviour.
Using docker exec
directly (instead of docker compose exec
) will not apply all other configuration in docker compose such as mounted volumes, configs, networks, etc. Since the database network is not default
here, it is possible this change actually breaks the healthcheck.
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.
docker exec
runs a command in a container that is already created and already running. At the point where docker exec
is called here, all volumes, networks, configs, etc. are already associated with the container. Even docker compose exec
wouldn't apply any of the configurations you describe.
You can think of this as more or less the same as using ssh
to execute a command on a machine that is already running.
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.
This assumes that container_name
was not overridden, while docker compose exec
works with the service name instead of the container name.
birdhouse/deprecated-components/catalog/config/canarie-api/docker-compose-extra.yml
Show resolved
Hide resolved
birdhouse/pavics-compose.sh
Outdated
RELATIVE_FILE_PATH=${FILE#${adir}} | ||
DEST="${BUILD_DIR}/${CONF_NAME}/${RELATIVE_FILE_PATH#/}" | ||
mkdir -p "$(dirname "${DEST}")" | ||
ln "${FILE}" "${DEST}" |
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.
Using hardlink assume all external repos checkouts are on the same filesystem with this checkout. This is a sensible assumption but should be documented nonetheless. Before the external repos checkouts can be anywhere and it would still work.
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.
@tlvu What if we try the hardlink with a fallback strategy to copy the files. That way, we don't lose the ability to host these on other filesystems
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.
Oh yes, that would be best. Test for ln
exit code, if it errors out, use regular cp
instead.
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.
Could consider rsync
as well instead of cp
to skip copies where no deltas are detected.
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.
The build directory is emptied before this so there would never be a case where there are no deltas. If we didn't clear the build directory first, this could be useful. But I worry that we'd end up with a lot of leftover files between builds if the configuration changes.
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.
Could that be an option? Like allowing CLEAN_BUILD_DIR=true pavics-compose up -d
to take advantage of quicker rsync
? I think cleaning the build dir is valid to start from a fresh setup, but wiping it each time while developing the same feature is overkill.
done | ||
|
||
# we apply all the templates | ||
find "$BUILD_DIR" -name '*.template' | |
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.
Ah nice, so this scenario, we still have the original template next to the instantiated files, for easy template expansion verification.
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.
Yeah! I didn't even think of that but yes that's a good point.
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.
So instead of the double search matches in .template
and their instantiated files, we will now have 3 because of the duplicated template between the build and source dirs?
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
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.
Actually there is a 4th because the birdhouse directory has a symlink in docs
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.
For our dev machine, I think we can set BUILD_DIR
outside of birdhouse-deploy
checkout to avoid those duplicate files when searching.
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.
If BUILD_DIR=birdhouse
is supported, then this is a viable option for a dev machine.
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.
Currently, this is not supported since the arrangement of files in the BUILD_DIR
is not the same as the one in birdhouse
.
But... if we moved all of the components, from components
, config
, optional-components
, etc. into one single location that could be possible. If we did that though, you'd have to also put any additional components into that directory as well.
For me, I would just adjust my search filters to not read from the BUILD_DIR
(or make it outside of the project folder like @tlvu suggests). Then if I explicitly want to search in the BUILD_DIR
I can select that folder to search. This is what I currently do to avoid looking in the docs/source/birdhouse/
directory for duplicates.
I mostly use PyCharm and VSCode and it's pretty easy to set up these filters, I can demo it for you if you'd like. I'm sure most other IDEs support a similar search filtering.
birdhouse/pavics-compose.sh
Outdated
echo ${COMPOSE_CONF_LIST} | tr ' ' '\n' | grep -v '^-f' | ||
|
||
# the PROXY_SECURE_PORT is a little trick to make the compose file invalid without the usage of this wrapper script | ||
PROXY_SECURE_PORT=443 HOSTNAME=${PAVICS_FQDN} docker compose --project-directory "${BUILD_DIR}" ${COMPOSE_CONF_LIST} config -o "${COMPOSE_FILE}" |
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 have to keep the old docker-compose
, otherwise it won't work in the autodeploy container. Or you'll have to update the autodeploy container and ensure all existing deployments are up-to-date as well.
Or make pavics-compose.sh
not rely on the locally installed docker-compose
but use the same image as the autodeploy. This will also allow catching incompatibility bugs earlier than having to trigger autodeploy to catch the bug.
I think this docker upgrade should be in separate PR to not burden this one. So just keep docker-compose
for now.
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.
E2E Test ResultsDACCS-iac Pipeline ResultsBuild URL : http://daccs-jenkins.crim.ca:80/job/DACCS-iac-birdhouse/1950/Result : failure BIRDHOUSE_DEPLOY_BRANCH : build-to-build-dir DACCS_CONFIGS_BRANCH : master PAVICS_E2E_WORKFLOW_TESTS_BRANCH : master PAVICS_SDI_BRANCH : master DESTROY_INFRA_ON_EXIT : true PAVICS_HOST : https://host-140-166.rdext.crim.ca PAVICS-e2e-workflow-tests Pipeline ResultsTests URL : http://daccs-jenkins.crim.ca:80/job/PAVICS-e2e-workflow-tests/job/master/1261/NOTEBOOK TEST RESULTS |
Ok I've made a few updates in response to the comments here:
|
birdhouse/pavics-compose.sh
Outdated
DEST=${FILE%.template} | ||
cat ${FILE} | envsubst "$VARS" | envsubst "$OPTIONAL_VARS" > ${DEST} | ||
done | ||
BUILD_DIR="${BUILD_DIR:-"${COMPOSE_DIR}/build"}" |
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.
Sorry for nitpicking but I'd rather prefer this default value appear in birdhouse/default.env
rather than here so it is more easily discoverable by our users.
I know you already documented in env.local.example
but the easier any configs, in general, can be found, the better.
E2E Test ResultsDACCS-iac Pipeline ResultsBuild URL : http://daccs-jenkins.crim.ca:80/job/DACCS-iac-birdhouse/1955/Result : failure BIRDHOUSE_DEPLOY_BRANCH : build-to-build-dir DACCS_CONFIGS_BRANCH : master PAVICS_E2E_WORKFLOW_TESTS_BRANCH : master PAVICS_SDI_BRANCH : master DESTROY_INFRA_ON_EXIT : true PAVICS_HOST : https://host-140-166.rdext.crim.ca Infrastructure deployment failed. Instance has not been destroyed. @matprov |
E2E Test ResultsDACCS-iac Pipeline ResultsBuild URL : http://daccs-jenkins.crim.ca:80/job/DACCS-iac-birdhouse/1957/Result : failure BIRDHOUSE_DEPLOY_BRANCH : build-to-build-dir DACCS_CONFIGS_BRANCH : master PAVICS_E2E_WORKFLOW_TESTS_BRANCH : master PAVICS_SDI_BRANCH : master DESTROY_INFRA_ON_EXIT : true PAVICS_HOST : https:// Infrastructure deployment failed. Instance has not been destroyed. @matprov |
E2E Test ResultsDACCS-iac Pipeline ResultsBuild URL : http://daccs-jenkins.crim.ca:80/job/DACCS-iac-birdhouse/2096/Result : failure BIRDHOUSE_DEPLOY_BRANCH : build-to-build-dir DACCS_CONFIGS_BRANCH : master PAVICS_E2E_WORKFLOW_TESTS_BRANCH : master PAVICS_SDI_BRANCH : master DESTROY_INFRA_ON_EXIT : true PAVICS_HOST : https://host-140-90.rdext.crim.ca Infrastructure deployment failed. Instance has not been destroyed. @matprov |
E2E Test ResultsDACCS-iac Pipeline ResultsBuild URL : http://daccs-jenkins.crim.ca:80/job/DACCS-iac-birdhouse/2097/Result : failure BIRDHOUSE_DEPLOY_BRANCH : build-to-build-dir DACCS_CONFIGS_BRANCH : master PAVICS_E2E_WORKFLOW_TESTS_BRANCH : master PAVICS_SDI_BRANCH : master DESTROY_INFRA_ON_EXIT : true PAVICS_HOST : https://host-140-67.rdext.crim.ca Infrastructure deployment failed. Instance has not been destroyed. @matprov |
E2E Test ResultsDACCS-iac Pipeline ResultsBuild URL : http://daccs-jenkins.crim.ca:80/job/DACCS-iac-birdhouse/2128/Result : failure BIRDHOUSE_DEPLOY_BRANCH : build-to-build-dir DACCS_CONFIGS_BRANCH : master PAVICS_E2E_WORKFLOW_TESTS_BRANCH : master PAVICS_SDI_BRANCH : master DESTROY_INFRA_ON_EXIT : true PAVICS_HOST : https:// Infrastructure deployment failed. Instance has not been destroyed. @matprov |
Overview
docker compose
commands to a build directorydocker-compose.yml
file that can be easily inspected to see exactly what is being deployed..gitignore
file peppered throughout the repository.birdhouse/build
by default but can be changed by setting theBUILD_DIR
variable inenv.local
../birdhouse/docker-compose.yml
since these files are copied to the relevant location in the build directory.docker-compose-extra.yml
files were either absolute or relative paths from thebirdhouse/
directory.directory when the stack is started up).
Changes
Non-breaking changes
Breaking changes
docker-compose-extra.yml
files to specify the location of a bind-mounted directory on host relative to the component's directory, not relative to thebirdhouse/
directory.For example, a custom component with files at
birdhouse/optional-components/custom_wps/
:before:
after:
Related Issue / Discussion
Additional Information
.gitignore