-
Notifications
You must be signed in to change notification settings - Fork 141
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
Remove registratation socket file on shutdown #61
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 |
---|---|---|
|
@@ -20,7 +20,9 @@ import ( | |
"fmt" | ||
"net" | ||
"os" | ||
"os/signal" | ||
"runtime" | ||
"syscall" | ||
|
||
"google.golang.org/grpc" | ||
|
||
|
@@ -36,7 +38,7 @@ func nodeRegister( | |
// as gRPC server which replies to registration requests initiated by kubelet's | ||
// pluginswatcher infrastructure. Node labeling is done by kubelet's csi code. | ||
registrar := newRegistrationServer(csiDriverName, *kubeletRegistrationPath, supportedVersions) | ||
socketPath := fmt.Sprintf("/registration/%s-reg.sock", csiDriverName) | ||
socketPath := buildSocketPath(csiDriverName) | ||
if err := util.CleanupSocketFile(socketPath); err != nil { | ||
klog.Errorf("%+v", err) | ||
os.Exit(1) | ||
|
@@ -62,6 +64,7 @@ func nodeRegister( | |
// Registers kubelet plugin watcher api. | ||
registerapi.RegisterRegistrationServer(grpcServer, registrar) | ||
|
||
go removeRegSocket(csiDriverName) | ||
// Starts service | ||
if err := grpcServer.Serve(lis); err != nil { | ||
klog.Errorf("Registration Server stopped serving: %v", err) | ||
|
@@ -70,3 +73,20 @@ func nodeRegister( | |
// If gRPC server is gracefully shutdown, exit | ||
os.Exit(0) | ||
} | ||
|
||
func buildSocketPath(csiDriverName string) string { | ||
return fmt.Sprintf("/registration/%s-reg.sock", csiDriverName) | ||
} | ||
|
||
func removeRegSocket(csiDriverName string) { | ||
sigc := make(chan os.Signal, 1) | ||
signal.Notify(sigc, syscall.SIGTERM) | ||
<-sigc | ||
socketPath := buildSocketPath(csiDriverName) | ||
err := os.Remove(socketPath) | ||
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. In the main code path we remove it only if it's a socket (not sure why). Can you please clarify why we are removing in this path without doing the same check ? I think we should have a single code path for delete which is called from the sync path and from the async signal path as well. What do you say ? 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. the check is not required here, only we are going to delete the socket if it is not present we are discarding the error, we are good here 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. The original check seems wrong (if the file isn't a socket, proceed and listen to it anyway...?) It should be fixed but doesn't have to be in this PR I agree we don't need the check here - the socket should've been created by a node-driver-registrar and if something dropped a different file in its place, it did so at its own risk |
||
if err != nil && !os.IsNotExist(err) { | ||
klog.Errorf("failed to remove socket: %s with error: %+v", socketPath, err) | ||
os.Exit(1) | ||
} | ||
os.Exit(0) | ||
} |
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.
Any ideas whether this will work on windows ? @ddebroy - any ideas ?
If not can you please move this to linux utils ?
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.
if someone confirms it won't work in windows i will move it linunx_utils
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.
cc @jingxu97 as well
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.
Notifications on
syscall.SIGTERM
does work for Windows: https://golang.org/pkg/os/signal/#hdr-Windows. So LGTM from a Windows perspective. cc @jingxu97