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

Pull out SIGTERMHandler, flag-gate ingress and firewall controller, and run L7 NEG as default #2414

Merged
merged 4 commits into from
Feb 2, 2024
Merged

Pull out SIGTERMHandler, flag-gate ingress and firewall controller, and run L7 NEG as default #2414

merged 4 commits into from
Feb 2, 2024

Conversation

sawsa307
Copy link
Contributor

@sawsa307 sawsa307 commented Dec 22, 2023 •
edited
Loading

  • Pull out SIGTERM Handler so it is independent from the ingress controller and only closes closeCh when TERM signal is received.
  • Run ingress controller in its own goroutine.
  • Add a waitgroup to ensure cleanups are properly executed after close.
  • Exit when cleanups are finished or 30 seconds timeout is reached. Instead of waiting on cleanup indefenitely, this avoids possible hanging of the program.
  • Flag-gate ingress and firewall controller using runIngressController flag.

/assign @swetharepakula

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Dec 22, 2023
@k8s-ci-robot
Copy link
Contributor

Hi @sawsa307. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@k8s-ci-robot k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Dec 22, 2023
@sawsa307 sawsa307 changed the title Make SIGTERMHandler independent of the running of ingress controller. Pull out SIGTERMHandler, and run ingress controller in its own goroutine. Jan 3, 2024
@swetharepakula
Copy link
Member

/ok-to-test

@k8s-ci-robot k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jan 5, 2024
swetharepakula
swetharepakula reviewed Jan 5, 2024
View reviewed changes
cmd/glbc/main.go Outdated Show resolved Hide resolved
@sawsa307 sawsa307 force-pushed the update-sig-handler branch 3 times, most recently from 05f35f1 to ef15997 Compare January 9, 2024 23:29
@sawsa307 sawsa307 mentioned this pull request Jan 9, 2024
Closed
@sawsa307 sawsa307 changed the title Pull out SIGTERMHandler, and run ingress controller in its own goroutine. Pull out SIGTERMHandler, and flag-gate ingress and firewall controller. Jan 9, 2024
@sawsa307 sawsa307 force-pushed the update-sig-handler branch 5 times, most recently from bb82839 to fc0943d Compare January 10, 2024 21:29
@sawsa307
Copy link
Contributor Author

/hold

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 11, 2024
@sawsa307 sawsa307 mentioned this pull request Jan 11, 2024
Closed
@k8s-ci-robot k8s-ci-robot removed the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Jan 12, 2024
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 30, 2024
@sawsa307
Copy link
Contributor Author

/unhold

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 30, 2024
@sawsa307 sawsa307 force-pushed the update-sig-handler branch 2 times, most recently from 80b4d97 to 1946595 Compare January 30, 2024 19:58
@sawsa307
Copy link
Contributor Author

sawsa307 commented Jan 30, 2024 •
edited
Loading

Create #2453, #2454, #2455 to separate the clean up needed.

swetharepakula
swetharepakula reviewed Feb 2, 2024
View reviewed changes
cmd/glbc/main.go Outdated
go func() {
defer close(doneCh)
// Wait until related controllers are done with cleanup.
// If Ingress controller is not running, the program will exit immediately
Copy link
Member

Choose a reason for hiding this comment

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

does this comment still apply?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No it should not, I'll remove this.

@swetharepakula
Copy link
Member

LGTM, just needs to be rebased with the other PRs before this can merge.

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 2, 2024
@sawsa307
Pull out SIGTERMHandler, and run ingress controller in its own gorout…
e346779 
…ine.

* Currently, SIGTERMHandler triggers stopCh closes by calling
  lbc.Stop().
* We want to make SIGTERMHandler works regardless of whether ingress
  controller is running.
* Thus, now in the handler, we close stopCh instead of calling
  lbc.Stop().
* Also, ingress controller will run in its own goroutine like other
  controllers so it can be properly turned off if needed.
* In main.go, we will wait on the stopCh after starting all controllers
  so the binary won't exit early.
* Create closeStopCh function to make sure stopCh is only closed once.
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 2, 2024
@sawsa307
Copy link
Contributor Author

sawsa307 commented Feb 2, 2024

/retest

swetharepakula
swetharepakula reviewed Feb 2, 2024
View reviewed changes
Copy link
Member

@swetharepakula swetharepakula left a comment

Choose a reason for hiding this comment

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

couple nits on logging, otherwise LGTM.

Thanks!

cmd/glbc/main.go Outdated Show resolved Hide resolved
cmd/glbc/main.go Show resolved Hide resolved
@sawsa307
Add a waitGroup to wait for controllers to finish cleaning up.
8b28a50 
* Add a waitGroup in runControllers to make sure when Ingress controller
  is not running, other controllers(NEG controller) have time to finish
  cleanng up.
* This is particularly necessary if NEG controller is separated from
  other controllers.
@sawsa307
Exit when cleanups are finished or 30 second timeout is reached.
9a391d7
@sawsa307
Flag-gate ingress and firewall controllers.
6193662 
* Update run-ingress-controller flag description.
* Flag-gate ingress and firewall controller using run-ingress-controller flag.
@sawsa307
Copy link
Contributor Author

sawsa307 commented Feb 2, 2024

couple nits on logging, otherwise LGTM.

Thanks!

Comments are addressed. PTAL!

swetharepakula
swetharepakula reviewed Feb 2, 2024
View reviewed changes
Copy link
Member

@swetharepakula swetharepakula left a comment

Choose a reason for hiding this comment

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

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 2, 2024
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: sawsa307, swetharepakula

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 2, 2024
@k8s-ci-robot k8s-ci-robot merged commit 8caa8f2 into kubernetes:master Feb 2, 2024
5 checks passed
@sawsa307 sawsa307 mentioned this pull request Mar 12, 2024
Closed
9 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@swetharepakula swetharepakula swetharepakula left review comments

@aojea aojea Awaiting requested review from aojea

@gauravkghildiyal gauravkghildiyal Awaiting requested review from gauravkghildiyal

Assignees

@swetharepakula swetharepakula

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@sawsa307 @k8s-ci-robot @swetharepakula