-
Notifications
You must be signed in to change notification settings - Fork 22
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
OCPBUGS-11888: handle daemonSet pods restart #347
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3,8 +3,11 @@ package ebpfsyncer | |
import ( | ||
"context" | ||
"fmt" | ||
"os" | ||
"os/signal" | ||
"strings" | ||
"sync" | ||
"syscall" | ||
|
||
"github.com/openshift/ingress-node-firewall/api/v1alpha1" | ||
infv1alpha1 "github.com/openshift/ingress-node-firewall/api/v1alpha1" | ||
|
@@ -72,6 +75,8 @@ func (e *ebpfSingleton) SyncInterfaceIngressRules( | |
logger := e.log.WithName("syncIngressNodeFirewallResources") | ||
logger.Info("Running sync operation", "ifaceIngressRules", ifaceIngressRules, "isDelete", isDelete) | ||
|
||
sigc := make(chan os.Signal, 1) | ||
|
||
// Stop the poller for the time of this operation and start it again afterwards. | ||
if e.stats != nil { | ||
e.stats.StopPoll() | ||
|
@@ -82,6 +87,15 @@ func (e *ebpfSingleton) SyncInterfaceIngressRules( | |
}() | ||
} | ||
|
||
signal.Notify(sigc, os.Interrupt, syscall.SIGTERM) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you test if you can still sig TERM the process (if this is needed)? I tested this quickly and kill -9 obviously works still, but the TERM signal doesn't shut down the process any more:
When I add
This is not a suggestion to add os.Exit, it's merely a question from my side There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. when u delete ds or delete the pods with something like There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah my question is for the opposite case: when you run kill $pid, will the application be torn down and will the pod restart. Or if you test the process standalone, can you still CTRL-C and it will shut down There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. will try it out and see if I can do much in separate PR thanks for the review!!! |
||
go func(c chan os.Signal) { | ||
// Wait for a SIGTERM | ||
<-c | ||
if e.c != nil { | ||
e.resetAll() | ||
} | ||
}(sigc) | ||
|
||
// Create a new manager if none exists. | ||
if err := e.createNewManager(); err != nil { | ||
return err | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: The code changes in ingressnofirewallnodestate_controller.go and in ebpfsyncer.go are not related, correct? So perhaps break this into 2 commits?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah that was just unused arg