Skip to content
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

fix invalid weaver healthchecks #494

Merged
merged 2 commits into from
Jan 20, 2025
Merged

fix invalid weaver healthchecks #494

merged 2 commits into from
Jan 20, 2025

Conversation

fmigneault
Copy link
Collaborator

Overview

Fix Weaver healthchecks.

Changes

Non-breaking changes

  • Weaver: adjust missing WEAVER_MANAGER_NAME in healthchecks expecting matching SCRIPT_NAME configuration

    • Weaver's own healthcheck definition did not resolve to the expected endpoint.
    • Canarie-API monitoring endpoint for Weaver did not resolve to the expected endpoint.

Breaking changes

  • n/a

Related Issue / Discussion

CI Operations

birdhouse_daccs_configs_branch: master
birdhouse_skip_ci: false

@fmigneault fmigneault self-assigned this Jan 20, 2025
@github-actions github-actions bot added component/weaver Related to https://github.com/crim-ca/weaver documentation Improvements or additions to documentation feature/WPS Feature or service related to Web Processing Service labels Jan 20, 2025
Copy link
Collaborator

@mishaschwartz mishaschwartz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to double check... did the previous healthcheck pass when it shouldn't have? Because it doesn't seem to fail.

@fmigneault
Copy link
Collaborator Author

Just to double check... did the previous healthcheck pass when it shouldn't have? Because it doesn't seem to fail.

Both Weaver Docker and the Canarie API report 500 on my end.

{E2CD4082-CA9A-4D99-820A-73972F4CBF92}

On /canarie/node/service/stats

{39F92DF8-B6A0-4ABC-8A17-D37537CCFCE4}

I'm also investigating because we are working on the new DACCS-jenkins instance to bring back the CI with latest Jupyter kernel, but the initial check of the server's status on CanarieAPI's endpoint fails since around the time #492 was merged.

Copy link
Collaborator

@tlvu tlvu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as Misha, I find it weird that the old monitoring url without the weaver context root also worked !

But if you think this update is better, I have no objections.

@@ -26,7 +26,7 @@ SERVICES['Weaver'] = {
"monitoring": {
"Weaver": {
'request': {
'url': 'http://weaver:4001/'
'url': 'http://weaver:4001/${WEAVER_MANAGER_NAME}/'
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pour votre info, Ouranos production https://pavics.ouranos.ca/canarie/node/service/stats have the old 'url': 'http://weaver:4001/ and it reports success.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ouranos server is on 1.42.2, so it doesn't include the changes from https://github.com/bird-house/birdhouse-deploy/releases/tag/2.8.0 and https://github.com/bird-house/birdhouse-deploy/releases/tag/2.7.3 that, when combined cause the problem, but not detected when tested separately on their own.

HOSTNAME: ${BIRDHOUSE_FQDN}
# 'HTTP_HOST' and 'SCRIPT_NAME' are used to guide pyramid in the resolution of resources, such as
# when invoking the 'static_url' endpoint, so it can be made aware of reverse-proxy context
HTTP_HOST: ${BIRDHOUSE_FQDN}
SCRIPT_NAME: /${WEAVER_MANAGER_NAME}

Copy link
Collaborator Author

@fmigneault fmigneault Jan 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mishaschwartz ⬆️

This is why you didn't notice it as wrong.
The healthcheck was created before that change (so it worked), while I was testing Weaver before it was integrated, so never saw the error.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah right! That makes sense

@github-actions github-actions bot added the ci/operations Continuous Integration components label Jan 20, 2025
@fmigneault fmigneault merged commit 0f689f3 into master Jan 20, 2025
4 of 5 checks passed
@fmigneault fmigneault deleted the fix-weaver-healthchecks branch January 20, 2025 23:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci/operations Continuous Integration components component/weaver Related to https://github.com/crim-ca/weaver documentation Improvements or additions to documentation feature/WPS Feature or service related to Web Processing Service
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants