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
pilot: fix bug in endpoint updates #50746
Conversation
Skipping CI for Draft Pull Request. |
/test all |
ef975e3
to
1ee0d93
Compare
/test all |
@hzxuzhonghu @ramaraochavali wdyt? if desired ill write an optimized Equals function |
Yes. I agree we should compare. I think all of this used to be in |
TBH I am not sure how this isn't a bigger issue. Currently if an endpoint changes we are not triggering a push at all. In this case, it was port number, but could be others. Implementation is *bad* right now, but if this is desired we can swap for an optimized Equals implementation
1ee0d93
to
0a24f5c
Compare
// Endpoint update is triggered since its a brand new service | ||
if ev := fx.WaitOrFail(t, "xds full"); !ev.Reason.Has(model.EndpointUpdate) { | ||
t.Fatalf("xds push reason does not contain %v: %v", model.EndpointUpdate, ev) | ||
} | ||
// headless service update must trigger nds push, so we trigger a full push. | ||
ev := fx.WaitOrFail(t, "xds full") | ||
if !ev.Reason.Has(model.HeadlessEndpointUpdate) { | ||
t.Fatalf("xds push reason does not contain %v", model.HeadlessEndpointUpdate) | ||
if ev := fx.WaitOrFail(t, "xds full"); !ev.Reason.Has(model.HeadlessEndpointUpdate) { | ||
t.Fatalf("xds push reason does not contain %v: %v", model.HeadlessEndpointUpdate, ev) |
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.
This is NOT a production impact change. Only test.
Now our test properly mimics how the real istiod does pushes; in the real world we are triggering double pushes.
Note with debounce its mostly moot, though
0a24f5c
to
371ce1f
Compare
In response to a cherrypick label: new pull request created: #51025 |
In response to a cherrypick label: #50746 failed to apply on top of branch "release-1.21":
|
In response to a cherrypick label: new issue created for failed cherrypick: #51026 |
* pilot: fix bug in endpoint updates TBH I am not sure how this isn't a bigger issue. Currently if an endpoint changes we are not triggering a push at all. In this case, it was port number, but could be others. Implementation is *bad* right now, but if this is desired we can swap for an optimized Equals implementation * Prod impl (cherry picked from commit 53a49fe)
* pilot: fix bug in endpoint updates TBH I am not sure how this isn't a bigger issue. Currently if an endpoint changes we are not triggering a push at all. In this case, it was port number, but could be others. Implementation is *bad* right now, but if this is desired we can swap for an optimized Equals implementation * Prod impl (cherry picked from commit 53a49fe)
TBH I am not sure how this isn't a bigger issue. Currently if an
endpoint changes we are not triggering a push at all. In this case, it
was port number, but could be others.
Implementation is bad right now, but if this is desired we can swap
for an optimized Equals implementation