Skip to content

Commit e45e2ca

Browse files
xavpaicexavpaice
authored
Fix collector ordering: preserve order when grouping by type (#1935)
- Fix issue where EnsureClusterResourcesFirst ordering was lost when collectors were grouped by type into a map (Go maps have random iteration order) - Preserve collector type order by tracking collectorTypeOrder slice as collectors are added to the map - Apply fix to both pkg/preflight/collect.go and pkg/supportbundle/collect.go - Add comprehensive tests to verify clusterResources runs first and relative order of other collectors is preserved - Enhance EnsureClusterResourcesFirst tests with additional edge cases
1 parent da51c28 commit e45e2ca

File tree

4 files changed

+319
-3
lines changed

4 files changed

+319
-3
lines changed

‎pkg/collect/collect_test.go‎

Lines changed: 94 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,7 @@ func Test_ensureClusterResourcesFirst(t *testing.T) {
1515
list []*troubleshootv1beta2.Collect
1616
}{
1717
{
18-
name: "Reorg OK",
18+
name: "Reorg OK - clusterResources moved to front",
1919
want: []*troubleshootv1beta2.Collect{
2020
{
2121
ClusterResources: &troubleshootv1beta2.ClusterResources{},
@@ -33,6 +33,99 @@ func Test_ensureClusterResourcesFirst(t *testing.T) {
3333
},
3434
},
3535
},
36+
{
37+
name: "Already first - no change",
38+
want: []*troubleshootv1beta2.Collect{
39+
{
40+
ClusterResources: &troubleshootv1beta2.ClusterResources{},
41+
},
42+
{
43+
Data: &troubleshootv1beta2.Data{},
44+
},
45+
{
46+
Secret: &troubleshootv1beta2.Secret{},
47+
},
48+
},
49+
list: []*troubleshootv1beta2.Collect{
50+
{
51+
ClusterResources: &troubleshootv1beta2.ClusterResources{},
52+
},
53+
{
54+
Data: &troubleshootv1beta2.Data{},
55+
},
56+
{
57+
Secret: &troubleshootv1beta2.Secret{},
58+
},
59+
},
60+
},
61+
{
62+
name: "Multiple clusterResources - all moved to front",
63+
want: []*troubleshootv1beta2.Collect{
64+
{
65+
ClusterResources: &troubleshootv1beta2.ClusterResources{},
66+
},
67+
{
68+
ClusterResources: &troubleshootv1beta2.ClusterResources{},
69+
},
70+
{
71+
Data: &troubleshootv1beta2.Data{},
72+
},
73+
{
74+
Secret: &troubleshootv1beta2.Secret{},
75+
},
76+
},
77+
list: []*troubleshootv1beta2.Collect{
78+
{
79+
Data: &troubleshootv1beta2.Data{},
80+
},
81+
{
82+
ClusterResources: &troubleshootv1beta2.ClusterResources{},
83+
},
84+
{
85+
Secret: &troubleshootv1beta2.Secret{},
86+
},
87+
{
88+
ClusterResources: &troubleshootv1beta2.ClusterResources{},
89+
},
90+
},
91+
},
92+
{
93+
name: "No clusterResources - no change",
94+
want: []*troubleshootv1beta2.Collect{
95+
{
96+
Data: &troubleshootv1beta2.Data{},
97+
},
98+
{
99+
Secret: &troubleshootv1beta2.Secret{},
100+
},
101+
},
102+
list: []*troubleshootv1beta2.Collect{
103+
{
104+
Data: &troubleshootv1beta2.Data{},
105+
},
106+
{
107+
Secret: &troubleshootv1beta2.Secret{},
108+
},
109+
},
110+
},
111+
{
112+
name: "Empty list - no change",
113+
want: []*troubleshootv1beta2.Collect{},
114+
list: []*troubleshootv1beta2.Collect{},
115+
},
116+
{
117+
name: "Only clusterResources - no change",
118+
want: []*troubleshootv1beta2.Collect{
119+
{
120+
ClusterResources: &troubleshootv1beta2.ClusterResources{},
121+
},
122+
},
123+
list: []*troubleshootv1beta2.Collect{
124+
{
125+
ClusterResources: &troubleshootv1beta2.ClusterResources{},
126+
},
127+
},
128+
},
36129
}
37130
for _, tc := range testCases {
38131
t.Run(tc.name, func(t *testing.T) {

‎pkg/preflight/collect.go‎

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -183,6 +183,7 @@ func CollectWithContext(ctx context.Context, opts CollectOpts, p *troubleshootv1
183183
}
184184

185185
allCollectorsMap := make(map[reflect.Type][]collect.Collector)
186+
collectorTypeOrder := make([]reflect.Type, 0) // Preserve order of collector types
186187
allCollectedData := make(map[string][]byte)
187188

188189
for _, desiredCollector := range collectSpecs {
@@ -193,14 +194,19 @@ func CollectWithContext(ctx context.Context, opts CollectOpts, p *troubleshootv1
193194
return nil, errors.Wrap(err, "failed to check RBAC for collectors")
194195
}
195196
collectorType := reflect.TypeOf(collector)
197+
if _, exists := allCollectorsMap[collectorType]; !exists {
198+
collectorTypeOrder = append(collectorTypeOrder, collectorType)
199+
}
196200
allCollectorsMap[collectorType] = append(allCollectorsMap[collectorType], collector)
197201
}
198202
}
199203
}
200204

201205
collectorList := map[string]CollectorStatus{}
202206

203-
for _, collectors := range allCollectorsMap {
207+
// Iterate over collector types in the order they appeared in collectSpecs
208+
for _, collectorType := range collectorTypeOrder {
209+
collectors := allCollectorsMap[collectorType]
204210
if mergeCollector, ok := collectors[0].(collect.MergeableCollector); ok {
205211
mergedCollectors, err := mergeCollector.Merge(collectors)
206212
if err != nil {

‎pkg/preflight/collect_test.go‎

Lines changed: 211 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,211 @@
1+
package preflight
2+
3+
import (
4+
"reflect"
5+
"testing"
6+
7+
troubleshootv1beta2 "github.com/replicatedhq/troubleshoot/pkg/apis/troubleshoot/v1beta2"
8+
"github.com/replicatedhq/troubleshoot/pkg/collect"
9+
"github.com/stretchr/testify/assert"
10+
"github.com/stretchr/testify/require"
11+
"k8s.io/client-go/kubernetes/fake"
12+
"k8s.io/client-go/rest"
13+
)
14+
15+
// TestCollectWithContext_ClusterResourcesFirst verifies that clusterResources
16+
// collector runs first, even when it's not first in the spec.
17+
func TestCollectWithContext_ClusterResourcesFirst(t *testing.T) {
18+
// Create a preflight spec with collectors in a specific order
19+
// where clusterResources is NOT first
20+
preflight := &troubleshootv1beta2.Preflight{
21+
Spec: troubleshootv1beta2.PreflightSpec{
22+
Collectors: []*troubleshootv1beta2.Collect{
23+
{
24+
Data: &troubleshootv1beta2.Data{
25+
CollectorMeta: troubleshootv1beta2.CollectorMeta{
26+
CollectorName: "test-data",
27+
},
28+
Name: "test.json",
29+
Data: `{"test": "data"}`,
30+
},
31+
},
32+
{
33+
ClusterResources: &troubleshootv1beta2.ClusterResources{},
34+
},
35+
},
36+
},
37+
}
38+
39+
// Use a fake Kubernetes client to avoid network calls
40+
fakeClient := fake.NewSimpleClientset()
41+
restConfig := &rest.Config{
42+
Host: "https://fake-host",
43+
}
44+
45+
opts := CollectOpts{
46+
Namespace: "default",
47+
KubernetesRestConfig: restConfig,
48+
ProgressChan: make(chan interface{}, 100),
49+
BundlePath: t.TempDir(),
50+
IgnorePermissionErrors: true, // Ignore RBAC errors in tests
51+
}
52+
53+
// Manually test the ordering logic by simulating what CollectWithContext does
54+
collectSpecs := make([]*troubleshootv1beta2.Collect, 0)
55+
if preflight.Spec.Collectors != nil {
56+
collectSpecs = append(collectSpecs, preflight.Spec.Collectors...)
57+
}
58+
collectSpecs = collect.EnsureCollectorInList(
59+
collectSpecs, troubleshootv1beta2.Collect{ClusterInfo: &troubleshootv1beta2.ClusterInfo{}},
60+
)
61+
collectSpecs = collect.EnsureCollectorInList(
62+
collectSpecs, troubleshootv1beta2.Collect{ClusterResources: &troubleshootv1beta2.ClusterResources{}},
63+
)
64+
collectSpecs = collect.DedupCollectors(collectSpecs)
65+
collectSpecs = collect.EnsureClusterResourcesFirst(collectSpecs)
66+
67+
// Verify clusterResources is first in the specs
68+
require.NotEmpty(t, collectSpecs, "should have collectors")
69+
require.NotNil(t, collectSpecs[0].ClusterResources, "first collector should be clusterResources")
70+
71+
// Now simulate the map grouping and order preservation
72+
allCollectorsMap := make(map[reflect.Type][]collect.Collector)
73+
collectorTypeOrder := make([]reflect.Type, 0)
74+
75+
for _, desiredCollector := range collectSpecs {
76+
if collectorInterface, ok := collect.GetCollector(desiredCollector, opts.BundlePath, opts.Namespace, opts.KubernetesRestConfig, fakeClient, nil); ok {
77+
if collector, ok := collectorInterface.(collect.Collector); ok {
78+
// Skip RBAC check for this unit test
79+
collectorType := reflect.TypeOf(collector)
80+
if _, exists := allCollectorsMap[collectorType]; !exists {
81+
collectorTypeOrder = append(collectorTypeOrder, collectorType)
82+
}
83+
allCollectorsMap[collectorType] = append(allCollectorsMap[collectorType], collector)
84+
}
85+
}
86+
}
87+
88+
// Verify that clusterResources type is first in the order
89+
require.NotEmpty(t, collectorTypeOrder, "should have collector types")
90+
91+
// Find the clusterResources type by checking the actual collectors
92+
var clusterResourcesType reflect.Type
93+
for collectorType, collectors := range allCollectorsMap {
94+
if len(collectors) > 0 {
95+
if _, ok := collectors[0].(*collect.CollectClusterResources); ok {
96+
clusterResourcesType = collectorType
97+
break
98+
}
99+
}
100+
}
101+
require.NotNil(t, clusterResourcesType, "should find clusterResources type")
102+
assert.Equal(t, clusterResourcesType, collectorTypeOrder[0], "clusterResources type should be first in collectorTypeOrder")
103+
}
104+
105+
// TestCollectWithContext_PreservesOrderAfterClusterResources verifies that
106+
// after clusterResources, other collectors maintain their relative order.
107+
func TestCollectWithContext_PreservesOrderAfterClusterResources(t *testing.T) {
108+
// Create a preflight spec with multiple collectors in a specific order
109+
preflight := &troubleshootv1beta2.Preflight{
110+
Spec: troubleshootv1beta2.PreflightSpec{
111+
Collectors: []*troubleshootv1beta2.Collect{
112+
{
113+
Data: &troubleshootv1beta2.Data{
114+
CollectorMeta: troubleshootv1beta2.CollectorMeta{
115+
CollectorName: "data-first",
116+
},
117+
Name: "first.json",
118+
Data: `{"first": "data"}`,
119+
},
120+
},
121+
{
122+
Secret: &troubleshootv1beta2.Secret{
123+
CollectorMeta: troubleshootv1beta2.CollectorMeta{
124+
CollectorName: "secret-second",
125+
},
126+
},
127+
},
128+
},
129+
},
130+
}
131+
132+
// Use a fake Kubernetes client
133+
fakeClient := fake.NewSimpleClientset()
134+
restConfig := &rest.Config{
135+
Host: "https://fake-host",
136+
}
137+
138+
opts := CollectOpts{
139+
Namespace: "default",
140+
KubernetesRestConfig: restConfig,
141+
ProgressChan: make(chan interface{}, 100),
142+
BundlePath: t.TempDir(),
143+
IgnorePermissionErrors: true,
144+
}
145+
146+
// Simulate the ordering logic
147+
collectSpecs := make([]*troubleshootv1beta2.Collect, 0)
148+
if preflight.Spec.Collectors != nil {
149+
collectSpecs = append(collectSpecs, preflight.Spec.Collectors...)
150+
}
151+
collectSpecs = collect.EnsureCollectorInList(
152+
collectSpecs, troubleshootv1beta2.Collect{ClusterInfo: &troubleshootv1beta2.ClusterInfo{}},
153+
)
154+
collectSpecs = collect.EnsureCollectorInList(
155+
collectSpecs, troubleshootv1beta2.Collect{ClusterResources: &troubleshootv1beta2.ClusterResources{}},
156+
)
157+
collectSpecs = collect.DedupCollectors(collectSpecs)
158+
collectSpecs = collect.EnsureClusterResourcesFirst(collectSpecs)
159+
160+
// Group collectors by type and track order
161+
allCollectorsMap := make(map[reflect.Type][]collect.Collector)
162+
collectorTypeOrder := make([]reflect.Type, 0)
163+
164+
for _, desiredCollector := range collectSpecs {
165+
if collectorInterface, ok := collect.GetCollector(desiredCollector, opts.BundlePath, opts.Namespace, opts.KubernetesRestConfig, fakeClient, nil); ok {
166+
if collector, ok := collectorInterface.(collect.Collector); ok {
167+
collectorType := reflect.TypeOf(collector)
168+
if _, exists := allCollectorsMap[collectorType]; !exists {
169+
collectorTypeOrder = append(collectorTypeOrder, collectorType)
170+
}
171+
allCollectorsMap[collectorType] = append(allCollectorsMap[collectorType], collector)
172+
}
173+
}
174+
}
175+
176+
// Verify clusterResources is first
177+
require.NotEmpty(t, collectorTypeOrder, "should have collector types")
178+
179+
// Find the actual types from the collectors
180+
var clusterResourcesType, dataType, secretType reflect.Type
181+
for collectorType, collectors := range allCollectorsMap {
182+
if len(collectors) > 0 {
183+
switch collectors[0].(type) {
184+
case *collect.CollectClusterResources:
185+
clusterResourcesType = collectorType
186+
case *collect.CollectData:
187+
dataType = collectorType
188+
case *collect.CollectSecret:
189+
secretType = collectorType
190+
}
191+
}
192+
}
193+
194+
require.NotNil(t, clusterResourcesType, "should find clusterResources type")
195+
assert.Equal(t, clusterResourcesType, collectorTypeOrder[0], "clusterResources should be first")
196+
197+
dataIndex := -1
198+
secretIndex := -1
199+
for i, ct := range collectorTypeOrder {
200+
if ct == dataType {
201+
dataIndex = i
202+
}
203+
if ct == secretType {
204+
secretIndex = i
205+
}
206+
}
207+
208+
if dataIndex >= 0 && secretIndex >= 0 {
209+
assert.Less(t, dataIndex, secretIndex, "data collectors should come before secret collectors, preserving relative order")
210+
}
211+
}

‎pkg/supportbundle/collect.go‎

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -106,6 +106,7 @@ func runCollectors(ctx context.Context, collectors []*troubleshootv1beta2.Collec
106106
}
107107

108108
allCollectorsMap := make(map[reflect.Type][]collect.Collector)
109+
collectorTypeOrder := make([]reflect.Type, 0) // Preserve order of collector types
109110
allCollectedData := make(map[string][]byte)
110111

111112
for _, desiredCollector := range collectSpecs {
@@ -116,12 +117,17 @@ func runCollectors(ctx context.Context, collectors []*troubleshootv1beta2.Collec
116117
return nil, errors.Wrap(err, "failed to check RBAC for collectors")
117118
}
118119
collectorType := reflect.TypeOf(collector)
120+
if _, exists := allCollectorsMap[collectorType]; !exists {
121+
collectorTypeOrder = append(collectorTypeOrder, collectorType)
122+
}
119123
allCollectorsMap[collectorType] = append(allCollectorsMap[collectorType], collector)
120124
}
121125
}
122126
}
123127

124-
for _, collectors := range allCollectorsMap {
128+
// Iterate over collector types in the order they appeared in collectSpecs
129+
for _, collectorType := range collectorTypeOrder {
130+
collectors := allCollectorsMap[collectorType]
125131
if mergeCollector, ok := collectors[0].(collect.MergeableCollector); ok {
126132
mergedCollectors, err := mergeCollector.Merge(collectors)
127133
if err != nil {

0 commit comments

Comments
 (0)