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

CP/DP Split: Remove NGINX manager and deployment #2936

Open
wants to merge 6 commits into
base: change/control-data-plane-split
Choose a base branch
from

Conversation

sjberman
Copy link
Contributor

Removing the nginx runtime manager and deployment container since nginx will live in its own pod managed by agent. Temporarily saving the nginx deployment and service for future use.

Updated the control plane liveness probe to return true once it's processed all resources, instead of after it's written config to nginx (since nginx may not be started yet in the future architecture).

Testing: Verified that the control plane is able to start and run on its own.

Closes #2838

@sjberman sjberman requested a review from a team as a code owner December 19, 2024 20:54
@github-actions github-actions bot added documentation Improvements or additions to documentation dependencies Pull requests that update a dependency file chore Pull requests for routine tasks helm-chart Relates to helm chart labels Dec 19, 2024
Copy link

codecov bot commented Dec 19, 2024

Codecov Report

Attention: Patch coverage is 31.37255% with 35 lines in your changes missing coverage. Please review.

Project coverage is 90.50%. Comparing base (06fd179) to head (b7d53db).

Files with missing lines Patch % Lines
internal/mode/static/manager.go 0.00% 25 Missing ⚠️
internal/mode/static/nginx/agent/agent.go 0.00% 7 Missing ⚠️
internal/mode/static/handler.go 81.81% 1 Missing and 1 partial ⚠️
cmd/gateway/initialize.go 0.00% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@                         Coverage Diff                         @@
##           change/control-data-plane-split    #2936      +/-   ##
===================================================================
+ Coverage                            89.93%   90.50%   +0.56%     
===================================================================
  Files                                  111      105       -6     
  Lines                                11421    10979     -442     
  Branches                                50       50              
===================================================================
- Hits                                 10271     9936     -335     
+ Misses                                1089      990      -99     
+ Partials                                61       53       -8     

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

internal/mode/static/nginx/agent/agent.go Outdated Show resolved Hide resolved
internal/mode/static/nginx/agent/agent.go Outdated Show resolved Hide resolved
@sjberman sjberman requested a review from kate-osborn December 20, 2024 15:59
Copy link
Contributor

@kate-osborn kate-osborn left a comment

Choose a reason for hiding this comment

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

See my review for a small nit, but otherwise LGTM

internal/mode/static/nginx/agent/agent.go Outdated Show resolved Hide resolved
@salonichf5
Copy link
Contributor

nit: I see comments in various places using we've or we are ready. Do you think it would be better to use control plane or NGINX Gateway Fabric to be clearer for future readers?

Removing the nginx runtime manager and deployment container since nginx will live in its own pod managed by agent. Temporarily saving the nginx deployment and service for future use.

Updated the control plane liveness probe to return true once it's processed all resources, instead of after it's written config to nginx (since nginx may not be started yet in the future architecture).
@sjberman sjberman force-pushed the chore/remove-nginx-mgr branch from 09b399d to cdb28c7 Compare December 26, 2024 15:07
@sjberman
Copy link
Contributor Author

nit: I see comments in various places using we've or we are ready. Do you think it would be better to use control plane or NGINX Gateway Fabric to be clearer for future readers?

I don't feel strongly about the wording in this case

@sjberman sjberman requested a review from salonichf5 December 26, 2024 16:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
chore Pull requests for routine tasks dependencies Pull requests that update a dependency file documentation Improvements or additions to documentation helm-chart Relates to helm chart
Projects
Status: 🏗 In Progress
Development

Successfully merging this pull request may close these issues.

3 participants