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

Remove registratation socket file on shutdown #61

Merged
merged 1 commit into from
Jul 31, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 0 additions & 4 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -76,10 +76,6 @@ the actual driver's name.
args:
- "--csi-address=/csi/csi.sock"
- "--kubelet-registration-path=/var/lib/kubelet/plugins/<drivername.example.com>/csi.sock"
lifecycle:
preStop:
exec:
command: ["/bin/sh", "-c", "rm -rf /registration/<plugin> /registration/<drivername.example.com>-reg.sock"]
volumeMounts:
- name: plugin-dir
mountPath: /csi
Expand Down
22 changes: 21 additions & 1 deletion cmd/csi-node-driver-registrar/node_register.go
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,9 @@ import (
"fmt"
"net"
"os"
"os/signal"
"runtime"
"syscall"

"google.golang.org/grpc"

Expand All @@ -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)
Expand All @@ -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)
Expand All @@ -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)
Copy link
Contributor

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 ?

Copy link
Contributor Author

@Madhu-1 Madhu-1 Feb 10, 2020

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

Copy link
Contributor

Choose a reason for hiding this comment

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

cc @jingxu97 as well

Copy link
Contributor

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

<-sigc
socketPath := buildSocketPath(csiDriverName)
err := os.Remove(socketPath)
Copy link
Contributor

@kkmsft kkmsft Feb 6, 2020

Choose a reason for hiding this comment

The 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 ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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

Copy link
Contributor

Choose a reason for hiding this comment

The 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)
}