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

agent: cache notifications work after error if the underlying RPC returns index=1 #6547

Merged
merged 2 commits into from
Sep 26, 2019

Conversation

rboyer
Copy link
Member

@rboyer rboyer commented Sep 25, 2019

Fixes #6521

Ensure that initial failures to fetch an agent cache entry using the
notify API where the underlying RPC returns a synthetic index of 1
correctly recovers when those RPCs resume working.

The bug in the Cache.notifyBlockingQuery used to incorrectly "fix" the
index for the next query from 0 to 1 for all queries, when it should
have not done so for queries that errored.

Also fixed some things that made debugging difficult:

  • config entry read/list endpoints send back QueryMeta headers
  • xds event loops don't swallow the cache notification errors

@rboyer rboyer requested a review from a team September 25, 2019 22:32
@rboyer rboyer self-assigned this Sep 25, 2019
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.

Awesome job @rboyer. Very impressive cache sluething!

agent/agent_test.go Outdated Show resolved Hide resolved
t.Logf("took %s to get first success", time.Since(start))
case <-deadlineCh:
t.Fatal("did not get notified successfully")
}
Copy link
Member

Choose a reason for hiding this comment

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

Awesome you managed to write a test to exercise this issue!

@@ -126,7 +126,7 @@ func (c *Cache) notifyBlockingQuery(ctx context.Context, t string, r Request, co
}
}
// Sanity check we always request blocking on second pass
if index < 1 {
if err == nil && index < 1 {
Copy link
Member

Choose a reason for hiding this comment

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

There is is!

…urns index=1

Fixes #6521

Ensure that initial failures to fetch an agent cache entry using the
notify API where the underlying RPC returns a synthetic index of 1
correctly recovers when those RPCs resume working.

The bug in the Cache.notifyBlockingQuery used to incorrectly "fix" the
index for the next query from 0 to 1 for all queries, when it should
have not done so for queries that errored.

Also fixed some things that made debugging difficult:

- config entry read/list endpoints send back QueryMeta headers
- xds event loops don't swallow the cache notification errors

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
@rboyer rboyer force-pushed the cache-notify-after-error-index1 branch from f802c16 to dbd027b Compare September 26, 2019 15:16
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.

LGTM again

@rboyer rboyer merged commit 9566df5 into master Sep 26, 2019
rboyer added a commit that referenced this pull request Sep 26, 2019
…urns index=1 (#6547)

Fixes #6521

Ensure that initial failures to fetch an agent cache entry using the
notify API where the underlying RPC returns a synthetic index of 1
correctly recovers when those RPCs resume working.

The bug in the Cache.notifyBlockingQuery used to incorrectly "fix" the
index for the next query from 0 to 1 for all queries, when it should
have not done so for queries that errored.

Also fixed some things that made debugging difficult:

- config entry read/list endpoints send back QueryMeta headers
- xds event loops don't swallow the cache notification errors
@rboyer rboyer deleted the cache-notify-after-error-index1 branch September 26, 2019 15:49
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.

Connect Envoy sidecar proxy "cannot create upstream cluster without discovery chain"
2 participants