fix(collectors): Fix logs collection in longhorn collector (#886)

* fix(collectors): Fix logs collection in longhorn collector

* Small typo

* Run go fmt on added changes

Author: banjoh

pkg/collect/ceph.go

@@ -172,7 +172,7 @@ func cephCommandExec(ctx context.Context, progressChan chan<- interface{}, c *Co
 	}
 
 	pathPrefix := GetCephCollectorFilepath(cephCollector.CollectorName, cephCollector.Namespace)
-	for srcFilename, _ := range results {
+	for srcFilename := range results {
 		var dstFileName string
 		switch {
 		case strings.HasSuffix(srcFilename, "-stdout.txt"):

pkg/collect/logs.go

@@ -20,7 +20,7 @@ import (
 type CollectLogs struct {
 	Collector    *troubleshootv1beta2.Logs
 	BundlePath   string
-	Namespace    string
+	Namespace    string // There is a Namespace parameter in troubleshootv1beta2.Logs. Should we remove this?
 	ClientConfig *rest.Config
 	Client       kubernetes.Interface
 	Context      context.Context

pkg/collect/longhorn.go

@@ -5,7 +5,6 @@ import (
 	"bytes"
 	"context"
 	"fmt"
-	"path"
 	"path/filepath"
 	"regexp"
 	"sync"
@@ -214,27 +213,10 @@ func (c *CollectLonghorn) Collect(progressChan chan<- interface{}) (CollectorRes
 	}
 	output.SaveResult(c.BundlePath, settingsKey, bytes.NewBuffer(settingsB))
 
-	// logs of all pods in namespace
-	logsCollectorSpec := &troubleshootv1beta2.Logs{
-		Selector:  []string{""},
-		Namespace: ns,
-	}
-
-	rbacErrors := c.GetRBACErrors()
-	logsCollector := &CollectLogs{logsCollectorSpec, c.BundlePath, c.Namespace, c.ClientConfig, c.Client, c.Context, nil, rbacErrors}
-
-	logs, err := logsCollector.Collect(progressChan)
+	err = c.collectLonghornLogs(ns, output, progressChan)
 	if err != nil {
 		return nil, errors.Wrap(err, "collect longhorn logs")
 	}
-	logsDir := GetLonghornLogsDirectory(ns)
-	for srcFilename, _ := range logs {
-		dstFileName := path.Join(logsDir, srcFilename)
-		err := copyResult(logs, output, c.BundlePath, srcFilename, dstFileName)
-		if err != nil {
-			logger.Printf("Failed to copy file %s; %v", srcFilename, err)
-		}
-	}
 
 	// https://longhorn.io/docs/1.1.1/advanced-resources/data-recovery/corrupted-replica/
 
@@ -307,6 +289,29 @@ func (c *CollectLonghorn) Collect(progressChan chan<- interface{}) (CollectorRes
 	return output, nil
 }
 
+func (c *CollectLonghorn) collectLonghornLogs(namespace string, results CollectorResult, progressChan chan<- interface{}) error {
+	// logs of all pods in namespace
+	logsCollectorSpec := &troubleshootv1beta2.Logs{
+		Selector:  []string{""},
+		Name:      GetLonghornLogsDirectory(namespace), // Logs (symlinks) will be stored in this directory
+		Namespace: namespace,
+	}
+
+	rbacErrors := c.GetRBACErrors()
+	logsCollector := &CollectLogs{logsCollectorSpec, c.BundlePath, namespace, c.ClientConfig, c.Client, c.Context, nil, rbacErrors}
+
+	logs, err := logsCollector.Collect(progressChan)
+	if err != nil {
+		return err
+	}
+
+	// Add logs collector results to the rest of
+	// the longhorn collector results for later consumption
+	results.AddResult(logs)
+
+	return nil
+}
+
 func GetLonghornNodesDirectory(namespace string) string {
 	return fmt.Sprintf("longhorn/%s/nodes", namespace)
 }

pkg/collect/result.go

@@ -22,7 +22,7 @@ func NewResult() CollectorResult {
 // is empty, no symlink is created. The relativeLinkPath is always saved in the result map.
 func (r CollectorResult) SymLinkResult(bundlePath, relativeLinkPath, relativeFilePath string) error {
 	// We should have saved the result this symlink is pointing to prior to creating it
-	klog.Info("Creating symlink ", relativeLinkPath, " -> ", relativeFilePath)
+	klog.V(2).Info("Creating symlink ", relativeLinkPath, " -> ", relativeFilePath)
 	data, ok := r[relativeFilePath]
 	if !ok {
 		return errors.Errorf("cannot create symlink, result in %q not found", relativeFilePath)
@@ -72,13 +72,23 @@ func (r CollectorResult) SymLinkResult(bundlePath, relativeLinkPath, relativeFil
 		return errors.Wrap(err, "failed to create symlink")
 	}
 
-	klog.V(2).Infof("Created '%s' symlink of '%s'", relativeLinkPath, relativeFilePath)
+	klog.V(2).Infof("Created %q symlink of %q", relativeLinkPath, relativeFilePath)
 	// store the file name referencing the symlink to have archived
 	r[relativeLinkPath] = nil
 
 	return nil
 }
 
+// AddResult combines another results object into this collector result.
+// This ensures when archiving a bundle from the result, all files are included.
+// It also ensures that when operating on the results in memory (e.g preflights),
+// all files are included.
+func (r CollectorResult) AddResult(other CollectorResult) {
+	for k, v := range other {
+		r[k] = v
+	}
+}
+
 // SaveResult saves the collector result to relativePath file on disk. If bundlePath is
 // empty, no file is created on disk. The relativePath is always saved in the result map.
 func (r CollectorResult) SaveResult(bundlePath string, relativePath string, reader io.Reader) error {

pkg/collect/result_test.go

@@ -0,0 +1,18 @@
+package collect
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+)
+
+func TestCollectorResult_AddResult(t *testing.T) {
+	r := CollectorResult{"a": []byte("a")}
+
+	other := CollectorResult{"b": []byte("b")}
+	r.AddResult(other)
+
+	assert.Equal(t, 2, len(r))
+	assert.Equal(t, []byte("a"), r["a"])
+	assert.Equal(t, []byte("b"), r["b"])
+}