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

[NET11250]Added fix for health check update for consul-dataplane #267

Merged
merged 64 commits into from
Jan 13, 2025

Conversation

abhishek-hashicorp
Copy link
Collaborator

@abhishek-hashicorp abhishek-hashicorp commented Dec 10, 2024

When we update the consul-dataplane health status, we should take into account the service health status as well

Changes proposed in this PR:

  • Modified the health sync for consul-dataplane to take into account the service health as well during health-sync checks

How I've tested this PR:

Created an AWS ECS cluster with 2 services frontend and backend. The port for the backend health was misconfigured so that the service fails.
Before the fix:
On accessing curl 0:19000/clusters from the frontend service, we found that the health for backend service was showing as healthy.
image

After the fix:
The curl 0:19000/clusters was showing
image

Once the port was fixed, the backend service was again showing as healthy using the curl 0:19000/clusters endpoint from frontend

How I expect reviewers to test this PR:

👀

Checklist:

  • Tests added
  • CHANGELOG entry added

@abhishek-hashicorp abhishek-hashicorp changed the title Abhishekhashicorp/fix health check [NET11250]Added fix for health check update for consul-dataplane Dec 10, 2024
@abhishek-hashicorp abhishek-hashicorp marked this pull request as ready for review December 10, 2024 21:00
@abhishek-hashicorp abhishek-hashicorp requested a review from a team as a code owner December 10, 2024 21:00
@abhishek-hashicorp abhishek-hashicorp added this to the m milestone Dec 11, 2024
Copy link
Contributor

@dhiaayachi dhiaayachi left a comment

Choose a reason for hiding this comment

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

Hey @abhishek-hashicorp
Thank you for working on this! I left a few comments for your consideration.

subcommand/health-sync/checks.go Outdated Show resolved Hide resolved
subcommand/health-sync/checks.go Outdated Show resolved Hide resolved
subcommand/health-sync/checks.go Outdated Show resolved Hide resolved
@abhishek-hashicorp abhishek-hashicorp temporarily deployed to dockerhub/hashicorpdev January 9, 2025 15:55 — with GitHub Actions Inactive
dhiaayachi
dhiaayachi previously approved these changes Jan 10, 2025
Copy link
Contributor

@dhiaayachi dhiaayachi left a comment

Choose a reason for hiding this comment

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

Thank you for going through my comments @abhishek-hashicorp, I left 2 minor comments, but nothing blocking 🚀

subcommand/health-sync/checks.go Outdated Show resolved Hide resolved
subcommand/health-sync/checks.go Outdated Show resolved Hide resolved
Copy link

@srahul3 srahul3 left a comment

Choose a reason for hiding this comment

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

✅ LGTM

@abhishek-hashicorp abhishek-hashicorp merged commit ad41036 into main Jan 13, 2025
27 checks passed
@abhishek-hashicorp abhishek-hashicorp deleted the abhishekhashicorp/fix-health-check branch January 13, 2025 06:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants