Skip to content
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.

Sign up for GitHub

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

Jump to bottom

fix: decoup gateway status updates from the reconciler #4767

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

fix: decoup gateway status updates from the reconciler #4767

wants to merge 6 commits into from
+73 −55

Conversation

zhaohuabing
Copy link
Member

@zhaohuabing zhaohuabing commented Nov 22, 2024 •
edited
Loading

Fixes: #4685
Release Note: Yes

@zhaohuabing zhaohuabing requested a review from a team as a code owner November 22, 2024 07:50
@zhaohuabing zhaohuabing changed the title decoup gateway status update fix: decoup gateway status update Nov 22, 2024
@zhaohuabing zhaohuabing mentioned this pull request Nov 22, 2024
Open
@zhaohuabing
decoup gateway status update
8c84649 
Signed-off-by: Huabing Zhao <[email protected]>
@zhaohuabing zhaohuabing mentioned this pull request Nov 22, 2024
Open
@codecov Codecov
Copy link

codecov bot commented Nov 22, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 75.00000% with 13 lines in your changes missing coverage. Please review.

Project coverage is 65.64%. Comparing base (71c0b54) to head (e406088).

Files with missing lines Patch % Lines
internal/provider/kubernetes/controller.go 50.00% 6 Missing ⚠️
internal/provider/kubernetes/status.go 71.42% 4 Missing and 2 partials ⚠️
internal/provider/kubernetes/predicates.go 92.85% 1 Missing ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #4767      +/-   ##
==========================================
+ Coverage   65.61%   65.64%   +0.03%     
==========================================
  Files         211      211              
  Lines       31961    31981      +20     
==========================================
+ Hits        20972    20995      +23     
+ Misses       9750     9744       -6     
- Partials     1239     1242       +3

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚨 Try these New Features:

@zhaohuabing zhaohuabing changed the title fix: decoup gateway status update fix: decoup gateway status updates from the reconciler Nov 22, 2024
arkodg
arkodg reviewed Nov 22, 2024
View reviewed changes
internal/provider/kubernetes/status.go
@@ -564,34 +592,3 @@ func (r *gatewayAPIReconciler) updateStatusForGateway(ctx context.Context, gtw *
}),
})
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what about

gateway/internal/provider/kubernetes/status.go

Line 524 in 71c0b54

func (r *gatewayAPIReconciler) updateStatusForGateway(ctx context.Context, gtw *gwapiv1.Gateway) {

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suspect the call to updateStatusForGateway from inside the predicate blocked the reconciler.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aren't we deleting update status for gateway ?

@arkodg
Copy link
Contributor

arkodg commented Nov 22, 2024

were we deadlocking ?

arkodg
arkodg reviewed Nov 22, 2024
View reviewed changes
internal/provider/kubernetes/predicates.go Outdated
@@ -294,7 +294,7 @@ func (r *gatewayAPIReconciler) validateServiceForReconcile(obj client.Object) bo
// Check if the Service belongs to a Gateway, if so, update the Gateway status.
gtw := r.findOwningGateway(ctx, labels)
if gtw != nil {
r.updateStatusForGateway(ctx, gtw)
r.resources.GatewayStatuses.Store(utils.NamespacedName(gtw), &gtw.Status)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is not documented well, but the reason this existed, was so that this flow could update the Status.Address field and Status.Conditions while the other flow (published from Gateway API) would update Status.Conditions

wanted to highlight this

Copy link
Member Author

@zhaohuabing zhaohuabing Nov 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, will add a line in the comment to explain why need to send an update here and how the gateway address is updated.

@zhaohuabing
Copy link
Member Author

zhaohuabing commented Nov 23, 2024 •
edited
Loading

were we deadlocking ?

It's my suspicion. I can't reproduce this issue, but based on the description of #4685, it seems that the reconciler has been blocked - the pods changes haven't been propogated to the Envoy through xDS, and the only PR in v1.1.3 that might cause blocking is #4337 .

@zhaohuabing
fix test
fc97316 
Signed-off-by: Huabing Zhao <[email protected]>
@zhaohuabing
add comment
bccc70c 
Signed-off-by: Huabing Zhao <[email protected]>
@zhaohuabing zhaohuabing force-pushed the fix-gateway-status-blocking branch 2 times, most recently from 91fe885 to bb391aa Compare November 23, 2024 07:37
@zhaohuabing
fix test
7a4c51e 
Signed-off-by: Huabing Zhao <[email protected]>
@zhaohuabing zhaohuabing marked this pull request as draft November 23, 2024 08:32
@zhaohuabing
fix test
e406088 
Signed-off-by: Huabing Zhao <[email protected]>
@zhaohuabing zhaohuabing marked this pull request as ready for review November 23, 2024 12:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@arkodg arkodg Awaiting requested review from arkodg

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

503s and _No_route_to_host errors due to routing to non-existent Endpoints
2 participants
@zhaohuabing @arkodg