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

local: mark service as InSync when added to local agent state #9284

Merged
merged 1 commit into from
Nov 27, 2020

Conversation

dnephin
Copy link
Contributor

@dnephin dnephin commented Nov 26, 2020

When the existing service is the same as the new service.

Fixes #9266

@dnephin dnephin added the type/bug Feature does not function as expected label Nov 26, 2020
@dnephin dnephin requested review from banks, lkysow and a team November 26, 2020 19:04
Copy link
Member

@banks banks left a comment

Choose a reason for hiding this comment

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

This looks great Daniel. I could be wrong but I think we need to also add the IsSame check for Checks that are part of the registration.

I think this would be easy to confirm by adding a service-level check to the registration in the test - I think if you do that you'll see it fail again as the check would still be marked as InSync = false even when it hasn't changed so will cause an extra RPC.

agent/local/state_test.go Outdated Show resolved Hide resolved
@dnephin dnephin force-pushed the dnephin/agent-service-register branch from 1b2829c to 3b3e907 Compare November 27, 2020 17:27
Copy link
Member

@banks banks left a comment

Choose a reason for hiding this comment

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

Perfect! Looks great!

If the existing service and checks are the same as the new registration.
@dnephin dnephin force-pushed the dnephin/agent-service-register branch from 3b3e907 to 33b8106 Compare November 27, 2020 20:31
@dnephin dnephin merged commit 9f0f2bd into master Nov 27, 2020
@dnephin dnephin deleted the dnephin/agent-service-register branch November 27, 2020 20:49
@hashicorp-ci
Copy link
Contributor

🍒 If backport labels were added before merging, cherry-picking will start automatically.

To retroactively trigger a backport after merging, add backport labels and re-run https://circleci.com/gh/hashicorp/consul/289221.

@hashicorp-ci
Copy link
Contributor

🍒✅ Cherry pick of commit 9f0f2bd onto release/1.9.x succeeded!

hashicorp-ci pushed a commit that referenced this pull request Nov 27, 2020
local: mark service as InSync when added to local agent state
@hashicorp-ci
Copy link
Contributor

🍒❌ Cherry pick of commit 9f0f2bd onto release/1.8.x failed! Build Log

@mikemorris
Copy link
Contributor

Do we want to manually backport this to 1.8 and 1.7?

@dnephin
Copy link
Contributor Author

dnephin commented Jan 5, 2021

This was a relatively minor problem that we only discovered when testing at a large scale, and even then it wasn't really breaking anything it was just surprising. So I think we shouldn't spend the time to manually backport unless it becomes a problem for someone.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type/bug Feature does not function as expected
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Agent: don't send RPCs to Re-register service if it hasn't changed.
5 participants