Skip to content

Fix revert commit for leader election #3136

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

Merged

Conversation

salonichf5
Copy link
Contributor

@salonichf5 salonichf5 commented Feb 14, 2025

Fix revert commit for leader election story

@salonichf5 salonichf5 requested a review from a team as a code owner February 14, 2025 18:31
@github-actions github-actions bot added the bug Something isn't working label Feb 14, 2025
Copy link

codecov bot commented Feb 14, 2025

Codecov Report

Attention: Patch coverage is 57.14286% with 6 lines in your changes missing coverage. Please review.

Project coverage is 79.57%. Comparing base (6337c97) to head (00f2ffa).
Report is 124 commits behind head on change/control-data-plane-split.

Files with missing lines Patch % Lines
internal/mode/static/manager.go 0.00% 6 Missing ⚠️

❗ There is a different number of reports uploaded between BASE (6337c97) and HEAD (00f2ffa). Click for more details.

HEAD has 1 upload less than BASE
Flag BASE (6337c97) HEAD (00f2ffa)
3 2
Additional details and impacted files
@@                         Coverage Diff                          @@
##           change/control-data-plane-split    #3136       +/-   ##
====================================================================
- Coverage                            89.74%   79.57%   -10.18%     
====================================================================
  Files                                  109      124       +15     
  Lines                                11150    13470     +2320     
  Branches                                50       62       +12     
====================================================================
+ Hits                                 10007    10719      +712     
- Misses                                1083     2679     +1596     
- Partials                                60       72       +12     

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

@bjee19
Copy link
Contributor

bjee19 commented Feb 14, 2025

One thing to note is in the nginx provisioner, once NGF becomes leader it does not provision nginx if there was an existing gateway. @sjberman by reverting the leader election code there may be a little bit of refactoring of the NginxProvisioner.

Copy link
Collaborator

@sjberman sjberman left a comment

Choose a reason for hiding this comment

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

We should still be able to use the EnableAfterBecameLeader type like we did before Ben's work, and just call it twice for status updater and provisioner, since both have Enable functions.

@sjberman
Copy link
Collaborator

@bjee19 I'll keep this in mind with my current work

@salonichf5 salonichf5 merged commit 85c6f78 into change/control-data-plane-split Feb 18, 2025
24 of 37 checks passed
@salonichf5 salonichf5 deleted the fix/revert-leader-election branch February 18, 2025 18:10
sjberman pushed a commit that referenced this pull request Feb 25, 2025
* Add back runnables change and call to nginx provisioner enable

---------

Co-authored-by: Benjamin Jee <[email protected]>
sjberman pushed a commit that referenced this pull request Mar 4, 2025
* Add back runnables change and call to nginx provisioner enable

---------

Co-authored-by: Benjamin Jee <[email protected]>
sjberman pushed a commit that referenced this pull request Apr 23, 2025
* Add back runnables change and call to nginx provisioner enable

---------

Co-authored-by: Benjamin Jee <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants