-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
set_node_on_first_message_only seems to never set node ID #9682
Comments
Can you point me to a binary and confirm this is a CDS request? |
@kyessenov its from this PR on Istio side: istio/istio#19266. The above is the very first request we see. its possible we are somehow dropping a request, but I was logging right after |
I'm not seeing an issue with envoy on master, let me check istio build. |
Fetched latest envoy build from istio, and tested both ADS and CDS. I seem to get the node ID supplied. Can you help reproducing this? |
CC: @lambdai |
This issue has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in the next 7 days unless it is tagged "help wanted" or other activity occurs. Thank you for your contributions. |
This issue has been automatically closed because it has not had activity in the last 37 days. If this issue is still valid, please ping a maintainer and ask them to label it as "help wanted". Thank you for your contributions. |
Can this issue be re-opened please? |
It looks like what happens is:
So basically we just need to make sure that when connecting to a new upstream we are still sending the node meta |
It appears that the flag was disabled in tests by @fredlas during incremental xDS work and not restored back. Something broke it in the mean time. I'll take a look. |
I know the one you mean - #8478 restored it, but was later rolled back. This sounds like more reason to get that change fully in. |
@fredlas I don't have a clear picture on the state of things. Should I attempt to restore |
#8478 was abandoned and @wgallagher is working on the merge now. He is on vacation this week though. @kyessenov if this is not super urgent I would wait until next week to talk to @wgallagher. If urgent I think it's OK to figure out a fix on top of current master. |
I'm fine waiting. It's unfortunate though since the flag is considered stable. |
Is this by any chance related to envoyproxy/go-control-plane#269 Also a side note, I couldn't find any docs describing |
@wgallagher is going to fix this. Thank you! |
The node metadata is pretty large, so sending it on every request adds a lot of overhead |
@wgallagher are you still working on this? |
…ating envoy bootstrap config When consul is restarted and envoy that had already sent DiscoveryRequests to the previous consul process sends a request to the new process it doesn't respect the setting and never populates DiscoveryRequest.Node for the life of the new consul process due to this bug: envoyproxy/envoy#9682 Fixes #8430
…ating envoy bootstrap config (#8440) When consul is restarted and an envoy that had already sent DiscoveryRequests to the previous consul process sends a request to the new process it doesn't respect the setting and never populates DiscoveryRequest.Node for the life of the new consul process due to this bug: envoyproxy/envoy#9682 Fixes #8430
…ating envoy bootstrap config (#8440) When consul is restarted and an envoy that had already sent DiscoveryRequests to the previous consul process sends a request to the new process it doesn't respect the setting and never populates DiscoveryRequest.Node for the life of the new consul process due to this bug: envoyproxy/envoy#9682 Fixes #8430
Any updates? The node info from requests represents a sizeable amount of our network traffic |
Is anyone still working on this? I think I found the reason and a fix, but the time-consuming part would probably be writing a test and confirming the fix works; don't want to duplicate effort... |
Previously, first_stream_request_ wasn't working as intended because api_state_ is still cached from the previous stream Risk Level: Low Testing: Modified test was failing before the accompanying change Docs Changes: N/A Fixes #9682 Signed-off-by: Taylor Barrella <tabarr@google.com>
Issue Template
Title: set_node_on_first_message_only seems to never set node ID
Description:
I turned on
set_node_on_first_message_only
, and now things are breaking because it appears the node id is actually never sent. Turning on verbose logging on the XDS server shows the first requests we get looks like:Note the missing node id
@kyessenov
The text was updated successfully, but these errors were encountered: