Further simplify visitor command api

Signed-off-by: Pablo Chacin <[email protected]>

pkg/disruptors/commads.go

@@ -127,27 +127,22 @@ type PodHTTPFaultCommand struct {
 	options  HTTPDisruptionOptions
 }
 
-// Exec return the command for injecting a HttpFault in a Pod
-func (c PodHTTPFaultCommand) Exec(pod corev1.Pod) ([]string, error) {
+// Commands return the command for injecting a HttpFault in a Pod
+func (c PodHTTPFaultCommand) Commands(pod corev1.Pod) ([]string, []string, error) {
 	if !utils.HasPort(pod, c.fault.Port) {
-		return nil, fmt.Errorf("pod %q does not expose port %d", pod.Name, c.fault.Port)
+		return nil, nil, fmt.Errorf("pod %q does not expose port %d", pod.Name, c.fault.Port)
 	}
 
 	if utils.HasHostNetwork(pod) {
-		return nil, fmt.Errorf("pod %q cannot be safely injected as it has hostNetwork set to true", pod.Name)
+		return nil, nil, fmt.Errorf("pod %q cannot be safely injected as it has hostNetwork set to true", pod.Name)
 	}
 
 	targetAddress, err := utils.PodIP(pod)
 	if err != nil {
-		return nil, err
+		return nil, nil, err
 	}
 
-	return buildHTTPFaultCmd(targetAddress, c.fault, c.duration, c.options), nil
-}
-
-// Cleanup defines the command to execute for cleaning up if command execution fails
-func (c PodHTTPFaultCommand) Cleanup(_ corev1.Pod) []string {
-	return buildCleanupCmd()
+	return buildHTTPFaultCmd(targetAddress, c.fault, c.duration, c.options), buildCleanupCmd(), nil
 }
 
 // PodGrpcFaultCommand implements the PodVisitCommands interface for injecting GrpcFaults in a Pod
@@ -157,23 +152,18 @@ type PodGrpcFaultCommand struct {
 	options  GrpcDisruptionOptions
 }
 
-// Exec return the command for injecting a GrpcFault in a Pod
-func (c PodGrpcFaultCommand) Exec(pod corev1.Pod) ([]string, error) {
+// Commands return the command for injecting a GrpcFault in a Pod
+func (c PodGrpcFaultCommand) Commands(pod corev1.Pod) ([]string, []string, error) {
 	if !utils.HasPort(pod, c.fault.Port) {
-		return nil, fmt.Errorf("pod %q does not expose port %d", pod.Name, c.fault.Port)
+		return nil, nil, fmt.Errorf("pod %q does not expose port %d", pod.Name, c.fault.Port)
 	}
 
 	targetAddress, err := utils.PodIP(pod)
 	if err != nil {
-		return nil, err
+		return nil, nil, err
 	}
 
-	return buildGrpcFaultCmd(targetAddress, c.fault, c.duration, c.options), nil
-}
-
-// Cleanup defines the command to execute for cleaning up if command execution fails
-func (c PodGrpcFaultCommand) Cleanup(_ corev1.Pod) []string {
-	return buildCleanupCmd()
+	return buildGrpcFaultCmd(targetAddress, c.fault, c.duration, c.options), buildCleanupCmd(), nil
 }
 
 // ServiceHTTPFaultCommand implements the PodVisitCommands interface for injecting HttpFaults in a Pod
@@ -184,15 +174,15 @@ type ServiceHTTPFaultCommand struct {
 	options  HTTPDisruptionOptions
 }
 
-// Exec return the command for injecting a HttpFault in a Service
-func (c ServiceHTTPFaultCommand) Exec(pod corev1.Pod) ([]string, error) {
+// Commands return the command for injecting a HttpFault in a Service
+func (c ServiceHTTPFaultCommand) Commands(pod corev1.Pod) ([]string, []string, error) {
 	port, err := utils.MapPort(c.service, c.fault.Port, pod)
 	if err != nil {
-		return nil, err
+		return nil, nil, err
 	}
 
 	if utils.HasHostNetwork(pod) {
-		return nil, fmt.Errorf("pod %q cannot be safely injected as it has hostNetwork set to true", pod.Name)
+		return nil, nil, fmt.Errorf("pod %q cannot be safely injected as it has hostNetwork set to true", pod.Name)
 	}
 
 	// copy fault to change target port for the pod
@@ -201,10 +191,10 @@ func (c ServiceHTTPFaultCommand) Exec(pod corev1.Pod) ([]string, error) {
 
 	targetAddress, err := utils.PodIP(pod)
 	if err != nil {
-		return nil, err
+		return nil, nil, err
 	}
 
-	return buildHTTPFaultCmd(targetAddress, podFault, c.duration, c.options), nil
+	return buildHTTPFaultCmd(targetAddress, podFault, c.duration, c.options), buildCleanupCmd(), nil
 }
 
 // Cleanup defines the command to execute for cleaning up if command execution fails
@@ -221,25 +211,20 @@ type ServiceGrpcFaultCommand struct {
 	options  GrpcDisruptionOptions
 }
 
-// Exec return the VisitCommands for injecting a GrpcFault in a Service
-func (c ServiceGrpcFaultCommand) Exec(pod corev1.Pod) ([]string, error) {
+// Commands return the VisitCommands for injecting a GrpcFault in a Service
+func (c ServiceGrpcFaultCommand) Commands(pod corev1.Pod) ([]string, []string, error) {
 	port, err := utils.MapPort(c.service, c.fault.Port, pod)
 	if err != nil {
-		return nil, err
+		return nil, nil, err
 	}
 
 	podFault := c.fault
 	podFault.Port = port
 
 	targetAddress, err := utils.PodIP(pod)
 	if err != nil {
-		return nil, err
+		return nil, nil, err
 	}
 
-	return buildGrpcFaultCmd(targetAddress, podFault, c.duration, c.options), nil
-}
-
-// Cleanup defines the command to execute for cleaning up if command execution fails
-func (c ServiceGrpcFaultCommand) Cleanup(_ corev1.Pod) []string {
-	return buildCleanupCmd()
+	return buildGrpcFaultCmd(targetAddress, podFault, c.duration, c.options), buildCleanupCmd(), nil
 }

pkg/disruptors/commads_test.go

@@ -159,7 +159,7 @@ func Test_PodHTTPFaultCommandGenerator(t *testing.T) {
 				options:  tc.opts,
 			}
 
-			exec, err := cmd.Exec(tc.target)
+			exec, _, err := cmd.Commands(tc.target)
 			if tc.expectError && err == nil {
 				t.Errorf("should had failed")
 				return
@@ -266,7 +266,7 @@ func Test_PodGrpcPFaultCommandGenerator(t *testing.T) {
 				options:  tc.opts,
 			}
 
-			exec, err := cmd.Exec(tc.target)
+			exec, _, err := cmd.Commands(tc.target)
 
 			if tc.expectError && err == nil {
 				t.Errorf("should had failed")

pkg/disruptors/controller.go

@@ -16,10 +16,8 @@ import (
 
 // PodVisitCommand define the commands used for visiting a Pod
 type PodVisitCommand interface {
-	// Exec defines the command to be executed
-	Exec(corev1.Pod) ([]string, error)
-	// Cleanup defines the command to execute for cleaning up if command execution fails
-	Cleanup(corev1.Pod) []string
+	// Commands defines the command to be executed, and optionally a cleanup command
+	Commands(corev1.Pod) ([]string, []string, error)
 }
 
 // PodController defines the interface for controlling a set of target Pods
@@ -108,20 +106,18 @@ func (c *PodAgentVisitor) Visit(ctx context.Context, pod corev1.Pod, commands Po
 	}
 
 	// get the command to execute in the target
-	execCommand, err := commands.Exec(pod)
+	exec, cleanup, err := commands.Commands(pod)
 	if err != nil {
 		return fmt.Errorf("unable to get command for pod %q: %w", pod.Name, err)
 	}
 
-	_, stderr, err := c.helper.Exec(ctx, pod.Name, "xk6-agent", execCommand, []byte{})
+	_, stderr, err := c.helper.Exec(ctx, pod.Name, "xk6-agent", exec, []byte{})
 
-	// if command failed, ensure the agent execution is terminated
-	cleanupCommand := commands.Cleanup(pod)
-	if err != nil && cleanupCommand != nil {
+	if err != nil && cleanup != nil {
 		// we ignore errors because we are reporting the reason of the exec failure
 		// we use a fresh context because the context used in exec may have been cancelled or expired
 		//nolint:contextcheck
-		_, _, _ = c.helper.Exec(context.TODO(), pod.Name, "xk6-agent", cleanupCommand, []byte{})
+		_, _, _ = c.helper.Exec(context.TODO(), pod.Name, "xk6-agent", cleanup, []byte{})
 	}
 
 	// if the context is cancelled, don't report error (we assume the caller is reporting this error)

pkg/disruptors/controller_test.go

@@ -20,12 +20,8 @@ type fakeCommand struct {
 	cleanup []string
 }
 
-func (f fakeCommand) Exec(_ corev1.Pod) ([]string, error) {
-	return f.exec, f.err
-}
-
-func (f fakeCommand) Cleanup(_ corev1.Pod) []string {
-	return f.cleanup
+func (f fakeCommand) Commands(_ corev1.Pod) ([]string, []string, error) {
+	return f.exec, f.cleanup, f.err
 }
 
 func visitCommands() PodVisitCommand {