Repository: code-ready/crc
Pull request #1825 (2021-01-07 13:09:02)

Handle SIGTERM in crc daemon
* Catch SIGTERM so that crc daemon ends like other commands (by sending a message to segment)
* runDaemon() can now return an error if it can't listen the unix socket (instead of panic)
* couple of refactorings.

openshift-ci-robot: [APPROVALNOTIFIER] This PR is **APPROVED**

This pull-request has been approved by: *<a href="https://github.com/code-ready/crc/pull/1825#" title="Author self-approved">guillaumerose</a>*

The full list of commands accepted by this bot can be found [here](https://go.k8s.io/bot-commands?repo=code-ready%2Fcrc).

The pull request process is described [here](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process)

<details >
Needs approval from an approver in each of these files:

- ~~[OWNERS](https://github.com/code-ready/crc/blob/master/OWNERS)~~ [guillaumerose]

Approvers can indicate their approval by writing `/approve` in a comment
Approvers can cancel approval by writing `/approve cancel` in a comment
</details>
<!-- META={"approvers":[]} -->
guillaumerose: > In Catch SIGTERM when running crc daemon to gracefully shutdown it, I'm not fully clear what kind of segment events we want to link to daemon stopping? "Amount of time the crc daemon run"?

Yes seems weird. Just wanted to know if someone is using crc daemon and verify we correctly receive failures.
guillaumerose: Everything should be good now. PTAL.
guillaumerose: Waiting for https://github.com/code-ready/crc/pull/1781
guillaumerose: Rebased. This is now way lighter!
guillaumerose: I separated the rename, it's better now.
openshift-ci-robot: [APPROVALNOTIFIER] This PR is **APPROVED**

This pull-request has been approved by: *<a href="https://github.com/code-ready/crc/pull/1825#pullrequestreview-579233388" title="Approved">cfergeau</a>*, *<a href="https://github.com/code-ready/crc/pull/1825#" title="Author self-approved">guillaumerose</a>*

The full list of commands accepted by this bot can be found [here](https://go.k8s.io/bot-commands?repo=code-ready%2Fcrc).

The pull request process is described [here](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process)

<details >
Needs approval from an approver in each of these files:

- ~~[OWNERS](https://github.com/code-ready/crc/blob/master/OWNERS)~~ [cfergeau,guillaumerose]

Approvers can indicate their approval by writing `/approve` in a comment
Approvers can cancel approval by writing `/approve cancel` in a comment
</details>
<!-- META={"approvers":[]} -->
openshift-ci-robot: New changes are detected. LGTM label has been removed.
guillaumerose: Oups sorry, fixed the message.

Patch:
diff --git a/cmd/crc/cmd/daemon.go b/cmd/crc/cmd/daemon.go
index fd1b37875..13d0fc267 100644
--- a/cmd/crc/cmd/daemon.go
+++ b/cmd/crc/cmd/daemon.go
@@ -5,9 +5,11 @@ import (
 	"net"
 	"net/http"
 	"os"
+	"os/signal"
 	"path/filepath"
 	"regexp"
 	"runtime"
+	"syscall"
 	"time"
 
 	"github.com/code-ready/crc/pkg/crc/api"
@@ -37,8 +39,6 @@ var daemonCmd = &cobra.Command{
 		logging.CloseLogging()
 		logging.InitLogrus(logging.LogLevel, constants.DaemonLogFilePath)
 
-		go runDaemon()
-
 		var endpoints []string
 		if runtime.GOOS == "windows" {
 			endpoints = append(endpoints, transport.DefaultURL)
@@ -130,7 +130,21 @@ func run(configuration *types.Configuration, endpoints []string) error {
 			}
 		}()
 	}
-	return <-errCh
+
+	go func() {
+		if err := runDaemon(); err != nil {
+			errCh <- err
+		}
+	}()
+
+	c := make(chan os.Signal, 1)
+	signal.Notify(c, os.Interrupt, syscall.SIGTERM)
+	select {
+	case <-c:
+		return nil
+	case err := <-errCh:
+		return err
+	}
 }
 
 func newConfig() (crcConfig.Storage, error) {
@@ -138,12 +152,12 @@ func newConfig() (crcConfig.Storage, error) {
 	return config, err
 }
 
-func runDaemon() {
+func runDaemon() error {
 	// Remove if an old socket is present
 	os.Remove(constants.DaemonSocketPath)
-	crcAPIServer, err := api.CreateAPIServer(constants.DaemonSocketPath, newConfig, newMachineWithConfig)
+	apiServer, err := api.CreateServer(constants.DaemonSocketPath, newConfig, newMachineWithConfig)
 	if err != nil {
-		logging.Fatal("Failed to launch daemon", err)
+		return err
 	}
-	crcAPIServer.Serve()
+	return apiServer.Serve()
 }
diff --git a/pkg/crc/api/api.go b/pkg/crc/api/api.go
index 076b35e8d..2a0b6bab6 100644
--- a/pkg/crc/api/api.go
+++ b/pkg/crc/api/api.go
@@ -9,17 +9,17 @@ import (
 	"github.com/code-ready/crc/pkg/crc/logging"
 )
 
-func CreateAPIServer(socketPath string, config newConfigFunc, machine newMachineFunc) (CrcAPIServer, error) {
+func CreateServer(socketPath string, config newConfigFunc, machine newMachineFunc) (Server, error) {
 	listener, err := net.Listen("unix", socketPath)
 	if err != nil {
 		logging.Error("Failed to create socket: ", err.Error())
-		return CrcAPIServer{}, err
+		return Server{}, err
 	}
-	return createAPIServerWithListener(listener, config, machine)
+	return createServerWithListener(listener, config, machine)
 }
 
-func createAPIServerWithListener(listener net.Listener, config newConfigFunc, machine newMachineFunc) (CrcAPIServer, error) {
-	apiServer := CrcAPIServer{
+func createServerWithListener(listener net.Listener, config newConfigFunc, machine newMachineFunc) (Server, error) {
+	apiServer := Server{
 		listener:               listener,
 		clusterOpsRequestsChan: make(chan clusterOpsRequest, 10),
 		handlerFactory: func() (RequestHandler, error) {
@@ -36,7 +36,7 @@ func createAPIServerWithListener(listener net.Listener, config newConfigFunc, ma
 	return apiServer, nil
 }
 
-func (api CrcAPIServer) Serve() {
+func (api Server) Serve() error {
 	go api.handleClusterOperations() // go routine that handles start, stop and delete calls
 	for {
 		conn, err := api.listener.Accept()
@@ -48,13 +48,13 @@ func (api CrcAPIServer) Serve() {
 	}
 }
 
-func (api CrcAPIServer) handleClusterOperations() {
+func (api Server) handleClusterOperations() {
 	for req := range api.clusterOpsRequestsChan {
 		api.handleRequest(req.command, req.socket)
 	}
 }
 
-func (api CrcAPIServer) handleRequest(req commandRequest, conn net.Conn) {
+func (api Server) handleRequest(req commandRequest, conn net.Conn) {
 	defer conn.Close()
 	var result string
 
@@ -91,7 +91,7 @@ func (api CrcAPIServer) handleRequest(req commandRequest, conn net.Conn) {
 	writeStringToSocket(conn, result)
 }
 
-func (api CrcAPIServer) handleConnections(conn net.Conn) {
+func (api Server) handleConnections(conn net.Conn) {
 	inBuffer := make([]byte, 1024)
 	var req commandRequest
 	numBytes, err := conn.Read(inBuffer)
diff --git a/pkg/crc/api/types.go b/pkg/crc/api/types.go
index 90ffeb477..827658758 100644
--- a/pkg/crc/api/types.go
+++ b/pkg/crc/api/types.go
@@ -16,7 +16,7 @@ type commandError struct {
 	Err string
 }
 
-type CrcAPIServer struct {
+type Server struct {
 	handlerFactory         newHandlerFunc
 	listener               net.Listener
 	clusterOpsRequestsChan chan clusterOpsRequest
diff --git a/pkg/crc/preflight/preflight_checks_tray_windows.go b/pkg/crc/preflight/preflight_checks_tray_windows.go
index 93ffce098..b1a506d37 100644
--- a/pkg/crc/preflight/preflight_checks_tray_windows.go
+++ b/pkg/crc/preflight/preflight_checks_tray_windows.go
@@ -100,8 +100,9 @@ func fixTrayInstalled() error {
 }
 
 func removeTray() error {
+	trayShortcutPath := filepath.Join(constants.StartupFolder, constants.TrayShortcutName)
 	_ = stopTray()
-	return goos.Remove(filepath.Join(constants.StartupFolder, constants.TrayShortcutName))
+	return os.RemoveFileIfExists(trayShortcutPath)
 }
 
 func stopTray() error {
@@ -116,10 +117,10 @@ func stopTray() error {
 func removeDaemon() error {
 	_ = stopDaemon()
 	var mErr errors.MultiError
-	if err := goos.Remove(constants.DaemonBatchFilePath); err != nil {
+	if err := os.RemoveFileIfExists(constants.DaemonBatchFilePath); err != nil {
 		mErr.Collect(err)
 	}
-	if err := goos.Remove(constants.DaemonPSScriptPath); err != nil {
+	if err := os.RemoveFileIfExists(constants.DaemonPSScriptPath); err != nil {
 		mErr.Collect(err)
 	}
 	if len(mErr.Errors) == 0 {
diff --git a/pkg/crc/preflight/preflight_windows.go b/pkg/crc/preflight/preflight_windows.go
index 57832981a..5ac5f376d 100644
--- a/pkg/crc/preflight/preflight_windows.go
+++ b/pkg/crc/preflight/preflight_windows.go
@@ -97,11 +97,6 @@ var traySetupChecks = [...]Check{
 		cleanup:            removeTray,
 		flags:              SetupOnly,
 	},
-	{
-		cleanupDescription: "Stopping crc tray if running",
-		cleanup:            stopTray,
-		flags:              CleanUpOnly,
-	},
 }
 
 var vsockChecks = [...]Check{
diff --git a/pkg/os/util.go b/pkg/os/util.go
index 14e154e07..0554a15dd 100644
--- a/pkg/os/util.go
+++ b/pkg/os/util.go
@@ -102,3 +102,10 @@ func FileExists(path string) bool {
 	}
 	return !info.IsDir()
 }
+
+func RemoveFileIfExists(path string) error {
+	if FileExists(path) {
+		return os.Remove(path)
+	}
+	return nil
+}


Test patch:
diff --git a/pkg/crc/api/api_test.go b/pkg/crc/api/api_test.go
index d96231958..4811efa4f 100644
--- a/pkg/crc/api/api_test.go
+++ b/pkg/crc/api/api_test.go
@@ -11,13 +11,13 @@ import (
 	"strings"
 	"testing"
 
-	"github.com/code-ready/crc/pkg/crc/machine"
-
 	cmdConfig "github.com/code-ready/crc/cmd/crc/cmd/config"
 	"github.com/code-ready/crc/pkg/crc/config"
+	"github.com/code-ready/crc/pkg/crc/machine"
 	"github.com/code-ready/crc/pkg/crc/machine/fakemachine"
 	"github.com/code-ready/crc/pkg/crc/preflight"
 	"github.com/code-ready/crc/pkg/crc/version"
+	log "github.com/sirupsen/logrus"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 )
@@ -32,11 +32,15 @@ func TestApi(t *testing.T) {
 	require.NoError(t, err)
 
 	client := fakemachine.NewClient()
-	api, err := createAPIServerWithListener(listener, setupNewInMemoryConfig, func(_ config.Storage) machine.Client {
+	api, err := createServerWithListener(listener, setupNewInMemoryConfig, func(_ config.Storage) machine.Client {
 		return client
 	})
 	require.NoError(t, err)
-	go api.Serve()
+	go func() {
+		if err := api.Serve(); err != nil {
+			log.Error(err)
+		}
+	}()
 
 	tt := []struct {
 		command       string
@@ -217,11 +221,15 @@ func setupAPIServer(t *testing.T) (string, func()) {
 	require.NoError(t, err)
 
 	client := fakemachine.NewClient()
-	api, err := createAPIServerWithListener(listener, setupNewInMemoryConfig, func(_ config.Storage) machine.Client {
+	api, err := createServerWithListener(listener, setupNewInMemoryConfig, func(_ config.Storage) machine.Client {
 		return client
 	})
 	require.NoError(t, err)
-	go api.Serve()
+	go func() {
+		if err := api.Serve(); err != nil {
+			log.Error(err)
+		}
+	}()
 
 	return socket, func() { os.RemoveAll(dir) }
 }
diff --git a/pkg/crc/preflight/preflight_windows_test.go b/pkg/crc/preflight/preflight_windows_test.go
index cd4091389..37ba75340 100644
--- a/pkg/crc/preflight/preflight_windows_test.go
+++ b/pkg/crc/preflight/preflight_windows_test.go
@@ -16,8 +16,8 @@ func TestCountConfigurationOptions(t *testing.T) {
 
 func TestCountPreflights(t *testing.T) {
 	assert.Len(t, getPreflightChecks(false, network.DefaultMode), 15)
-	assert.Len(t, getPreflightChecks(true, network.DefaultMode), 19)
+	assert.Len(t, getPreflightChecks(true, network.DefaultMode), 18)
 
 	assert.Len(t, getPreflightChecks(false, network.VSockMode), 16)
-	assert.Len(t, getPreflightChecks(true, network.VSockMode), 20)
+	assert.Len(t, getPreflightChecks(true, network.VSockMode), 19)
 }