From cd1511a8fb9eaf247d441307e676a4cecc7f0664 Mon Sep 17 00:00:00 2001 From: Evans Mungai Date: Wed, 14 Dec 2022 18:47:20 +0000 Subject: [PATCH] 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 --- pkg/collect/cluster_resources.go | 11 +++-------- pkg/collect/logs.go | 16 +++++++++++----- pkg/collect/logs_test.go | 16 +++++++++++++++- pkg/collect/run_pod.go | 2 +- 4 files changed, 30 insertions(+), 15 deletions(-) diff --git a/pkg/collect/cluster_resources.go b/pkg/collect/cluster_resources.go index ed3d157c..03f91d50 100644 --- a/pkg/collect/cluster_resources.go +++ b/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) } } diff --git a/pkg/collect/logs.go b/pkg/collect/logs.go index cb29207c..0a18d80e 100644 --- a/pkg/collect/logs.go +++ b/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) diff --git a/pkg/collect/logs_test.go b/pkg/collect/logs_test.go index e23572c0..a9132ef4 100644 --- a/pkg/collect/logs_test.go +++ b/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,6 +115,7 @@ 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"), @@ -119,6 +123,16 @@ func Test_savePodLogs(t *testing.T) { "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) }) diff --git a/pkg/collect/run_pod.go b/pkg/collect/run_pod.go index 765c905f..9d270911 100644 --- a/pkg/collect/run_pod.go +++ b/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") }