Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Set a bundle path when using preflight #1308

Open
CpuID opened this issue Aug 22, 2023 · 8 comments
Open

Set a bundle path when using preflight #1308

CpuID opened this issue Aug 22, 2023 · 8 comments
Labels
type::feature New feature or request

Comments

@CpuID
Copy link
Contributor

CpuID commented Aug 22, 2023

Describe the rationale for the suggested feature.

We should configure a bundle path for preflight usage, even if we don't store the final outcomes. This would allow using a lot of collector/analyzer combinations that expect a result written using SaveResult and then read in via the analyzer.

eg. using the run collector with the textAnalyze analyzer in preflights.

Describe the feature

What we do now in preflight - https://github.com/replicatedhq/troubleshoot/blob/main/pkg/preflight/collect.go#L108 (empty "" for bundle path)

What we do in support bundle - https://github.com/replicatedhq/troubleshoot/blob/main/pkg/supportbundle/collect.go#L34

Make this behaviour consistent across both.

Describe alternatives you've considered

N/A

Additional context

N/A

@CpuID CpuID added the type::feature New feature or request label Aug 22, 2023
@CpuID
Copy link
Contributor Author

CpuID commented Aug 22, 2023

Considerations: does this introduce any minimum disk space requirements for preflight usage? If results are being stored?

@DexterYan
Copy link
Member

DexterYan commented Aug 22, 2023

The preflight and support-bundle are using different way of processing collectResults.

In preflight, we are using allCollectedData to pass the result to analyze from memory.
Here is the code explain

if specs.HostPreflightSpec != nil {

r, err := collectHost(ctx, specs.HostPreflightSpec, progressCh)
collectResults = append(collectResults, *r)
...
for _, res := range collectResults {
	analyzeResults = append(analyzeResults, res.Analyze()...)
}

You can see our collector pass the data directly to res.Analyze()

In the Analyzer()

https://github.com/replicatedhq/troubleshoot/blob/6972789bea2f4350a6753723618dec3796c9b676/pkg/preflight/analyze.go#L22C9-L22C9

It is using c.AllCollectedData

And in doAnalyzer()

https://github.com/replicatedhq/troubleshoot/blob/6972789bea2f4350a6753723618dec3796c9b676/pkg/preflight/analyze.go#L60C3-L60C3

That can confirm that getCollectedFileContents is reading from memory.

To explain of sharing AllCollectedData, you can check collectHost, we are using CollectHostWithContext

collectResult.AllCollectedData = allCollectedData

The collectResult is HostCollectResult
https://github.com/replicatedhq/troubleshoot/blob/6972789bea2f4350a6753723618dec3796c9b676/pkg/preflight/collect.go#L69C6-L69C23

decoupled from

type CollectResult interface {

since they have Analyze() and IsRBACAllowed() method

@DexterYan
Copy link
Member

DexterYan commented Aug 22, 2023

However, in our support-bundle

we are using AnalyzeSupportBundle(ctx, spec, bundlePath)
https://github.com/replicatedhq/troubleshoot/blob/6972789bea2f4350a6753723618dec3796c9b676/pkg/supportbundle/supportbundle.go#L154C68-L154C68

And in AnalyzeSupportBundle, we have AnalyzeLocal to process the collected data
https://github.com/replicatedhq/troubleshoot/blob/6972789bea2f4350a6753723618dec3796c9b676/pkg/supportbundle/supportbundle.go#L259C34-L259C46

AnalyzeLocal is sharing the same of Analyze(), however, the difference is how we pass getCollectedFileContents
https://github.com/replicatedhq/troubleshoot/blob/6972789bea2f4350a6753723618dec3796c9b676/pkg/analyze/download.go#L43C1-L44C1
In support bundle, it is using fcp.getFileContents, fcp.getChildFileContents

@DexterYan
Copy link
Member

DexterYan commented Aug 22, 2023

I think we may need to use the same way as preflight of reading from memory, since we return collectedData as runHostOutput in

runHostOutput := map[string][]byte{
resultInfo: b,
result: stdout.Bytes(),
}
return runHostOutput, nil

@diamonwiggins
Copy link
Member

diamonwiggins commented Aug 23, 2023

This would allow using a lot of collector/analyzer combinations that expect a result written using SaveResult and then read in via the analyzer.

I'm not saying we shouldn't add a bundlePath for Preflight for whatever reason, but have we confirmed that the above is true? Are there analyzers which can't be used in kind: Preflight or kind: HostPreflight because it expects to read results from disk instead of from memory? If so, I'd argue the issue is either:

  1. Analyzer not reading from memory when it should be
  2. Collect result for a particular collector not always being stored in memory.

@banjoh
Copy link
Member

banjoh commented Aug 24, 2023

I looked at the spec in question (below)

apiVersion: troubleshoot.sh/v1beta2
kind: HostPreflight
spec:
  collectors:
    - run:
        name: iptables
        collectorName: iptables-forward-chain
        command: iptables
        args:
          - '-L'
          - FORWARD
  analyzers:
    - textAnalyze:
        checkName: forward-chain-reject-check
        fileName: iptables/iptables-forward-chain.log
        regex: ^REJECT\s+all\s+.*\s+anywhere\s+anywhere
        outcomes:
          - pass:
              message: No default REJECT rule found in iptables FORWARD chain
              when: "false"
          - fail:
              message: |
                Default REJECT rule found in iptables FORWARD chain.
                Please remove it and re-run the installer.
              when: "true"

and noticed that the path specified in the analyser is wrong. It ought to be fileName: host-collectors/run-host/iptables-forward-chain.txt. As per the collector's documentation, collected files will be stored in host-collectors/run-host/ directory. It however wrongly states that the filename would be [collector-name].json. That bit of the doc needs updating.

Recommended doc update

  • Correct the file name of collected output
  • Add missing host-collectors/run-host/<collector-name>-info.json file which is also collected
  • Add an analyser example

@banjoh
Copy link
Member

banjoh commented Aug 24, 2023

As a debugging tip, run the preflight binary with -v=2 to print out the collected paths of collected data

[evans] $ preflight spec.yaml -v=2
I0824 12:24:34.837202   35937 loader.go:204] loaded troubleshoot specs successfully
 * [etc-hosts] Running collector...
I0824 12:24:34.840306   35937 result.go:105] Added "host-collectors/run-host/etc-hosts-info.json" to bundle output
I0824 12:24:34.840323   35937 result.go:105] Added "host-collectors/run-host/etc-hosts.txt" to bundle output

============ Collectors summary =============
Succeeded (S), eXcluded (X), Failed (F)
=============================================
etc-hosts (S) : 3ms

============ Redactors summary =============
No redactors executed

============= Analyzers summary =============
Succeeded (S), eXcluded (X), Failed (F)
=============================================
check-etc-hosts (S) : 0ms

Duration: 1,987ms

@banjoh
Copy link
Member

banjoh commented Aug 24, 2023

Docs update PR: replicatedhq/troubleshoot.sh#521

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type::feature New feature or request
Projects
None yet
Development

No branches or pull requests

4 participants