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.

Sign up for GitHub

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

Jump to bottom

Restore a few more service-kind index updates so blocking in ServiceDump works in more cases #6948

Merged
merged 3 commits into from
Dec 19, 2019
Merged

Restore a few more service-kind index updates so blocking in ServiceDump works in more cases #6948

merged 3 commits into from
Dec 19, 2019

Conversation

rboyer
Copy link
Member

@rboyer rboyer commented Dec 13, 2019 •
edited by crhino
Loading

Namely one omission was that check updates for dumped services were not
unblocking.

Follow-on from #6919

Note: while writing a unit test for this I discovered that the standard ServiceDump didn't wake up a watch as often as the kind version, so that was fixed.

@rboyer rboyer requested a review from a team December 13, 2019 21:22
@rboyer rboyer self-assigned this Dec 13, 2019
@codecov
Copy link

codecov bot commented Dec 13, 2019 •
edited
Loading

Codecov Report

❗ No coverage uploaded for pull request base (master@37dfe6d). Click here to learn what that means.
The diff coverage is 36.36%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #6948   +/-   ##
=========================================
  Coverage          ?   65.66%           
=========================================
  Files             ?      443           
  Lines             ?    53317           
  Branches          ?        0           
=========================================
  Hits              ?    35013           
  Misses            ?    14073           
  Partials          ?     4231
Impacted Files Coverage Δ
agent/consul/state/catalog.go 68.61% <14.28%> (ø)
agent/consul/state/catalog_oss.go 88.73% <75%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 37dfe6d...16ce54e. Read the comment docs.

@rboyer
Restore a few more service-kind index updates so blocking in ServiceD…
4fc1f94 
…ump works in more cases

Namely one omission was that check updates for dumped services were not
unblocking.

Follow-on from #6916
@rboyer
Add a ServiceDump state store test and also fix a watch bug with the
91d1e9d 
normal dump.
@rboyer
try to exercise one of the other lines added
16ce54e
@rboyer rboyer force-pushed the bugfix/more-svc-kind-blocking branch from 51a3745 to 16ce54e Compare December 17, 2019 18:11
@crhino
Copy link
Contributor

crhino commented Dec 19, 2019

For my own edification, I believe the impact of this bug is that in certain circumstances (i.e. places where ServiceKind index updates were not made), mesh gateways would not be notified of changes to their watched services/nodes/something. Is that a true statement?

My conclusions came from looking at the usage of ServiceDumpRequest. The proxycfg package is the only one that uses a non-default ServiceKind value when requesting notifications about mesh gateways.

@rboyer rboyer added this to the 1.7.0 milestone Dec 19, 2019
@rboyer rboyer added the type/bug Feature does not function as expected label Dec 19, 2019
mkeeler
mkeeler approved these changes Dec 19, 2019
View reviewed changes
Copy link
Member

@mkeeler mkeeler left a comment

Choose a reason for hiding this comment

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

LGTM

@rboyer rboyer merged commit abb1603 into master Dec 19, 2019
@rboyer rboyer deleted the bugfix/more-svc-kind-blocking branch December 19, 2019 16:15
@ghost
Copy link

ghost commented Jan 25, 2020

Hey there,

This issue has been automatically locked because it is closed and there hasn't been any activity for at least 30 days.

If you are still experiencing problems, or still have questions, feel free to open a new one 👍.

@ghost ghost locked and limited conversation to collaborators Jan 25, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@mkeeler mkeeler mkeeler approved these changes

Assignees

@rboyer rboyer

Labels
type/bug Feature does not function as expected
Projects
None yet
Milestone
1.7.0
Development

Successfully merging this pull request may close these issues.
3 participants
@rboyer @crhino @mkeeler