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 service syncing churn in 1.2.3/1.2.4 #5096

Merged
merged 1 commit into from
Dec 20, 2018

Conversation

42wim
Copy link
Contributor

@42wim 42wim commented Dec 13, 2018

Since upgrading from 1.2.2 to 1.2.4 a few days ago I'm getting Synced service messages every ~2 minutes (and a higher load on the cluster).

Found the culprit to be #4468 introduced in 1.2.3 which does not set a default weights value when registering the service, causing the service comparison to fail and keeps syncing the service.

PR against backport/1.2.x

@pierresouchay
Copy link
Contributor

Nice catch!

Copy link
Contributor

@pierresouchay pierresouchay left a comment

Choose a reason for hiding this comment

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

LGTM

@banks banks added this to the 1.4.1 milestone Dec 20, 2018
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.

Great catch @42wim thanks!

@banks banks merged commit ccd4176 into hashicorp:backport/1.2.x Dec 20, 2018
@banks
Copy link
Member

banks commented Dec 20, 2018

Oh I missed the PR against backport/1.2.x. Before merge 🤦‍♂️ .

We currently don't backport small fixes like this since there is a lot of overhead in publishing and testing many versions - the backport branches only exist for high-impact security fixes currently.

I'm going to revert the merge and merge to master instead.

Next year we have plans to come up with a more coherent policy on long-term support for older version branches but for now we have no plans to cut backports to 1.2.x and 1.3.x on a regular basis so this would just get lost.

banks added a commit that referenced this pull request Dec 20, 2018
banks added a commit that referenced this pull request Dec 20, 2018
@banks
Copy link
Member

banks commented Dec 20, 2018

Ran out of time to create new PR on master for this but I will do it later! Thanks again @42wim

banks added a commit that referenced this pull request Dec 20, 2018
…ate to prevent constant AE re-sync.

This fix was contributed by @42wim in #5096 but was merged against the wrong base. This adds it to master and adds a test to cover the behaviour.
banks added a commit that referenced this pull request Jan 8, 2019
…urn (#5126)

* Add default weights when adding a service with no weights to local state to prevent constant AE re-sync.

This fix was contributed by @42wim in #5096 but was merged against the wrong base. This adds it to master and adds a test to cover the behaviour.

* Fix tests that broke due to comparing internal state which now has default weights
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