diff --git a/cmd/troubleshootv2/cli/collect.go b/cmd/troubleshootv2/cli/collect.go index d975e5fe3..0047dd6e1 100644 --- a/cmd/troubleshootv2/cli/collect.go +++ b/cmd/troubleshootv2/cli/collect.go @@ -9,12 +9,15 @@ import ( "sync" "time" - "github.com/replicatedhq/troubleshoot/internal/bundleimpl" + "github.com/replicatedhq/troubleshoot/internal/tsbundle" "github.com/replicatedhq/troubleshoot/internal/util" "github.com/replicatedhq/troubleshoot/pkg/bundle" + "github.com/replicatedhq/troubleshoot/pkg/constants" "github.com/replicatedhq/troubleshoot/pkg/loader" "github.com/replicatedhq/troubleshoot/pkg/logger" + "github.com/replicatedhq/troubleshoot/pkg/supportbundle" "github.com/spf13/cobra" + "go.opentelemetry.io/otel" "k8s.io/klog/v2" ) @@ -24,11 +27,9 @@ func CollectCmd() *cobra.Command { Short: "Collect bundle from a cluster or host", Long: "Collect bundle from a cluster or host", RunE: func(cmd *cobra.Command, args []string) error { - ctx := cmd.Context() - - err := doRun(ctx, args) + err := doRun(cmd.Context(), args) if err != nil { - klog.Errorf("Failed to run: %v", err) + klog.Errorf("Failure collecting support bundle: %v", err) return err } @@ -58,9 +59,14 @@ func doRun(ctx context.Context, args []string) error { go func() { defer wg.Done() for msg := range progressChan { + // TODO: Expect error or string types klog.Infof("Collecting bundle: %v", msg) } }() + ctxWrap, root := otel.Tracer(constants.LIB_TRACER_NAME).Start( + ctx, constants.TROUBLESHOOT_ROOT_SPAN_NAME, + ) + defer root.End() // 1. Load troubleshoot specs from args // TODO: "RawSpecsFromArgs" missing the logic to load specs from the cluster @@ -68,7 +74,7 @@ func doRun(ctx context.Context, args []string) error { if err != nil { return err } - kinds, err := loader.LoadSpecs(ctx, loader.LoadOptions{ + kinds, err := loader.LoadSpecs(ctxWrap, loader.LoadOptions{ RawSpecs: rawSpecs, }) if err != nil { @@ -82,11 +88,11 @@ func doRun(ctx context.Context, args []string) error { } defer os.RemoveAll(bundleDir) - bdl := bundleimpl.NewTroubleshootBundle(bundleimpl.TroubleshootBundleOptions{ + bdl := tsbundle.NewTroubleshootBundle(tsbundle.TroubleshootBundleOptions{ ProgressChan: progressChan, }) - klog.Infof("Starting collecting bundle") - err = bdl.Collect(ctx, bundle.CollectOptions{ + klog.Infof("Collect support bundle") + err = bdl.Collect(ctxWrap, bundle.CollectOptions{ Specs: kinds, BundleDir: bundleDir, }) @@ -96,8 +102,8 @@ func doRun(ctx context.Context, args []string) error { // 3. Analyze the support bundle // TODO: Add results to the support bundle - klog.Infof("Starting to analyse bundle") - out, err := bdl.Analyze(ctx, bundle.AnalyzeOptions{ + klog.Infof("Analyse support bundle") + out, err := bdl.Analyze(ctxWrap, bundle.AnalyzeOptions{ Specs: kinds, }) if err != nil { @@ -110,13 +116,14 @@ func doRun(ctx context.Context, args []string) error { if err != nil { return err } - err = bdl.BundleData().Data().SaveResult(bundleDir, "analysis.json", bytes.NewBuffer(analysis)) + err = bdl.BundleData().Data().SaveResult(bundleDir, supportbundle.AnalysisFilename, bytes.NewBuffer(analysis)) if err != nil { return err } // 4. Redact the support bundle - err = bdl.Redact(ctx, bundle.RedactOptions{ + klog.Infof("Redact support bundle") + err = bdl.Redact(ctxWrap, bundle.RedactOptions{ Specs: kinds, }) if err != nil { @@ -124,15 +131,27 @@ func doRun(ctx context.Context, args []string) error { } // 5. Archive the support bundle + klog.Infof("Archive support bundle") supportBundlePath := path.Join(util.HomeDir(), fmt.Sprintf("support-bundle-%s.tgz", time.Now().Format("2006-01-02T15_04_05"))) - err = bdl.Archive(ctx, bundle.ArchiveOptions{ + err = bdl.Archive(ctxWrap, bundle.ArchiveOptions{ ArchivePath: supportBundlePath, }) if err != nil { return err } - // 6. Print outro i.e. "Support bundle saved to " + // 6. Save the bundle to a file + klog.Infof("Save version info to support bundle") + reader, err := supportbundle.GetVersionFile() + if err != nil { + return err + } + err = bdl.BundleData().Data().SaveResult(bundleDir, constants.VERSION_FILENAME, reader) + if err != nil { + return err + } + + // 7. Print outro i.e. "Support bundle saved to " // Print to screen output of bdl.Analyze i.e "analysisResults" fmt.Printf("Support bundle saved to %s\n", supportBundlePath) diff --git a/internal/bundleimpl/bundle.go b/internal/tsbundle/bundle.go similarity index 61% rename from internal/bundleimpl/bundle.go rename to internal/tsbundle/bundle.go index 28406c327..75f4ba1a0 100644 --- a/internal/bundleimpl/bundle.go +++ b/internal/tsbundle/bundle.go @@ -1,5 +1,4 @@ -// Is this naming convention correct? i.e impl is for implementation -package bundleimpl +package tsbundle import ( "context" @@ -8,6 +7,7 @@ import ( "github.com/pkg/errors" "github.com/replicatedhq/troubleshoot/pkg/bundle" "github.com/replicatedhq/troubleshoot/pkg/collect" + "github.com/replicatedhq/troubleshoot/pkg/supportbundle" ) // Bundle is the Troubleshoot implementation of the bundle.Bundler interface @@ -17,7 +17,6 @@ type Bundle struct { } type TroubleshootBundleOptions struct { - // TODO: Consider just a callback function. Channels can block if not read from ProgressChan chan any // a channel to write progress information to Namespace string // namespace to limit scope the collectors need to run in } @@ -29,24 +28,50 @@ func NewTroubleshootBundle(opt TroubleshootBundleOptions) bundle.Bundler { } } +func (b *Bundle) reportProgress(msg any) { + if b.progressChan != nil { + // Non-blocking write to channel. + // In case there is no listener drop the message. + select { + case b.progressChan <- msg: + default: + } + } +} + func (b *Bundle) Collect(ctx context.Context, opt bundle.CollectOptions) error { - // TODO: Error if b.data is not nil. We do not want to overwrite existing data if b.data != nil { - return fmt.Errorf("bundle already has data") + return fmt.Errorf("we cannot run collectors if a bundle already exists") } - b.data = collect.NewBundleData(opt.BundleDir) - results, err := b.doCollect(ctx, opt) if err != nil { return err } - b.data.Data().AddResult(results) + + b.data = collect.NewBundleData(collect.BundleDataOptions{ + Data: results, + BundleDir: opt.BundleDir, + }) return nil } func (b *Bundle) Analyze(ctx context.Context, opt bundle.AnalyzeOptions) (bundle.AnalyzeOutput, error) { - return bundle.AnalyzeOutput{}, nil + if b.data == nil { + return bundle.AnalyzeOutput{}, errors.New("no bundle data to analyze") + } + + sbSpec := supportbundle.ConcatSpecs(opt.Specs.SupportBundlesV1Beta2...) + + // Run Analyzers + analyzeResults, err := supportbundle.AnalyzeSupportBundle(ctx, &sbSpec.Spec, b.data.BundleDir()) + if err != nil { + return bundle.AnalyzeOutput{}, err + } + + return bundle.AnalyzeOutput{ + Results: analyzeResults, + }, nil } func (b *Bundle) BundleData() *collect.BundleData { @@ -66,7 +91,10 @@ func (b *Bundle) Archive(ctx context.Context, opt bundle.ArchiveOptions) error { } func (b *Bundle) Load(ctx context.Context, opt bundle.LoadBundleOptions) error { - // TODO: Error if b.data is not nil. We do not want to overwrite existing data + if b.data != nil { + return fmt.Errorf("we cannot run collectors if a bundle already exists") + } + // TODO: Load bundle from disk or url return nil } diff --git a/internal/bundleimpl/collect.go b/internal/tsbundle/collect.go similarity index 83% rename from internal/bundleimpl/collect.go rename to internal/tsbundle/collect.go index 600a71886..a9fb38357 100644 --- a/internal/bundleimpl/collect.go +++ b/internal/tsbundle/collect.go @@ -1,4 +1,4 @@ -package bundleimpl +package tsbundle import ( "context" @@ -24,27 +24,17 @@ import ( func (b *Bundle) doCollect( ctx context.Context, opt bundle.CollectOptions, ) (collect.CollectorResult, error) { - ctxWrap, root := otel.Tracer(constants.LIB_TRACER_NAME).Start( - ctx, constants.TROUBLESHOOT_ROOT_SPAN_NAME, - ) - defer func() { - // If this function returns an error, root.End() may not be called. - // We want to ensure this happens, so we defer it. It is safe to call - // root.End() multiple times. - root.End() - }() - allResults := collect.NewResult() - sbSpec := concatSpecs(opt.Specs.SupportBundlesV1Beta2...) + sbSpec := supportbundle.ConcatSpecs(opt.Specs.SupportBundlesV1Beta2...) - collectedResults, err := b.collectFromHost(ctxWrap, sbSpec.Spec.HostCollectors, opt.BundleDir, b.progressChan) + collectedResults, err := b.collectFromHost(ctx, sbSpec.Spec.HostCollectors, opt.BundleDir) if err != nil { return nil, errors.Wrap(err, "failed to collect bundle from host") } allResults.AddResult(collectedResults) - collectedResults, err = b.collectFromCluster(ctxWrap, sbSpec.Spec.Collectors, opt.BundleDir, opt.Namespace) + collectedResults, err = b.collectFromCluster(ctx, sbSpec.Spec.Collectors, opt.BundleDir, opt.Namespace) if err != nil { return nil, errors.Wrap(err, "failed to collect bundle from cluster") } @@ -113,7 +103,7 @@ func (b *Bundle) collectFromCluster( mergedCollectors, err := mergeCollector.Merge(collectors) if err != nil { msg := fmt.Sprintf("failed to merge collector: %s: %s", mergeCollector.Title(), err) - b.progressChan <- msg + b.reportProgress(msg) } allCollectors = append(allCollectors, mergedCollectors...) } else { @@ -124,7 +114,7 @@ func (b *Bundle) collectFromCluster( for _, collector := range collectors { for _, e := range collector.GetRBACErrors() { foundForbidden = true - b.progressChan <- e + b.reportProgress(e) } } } @@ -141,7 +131,7 @@ func (b *Bundle) collectFromCluster( isExcluded, _ := collector.IsExcluded() if isExcluded { msg := fmt.Sprintf("excluding %q collector", collector.Title()) - b.progressChan <- msg + b.reportProgress(msg) span.SetAttributes(attribute.Bool(constants.EXCLUDED, true)) span.End() continue @@ -151,18 +141,18 @@ func (b *Bundle) collectFromCluster( if collector.HasRBACErrors() { if _, ok := collector.(*collect.CollectClusterResources); !ok { msg := fmt.Sprintf("skipping collector %q with insufficient RBAC permissions", collector.Title()) - b.progressChan <- msg + b.reportProgress(msg) span.SetStatus(codes.Error, "skipping collector, insufficient RBAC permissions") span.End() continue } } - b.progressChan <- collector.Title() + b.reportProgress(collector.Title()) result, err := collector.Collect(b.progressChan) if err != nil { span.SetStatus(codes.Error, err.Error()) - b.progressChan <- errors.Errorf("failed to run collector: %s: %v", collector.Title(), err) + b.reportProgress(errors.Errorf("failed to run collector: %s: %v", collector.Title(), err)) } for k, v := range result { @@ -175,7 +165,7 @@ func (b *Bundle) collectFromCluster( } func (b *Bundle) collectFromHost( - ctx context.Context, collectSpecs []*troubleshootv1beta2.HostCollect, bundlePath string, progressChan chan any, + ctx context.Context, collectSpecs []*troubleshootv1beta2.HostCollect, bundlePath string, ) (collect.CollectorResult, error) { // TODO: Duplicate of runHostCollectors from pkg/supportbundle/collect.go. DRY me up! collectResult := collect.NewResult() @@ -195,18 +185,18 @@ func (b *Bundle) collectFromHost( isExcluded, _ := collector.IsExcluded() if isExcluded { - progressChan <- fmt.Sprintf("[%s] Excluding host collector", collector.Title()) + b.reportProgress(fmt.Sprintf("[%s] Excluding host collector", collector.Title())) span.SetAttributes(attribute.Bool(constants.EXCLUDED, true)) span.End() continue } - progressChan <- fmt.Sprintf("[%s] Running host collector...", collector.Title()) + b.reportProgress(fmt.Sprintf("[%s] Running host collector...", collector.Title())) // TODO: Convert return type to CollectorResult - result, err := collector.Collect(progressChan) + result, err := collector.Collect(b.progressChan) if err != nil { span.SetStatus(codes.Error, err.Error()) - progressChan <- errors.Errorf("failed to run host collector: %s: %v", collector.Title(), err) + b.reportProgress(errors.Errorf("failed to run host collector: %s: %v", collector.Title(), err)) } span.End() for k, v := range result { @@ -217,14 +207,6 @@ func (b *Bundle) collectFromHost( return collectResult, nil } -func concatSpecs(specs ...troubleshootv1beta2.SupportBundle) *troubleshootv1beta2.SupportBundle { - target := &troubleshootv1beta2.SupportBundle{} - for _, s := range specs { - target = supportbundle.ConcatSpec(target, &s) - } - return target -} - func parseTimeFlags(v *viper.Viper) (*time.Time, error) { // TODO: Copied from cmd/troubleshoot/cli/run.go. DRY me up! var ( diff --git a/pkg/bundle/bundler.go b/pkg/bundle/bundler.go index 2b473bcab..e8be8f023 100644 --- a/pkg/bundle/bundler.go +++ b/pkg/bundle/bundler.go @@ -65,7 +65,6 @@ type ArchiveOptions struct { } type AnalyzeOutput struct { - Bundle Bundler // include bundle metadata // TODO: Does this need to be a slice of pointers? It's a slice and slices are already pointers Results []*analyze.AnalyzeResult // analysis results } diff --git a/pkg/collect/result.go b/pkg/collect/result.go index 7080e0dbf..fdbc45cdb 100644 --- a/pkg/collect/result.go +++ b/pkg/collect/result.go @@ -27,10 +27,15 @@ type BundleData struct { bundleDir string } -func NewBundleData(bundleDir string) *BundleData { +type BundleDataOptions struct { + Data CollectorResult + BundleDir string +} + +func NewBundleData(opt BundleDataOptions) *BundleData { return &BundleData{ - data: NewResult(), - bundleDir: bundleDir, + data: opt.Data, + bundleDir: opt.BundleDir, } } diff --git a/pkg/supportbundle/collect.go b/pkg/supportbundle/collect.go index 60c8cb868..244f3be5b 100644 --- a/pkg/supportbundle/collect.go +++ b/pkg/supportbundle/collect.go @@ -221,7 +221,8 @@ func findFileName(basename, extension string) (string, error) { } } -func getVersionFile() (io.Reader, error) { +func GetVersionFile() (io.Reader, error) { + // TODO: Move to internal package version := troubleshootv1beta2.SupportBundleVersion{ ApiVersion: "troubleshoot.sh/v1beta2", Kind: "SupportBundle", diff --git a/pkg/supportbundle/supportbundle.go b/pkg/supportbundle/supportbundle.go index a4220acf0..52fb44e4b 100644 --- a/pkg/supportbundle/supportbundle.go +++ b/pkg/supportbundle/supportbundle.go @@ -140,7 +140,7 @@ func CollectSupportBundleFromSpec( return nil, errors.Wrap(err, "failed to generate support bundle") } - version, err := getVersionFile() + version, err := GetVersionFile() if err != nil { return nil, errors.Wrap(err, "failed to get version file") } @@ -282,3 +282,11 @@ func ConcatSpec(target *troubleshootv1beta2.SupportBundle, source *troubleshootv } return newBundle } + +func ConcatSpecs(specs ...troubleshootv1beta2.SupportBundle) *troubleshootv1beta2.SupportBundle { + target := &troubleshootv1beta2.SupportBundle{} + for _, s := range specs { + target = ConcatSpec(target, &s) + } + return target +}