Fix: hots output (#21)

* fix(dumper): fix time formatting for artifact name Signed-off-by: ArtemTrofimushkin <[email protected]> * feat(cli, dumper): extract output folder to options & replace usages Signed-off-by: ArtemTrofimushkin <[email protected]> * fix(test): fix test usage Signed-off-by: ArtemTrofimushkin <[email protected]> * chore(utils): add helper ignore method to clean up warnings Signed-off-by: ArtemTrofimushkin <[email protected]> * chore(docs): update generated docs Signed-off-by: ArtemTrofimushkin <[email protected]>
dodopizza · Feb 26, 2022 · fbca829 · fbca829
1 parent 93d33c3
commit fbca829
Show file tree

Hide file tree

Showing 13 changed files with 76 additions and 49 deletions.
diff --git a/cli/cmd/command_builder.go b/cli/cmd/command_builder.go
@@ -19,6 +19,7 @@ type CommonOptions struct {
 	Image             string
 	Pod               string
 	Output            string
+	OutputHostPath    string
 	StoreOutputOnHost bool
 
 	kubeConfig *genericclioptions.ConfigFlags
@@ -32,7 +33,7 @@ type CommandBuilder struct {
 }
 
 // GetFlags return FlagSet that describes generic options
-func (options *CommonOptions) GetFlags(tool string) *pflag.FlagSet {
+func (options *CommonOptions) GetFlags() *pflag.FlagSet {
 	fs := pflag.NewFlagSet("common", pflag.ExitOnError)
 	fs.StringVarP(
 		&options.Container,
@@ -44,7 +45,7 @@ func (options *CommonOptions) GetFlags(tool string) *pflag.FlagSet {
 	fs.StringVar(
 		&options.Image,
 		"image",
-		globals.GetDumperImage(),
+		options.Image,
 		"Image of dumper to use for job",
 	)
 	fs.StringVar(
@@ -58,15 +59,21 @@ func (options *CommonOptions) GetFlags(tool string) *pflag.FlagSet {
 		&options.Output,
 		"output",
 		"o",
-		fmt.Sprintf("./output.%s", tool),
+		options.Output,
 		"Output file",
 	)
+	fs.StringVar(
+		&options.OutputHostPath,
+		"output-host-path",
+		options.OutputHostPath,
+		"Host folder, where will be stored artifact",
+	)
 	fs.BoolVarP(
 		&options.StoreOutputOnHost,
 		"store-output-on-host",
 		"t",
 		options.StoreOutputOnHost,
-		"Flag, indicating that output should be stored on host /tmp folder")
+		"Store output on node instead of downloading it locally")
 
 	options.kubeConfig = genericclioptions.NewConfigFlags(false)
 	options.kubeConfig.AddFlags(fs)
@@ -75,9 +82,15 @@ func (options *CommonOptions) GetFlags(tool string) *pflag.FlagSet {
 }
 
 func NewCommandBuilder(factory flags.DotnetToolFactory) *CommandBuilder {
+	tool := factory()
+
 	return &CommandBuilder{
-		CommonOptions: &CommonOptions{},
-		tool:          factory(),
+		CommonOptions: &CommonOptions{
+			Image:          globals.GetDumperImage(),
+			Output:         fmt.Sprintf("./output.%s", tool.ToolName()),
+			OutputHostPath: fmt.Sprintf("%s/%s", globals.PathTmpFolder, globals.PluginName),
+		},
+		tool: tool,
 	}
 }
 
@@ -104,7 +117,7 @@ func (cb *CommandBuilder) Tool() string {
 
 func (cb *CommandBuilder) parse() *pflag.FlagSet {
 	fs := pflag.NewFlagSet(cb.tool.ToolName(), pflag.ExitOnError)
-	fs.AddFlagSet(cb.CommonOptions.GetFlags(cb.tool.ToolName()))
+	fs.AddFlagSet(cb.CommonOptions.GetFlags())
 	fs.AddFlagSet(cb.tool.GetFlags())
 	return fs
 }
diff --git a/cli/cmd/launch.go b/cli/cmd/launch.go
@@ -11,6 +11,7 @@ import (
 	"github.com/dodopizza/kubectl-shovel/internal/flags"
 	"github.com/dodopizza/kubectl-shovel/internal/globals"
 	"github.com/dodopizza/kubectl-shovel/internal/kubernetes"
+	"github.com/dodopizza/kubectl-shovel/internal/utils"
 	"github.com/dodopizza/kubectl-shovel/internal/watchdog"
 )
 
@@ -61,8 +62,8 @@ func (cb *CommandBuilder) copyOutput(pod *kubernetes.PodInfo, output string) err
 	return nil
 }
 
-func (*CommandBuilder) storeOutputOnHost(pod *kubernetes.PodInfo, output string) error {
-	hostOutput := fmt.Sprintf("%s/%s/%s", globals.PathTmpFolder, globals.PluginName, output)
+func (cb *CommandBuilder) storeOutputOnHost(pod *kubernetes.PodInfo, output string) error {
+	hostOutput := fmt.Sprintf("%s/%s", cb.CommonOptions.OutputHostPath, output)
 	fmt.Printf("Output located on host: %s, at path: %s\n", pod.Node, hostOutput)
 	return nil
 }
@@ -96,7 +97,7 @@ func (cb *CommandBuilder) launch() error {
 	}
 
 	if cb.CommonOptions.StoreOutputOnHost {
-		jobSpec.WithHostTmpVolume()
+		jobSpec.WithHostTmpVolume(cb.CommonOptions.OutputHostPath)
 	}
 
 	fmt.Printf("Spawning diagnostics job with command:\n%s\n", strings.Join(jobSpec.Args, " "))
@@ -114,7 +115,7 @@ func (cb *CommandBuilder) launch() error {
 	if err != nil {
 		return errors.Wrap(err, "Failed to read logs from diagnostics job targetPod")
 	}
-	defer jobPodLogs.Close()
+	defer utils.Ignore(jobPodLogs.Close)
 
 	awaiter := events.NewEventAwaiter()
 	output, err := awaiter.AwaitCompletedEvent(jobPodLogs)

diff --git a/cli/cmd/root.go b/cli/cmd/root.go
@@ -18,7 +18,6 @@ func NewShovelCommand() *cobra.Command {
 
 	cmd.AddCommand(NewDocCommand())
 	cmd.AddCommand(NewVersionCommand())
-
 	cmd.AddCommand(NewGCDumpCommand())
 	cmd.AddCommand(NewTraceCommand())
 	cmd.AddCommand(NewDumpCommand())

diff --git a/cli/docs/kubectl-shovel_dump.md b/cli/docs/kubectl-shovel_dump.md
@@ -52,11 +52,12 @@ Also use `-n`/`--namespace` if your pod is not in current context's namespace:
       --kubeconfig string              Path to the kubeconfig file to use for CLI requests.
   -n, --namespace string               If present, the namespace scope for this CLI request
   -o, --output string                  Output file (default "./output.dump")
+      --output-host-path string        Host folder, where will be stored artifact (default "/tmp/kubectl-shovel")
       --pod-name string                Target pod
   -p, --process-id int                 The process ID to collect the trace from (default 1)
       --request-timeout string         The length of time to wait before giving up on a single server request. Non-zero values should contain a corresponding time unit (e.g. 1s, 2m, 3h). A value of zero means don't timeout requests. (default "0")
   -s, --server string                  The address and port of the Kubernetes API server
-  -t, --store-output-on-host           Flag, indicating that output should be stored on host /tmp folder
+  -t, --store-output-on-host           Store output on node instead of downloading it locally
       --tls-server-name string         Server name to use for server certificate validation. If it is not provided, the hostname used to contact the server is used
       --token string                   Bearer token for authentication to the API server
       --type type                      The kinds of information that are collected from process. Supported types:

diff --git a/cli/docs/kubectl-shovel_gcdump.md b/cli/docs/kubectl-shovel_gcdump.md
@@ -50,11 +50,12 @@ Also use `-n`/`--namespace` if your pod is not in current context's namespace:
       --kubeconfig string              Path to the kubeconfig file to use for CLI requests.
   -n, --namespace string               If present, the namespace scope for this CLI request
   -o, --output string                  Output file (default "./output.gcdump")
+      --output-host-path string        Host folder, where will be stored artifact (default "/tmp/kubectl-shovel")
       --pod-name string                Target pod
   -p, --process-id int                 The process ID to collect the trace from (default 1)
       --request-timeout string         The length of time to wait before giving up on a single server request. Non-zero values should contain a corresponding time unit (e.g. 1s, 2m, 3h). A value of zero means don't timeout requests. (default "0")
   -s, --server string                  The address and port of the Kubernetes API server
-  -t, --store-output-on-host           Flag, indicating that output should be stored on host /tmp folder
+  -t, --store-output-on-host           Store output on node instead of downloading it locally
       --timeout timeout                Give up on collecting the GC dump if it takes longer than this many seconds.
                                        Valid time units are "ns", "us" (or "µs"), "ms", "s", "m", "h".
                                        Will be rounded to seconds. If no unit provided defaults to seconds.

diff --git a/cli/docs/kubectl-shovel_trace.md b/cli/docs/kubectl-shovel_trace.md
@@ -72,6 +72,7 @@ Or convert any other format to speedscope format with:
       --kubeconfig string              Path to the kubeconfig file to use for CLI requests.
   -n, --namespace string               If present, the namespace scope for this CLI request
   -o, --output string                  Output file (default "./output.trace")
+      --output-host-path string        Host folder, where will be stored artifact (default "/tmp/kubectl-shovel")
       --pod-name string                Target pod
   -p, --process-id int                 The process ID to collect the trace from (default 1)
       --profile profile                A named pre-defined set of provider configurations that allowscommon tracing scenarios to be specified succinctly.
@@ -92,7 +93,7 @@ Or convert any other format to speedscope format with:
                                        https://docs.microsoft.com/en-us/dotnet/core/diagnostics/well-known-event-providers
       --request-timeout string         The length of time to wait before giving up on a single server request. Non-zero values should contain a corresponding time unit (e.g. 1s, 2m, 3h). A value of zero means don't timeout requests. (default "0")
   -s, --server string                  The address and port of the Kubernetes API server
-  -t, --store-output-on-host           Flag, indicating that output should be stored on host /tmp folder
+  -t, --store-output-on-host           Store output on node instead of downloading it locally
       --tls-server-name string         Server name to use for server certificate validation. If it is not provided, the hostname used to contact the server is used
       --token string                   Bearer token for authentication to the API server
       --user string                    The name of the kubeconfig user to use

diff --git a/dumper/cmd/launch.go b/dumper/cmd/launch.go
@@ -71,12 +71,12 @@ func (cb *CommandBuilder) launch() error {
 
 	if cb.CommonOptions.StoreOutputOnHost {
 		outputHost := fmt.Sprintf("%s/%s.%s.%s.%s.%s",
-			globals.PathHostTmpFolder,
+			globals.PathHostOutputFolder,
 			cb.CommonOptions.PodNamespace,
 			cb.CommonOptions.PodName,
 			cb.CommonOptions.ContainerName,
 			filepath.Base(output),
-			time.Now().UTC().Format("2006-04-02-15-04-05"),
+			time.Now().UTC().Format("2006-01-02-15-04-05"),
 		)
 
 		if err := utils.MoveFile(output, outputHost); err != nil {

diff --git a/internal/globals/constants.go b/internal/globals/constants.go
@@ -8,7 +8,7 @@ const (
 	PluginName            = "kubectl-shovel"
 	DumperImageName       = "dodopizza/kubectl-shovel-dumper"
 	PathTmpFolder         = "/tmp"
-	PathHostTmpFolder     = "/host-tmp"
+	PathHostOutputFolder  = "/host-output"
 	PathContainerDFS      = "/run/containerd"
 	PathContainerDVolumes = "/var/lib/kubelet/pods"
 	PathDockerFS          = "/var/lib/docker"

diff --git a/internal/kubernetes/jobs.go b/internal/kubernetes/jobs.go
@@ -2,7 +2,6 @@ package kubernetes
 
 import (
 	"context"
-	"fmt"
 	"strings"
 
 	"github.com/google/uuid"
@@ -65,11 +64,11 @@ func (j *JobRunSpec) WithContainerMountsVolume(container *ContainerInfo) *JobRun
 	return j
 }
 
-func (j *JobRunSpec) WithHostTmpVolume() *JobRunSpec {
+func (j *JobRunSpec) WithHostTmpVolume(path string) *JobRunSpec {
 	j.appendVolume(JobVolume{
-		Name:      "hosttmp",
-		HostPath:  fmt.Sprintf("%s/%s", globals.PathTmpFolder, globals.PluginName),
-		MountPath: globals.PathHostTmpFolder,
+		Name:      "hostoutput",
+		HostPath:  path,
+		MountPath: globals.PathHostOutputFolder,
 	})
 	return j
 }

diff --git a/internal/kubernetes/jobs_test.go b/internal/kubernetes/jobs_test.go
@@ -103,12 +103,12 @@ func Test_JobWithHostTmpVolume(t *testing.T) {
 			})
 
 			jobSpec := NewJobRunSpec([]string{"sleep"}, "alpine", pod).
-				WithHostTmpVolume()
+				WithHostTmpVolume("/tmp/testing")
 
 			volume := jobSpec.Volumes[0]
-			require.Equal(t, "hosttmp", volume.Name)
-			require.Equal(t, fmt.Sprintf("%s/%s", globals.PathTmpFolder, globals.PluginName), volume.HostPath)
-			require.Equal(t, globals.PathHostTmpFolder, volume.MountPath)
+			require.Equal(t, "hostoutput", volume.Name)
+			require.Equal(t, "/tmp/testing", volume.HostPath)
+			require.Equal(t, globals.PathHostOutputFolder, volume.MountPath)
 		})
 	}
 }
diff --git a/internal/utils/exec.go b/internal/utils/exec.go
@@ -2,8 +2,6 @@ package utils
 
 import (
 	"bytes"
-	"io"
-	"os"
 	"os/exec"
 
 	"github.com/pkg/errors"
@@ -26,23 +24,3 @@ func ExecCommand(executable string, args ...string) error {
 
 	return nil
 }
-
-// MoveFile physically moves file from source path to destination path
-// If dest already exists, MoveFile replaces it
-func MoveFile(source, dest string) error {
-	output, _ := os.Create(dest)
-	defer output.Close()
-
-	input, _ := os.Open(source)
-	_, err := io.Copy(output, input)
-	_ = input.Close()
-	if err != nil {
-		return errors.Wrapf(err, "failed to move from: %s to: %s", source, dest)
-	}
-
-	err = os.Remove(source)
-	if err != nil {
-		return err
-	}
-	return nil
-}
diff --git a/internal/utils/files.go b/internal/utils/files.go
@@ -0,0 +1,28 @@
+package utils
+
+import (
+	"io"
+	"os"
+
+	"github.com/pkg/errors"
+)
+
+// MoveFile physically moves file from source path to destination path
+// If dest already exists, MoveFile replaces it
+func MoveFile(source, dest string) error {
+	output, _ := os.Create(dest)
+	defer Ignore(output.Close)
+
+	input, _ := os.Open(source)
+	_, err := io.Copy(output, input)
+	_ = input.Close()
+	if err != nil {
+		return errors.Wrapf(err, "failed to move from: %s to: %s", source, dest)
+	}
+
+	err = os.Remove(source)
+	if err != nil {
+		return err
+	}
+	return nil
+}
diff --git a/internal/utils/ignore.go b/internal/utils/ignore.go
@@ -0,0 +1,6 @@
+package utils
+
+// Ignore invokes handler and explicitly ignores error
+func Ignore(handler func() error) {
+	_ = handler()
+}