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

fix: trigger a resync to avoid missing any status updates #4878

Closed
wants to merge 2 commits into from

Conversation

zhaohuabing
Copy link
Member

@zhaohuabing zhaohuabing commented Dec 9, 2024

This PR does two things:

  • Remove wait to make sure the status updater won't block the reconcile loop in a non-leader scenario.
  • Trigger a reconciliation after starting the status updater to avoid missing any status updates.

Fixes: #4845

Release Note: Yes

@zhaohuabing zhaohuabing requested a review from a team as a code owner December 9, 2024 05:58
@zhaohuabing zhaohuabing marked this pull request as draft December 9, 2024 05:58
Copy link

codecov bot commented Dec 9, 2024

Codecov Report

Attention: Patch coverage is 91.66667% with 4 lines in your changes missing coverage. Please review.

Project coverage is 66.31%. Comparing base (05ee5f4) to head (a6f0b60).
Report is 3 commits behind head on main.

Files with missing lines Patch % Lines
internal/provider/kubernetes/controller.go 91.66% 2 Missing ⚠️
internal/provider/kubernetes/kubernetes.go 75.00% 0 Missing and 1 partial ⚠️
internal/provider/kubernetes/status_updater.go 95.00% 1 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main    #4878   +/-   ##
=======================================
  Coverage   66.31%   66.31%           
=======================================
  Files         209      209           
  Lines       31956    31987   +31     
=======================================
+ Hits        21191    21212   +21     
- Misses       9518     9525    +7     
- Partials     1247     1250    +3     

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

@zhaohuabing zhaohuabing changed the title fix: Trigger a resync to avoid missing any status update fix: Trigger a resync to avoid missing any status updates Dec 9, 2024
Signed-off-by: Huabing Zhao <[email protected]>
@zhaohuabing zhaohuabing marked this pull request as ready for review December 9, 2024 06:46
@zhaohuabing zhaohuabing changed the title fix: Trigger a resync to avoid missing any status updates fix: trigger a resync to avoid missing any status updates Dec 9, 2024
@arkodg
Copy link
Contributor

arkodg commented Dec 9, 2024

@zhaohuabing I dont think this will solve the original problem because the re sync will not trigger the status watchable subscribed handler to get any updates, because nothing would have changed ( in the watchable store), so the original problem of missing status updates will still persist

@arkodg
Copy link
Contributor

arkodg commented Dec 9, 2024

my suggestion would be to

  1. always push status updates via watchable, which was implemented in fix: decouple gateway status updates from the reconciler #4767
    and
  2. we should block the status updater watchable subscribe goroutine similar to
    // When leader election is active, infrastructure initialization occurs only upon acquiring leadership
    and only enable it once its part of the controller thats the leader
    wdyt @alexwo @guydc

@alexwo
Copy link
Contributor

alexwo commented Dec 9, 2024

my suggestion would be to

  1. always push status updates via watchable, which was implemented in fix: decoup gateway status updates from the reconciler #4767
    and
  2. we should block the status updater watchable subscribe goroutine similar to
    // When leader election is active, infrastructure initialization occurs only upon acquiring leadership

    and only enable it once its part of the controller thats the leader
    wdyt @alexwo @guydc

+1 sounds good to me.

@zhaohuabing zhaohuabing marked this pull request as draft December 10, 2024 02:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Proxies connected to the secondary gateway do not receive configuration
3 participants