feat(support-bundle): add runHostCollectorsInPod in spec #1608

DexterYan · 2024-09-09T06:12:54Z

Description, Motivation and Context

Add runHostCollectorsInPod for SupportBundle yaml

apiVersion: troubleshoot.sh/v1beta2
kind: SupportBundle
metadata:
  name: sb
  runHostCollectorsInPod: true  # default is false 
spec:
  hostCollectors:
  - hostOS: {}

Re-use current CollectRemote function to host collect

type HostCollector interface {
	Title() string
	IsExcluded() (bool, error)
	Collect(progressChan chan<- interface{}) (map[string][]byte, error)
	RemoteCollect(progressChan chan<- interface{}) (map[string][]byte, error)
	HasRemoted() bool
}

Copy CollectRemote into common go for avoiding import cycle
Support multiple node HostOS collector result but save only one for now
Enable HostOS collector for running a privileged pod

sc-111270

Test Yaml

apiVersion: troubleshoot.sh/v1beta2
kind: SupportBundle
metadata:
  name: sb
  runHostCollectorsInPod: true  # default is false 
spec:
  hostCollectors:
  - hostOS: {}
  hostAnalyzers:
  - hostOS: {}

Result
support-bundle-2024-09-12T16_02_26.tar.gz

Checklist

New and existing tests pass locally with introduced changes.
Tests for the changes have been added (for bug fixes / features)
The commit message(s) are informative and highlight any breaking changes
Any documentation required has been added/updated. For changes to https://troubleshoot.sh/ create a PR here

Does this PR introduce a breaking change?

Yes
No

This PR is backward compatible.

diamonwiggins · 2024-09-16T15:47:50Z

pkg/collect/host_collector.go

@@ -8,6 +8,8 @@ type HostCollector interface {
 	Title() string
 	IsExcluded() (bool, error)
 	Collect(progressChan chan<- interface{}) (map[string][]byte, error)
+	RemoteCollect(progressChan chan<- interface{}) (map[string][]byte, error)
+	HasRemoted() bool


What is this for? The naming has me a bit confused.

I think i see now. I wonder if this could be renamed to something that would make it clearer what this is for.

diamonwiggins · 2024-09-16T21:32:52Z

cmd/troubleshoot/cli/run.go

@@ -183,6 +183,7 @@ func runTroubleshoot(v *viper.Viper, args []string) error {
 		OutputPath:                v.GetString("output"),
 		Redact:                    v.GetBool("redact"),
 		FromCLI:                   true,
+		RunHostCollectorsInPod:    false || mainBundle.Metadata.RunHostCollectorsInPod,


Can this just be set to mainBundle.Metadata.RunHostCollectorsInPod?

good catch, it has been set to mainBundle.Metadata.RunHostCollectorsInPod

diamonwiggins · 2024-09-16T21:38:13Z

cmd/troubleshoot/cli/run.go

 	for _, sb := range kinds.SupportBundlesV1Beta2 {
 		sb := sb
 		mainBundle = supportbundle.ConcatSpec(mainBundle, &sb)
+		enableRunHostCollectorsInPod = enableRunHostCollectorsInPod || sb.Metadata.RunHostCollectorsInPod


Will this cause a panic if sb.Metadata was nil for some reason? Should we check that it isn't nil here and then just set enableRunHostCollecorsInPod := sb.Metadata.RunHostCollectorsInPod?

yes, we should check that. I have added the checking part

diamonwiggins

Left a few comments. Also a couple questions:

Should we refactor things in this PR so that we move the remote collector functions to internal and then have both Collectors and RemoteCollectors use those? That way we're in a better place to deprecate RemoteCollectors soon? Or would you rather just do that in a follow up?
I don't see any of the code for actually making the pod that's used as the runner privileged. Is that going to be in a follow up as well?

DexterYan · 2024-09-17T19:26:20Z

The runner privileged has been added into the current code. For moving things to internal will be another PR

diamonwiggins · 2024-09-18T17:09:57Z

pkg/collect/host_collector.go

@@ -8,6 +8,8 @@ type HostCollector interface {
 	Title() string
 	IsExcluded() (bool, error)
 	Collect(progressChan chan<- interface{}) (map[string][]byte, error)
+	RemoteCollect(progressChan chan<- interface{}) (map[string][]byte, error) // RemoteCollect is used to priviledge pods to collect data from different nodes
+	HasRemoted() bool                                                         // HasRemoted returns true if the collector has a remote collector implementation


I wonder if HasRemoteCollector would be more descriptive here. Just nitpicking mostly, will leave it up to you.

Yes, this function is not easy to read. I have removed it. And use

if err == collect.ErrRemoteCollectorNotImplemented {

to check

updated to use

if errors.Is(err, collect.ErrRemoteCollectorNotImplemented) {

diamonwiggins

minor comment, but otherwise lgtm

banjoh · 2024-09-18T17:23:44Z

pkg/collect/host_os_info.go

@@ -18,6 +19,9 @@ type HostOSInfo struct {

 const HostOSInfoPath = `host-collectors/system/hostos_info.json`

+type NodeInfo struct {
+	HostOSInfo HostOSInfo `json:"host-collectors/system/hostos_info.json"`


Where is json:"host-collectors/system/hostos_info.json" used? Its an interesting use of json tags where the tag is a path. I've not seen before

good catch, it has been removed.

banjoh

PR looks good. I requested a few changes

In a future PR we want to start using "replicated/troubleshoot:<version>" instead of "replicated/troubleshoot:latest" so as to ensure the same version of the image is used as by the binary calling it. There will be an issue with tests passing since the image will most likely not be available.

banjoh · 2024-09-18T17:31:49Z

pkg/collect/host_os_info.go

+		return nil, errors.Wrap(err, "failed to convert kube flags to rest config")
+	}
+
+	progressCh := make(chan interface{})


Should we be using progressChan channel for sending progress feedback, instead of creating a new one? The new one is also not doing anything.

awesome, that has been fixed

banjoh · 2024-09-18T19:20:09Z

pkg/collect/host_certificate.go

+	return nil, errors.New("not implemented")
+}
+
+func (c *CollectHostCertificate) HasRemoted() bool {


If the only purpose of HasRemote() is to check that a collector has implemented the RemoteCollect() function, I'd leave the function out in favour or checking the not implemented error. It would be good to keep public functions at a minimum

You could do something like

var ErrNotImplemented = errors.New("not implemented") ... result, err := collector.RemoteCollect(opts.ProgressChan) if errors.Is(err, collect.ErrNotImplemented) { result, err = collector.Collect(opts.ProgressChan) ... } if err != nil { ... }

good idea, I have changed to

if errors.Is(err, collect.ErrRemoteCollectorNotImplemented) {

banjoh · 2024-09-18T21:00:30Z

pkg/collect/host_os_info.go

@@ -20,7 +20,7 @@ type HostOSInfo struct {
 const HostOSInfoPath = `host-collectors/system/hostos_info.json`

 type NodeInfo struct {


I cannot see where this is used.

banjoh

Can we add an e2e test for this use case?

DexterYan · 2024-09-19T16:23:19Z

e2e test added

banjoh · 2024-09-20T16:05:25Z

test/e2e/support-bundle/hostOS_remote_collector_e2e_test.go

+			supportbundleName := "host-os-remote-collector"
+			tarPath := fmt.Sprintf("%s.tar.gz", supportbundleName)
+			targetFolder := fmt.Sprintf("%s/host-collectors/system/", supportbundleName)
+			cmd := exec.CommandContext(context.Background(), sbBinary(), "spec/hostOSRemoteCollector.yaml", "--interactive=false", fmt.Sprintf("-o=%s", supportbundleName))


Relace context.Background() with ctx parameter

DexterYan requested a review from a team as a code owner September 9, 2024 06:12

DexterYan marked this pull request as draft September 9, 2024 06:13

DexterYan added the type::feature New feature or request label Sep 9, 2024

DexterYan changed the title ~~WIP feat(support-bundle): add runHostCollectorsInPod in spec~~ feat(support-bundle): add runHostCollectorsInPod in spec Sep 12, 2024

DexterYan marked this pull request as ready for review September 12, 2024 06:13

diamonwiggins reviewed Sep 16, 2024

View reviewed changes

diamonwiggins requested changes Sep 16, 2024

View reviewed changes

DexterYan added 11 commits September 17, 2024 14:23

feat(support-bundle): add runHostCollectorsInPod in spec

1050971

add test

5eff9bd

use collectorRunOpts to pass runHostCollectorsInPod

33be231

enable hostOS in remote privileged pod

eb90556

update hostOS collector

241317f

change to HasRemoted

e4ec307

remove common library

5c52dae

add name prefix

76ceb8d

use mainBundle.Metadata.RunHostCollectorsInPod

357086f

add metadata check

a3fdb0f

use privileged pod

55e6760

DexterYan force-pushed the dx/sc-111270/host-os-collector branch from dfd63fa to 55e6760 Compare September 17, 2024 19:23

diamonwiggins reviewed Sep 18, 2024

View reviewed changes

diamonwiggins previously approved these changes Sep 18, 2024

View reviewed changes

banjoh reviewed Sep 18, 2024

View reviewed changes

banjoh requested changes Sep 18, 2024

View reviewed changes

remove HasRemoted func

a7f4a6b

DexterYan dismissed diamonwiggins’s stale review via a7f4a6b September 18, 2024 19:59

DexterYan added 2 commits September 18, 2024 15:03

change error compare

fd719de

remove unused json tag

65bc34d

fix chan interface

acd1db2

banjoh reviewed Sep 18, 2024

View reviewed changes

remove nodeinfo

2967253

banjoh approved these changes Sep 18, 2024

View reviewed changes

banjoh requested changes Sep 18, 2024

View reviewed changes

add e2e test

22778b4

DexterYan mentioned this pull request Sep 20, 2024

feat(analyzer): enable host os info analyzer to support multiple nodes #1618

Merged

6 tasks

banjoh previously approved these changes Sep 20, 2024

View reviewed changes

fix ctx

a2e4d9c

DexterYan dismissed banjoh’s stale review via a2e4d9c September 20, 2024 16:09

banjoh previously approved these changes Sep 20, 2024

View reviewed changes

DexterYan added 2 commits September 20, 2024 11:44

Merge branch 'main' into dx/sc-111270/host-os-collector

13401a1

add RemoteCollect for host dns

5e855d5

DexterYan dismissed banjoh’s stale review via 5e855d5 September 20, 2024 16:46

banjoh approved these changes Sep 20, 2024

View reviewed changes

DexterYan merged commit e97b961 into main Sep 20, 2024
27 checks passed

DexterYan deleted the dx/sc-111270/host-os-collector branch September 20, 2024 16:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(support-bundle): add runHostCollectorsInPod in spec #1608

feat(support-bundle): add runHostCollectorsInPod in spec #1608

DexterYan commented Sep 9, 2024 •

edited

Loading

diamonwiggins Sep 16, 2024

diamonwiggins Sep 16, 2024

diamonwiggins Sep 16, 2024

DexterYan Sep 17, 2024

diamonwiggins Sep 16, 2024

DexterYan Sep 17, 2024

diamonwiggins left a comment

DexterYan commented Sep 17, 2024

diamonwiggins Sep 18, 2024

DexterYan Sep 18, 2024

DexterYan Sep 18, 2024

diamonwiggins left a comment

banjoh Sep 18, 2024 •

edited

Loading

DexterYan Sep 18, 2024

banjoh left a comment

banjoh Sep 18, 2024

DexterYan Sep 18, 2024

banjoh Sep 18, 2024

DexterYan Sep 18, 2024

banjoh Sep 18, 2024

DexterYan Sep 18, 2024

banjoh left a comment

DexterYan commented Sep 19, 2024

banjoh Sep 20, 2024

DexterYan Sep 20, 2024

		@@ -20,7 +20,7 @@ type HostOSInfo struct {
		const HostOSInfoPath = `host-collectors/system/hostos_info.json`

		type NodeInfo struct {

feat(support-bundle): add runHostCollectorsInPod in spec #1608

feat(support-bundle): add runHostCollectorsInPod in spec #1608

Conversation

DexterYan commented Sep 9, 2024 • edited Loading

Description, Motivation and Context

Checklist

Does this PR introduce a breaking change?

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

diamonwiggins left a comment

Choose a reason for hiding this comment

DexterYan commented Sep 17, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

diamonwiggins left a comment

Choose a reason for hiding this comment

banjoh Sep 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

banjoh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

banjoh left a comment

Choose a reason for hiding this comment

DexterYan commented Sep 19, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DexterYan commented Sep 9, 2024 •

edited

Loading

banjoh Sep 18, 2024 •

edited

Loading