fix(collectors): store unhealthy pod logs correctly (#909)

The symlinking logs feature led to a regression where symlinks of
unhealthy pods were overwritting logs in the support bundle. This
fix allows the cluster resources collector to instruct the logs
collector not to symlink logs, which in turn ensures logs are not
overwritten.

Fixes: #908

Author: banjoh

pkg/collect/cluster_resources.go

@@ -167,21 +167,16 @@ func (c *CollectClusterResources) Collect(progressChan chan<- interface{}) (Coll
 	for _, pod := range unhealthyPods {
 		allContainers := append(pod.Spec.InitContainers, pod.Spec.Containers...)
 		for _, container := range allContainers {
-			logsRoot := ""
-			if c.BundlePath != "" {
-				logsRoot = path.Join(c.BundlePath, "cluster-resources", "pods", "logs", pod.Namespace)
-			}
 			limits := &troubleshootv1beta2.LogLimits{
 				MaxLines: 500,
 			}
-			podLogs, err := savePodLogs(ctx, logsRoot, client, &pod, "", container.Name, limits, false)
+			podLogs, err := savePodLogs(ctx, c.BundlePath, client, &pod, "", container.Name, limits, false, false)
 			if err != nil {
 				errPath := filepath.Join("cluster-resources", "pods", "logs", pod.Namespace, pod.Name, fmt.Sprintf("%s-logs-errors.log", container.Name))
 				output.SaveResult(c.BundlePath, errPath, bytes.NewBuffer([]byte(err.Error())))
 			}
-			for k, v := range podLogs {
-				output[filepath.Join("cluster-resources", "pods", "logs", pod.Namespace, k)] = v
-			}
+			// Add logs collector results to the rest of the output
+			output.AddResult(podLogs)
 		}
 	}
 

pkg/collect/logs.go

@@ -71,7 +71,7 @@ func (c *CollectLogs) Collect(progressChan chan<- interface{}) (CollectorResult,
 				}
 
 				for _, containerName := range containerNames {
-					podLogs, err := savePodLogs(ctx, c.BundlePath, client, &pod, c.Collector.Name, containerName, c.Collector.Limits, false)
+					podLogs, err := savePodLogs(ctx, c.BundlePath, client, &pod, c.Collector.Name, containerName, c.Collector.Limits, false, true)
 					if err != nil {
 						key := fmt.Sprintf("%s/%s-errors.json", c.Collector.Name, pod.Name)
 						if containerName != "" {
@@ -89,7 +89,7 @@ func (c *CollectLogs) Collect(progressChan chan<- interface{}) (CollectorResult,
 				}
 			} else {
 				for _, container := range c.Collector.ContainerNames {
-					containerLogs, err := savePodLogs(ctx, c.BundlePath, client, &pod, c.Collector.Name, container, c.Collector.Limits, false)
+					containerLogs, err := savePodLogs(ctx, c.BundlePath, client, &pod, c.Collector.Name, container, c.Collector.Limits, false, true)
 					if err != nil {
 						key := fmt.Sprintf("%s/%s/%s-errors.json", c.Collector.Name, pod.Name, container)
 						err := output.SaveResult(c.BundlePath, key, marshalErrors([]string{err.Error()}))
@@ -132,8 +132,9 @@ func savePodLogs(
 	collectorName, container string,
 	limits *troubleshootv1beta2.LogLimits,
 	follow bool,
+	createSymLinks bool,
 ) (CollectorResult, error) {
-	return savePodLogsWithInterface(ctx, bundlePath, client, pod, collectorName, container, limits, follow)
+	return savePodLogsWithInterface(ctx, bundlePath, client, pod, collectorName, container, limits, follow, createSymLinks)
 }
 
 func savePodLogsWithInterface(
@@ -144,6 +145,7 @@ func savePodLogsWithInterface(
 	collectorName, container string,
 	limits *troubleshootv1beta2.LogLimits,
 	follow bool,
+	createSymLinks bool,
 ) (CollectorResult, error) {
 	podLogOpts := corev1.PodLogOptions{
 		Follow:    follow,
@@ -185,7 +187,9 @@ func savePodLogsWithInterface(
 		return nil, errors.Wrap(err, "failed to get log writer")
 	}
 	// NOTE: deferred calls are executed in LIFO order i.e called in reverse order
-	defer result.SymLinkResult(bundlePath, linkRelPathPrefix+".log", filePathPrefix+".log")
+	if createSymLinks {
+		defer result.SymLinkResult(bundlePath, linkRelPathPrefix+".log", filePathPrefix+".log")
+	}
 	defer result.CloseWriter(bundlePath, filePathPrefix+".log", logWriter)
 
 	_, err = io.Copy(logWriter, podLogs)
@@ -207,7 +211,9 @@ func savePodLogsWithInterface(
 		return nil, errors.Wrap(err, "failed to get previous log writer")
 	}
 	// NOTE: deferred calls are executed in LIFO order i.e called in reverse order
-	defer result.SymLinkResult(bundlePath, linkRelPathPrefix+"-previous.log", filePathPrefix+"-previous.log")
+	if createSymLinks {
+		defer result.SymLinkResult(bundlePath, linkRelPathPrefix+"-previous.log", filePathPrefix+"-previous.log")
+	}
 	defer result.CloseWriter(bundlePath, filePathPrefix+"-previous.log", logWriter)
 
 	_, err = io.Copy(prevLogWriter, podLogs)

pkg/collect/logs_test.go

@@ -85,12 +85,14 @@ func Test_savePodLogs(t *testing.T) {
 		name              string
 		withContainerName bool
 		collectorName     string
+		createSymLinks    bool
 		want              CollectorResult
 	}{
 		{
 			name:              "with container name",
 			withContainerName: true,
 			collectorName:     "all-logs",
+			createSymLinks:    true,
 			want: CollectorResult{
 				"all-logs/test-pod/nginx.log":                                          []byte("fake logs"),
 				"all-logs/test-pod/nginx-previous.log":                                 []byte("fake logs"),
@@ -102,6 +104,7 @@ func Test_savePodLogs(t *testing.T) {
 			name:              "without container name",
 			withContainerName: false,
 			collectorName:     "all-logs",
+			createSymLinks:    true,
 			want: CollectorResult{
 				"all-logs/test-pod.log":                                                []byte("fake logs"),
 				"all-logs/test-pod-previous.log":                                       []byte("fake logs"),
@@ -112,13 +115,24 @@ func Test_savePodLogs(t *testing.T) {
 		{
 			name:              "without container or collector names",
 			withContainerName: false,
+			createSymLinks:    true,
 			want: CollectorResult{
 				"/test-pod.log":          []byte("fake logs"),
 				"/test-pod-previous.log": []byte("fake logs"),
 				"cluster-resources/pods/logs/my-namespace/test-pod/nginx.log":          []byte("fake logs"),
 				"cluster-resources/pods/logs/my-namespace/test-pod/nginx-previous.log": []byte("fake logs"),
 			},
 		},
+		{
+			name:              "without sym links",
+			withContainerName: true,
+			collectorName:     "all-logs",
+			createSymLinks:    false,
+			want: CollectorResult{
+				"cluster-resources/pods/logs/my-namespace/test-pod/nginx.log":          []byte("fake logs"),
+				"cluster-resources/pods/logs/my-namespace/test-pod/nginx-previous.log": []byte("fake logs"),
+			},
+		},
 	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
@@ -144,7 +158,7 @@ func Test_savePodLogs(t *testing.T) {
 			if !tt.withContainerName {
 				containerName = ""
 			}
-			got, err := savePodLogsWithInterface(ctx, "", client, pod, tt.collectorName, containerName, limits, false)
+			got, err := savePodLogsWithInterface(ctx, "", client, pod, tt.collectorName, containerName, limits, false, tt.createSymLinks)
 			assert.NoError(t, err)
 			assert.Equal(t, tt.want, got)
 		})

pkg/collect/run_pod.go

@@ -179,7 +179,7 @@ func runWithoutTimeout(ctx context.Context, bundlePath string, clientConfig *res
 	limits := troubleshootv1beta2.LogLimits{
 		MaxLines: 10000,
 	}
-	podLogs, err := savePodLogs(ctx, bundlePath, client, pod, collectorName, "", &limits, true)
+	podLogs, err := savePodLogs(ctx, bundlePath, client, pod, collectorName, "", &limits, true, true)
 	if err != nil {
 		return nil, errors.Wrap(err, "failed to get pod logs")
 	}