-
Notifications
You must be signed in to change notification settings - Fork 462
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
Fix status syncer and fix Istio integration values for discoveryAddress, istioMetaMeshId and istioMetaClusterId #9625
Fix status syncer and fix Istio integration values for discoveryAddress, istioMetaMeshId and istioMetaClusterId #9625
Conversation
Issues linked to changelog: |
Visit the preview URL for this PR (updated for commit 85e4c59): https://gloo-edge--pr9625-npolshak-fix-proxy-s-6nbtf8fa.web.app (expires Thu, 27 Jun 2024 12:30:14 GMT) 🔥 via Firebase Hosting GitHub Action 🌎 Sign: 77c2b86e287749579b7ff9cadb81e099042ef677 |
projects/gateway2/helm/gloo-gateway/templates/gateway/proxy-deployment.yaml
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some questions around how we track the statuses, and how much we publish to users about the internals of the system. Thanks for the clear testing steps!
I don't know enough about the Proxy Syncer to assess those changes, but the GatewayParameters changes look good to me |
…ss, istioMetaMeshId and istioMetaClusterId (#9625) * initial fixes * cleanup * add log lines * pr feedback * pr feedback --------- Co-authored-by: soloio-bulldozer[bot] <48420018+soloio-bulldozer[bot]@users.noreply.github.com>
…or discoveryAddress, istioMetaMeshId and istioMetaClusterId (#9653) * Fix status syncer and fix Istio integration values for discoveryAddress, istioMetaMeshId and istioMetaClusterId (#9625) * initial fixes * cleanup * add log lines * pr feedback * pr feedback --------- Co-authored-by: soloio-bulldozer[bot] <48420018+soloio-bulldozer[bot]@users.noreply.github.com> * move changelog --------- Co-authored-by: soloio-bulldozer[bot] <48420018+soloio-bulldozer[bot]@users.noreply.github.com> Co-authored-by: Sam Heilbron <[email protected]>
Description
This PR fixes two bugs:
HandleProxyReports
is called beforeQueueStatusForProxies
The status syncer error can happen in several cases:
HandleProxyReports
to be called beforeQueueStatusForProxies
. This is a valid case that we want to emit a debug log. Once the proxy syncer is called,HandleProxyReports
will be called again and correctly sync the status.HandleProxyReports
can be called first. This is fixed by checking all iterations are handled before removing.The fix:
resyncsPerIteration
to check all iterations are handled before removing registryPerSyncAPI changes
None
Code changes
HandleProxyReports
is called beforeQueueStatusForProxies
CI changes
None
Docs changes
None
Context
Users ran into this bug doing ... \ Users needed this feature to ...
See slack conversation here
Interesting decisions
We chose to do things this way because ...
Testing steps
I applied these resources to test based on Rinor's comment on the issue:
I manually verified behavior by running a watch for the log line:
No logs are found for:
While running this script to delete and recreate the HTTP route:
While the script ran, the debug line was not present when running the control plane with LOG_LEVEL=debug:
Versus on main, the panic is hit when the control plane starts up as the script runs to make the HTTPRoute flicker, then stabelizes:
I used the same Gateway as before, and applied these resources to test:
Then created a script to apply/delete the upstream:
Then ran the script while watching the logs:
The status syncer logs were hit:
But the error did not occur:
Versus the original code on main (with log lines added to the start of QueueStatusForProxies and HandleProxyReports):
Notes for reviewers
None
Checklist:
BOT NOTES:
resolves https://github.com/solo-io/solo-projects/issues/6304
resolves https://github.com/solo-io/solo-projects/issues/6107