dapr stop -f fix and E2E tests for dapr stop and list (#1181)

* E2E tests for dapr stop and list

Signed-off-by: Pravin Pushkar <[email protected]>

* cmdstop to cmdStopWithAppID

Signed-off-by: Pravin Pushkar <[email protected]>

* review comments

Signed-off-by: Pravin Pushkar <[email protected]>

* stop all started processes for dapr apps with kill process group

Signed-off-by: Pravin Pushkar <[email protected]>

* stop all apps in run template test

Signed-off-by: Pravin Pushkar <[email protected]>

* fix tests

Signed-off-by: Pravin Pushkar <[email protected]>

* separating os syscall and renaming file

Signed-off-by: Pravin Pushkar <[email protected]>

* moving syscall.go to pkg/syscall

Signed-off-by: Pravin Pushkar <[email protected]>

* name change

Signed-off-by: Pravin Pushkar <[email protected]>

* fixed windows error

Signed-off-by: Pravin Pushkar <[email protected]>

* use syscall.kill

Signed-off-by: Pravin Pushkar <[email protected]>

* review comments

Signed-off-by: Pravin Pushkar <[email protected]>

---------

Signed-off-by: Pravin Pushkar <[email protected]>
Co-authored-by: Mukundan Sundararajan <[email protected]>

cmd/dashboard.go

@@ -180,7 +180,7 @@ dapr dashboard -k -p 0
 			}()
 
 			// url for dashboard after port forwarding.
-			var webURL string = fmt.Sprintf("http://%s", net.JoinHostPort(dashboardHost, fmt.Sprint(portForward.LocalPort)))
+			webURL := fmt.Sprintf("http://%s", net.JoinHostPort(dashboardHost, fmt.Sprint(portForward.LocalPort)))
 
 			print.InfoStatusEvent(os.Stdout, fmt.Sprintf("Dapr dashboard found in namespace:\t%s", foundNamespace))
 			print.InfoStatusEvent(os.Stdout, fmt.Sprintf("Dapr dashboard available at:\t%s\n", webURL))

cmd/run.go

@@ -32,6 +32,7 @@ import (
 	runExec "github.com/dapr/cli/pkg/runexec"
 	"github.com/dapr/cli/pkg/standalone"
 	"github.com/dapr/cli/pkg/standalone/runfileconfig"
+	daprsyscall "github.com/dapr/cli/pkg/syscall"
 	"github.com/dapr/cli/utils"
 )
 
@@ -191,7 +192,7 @@ dapr run --run-file /path/to/directory
 		// TODO: In future release replace following logic with the refactored functions seen below.
 
 		sigCh := make(chan os.Signal, 1)
-		setupShutdownNotify(sigCh)
+		daprsyscall.SetupShutdownNotify(sigCh)
 
 		daprRunning := make(chan bool, 1)
 		appRunning := make(chan bool, 1)
@@ -454,9 +455,15 @@ func executeRun(runFilePath string, apps []runfileconfig.App) (bool, error) {
 
 	// setup shutdown notify channel.
 	sigCh := make(chan os.Signal, 1)
-	setupShutdownNotify(sigCh)
+	daprsyscall.SetupShutdownNotify(sigCh)
 
 	runStates := make([]*runExec.RunExec, 0, len(apps))
+
+	// Creates a separate process group ID for current process i.e. "dapr run -f".
+	// All the subprocess and their grandchildren inherit this PGID.
+	// This is done to provide a better grouping, which can be used to control all the proceses started by "dapr run -f".
+	daprsyscall.CreateProcessGroupID()
+
 	for _, app := range apps {
 		print.StatusEvent(os.Stdout, print.LogInfo, "Validating config and starting app %q", app.RunConfig.AppID)
 		// Set defaults if zero value provided in config yaml.

pkg/standalone/runfileconfig/run_file_config_parser_test.go

@@ -208,7 +208,7 @@ func TestRunConfigFile(t *testing.T) {
 				expectedErr: true,
 			},
 			{
-				name:        "invalid run config - invalid path",
+				name:        "invalid run config - invalid app dir path",
 				input:       invalidRunFilePath1,
 				expectedErr: true,
 			},

pkg/standalone/stop.go

@@ -18,6 +18,7 @@ package standalone
 
 import (
 	"fmt"
+	"syscall"
 
 	"github.com/dapr/cli/utils"
 )
@@ -52,7 +53,15 @@ func StopAppsWithRunFile(runTemplatePath string) error {
 	}
 	for _, a := range apps {
 		if a.RunTemplatePath == runTemplatePath {
-			_, err := utils.RunCmdAndWait("kill", fmt.Sprintf("%v", a.CliPID))
+			// Get the process group id of the CLI process.
+			pgid, err := syscall.Getpgid(a.CliPID)
+			if err != nil {
+				// Fall back to cliPID if pgid is not available.
+				_, err = utils.RunCmdAndWait("kill", fmt.Sprintf("%v", a.CliPID))
+				return err
+			}
+			// Kill the whole process group.
+			err = syscall.Kill(-pgid, syscall.SIGINT)
 			return err
 		}
 	}

pkg/standalone/testdata/runfileconfig/test_run_config_empty_app_dir.yaml

@@ -1,25 +1,5 @@
 version: 1
 common:
-  resourcesPath: ./app/resources
-  appProtocol: HTTP
-  appHealthProbeTimeout: 10
-  env:
-    - name: DEBUG
-      value: false
-    - name: tty
-      value: sts
 apps:
   - appDirPath: ./webapp/
-    resourcesPath: ./resources
-    configFilePath: ./config.yaml
-    appPort: 8080
-    appHealthProbeTimeout: 1
-    command: ["python3", "app.py"]
-  - appID: backend
-    appProtocol: GRPC
-    appPort: 3000
-    unixDomainSocket: /tmp/test-socket
-    env:
-    - name: DEBUG
-      value: true
-    command: ["./backend"]
+  - appID: backend

pkg/standalone/testdata/runfileconfig/test_run_config_invalid_path.yaml

@@ -1,26 +1,6 @@
 version: 1
 common:
-  resourcesPath: ./app/resources
-  appProtocol: HTTP
-  appHealthProbeTimeout: 10
-  env:
-    - name: DEBUG
-      value: false
-    - name: tty
-      value: sts
 apps:
   - appDirPath: ./webapp/
-    resourcesPath: ./resources
-    configFilePath: ./config.yaml
-    appPort: 8080
-    appHealthProbeTimeout: 1
-    command: ["python3", "app.py"]
   - appID: backend
-    appDirPath: ./invalid_backend/
-    appProtocol: GRPC
-    appPort: 3000
-    unixDomainSocket: /tmp/test-socket
-    env:
-    - name: DEBUG
-      value: true
-    command: ["./backend"]
+    appDirPath: ./invalid_backend/

pkg/syscall/syscall.go

@@ -0,0 +1,42 @@
+//go:build !windows
+// +build !windows
+
+/*
+Copyright 2021 The Dapr Authors
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+    http://www.apache.org/licenses/LICENSE-2.0
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package syscall
+
+import (
+	"os"
+	"os/signal"
+	"syscall"
+
+	"github.com/dapr/cli/pkg/print"
+)
+
+func SetupShutdownNotify(sigCh chan os.Signal) {
+	signal.Notify(sigCh, syscall.SIGTERM, syscall.SIGINT)
+}
+
+// CreateProcessGroupID creates a process group ID for the current process.
+// Reference - https://man7.org/linux/man-pages/man2/setpgid.2.html.
+func CreateProcessGroupID() {
+	// Below is some excerpt from the above link Setpgid() -
+	// setpgid() sets the PGID of the process specified by pid to pgid.
+	// If pid is zero, then the process ID of the calling process is
+	// used.  If pgid is zero, then the PGID of the process specified by
+	// pid is made the same as its process ID.
+	if err := syscall.Setpgid(0, 0); err != nil {
+		print.WarningStatusEvent(os.Stdout, "Failed to create process group id: %s", err.Error())
+	}
+}

cmd/shutdown_windows.go → pkg/syscall/syscall_windows.go

@@ -11,7 +11,7 @@ See the License for the specific language governing permissions and
 limitations under the License.
 */
 
-package cmd
+package syscall
 
 import (
 	"fmt"
@@ -24,7 +24,7 @@ import (
 	"github.com/dapr/cli/pkg/print"
 )
 
-func setupShutdownNotify(sigCh chan os.Signal) {
+func SetupShutdownNotify(sigCh chan os.Signal) {
 	signal.Notify(sigCh, syscall.SIGTERM, syscall.SIGINT)
 
 	// Unlike Linux/Mac, you can't just send a SIGTERM from another process.
@@ -40,3 +40,9 @@ func setupShutdownNotify(sigCh chan os.Signal) {
 		sigCh <- os.Interrupt
 	}()
 }
+
+// CreateProcessGroupID creates a process group ID for the current process.
+func CreateProcessGroupID() {
+	// No-op on Windows
+	print.WarningStatusEvent(os.Stdout, "Creating process group id is not implemented on Windows")
+}

tests/e2e/standalone/commands.go

@@ -131,9 +131,20 @@ func cmdRunWithContext(ctx context.Context, unixDomainSocket string, args ...str
 	return spawn.CommandExecWithContext(ctx, common.GetDaprPath(), runArgs...)
 }
 
-// cmdStop stops the specified app and returns the command output and error.
-func cmdStop(appId string, args ...string) (string, error) {
+// cmdStopWithAppID stops the specified app with app id and returns the command output and error.
+func cmdStopWithAppID(appId string, args ...string) (string, error) {
 	stopArgs := append([]string{"stop", "--log-as-json", "--app-id", appId}, args...)
+	return daprStop(stopArgs...)
+}
+
+// cmdStopWithRunTemplate stops the apps started with run template file and returns the command output and error.
+func cmdStopWithRunTemplate(runTemplateFile string, args ...string) (string, error) {
+	stopArgs := append([]string{"stop", "--log-as-json", "-f", runTemplateFile}, args...)
+	return daprStop(stopArgs...)
+}
+
+// daprStop stops Dapr with the stop command and returns the command output and error.
+func daprStop(stopArgs ...string) (string, error) {
 	return spawn.Command(common.GetDaprPath(), stopArgs...)
 }
 

tests/e2e/standalone/init_negative_test.go

@@ -50,7 +50,7 @@ func TestStandaloneInitNegatives(t *testing.T) {
 	})
 
 	t.Run("stop without install", func(t *testing.T) {
-		output, err := cmdStop("test")
+		output, err := cmdStopWithAppID("test")
 		require.NoError(t, err, "expected no error on stop without install")
 		require.Contains(t, output, "failed to stop app id test: couldn't find app id test", "expected output to match")
 	})

tests/e2e/standalone/invoke_test.go

@@ -102,7 +102,7 @@ func TestStandaloneInvoke(t *testing.T) {
 				assert.Contains(t, output, "error invoking app invoke_e2e: 404 Not Found")
 			})
 
-			output, err := cmdStop("invoke_e2e")
+			output, err := cmdStopWithAppID("invoke_e2e")
 			t.Log(output)
 			require.NoError(t, err, "dapr stop failed")
 			assert.Contains(t, output, "app stopped successfully: invoke_e2e")

tests/e2e/standalone/list_test.go

@@ -60,7 +60,7 @@ func TestStandaloneList(t *testing.T) {
 		require.Error(t, err, "dapr list should fail with an invalid output format")
 
 		// We can call stop so as not to wait for the app to time out
-		output, err = cmdStop("dapr_e2e_list")
+		output, err = cmdStopWithAppID("dapr_e2e_list")
 		t.Log(output)
 		require.NoError(t, err, "dapr stop failed")
 		assert.Contains(t, output, "app stopped successfully: dapr_e2e_list")
@@ -89,7 +89,7 @@ func TestStandaloneList(t *testing.T) {
 		// TODO: remove this condition when `dapr stop` starts working for Windows.
 		// See https://github.com/dapr/cli/issues/1034.
 		if runtime.GOOS != "windows" {
-			output, err = cmdStop("daprd_e2e_list")
+			output, err = cmdStopWithAppID("daprd_e2e_list")
 			t.Log(output)
 			require.NoError(t, err, "dapr stop failed")
 			assert.Contains(t, output, "app stopped successfully: daprd_e2e_list")

tests/e2e/standalone/publish_test.go

@@ -148,7 +148,7 @@ func TestStandalonePublish(t *testing.T) {
 				assert.Equal(t, []byte("{\"cli\": \"is_working\"}"), event.Data)
 			})
 
-			output, err := cmdStop("pub_e2e")
+			output, err := cmdStopWithAppID("pub_e2e")
 			t.Log(output)
 			require.NoError(t, err, "dapr stop failed")
 			assert.Contains(t, output, "app stopped successfully: pub_e2e")

tests/e2e/standalone/run_template_test.go

@@ -53,13 +53,15 @@ func TestRunWithTemplateFile(t *testing.T) {
 	// These tests are dependent on run template files in ../testdata/run-template-files folder.
 
 	t.Run("invalid template file wrong emit metrics app run", func(t *testing.T) {
+		runFilePath := "../testdata/run-template-files/wrong_emit_metrics_app_dapr.yaml"
 		t.Cleanup(func() {
 			// assumption in the test is that there is only one set of app and daprd logs in the logs directory.
 			os.RemoveAll("../../apps/emit-metrics/.dapr/logs")
 			os.RemoveAll("../../apps/processor/.dapr/logs")
+			stopAllApps(t, runFilePath)
 		})
 		args := []string{
-			"-f", "../testdata/run-template-files/wrong_emit_metrics_app_dapr.yaml",
+			"-f", runFilePath,
 		}
 		ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second)
 		defer cancel()
@@ -101,13 +103,15 @@ func TestRunWithTemplateFile(t *testing.T) {
 	})
 
 	t.Run("valid template file", func(t *testing.T) {
+		runFilePath := "../testdata/run-template-files/dapr.yaml"
 		t.Cleanup(func() {
 			// assumption in the test is that there is only one set of app and daprd logs in the logs directory.
 			os.RemoveAll("../../apps/emit-metrics/.dapr/logs")
 			os.RemoveAll("../../apps/processor/.dapr/logs")
+			stopAllApps(t, runFilePath)
 		})
 		args := []string{
-			"-f", "../testdata/run-template-files/dapr.yaml",
+			"-f", runFilePath,
 		}
 		ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second)
 		defer cancel()
@@ -156,13 +160,15 @@ func TestRunWithTemplateFile(t *testing.T) {
 	})
 
 	t.Run("invalid template file env var not set", func(t *testing.T) {
+		runFilePath := "../testdata/run-template-files/env_var_not_set_dapr.yaml"
 		t.Cleanup(func() {
 			// assumption in the test is that there is only one set of app and daprd logs in the logs directory.
 			os.RemoveAll("../../apps/emit-metrics/.dapr/logs")
 			os.RemoveAll("../../apps/processor/.dapr/logs")
+			stopAllApps(t, runFilePath)
 		})
 		args := []string{
-			"-f", "../testdata/run-template-files/env_var_not_set_dapr.yaml",
+			"-f", runFilePath,
 		}
 		ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second)
 		defer cancel()
@@ -205,13 +211,15 @@ func TestRunWithTemplateFile(t *testing.T) {
 	})
 
 	t.Run("valid template file no app command", func(t *testing.T) {
+		runFilePath := "../testdata/run-template-files/no_app_command.yaml"
 		t.Cleanup(func() {
 			// assumption in the test is that there is only one set of app and daprd logs in the logs directory.
 			os.RemoveAll("../../apps/emit-metrics/.dapr/logs")
 			os.RemoveAll("../../apps/processor/.dapr/logs")
+			stopAllApps(t, runFilePath)
 		})
 		args := []string{
-			"-f", "../testdata/run-template-files/no_app_command.yaml",
+			"-f", runFilePath,
 		}
 		ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second)
 		defer cancel()
@@ -255,13 +263,15 @@ func TestRunWithTemplateFile(t *testing.T) {
 	})
 
 	t.Run("valid template file empty app command", func(t *testing.T) {
+		runFilePath := "../testdata/run-template-files/empty_app_command.yaml"
 		t.Cleanup(func() {
 			// assumption in the test is that there is only one set of app and daprd logs in the logs directory.
 			os.RemoveAll("../../apps/emit-metrics/.dapr/logs")
 			os.RemoveAll("../../apps/processor/.dapr/logs")
+			stopAllApps(t, runFilePath)
 		})
 		args := []string{
-			"-f", "../testdata/run-template-files/empty_app_command.yaml",
+			"-f", runFilePath,
 		}
 		ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second)
 		defer cancel()
@@ -363,3 +373,8 @@ func lookUpFileFullName(dirPath, partialFilename string) (string, error) {
 	}
 	return "", fmt.Errorf("failed to find file with partial name %s in directory %s", partialFilename, dirPath)
 }
+
+func stopAllApps(t *testing.T, runfile string) {
+	_, err := cmdStopWithRunTemplate(runfile)
+	require.NoError(t, err, "failed to stop apps")
+}