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.

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

keep Gateway's AttachedRoutes field up to date #4052

Merged
merged 7 commits into from
May 26, 2023
Merged

Conversation

mlavacca
Copy link
Member

@mlavacca mlavacca commented May 22, 2023

What this PR does / why we need it:

We need to update and keep up to date the Gateway's AttachedRoutes field, with the actual number of routes accepted by it.

Which issue this PR fixes:

Fixes #3120

Special notes for your reviewer:

PR Readiness Checklist:

Complete these before marking the PR as ready to review:

  • the CHANGELOG.md release notes have been updated to reflect any significant (and particularly user-facing) changes introduced by this PR

@codecov
Copy link

codecov bot commented May 22, 2023

Codecov Report

Patch coverage: 55.6% and no project coverage change.

Comparison is base (753e91f) 60.0% compared to head (55f96c7) 60.1%.

Additional details and impacted files
@@          Coverage Diff           @@
##            main   #4052    +/-   ##
======================================
  Coverage   60.0%   60.1%            
======================================
  Files        151     151            
  Lines      16612   16735   +123     
======================================
+ Hits        9982   10066    +84     
- Misses      5999    6035    +36     
- Partials     631     634     +3     
Impacted Files Coverage Δ
internal/controllers/gateway/gateway_controller.go 12.6% <13.0%> (+<0.1%) ⬆️
internal/controllers/gateway/gateway_utils.go 44.8% <58.4%> (+1.7%) ⬆️
internal/controllers/gateway/route_utils.go 76.8% <75.8%> (+1.8%) ⬆️
internal/util/builder/allowedroutes.go 66.6% <100.0%> (+16.6%) ⬆️

... and 3 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@pull-request-size pull-request-size bot added size/L and removed size/M labels May 23, 2023
@mlavacca mlavacca force-pushed the mlavacca/attached-routes branch 5 times, most recently from 1773e9b to 97cce40 Compare May 24, 2023 16:05
@mlavacca mlavacca marked this pull request as ready for review May 24, 2023 16:47
@mlavacca mlavacca requested a review from a team as a code owner May 24, 2023 16:47
@shaneutt shaneutt self-requested a review May 24, 2023 17:06
CHANGELOG.md Show resolved Hide resolved
internal/controllers/gateway/gateway_controller.go Outdated Show resolved Hide resolved
internal/controllers/gateway/gateway_controller.go Outdated Show resolved Hide resolved
internal/controllers/gateway/gateway_utils.go Outdated Show resolved Hide resolved
internal/controllers/gateway/gateway_utils.go Show resolved Hide resolved
internal/controllers/gateway/route_utils.go Show resolved Hide resolved
@mlavacca mlavacca force-pushed the mlavacca/attached-routes branch 3 times, most recently from 9d827d8 to f29af72 Compare May 25, 2023 14:24
@shaneutt shaneutt dismissed their stale review May 25, 2023 15:55

No remaining blockers.

@shaneutt shaneutt requested a review from pmalek May 25, 2023 15:56
internal/controllers/gateway/gateway_utils.go Show resolved Hide resolved
internal/controllers/gateway/gateway_utils.go Outdated Show resolved Hide resolved
internal/controllers/gateway/gateway_utils.go Outdated Show resolved Hide resolved
internal/controllers/gateway/gateway_utils.go Outdated Show resolved Hide resolved
internal/controllers/gateway/route_utils.go Show resolved Hide resolved
test/integration/helpers_test.go Show resolved Hide resolved
mlavacca and others added 6 commits May 26, 2023 13:17
The AttachedRoutes listenerStatus field gets updated and enforced by the
GatewayController.

Signed-off-by: Mattia Lavacca <lavacca.mattia@gmail.com>
Signed-off-by: Mattia Lavacca <lavacca.mattia@gmail.com>
Signed-off-by: Mattia Lavacca <lavacca.mattia@gmail.com>
Signed-off-by: Mattia Lavacca <lavacca.mattia@gmail.com>
Co-authored-by: Patryk Małek <patryk.malek@konghq.com>
Signed-off-by: Mattia Lavacca <lavacca.mattia@gmail.com>
shaneutt
shaneutt previously approved these changes May 26, 2023
Copy link
Contributor

@shaneutt shaneutt left a comment

Choose a reason for hiding this comment

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

Once the linter is happy and @pmalek's comments are resolved, LGTM

Signed-off-by: Mattia Lavacca <lavacca.mattia@gmail.com>
@mlavacca mlavacca requested a review from pmalek May 26, 2023 12:51
@pmalek pmalek added the area/gateway-api Relating to upstream Kubernetes SIG Networking Gateway API label May 26, 2023
@pmalek pmalek added this to the KIC v2.10.0 milestone May 26, 2023
@mlavacca mlavacca merged commit d8c1396 into main May 26, 2023
@mlavacca mlavacca deleted the mlavacca/attached-routes branch May 26, 2023 17:56
@gjkim42
Copy link
Contributor

gjkim42 commented Aug 4, 2023

I found that when we create more than 101 httproutes, the kong-ingress-controller cannot determine the attachedRoutes field correctly and the value keeps fluctuating.
It ends up making a huge amount of requests to kube-apiserver to update its fluctuating attachedRoutes continually.

Any idea in here?

@mlavacca

@mlavacca
Copy link
Member Author

Hi, @gjkim42, thanks for reporting this issue! does this behavior happen consistently at 101? What I mean is, can you confirm it does not happen with 100 routes, but it does with 101? Furthermore, do you have logs with throttling from the API server? It shouldn't happen, as the local cache should be used to retrieve Routes and Gateways 🤔

@gjkim42
Copy link
Contributor

gjkim42 commented Aug 10, 2023

I filed the bug and it has been addressed.

Please see #4456 (comment) here.

@mlavacca
Copy link
Member Author

Cool, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/gateway-api Relating to upstream Kubernetes SIG Networking Gateway API size/XL
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Gateway listeners ListenerStatus.AttachedRoutes not updated on route attachment
4 participants