Conversation
| } | ||
|
|
||
| ListenerManagerStats ListenerManagerImpl::generateStats(Stats::Scope& scope) { | ||
| std::string final_prefix = "listener_manager."; |
| ListenerHandle* listener_foo = expectListenerCreate(false); | ||
| EXPECT_CALL(listener_factory_, createListenSocket(_, true)); | ||
| EXPECT_TRUE(manager_->addOrUpdateListener(*loader)); | ||
| checkStats(1, 0, 0, 0, 1, 0); |
There was a problem hiding this comment.
I'm guilty of this as well (in the Subscription stats PR), but it's actually a bit unreadable these checkStats statements, since you don't know what each magic number means. It might be nicer to be more verbose and pass in a struct with named fields , e.g. {.warming = 1, .active = 0, ...} or the like. Up to you if you want to change anything here, I don't feel that strongly.
There was a problem hiding this comment.
I agree it's not very readable but the alternative is super verbose. I think I'm going to leave it as is.
There was a problem hiding this comment.
I can't make up my mind if this is actually a good suggestion or not but what the heck:
Call the method checkStatsAMRWAD(), at least it's always obvious which order the args are.
| workers_.size()); | ||
| // Must be done under lock. Set is used so hot restart / multiple processes converge to a | ||
| // single value. Same below inside the lambda. | ||
| stats_.total_listeners_draining_.set(draining_listeners_.size()); |
There was a problem hiding this comment.
For my education, can you elaborate on what is meant by convergence here? I think the idea is that the new process eventually "wins" since it's the last man standing and we accept its values for these gauges?
There was a problem hiding this comment.
Yes, exactly. Otherwise during hot restart if we use inc()/dec() on the gauge we get double the listeners. This way it basically converges and the gauges remain roughly static between the two processes. (For those not using hot restart it's a NOP). We do the same thing with healthy hosts, # of clusters, etc.
There was a problem hiding this comment.
Instead of saying "converge" it might be clearer if the comment just says that using set() avoids multiple modifiers problem during the multiple processes phase of hot restart.
There was a problem hiding this comment.
I will update the comment in the PR I'm working on now and tag you on it.
|
@htuch updated |
dnoe
left a comment
There was a problem hiding this comment.
Looks good, one suggestion on the "converge" comment language - take it or leave it.
| Listeners | ||
| ========= | ||
|
|
||
| .. toctree:: |
There was a problem hiding this comment.
Github rich diff mangles this, but it looks like correct .rst syntax to me?
| workers_.size()); | ||
| // Must be done under lock. Set is used so hot restart / multiple processes converge to a | ||
| // single value. Same below inside the lambda. | ||
| stats_.total_listeners_draining_.set(draining_listeners_.size()); |
There was a problem hiding this comment.
Instead of saying "converge" it might be clearer if the comment just says that using set() avoids multiple modifiers problem during the multiple processes phase of hot restart.
Description: implements an end-to-end test with the new python bindings interface. also makes some changes to the cc lib implementation, as it had some remaining bugs. Risk Level: Low Testing: This is the test! Signed-off-by: Cerek Hillen <chillen@lyft.com> Signed-off-by: JP Simard <jp@jpsim.com>
Description: implements an end-to-end test with the new python bindings interface. also makes some changes to the cc lib implementation, as it had some remaining bugs. Risk Level: Low Testing: This is the test! Signed-off-by: Cerek Hillen <chillen@lyft.com> Signed-off-by: JP Simard <jp@jpsim.com>
Mark arm64 register x8 as clobbered by syscall body inline assembly as it is being used to store the syscall number. Otherwise the compiler may try to use it for some other purpose. This fix is derived from a resolution to clang Bugzilla report https://bugs.llvm.org/show_bug.cgi?id=48798. See this report for a minimal reproducer derived from the code fixed here as well as the resolution. This should fix SEGFAULTs as reported in envoyproxy#14756. Fixes: envoyproxy#1241 Signed-off-by: Jarno Rajahalme <jarno@covalent.io>
**Description** This adds an end-to-end upgrade testing where we are supposed to test two test scenarios: simply rolling upgrade and control plane upgrade while keep making requests and verify no requests are dropped. What we found is that, as reported in #1241, there's a slight gap between Envoy stop receiving requests and extproc termination, hence users might experience requests being dropped during upgrade. The fundamental fix is to set extproc sidecar container in the k8s API sense, but it's only available after k8s v1.33 by default. So, this adds a common workaround to sleep before the context cancelation. The workaround fix is verified to work in the newly added e2e upgrade tests. After this is merged, adding k8s-version detection and enabling sidecar by default automatically as well as backporting the fix to v0.3 would be necessary. After that, we can enable the control-plane upgrade variant of the test case that is currently disabled in this commit. **Related Issues/PRs (if applicable)** Closes #1241 Closes #1060 --------- Signed-off-by: Takeshi Yoneda <t.y.mathetake@gmail.com>
No description provided.