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

Notify alias checks when aliased service is [de]registered #8456

Merged
merged 4 commits into from
Aug 12, 2020

Conversation

freddygv
Copy link
Contributor

@freddygv freddygv commented Aug 6, 2020

Currently when monitoring alias checks locally we have two mechanisms for updating the status of the alias check:

  1. Get a notification when a check for the service we are aliasing is updated
  2. Catch updates that didn't trigger a notification by checking every minute

PR #7384 added a check so that alias health checks do not pass if the underlying service doesn't exist. However, we did not notify existing alias checks when services were added/removed. This means we could wait up to a minute to update the check status in these cases (per item 2 above).

This PR triggers an alias check notification when services are added or removed from local state.

@freddygv freddygv requested a review from a team August 6, 2020 22:26
@@ -1912,6 +1912,59 @@ func TestAgent_AliasCheck(t *testing.T) {
}
}

func TestAgent_AliasCheck_ServiceNotification(t *testing.T) {
Copy link
Member

Choose a reason for hiding this comment

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

if you comment out the patch, does this test fail?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep this would fail. We only used to notify alias checks if checks were updated. Now we notify about changes to aliased services as well.

@freddygv freddygv requested a review from rboyer August 11, 2020 17:31
Copy link
Member

@rboyer rboyer left a comment

Choose a reason for hiding this comment

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

LGTM

@freddygv freddygv merged commit d72f72d into master Aug 12, 2020
@freddygv freddygv deleted the notify-alias branch August 12, 2020 15:47
@hashicorp-ci
Copy link
Contributor

🍒✅ Cherry pick of commit d72f72d onto release/1.8.x succeeded!

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