Switch Collector from struct to interface

[xavpaice]

Outline

With #665 we have Troubleshoot appending multiple specs provided on the CLI (or call) into one long spec. Some collectors which may be specified more than once might have specific de-duplication rules which need to be taken into account, to avoid collecting the same thing multiple times.

This task is to enable a means to run a collector specific merge which could de-duplicate, or aggregate, based on the collector type.

In scope:

• one generic Collector type

Out of scope:

• Analyzer and Redactor de-duplication.

Proposal

• change the Collector type to an interface with methods that include Collect()
• write a new generic type for collectors to use, that implements the interface
• modify each collector to use the generic type, so that it can be called with collector.Collect() to actually get the data
• Implement at least 1 merge to demonstrate how it works

assumptions

• Make Troubleshoot accept multiple specs #665 provides a spec with all the specs provided to Troubleshoot into one SupportBundle, no other changes are needed to receive multiple inputs.

Definition of done

• Interface implemented for Collector that includes a merge function with 1 collector implementing it
• Interface implemented for Collector that doesn't include merge functional with remaining collectors implementing it
• Documentation for how to make use of the interface for a new collector
• Test both interfaces to confirm merge and non-merge works as expected

Followup tasks

• Similar implementation for Analyzers and Redactors

[xavpaice]

See progress made by @diamonwiggins in #670

[chris-sanders]

Just a note I modified the final bullet of the proposal to clarify that we only need implement a single collector's merge function with this PR to demonstrate how it works and verify/test the code path. Implementing the function on all collectors should be out of scope and scheduled later after this change which provides the interfaces to do it.

I also edited the Definition of Done to clarify that desire for 1 collector to implement Merge with the rest just implementing the standard no-merge interface.

[diamonwiggins]

#670 has been updated with my latest changes. I'd still consider it to be a draft at this point. Ended up being more to work through than I expected. A few things to get the discussion going:

1. Need to clarify if in our Merge() function is where we expect deduplication to also take place? If any additional error handling as well as deduplication is essentially what we mean by Merge() then I worry about the amount of complexity being added with this second interface if all use cases are just dedup which wouldn't warrant a separate interface because every collector would need to implement it. Take for example:

spec A

- clusterResources: {}

spec B

- clusterResources:
     namespace: kube-system

Here we want the final spec to be spec A because it will encompass spec B as well as collect more information. I think this use case makes more sense in a dedup function and every collector will need some level of dedup so what are some clear uses cases for a separate "MergeableCollector" interface that some collectors may use and others no?

2. Expanding on some of the complexity that the seperate interface brings:

   troubleshoot/pkg/supportbundle/collect.go

   Lines 86 to 104 in 7490885

   	var newCollectors []collect.Collector
   	for _, desiredCollector := range collectSpecs {
   	if collector, ok := collect.GetCollector(desiredCollector, bundlePath, opts.Namespace, opts.KubernetesRestConfig, k8sClient, opts.SinceTime, RBACErrors); ok {
   	if regCollector, ok := collector.(collect.Collector); ok {
   	err := regCollector.CheckRBAC(context.Background(), desiredCollector)
   	if err != nil {
   	return nil, errors.Wrap(err, "failed to check RBAC for collectors")
   	}
   	//Not sure this makes sense here as we're still iterating through the entire spec
   	//Likely better to iterate by collector Type so that you only process each type once(eg. Logs, ClusterResources, etc.)
   	if mergeCollector, ok := collector.(collect.MergeableCollector); ok {
   	fmt.Println(mergeCollector.Merge())
   	}
   	newCollectors = append(newCollectors, regCollector)
   	}
   	}
   	}

   is where i'm doing the type checking to see if a given collector implements the MergeableCollector interface or not. Regardless of which type i still only append the collector to a list of collect.Collectors and I don't maintain a separate list for collect.MergeableCollector. I did this to reduce the amount of times later in the code I need to type check again. Figured it was worth a callout because it felt strange, but it does work because outside of running Merge() the collector.Collector interface implements everything else that needs to be done. However, should it be done this way? Curious how confusing this would be to others contributing.

3. A comment that I have in the previous snippet of code "Not sure this makes sense here as we're still iterating through the entire spec". To expand on this, what I mean is if we're still iterating through the concatenated spec provided as input from the user as well as other sources, it feels strange that we'd be running the Merge() interface on that same spec as we're iterating through it? Should we do something like the following:

apiVersion: troubleshoot.sh/v1beta2
kind: SupportBundle
metadata:
  name: example
spec:
  collectors:
    - clusterResources: {}
    - clusterResources:
          namespace: kube-system
    - secret:
        namespace: default
        name: secret1
    - secret:
        namespace: default
        name: secret2

1. Build list of all collector types in a spec (eg. [clusterResources, secret]
2. Iterate over all collector types and run the "Merge()" function which runs its logic and returns []collect.Collector but only with a list of the current collector type we've iterated on
3. Run collector.Collect() for the current Collector Type
4. Repeat for the next collector type in the list

4. Right now the current behavior is that redaction takes place for each collector that's run. When I added host collectors to support bundles, I made it so that redaction only happens one time right before the bundle is archived. This makes sense to me, and I'm not sure with redactors being in a separate spec altogether why anything would be happening on each run of collector.RunCollectorSync. See here - https://github.com/replicatedhq/troubleshoot/blob/main/pkg/collect/collector.go#L295. However, this has introduced a problem with the collector_test.go tests I currently have commented out because they are testing redaction, but expect redaction to happen when each individual collector is run. So my questions becomes, should I change the tests, or do we implement a mechanism for global redaction as well as redaction per collector? Maybe a Redact() function on the Collector interface? I mention this as there have been some discussions lately about bringing redactors into the support bundle spec and potentially having additional redaction logic specific to collectors.

5. Lastly, not saying we have to address it with this work, but i was surprised to see some of the differences between collect.go in Preflight vs. SupportBundle. I feel like we could do some refactoring to make things more consistent there, but I have modified it to get it working with the new interfaces

[chris-sanders]

Still working through the PR to fully understand the questions. I'll start with the one at the top, you have a lot of questions in a single comment so I'm not going to try and address them all at once.

Need to clarify if in our Merge() function is where we expect deduplication to also take place? If any additional error handling as well as deduplication is essentially what we mean by Merge() ...

Yes I think the intention of Merge has been to include any operations beyond a direct append of the collector so it would be error handling, deduplication, or any other shade of gray ranging from a simple append to full on item by item merging.

if all use cases are just dedup which wouldn't warrant a separate interface because every collector would need to implement it.

First, I don't think I agree with the statement that all use cases are just dedup. Even if all of todays cases are just dedup that doesn't follow that all possible future cases are just dedup. The distinction I'm making here is, we want to allow more than dedup even if current collectors and file paths don't make that reasonable today. So the question isn't what works right now, but what do we want to have available in the interface. Are you intending to say there is no use case for Merging at all as a concept on Collectors?

Next you propose that dedup shouldn't be repeated in each collector. Which is DRY and I appreciate that. My question, similar to above, is do you believe dedup can be written completely agnostic to the collector and it's configuration? Not for one specific collector, but for all possible collectors. I believe you need to have context about the collector to provide dedup, is that not the case? I'll revisit with the example below.

Here we want the final spec to be spec A because it will encompass spec B as well as collect more information. I think this use case makes more sense in a dedup function and every collector will need some level of dedup so what are some clear uses cases for a separate "MergeableCollector" interface that some collectors may use and others no?

First to say that every collector needs some dedup, I don't know that to be true. We could, for example, merge and do all appends and most collectors would work fine. Dedup then becomes an optimization and optimization is not required for every collector, even if it would be best if they all implemented the most optimized possible collection.

The example you gave does clearly present a use case for dedup. It relies on knowing the collector and what it does. I'm not so sure you can globally 'dedup' all collectors the same. Each collector defines what it's inputs mean, and what it collects. In the example you know how ClusterResources operates and you are using that to decide what the output of merge should be.

I was under the impression only name and exclude were defined on the common scheme although it looks like namespace is actually there too. My point here is, a global dedup would only be able to operate on the shared scheme because it has no guarantees about anything else. This means a shared dedup if even possible would be highly restricted.

To close the example out. What you've shown is the generic rule "If namespace isn't defined that overrides a defined namespace and the two specs can be deduplicated". That isn't true of the systctl collector which still needs a namespace and uses a default if none is provided. Two of these collectors can not be merged with the same logic you used above, so the decision would have to be made at the collector level instead where it knows what defaults are. While that isn't DRY only the collector in context of what it does with inputs can implement deduplication (which is just a subset of Merge).

[diamonwiggins]

Are you intending to say there is no use case for Merging at all as a concept on Collectors?

At this time this is the way that I see it, but certainly could be convinced otherwise.

You mention:

Yes I think the intention of Merge has been to include any operations beyond a direct append of the collector so it would be error handling, deduplication, or any other shade of gray ranging from a simple append to full on item by item merging.

I think the operations of direct append, error handling, and deduplication is something all the collectors would make use of, so the separate interface in my opinion would only make sense if we have actual use cases for full on item by item merging.

First to say that every collector needs some dedup, I don't know that to be true. We could, for example, merge and do all appends and most collectors would work fine. Dedup then becomes an optimization and optimization is not required for every collector, even if it would be best if they all implemented the most optimized possible collection.

My assumption was that every collector on some level would "dedup" in the since of if the same exact collector configuration is provided twice, you would discard the duplicates. As well as most every collector would also have handling around multiple collectors using the same output path in the bundle as the collectorName could technically be the same.

The example you gave does clearly present a use case for dedup. It relies on knowing the collector and what it does. I'm not so sure you can globally 'dedup' all collectors the same. Each collector defines what it's inputs mean, and what it collects. In the example you know how ClusterResources operates and you are using that to decide what the output of merge should be.

This is true, but I guess the only point I'm making is similar to the Collect() function for each interface which implements different logic per collector, wouldn't a Dedup() function on the Collector interface follow that same pattern and bypass need for a separate interface? This is based off of my assumption that all the use cases we have today squarely fit into this Dedup/Error handling function and that every collector will need to implement. However, let's say we had the following example of specs:

spec A

    - logs:
        namespace: kube-system
        selector:
          - k8s-app=kube-dns

spec B

    - logs:
        namespace: kube-system
        selector:
          - k8s-app=weave

result

    - logs:
       namespace: kube-system
       selector:
         - k8s-app=kube-dns
         - k8s-app=weave

If we had a use case like the above, then in my mind that would be a slam dunk example of something that fits into a separate MergeableCollector interface because we likely wouldn't be doing anything like this with any of the other collectors. Although the actual code for the dedup/error handling could look different for each collector, my thinking is that the Dedup() function will need to be implemented for each collector so doesn't need to be a separate interface.

A couple points to wrap here:

1. Despite my lengthy comments here so far this isn't a hill i'm willing to die on just hedging on the added complexity if we don't actually need it :)

2. In my previous comment, item number 4 is what's primarily blocking this from flipping to Reviewable. Need to get some feedback on if I should revert redaction to run on each collector run per what the tests expect as that's the old behavior. Or continue with what I think is an optimization in having redaction only run once which would require refactoring the tests.

[chris-sanders]

If we had a use case like the above, then in my mind that would be a slam dunk example of something that fits into a separate MergeableCollector interface because we likely wouldn't be doing anything like this with any of the other collectors.

I think that's a great use case we should strive to allow. If current log collector doesn't allow it, setting up the interface so that we can support this use case is a good start!

I'll review the concepts in number 4 next and see if I understand it enough to have an opinion on it.

[diamonwiggins]

I've pushed up my latest changes. An update on where all this stands now:

1. After our discussion about making this backward compatible, I started down that road, but wondered if the nature of what we're doing here can really be backwards compatible at all? We're changing the type of Collector to no longer be a Struct, but to be an interface that each individual collector can implement. It may be possible to keep things backwards compatible, but at the very least we'd need a separate interface not named Collector and would have to duplicate a bunch of code I feel. I confirmed that KOTS for example does use the collect package directly so it would break with these changes. I think this is the last remaining item that needs to be resolved before this work can be reviewed.

2. With regards to redaction, it no longer occurs on every run of a collector, and it no longer occurs on Preflight runs as I'm not sure there's a benefit in doing so.

3. Per my comment in item number 3 here, I think we are going to want to restructure code to do the following instead of iterating through the unordered spec:

   1. Build list of all collector types in a spec (eg. [clusterResources, secret]
   2. Iterate over all collector types and run the "Merge()" function which runs its logic and returns []collect.Collector but only with a list of the current collector type we've iterated on
   3. Run collector.Collect() for the current Collector Type.
   4. Repeat for the next collector type in the list

I think this will simplify the merge logic if it's able to look at all of its spec items and make a decision that returns []collect.Collector of its type. Otherwise, if you have:

apiVersion: troubleshoot.sh/v1beta2
kind: SupportBundle
metadata:
  name: example
spec:
  collectors:
    - clusterResources:
         namespaces:
           - default
           - myapp-namespace
    - clusterInfo: {}
    ....
    - clusterResources: {}

With the way the code is written now, the first clusterResources item will be added to the desired collectors to be run, but when the Merge() function is run again after iterating on clusterResources later in the spec, it now has to remove the earlier item because we want to keep things least specific. If Merge() is run a single time over the entire spec for each collectorType, it feels like it would be simpler.

4. I've implemented a simple append for clusterResources in it's Merge() function. No dedup will take place which is at least consistent with how Troubleshoot works today.

5. Questions on Definition of Done

Interface implemented for Collector that doesn't include merge functional with remaining collectors implementing it

I'm not sure I understand this item from the Definition of Done. Could use some clarification.

Test both interfaces to confirm merge and non-merge works as expected

We have limited use cases for merge at the moment. What do we expect to be tested here? Just that a function returns output, or some sort of collector specific merge logic that gets written?