server: suppress spurious blocking query returns where multiple config entries are involved #12362
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
rboyer
force-pushed
the
more-blocking-queries-not-found
branch
from
b59362a
to
5ac56a0
Compare
rboyer
commented
Feb 15, 2022
rboyer
commented
Feb 15, 2022
agent/consul/config_endpoint.go
Outdated
| for _, sid := range upstreamIDs { | ||
| if _, ok := seenUpstreams[sid]; !ok { | ||
| seenUpstreams[sid] = struct{}{} | ||
| // Fetch all relevant config entries. We will generate a |
There was a problem hiding this comment.
The body of this blockingQuery call was changed into two steps:
- Get all config entries we will need.
- Smoosh those config entries together to fabricate the result.
Iterations after the first one will stop after (1) and check if any of the config entries collected differ from the prior iteration. If they do it returns the new data, otherwise it signals to the RPC side that it is a not-changed scenario.
rboyer
changed the title
rboyer
changed the title
kisunji
reviewed
Feb 23, 2022
kisunji
approved these changes
Feb 23, 2022
Co-authored-by: Chris S. Kim <ckim@hashicorp.com>
|
🍒 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/597518. |
rboyer
added a commit
that referenced
this pull request
Feb 25, 2022
…ple config entries are involved Backport of #12362 to 1.11.x Conflicts: - agent/consul/config_endpoint_test.go (imports) - go.mod (adjacencies)
rboyer
added a commit
that referenced
this pull request
Feb 25, 2022
…ple config entries are involved Backport of #12362 to 1.10.x Conflicts: - agent/consul/config_endpoint.go - agent/consul/discovery_chain_endpoint_test.go - agent/consul/intention_endpoint_test.go - agent/consul/internal_endpoint.go - agent/consul/internal_endpoint_test.go - agent/consul/rpc.go
rboyer
added a commit
that referenced
this pull request
Feb 25, 2022
…ple config entries are involved (#12459) * [1.10.x] server: suppress spurious blocking query returns where multiple config entries are involved Backport of #12362 to 1.10.x Conflicts: - agent/consul/config_endpoint.go - agent/consul/discovery_chain_endpoint_test.go - agent/consul/intention_endpoint_test.go - agent/consul/internal_endpoint.go - agent/consul/internal_endpoint_test.go - agent/consul/rpc.go * update vendor
This was referenced Mar 3, 2022
rboyer
added a commit
that referenced
this pull request
Mar 4, 2022
…uery tests (#12518) Improves tests from #12362 These tests try to setup the following concurrent scenario: 1. (goroutine 1) execute read RPC with index=0 2. (goroutine 1) get response from (1) @ index=10 3. (goroutine 1) execute read RPC with index=10 and block 4. (goroutine 2) WHILE (3) is blocking, start slamming the system with stray writes that will cause the WatchSet to wakeup 5. (goroutine 2) after doing all writes, shut down the reader above 6. (goroutine 1) stops reading, double checks that it only ever woke up once (from 1)
hc-github-team-consul-core
pushed a commit
that referenced
this pull request
Mar 4, 2022
…uery tests (#12518) Improves tests from #12362 These tests try to setup the following concurrent scenario: 1. (goroutine 1) execute read RPC with index=0 2. (goroutine 1) get response from (1) @ index=10 3. (goroutine 1) execute read RPC with index=10 and block 4. (goroutine 2) WHILE (3) is blocking, start slamming the system with stray writes that will cause the WatchSet to wakeup 5. (goroutine 2) after doing all writes, shut down the reader above 6. (goroutine 1) stops reading, double checks that it only ever woke up once (from 1)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Starting from and extending the mechanism introduced in #12110 we can specially handle the 3 main special Consul RPC endpoints that react to many config entries in a single blocking query in Connect:
DiscoveryChain.GetConfigEntry.ResolveServiceConfigIntentions.MatchAll of these will internally watch for many config entries, and at least one of those will likely be not found in any given query. Because these are blends of multiple reads the exact solution from #12110 isn't perfectly aligned, but we can tweak the approach slightly and regain the utility of that mechanism.
No Config Entries Found
In this case, despite looking for many config entries none may be found at all. Unlike #12110 in this scenario we do not return an empty reply to the caller, but instead synthesize a struct from default values to return. This can be handled nearly identically to #12110 with the first 1-2 replies being non-empty payloads followed by the standard spurious wakeup suppression mechanism from #12110.
No Change Since Last Wakeup
Once a blocking query loop on the server has completed and slept at least once, there is a further optimization we can make here to detect if any of the config entries that were present at specific versions for the prior execution of the loop are identical for the loop we just woke up for. In that scenario we can return a slightly different internal sentinel error and basically externally handle it similar to #12110.
This would mean that even if 20 discovery chain read RPC handling goroutines wakeup due to the creation of an unrelated config entry, the only ones that will terminate and reply with a blob of data are those that genuinely have new data to report.
Extra Endpoints
Since this pattern is pretty reusable, other key config-entry-adjacent endpoints used by
agent/proxycfgalso were updated:ConfigEntry.ListInternal.IntentionUpstreams(tproxy)TODO:
service-intentionswould need similar treatment (YES)hashstructureis a good alternative to use for outputs of the state store instead of theIndexSignaturestructIntentions.MatchConfigEntry.ListInternal.IntentionUpstreams