Skip to content

Commit da9ed93

Browse files
authored
Merge pull request #991 from rabbitmq/flake-hunting
Fix flakes in integration suite
2 parents 342851f + 1bcc485 commit da9ed93

13 files changed

+668
-214
lines changed

Makefile

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -54,6 +54,7 @@ export KUBEBUILDER_ASSETS = $(LOCAL_TESTBIN)/k8s/$(ENVTEST_K8S_VERSION)-$(platfo
5454

5555
.PHONY: kubebuilder-assets
5656
kubebuilder-assets: $(KUBEBUILDER_ASSETS)
57+
@echo "export KUBEBUILDER_ASSETS = $(LOCAL_TESTBIN)/k8s/$(ENVTEST_K8S_VERSION)-$(platform)-$(ARCHITECTURE)"
5758

5859
$(KUBEBUILDER_ASSETS):
5960
setup-envtest --os $(platform) --arch $(ARCHITECTURE) --bin-dir $(LOCAL_TESTBIN) use $(ENVTEST_K8S_VERSION)
@@ -73,6 +74,8 @@ $(YTT): | $(LOCAL_BIN)
7374
##############
7475
#### Tests ###
7576
##############
77+
GINKGO := go run github.com/onsi/ginkgo/v2/ginkgo
78+
7679
.PHONY: unit-tests
7780
unit-tests::install-tools ## Run unit tests
7881
unit-tests::$(KUBEBUILDER_ASSETS)
@@ -84,7 +87,7 @@ unit-tests::just-unit-tests
8487

8588
.PHONY: just-unit-tests
8689
just-unit-tests:
87-
ginkgo -r --randomize-all api/ internal/ rabbitmqclient/
90+
$(GINKGO) -r --randomize-all api/ internal/ rabbitmqclient/
8891

8992
.PHONY: integration-tests
9093
integration-tests::install-tools ## Run integration tests. Use GINKGO_EXTRA="-some-arg" to append arguments to 'ginkgo run'
@@ -96,14 +99,14 @@ integration-tests::manifests
9699
integration-tests::just-integration-tests
97100

98101
just-integration-tests: $(KUBEBUILDER_ASSETS)
99-
ginkgo --randomize-all -r -p $(GINKGO_EXTRA) controllers/
102+
$(GINKGO) --randomize-all -r -p $(GINKGO_EXTRA) controllers/
100103

101104
.PHONY: local-tests
102105
local-tests: unit-tests integration-tests ## Run all local tests (unit & integration)
103106

104107
.PHONY: system-tests
105108
system-tests: ## Run E2E tests using current context in ~/.kube/config. Expects cluster operator and topology operator to be installed in the cluster
106-
NAMESPACE="rabbitmq-system" ginkgo --randomize-all -r $(GINKGO_EXTRA) system_tests/
109+
NAMESPACE="rabbitmq-system" $(GINKGO) --randomize-all -r $(GINKGO_EXTRA) system_tests/
107110

108111

109112
###################

controllers/binding_controller_test.go

Lines changed: 47 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -4,8 +4,11 @@ import (
44
"bytes"
55
"context"
66
"errors"
7+
"fmt"
8+
"github.com/rabbitmq/cluster-operator/v2/api/v1beta1"
79
"github.com/rabbitmq/messaging-topology-operator/controllers"
810
"io"
11+
"k8s.io/apimachinery/pkg/labels"
912
"net/http"
1013
ctrl "sigs.k8s.io/controller-runtime"
1114
"sigs.k8s.io/controller-runtime/pkg/cache"
@@ -34,14 +37,28 @@ var _ = Describe("bindingController", func() {
3437
k8sClient runtimeClient.Client
3538
)
3639

37-
BeforeEach(func() {
40+
initialiseManager := func(keyValPair ...string) {
41+
var sel labels.Selector
42+
if len(keyValPair) == 2 {
43+
var err error
44+
sel, err = labels.Parse(fmt.Sprintf("%s == %s", keyValPair[0], keyValPair[1]))
45+
Expect(err).NotTo(HaveOccurred())
46+
}
47+
3848
var err error
3949
bindingMgr, err = ctrl.NewManager(testEnv.Config, ctrl.Options{
4050
Metrics: server.Options{
4151
BindAddress: "0", // To avoid MacOS firewall pop-up every time you run this suite
4252
},
4353
Cache: cache.Options{
44-
DefaultNamespaces: map[string]cache.Config{bindingNamespace: {}},
54+
DefaultNamespaces: map[string]cache.Config{bindingNamespace: {
55+
LabelSelector: sel,
56+
}},
57+
ByObject: map[runtimeClient.Object]cache.ByObject{
58+
&v1beta1.RabbitmqCluster{}: {Namespaces: map[string]cache.Config{cache.AllNamespaces: {}}},
59+
&corev1.Secret{}: {Namespaces: map[string]cache.Config{cache.AllNamespaces: {}}},
60+
&corev1.Service{}: {Namespaces: map[string]cache.Config{cache.AllNamespaces: {}}},
61+
},
4562
},
4663
Logger: GinkgoLogr,
4764
Controller: config.Controller{
@@ -66,21 +83,9 @@ var _ = Describe("bindingController", func() {
6683
RabbitmqClientFactory: fakeRabbitMQClientFactory,
6784
ReconcileFunc: &controllers.BindingReconciler{},
6885
}).SetupWithManager(bindingMgr)).To(Succeed())
69-
})
86+
}
7087

71-
AfterEach(func() {
72-
managerCancel()
73-
// Sad workaround to avoid controllers racing for the reconciliation of other's
74-
// test cases. Without this wait, the last run test consistently fails because
75-
// the previous cancelled manager is just in time to reconcile the Queue of the
76-
// new/last test, and use the wrong/unexpected arguments in the queue declare call
77-
//
78-
// Eventual consistency is nice when you have good means of awaiting. That's not the
79-
// case with testenv and kubernetes controllers.
80-
<-time.After(time.Second)
81-
})
82-
83-
JustBeforeEach(func() {
88+
initialiseBinding := func() {
8489
binding = topology.Binding{
8590
ObjectMeta: metav1.ObjectMeta{
8691
Name: bindingName,
@@ -92,6 +97,18 @@ var _ = Describe("bindingController", func() {
9297
},
9398
},
9499
}
100+
}
101+
102+
AfterEach(func() {
103+
managerCancel()
104+
// Sad workaround to avoid controllers racing for the reconciliation of other's
105+
// test cases. Without this wait, the last run test consistently fails because
106+
// the previous cancelled manager is just in time to reconcile the Queue of the
107+
// new/last test, and use the wrong/unexpected arguments in the queue declare call
108+
//
109+
// Eventual consistency is nice when you have good means of awaiting. That's not the
110+
// case with testenv and kubernetes controllers.
111+
<-time.After(time.Second)
95112
})
96113

97114
When("creating a binding", func() {
@@ -106,6 +123,9 @@ var _ = Describe("bindingController", func() {
106123
Status: "418 I'm a teapot",
107124
StatusCode: 418,
108125
}, errors.New("some HTTP error"))
126+
initialiseBinding()
127+
binding.Labels = map[string]string{"test": "test-binding-http-error"}
128+
initialiseManager("test", "test-binding-http-error")
109129
})
110130

111131
It("sets the status condition to indicate a failure to reconcile", func() {
@@ -131,6 +151,9 @@ var _ = Describe("bindingController", func() {
131151
BeforeEach(func() {
132152
bindingName = "test-binding-go-error"
133153
fakeRabbitMQClient.DeclareBindingReturns(nil, errors.New("hit a exception"))
154+
initialiseBinding()
155+
binding.Labels = map[string]string{"test": "test-binding-go-error"}
156+
initialiseManager("test", "test-binding-go-error")
134157
})
135158

136159
It("sets the status condition to indicate a failure to reconcile", func() {
@@ -155,6 +178,8 @@ var _ = Describe("bindingController", func() {
155178

156179
When("Deleting a binding", func() {
157180
JustBeforeEach(func() {
181+
// Must use a JustBeforeEach to extract this common behaviour
182+
// JustBeforeEach runs AFTER all BeforeEach have completed
158183
fakeRabbitMQClient.DeclareBindingReturns(&http.Response{
159184
Status: "201 Created",
160185
StatusCode: http.StatusCreated,
@@ -183,6 +208,9 @@ var _ = Describe("bindingController", func() {
183208
StatusCode: http.StatusBadGateway,
184209
Body: io.NopCloser(bytes.NewBufferString("Hello World")),
185210
}, nil)
211+
initialiseBinding()
212+
binding.Labels = map[string]string{"test": "delete-binding-http-error"}
213+
initialiseManager("test", "delete-binding-http-error")
186214
})
187215

188216
It("raises an event to indicate a failure to delete", func() {
@@ -199,6 +227,9 @@ var _ = Describe("bindingController", func() {
199227
BeforeEach(func() {
200228
bindingName = "delete-binding-go-error"
201229
fakeRabbitMQClient.DeleteBindingReturns(nil, errors.New("some error"))
230+
initialiseBinding()
231+
binding.Labels = map[string]string{"test": "delete-binding-go-error"}
232+
initialiseManager("test", "delete-binding-go-error")
202233
})
203234

204235
It("raises an event to indicate a failure to delete", func() {

controllers/exchange_controller_test.go

Lines changed: 53 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,10 @@ import (
44
"bytes"
55
"context"
66
"errors"
7+
"fmt"
8+
"github.com/rabbitmq/cluster-operator/v2/api/v1beta1"
79
"io"
10+
"k8s.io/apimachinery/pkg/labels"
811
"net/http"
912
"time"
1013

@@ -35,14 +38,28 @@ var _ = Describe("exchange-controller", func() {
3538
k8sClient runtimeClient.Client
3639
)
3740

38-
BeforeEach(func() {
41+
initialiseManager := func(keyValPair ...string) {
42+
var sel labels.Selector
43+
if len(keyValPair) == 2 {
44+
var err error
45+
sel, err = labels.Parse(fmt.Sprintf("%s == %s", keyValPair[0], keyValPair[1]))
46+
Expect(err).NotTo(HaveOccurred())
47+
}
48+
3949
var err error
4050
exchangeMgr, err = ctrl.NewManager(testEnv.Config, ctrl.Options{
4151
Metrics: server.Options{
4252
BindAddress: "0", // To avoid MacOS firewall pop-up every time you run this suite
4353
},
4454
Cache: cache.Options{
45-
DefaultNamespaces: map[string]cache.Config{exchangeNamespace: {}},
55+
DefaultNamespaces: map[string]cache.Config{exchangeNamespace: {
56+
LabelSelector: sel,
57+
}},
58+
ByObject: map[runtimeClient.Object]cache.ByObject{
59+
&v1beta1.RabbitmqCluster{}: {Namespaces: map[string]cache.Config{cache.AllNamespaces: {}}},
60+
&corev1.Secret{}: {Namespaces: map[string]cache.Config{cache.AllNamespaces: {}}},
61+
&corev1.Service{}: {Namespaces: map[string]cache.Config{cache.AllNamespaces: {}}},
62+
},
4663
},
4764
Logger: GinkgoLogr,
4865
Controller: config.Controller{
@@ -67,22 +84,9 @@ var _ = Describe("exchange-controller", func() {
6784
RabbitmqClientFactory: fakeRabbitMQClientFactory,
6885
ReconcileFunc: &controllers.ExchangeReconciler{},
6986
}).SetupWithManager(exchangeMgr)).To(Succeed())
70-
})
87+
}
7188

72-
AfterEach(func() {
73-
managerCancel()
74-
// Sad workaround to avoid controllers racing for the reconciliation of other's
75-
// test cases. Without this wait, the last run test consistently fails because
76-
// the previous cancelled manager is just in time to reconcile the Queue of the
77-
// new/last test, and use the wrong/unexpected arguments in the queue declare call
78-
//
79-
// Eventual consistency is nice when you have good means of awaiting. That's not the
80-
// case with testenv and kubernetes controllers.
81-
<-time.After(time.Second)
82-
})
83-
84-
JustBeforeEach(func() {
85-
// this will be executed after all BeforeEach have run
89+
initialiseExchange := func() {
8690
exchange = topology.Exchange{
8791
ObjectMeta: metav1.ObjectMeta{
8892
Name: exchangeName,
@@ -94,6 +98,18 @@ var _ = Describe("exchange-controller", func() {
9498
},
9599
},
96100
}
101+
}
102+
103+
AfterEach(func() {
104+
managerCancel()
105+
// Sad workaround to avoid controllers racing for the reconciliation of other's
106+
// test cases. Without this wait, the last run test consistently fails because
107+
// the previous cancelled manager is just in time to reconcile the Queue of the
108+
// new/last test, and use the wrong/unexpected arguments in the queue declare call
109+
//
110+
// Eventual consistency is nice when you have good means of awaiting. That's not the
111+
// case with testenv and kubernetes controllers.
112+
<-time.After(time.Second)
97113
})
98114

99115
Context("creation", func() {
@@ -108,6 +124,9 @@ var _ = Describe("exchange-controller", func() {
108124
Status: "418 I'm a teapot",
109125
StatusCode: 418,
110126
}, errors.New("a failure"))
127+
initialiseExchange()
128+
exchange.Labels = map[string]string{"test": "test-http-error"}
129+
initialiseManager("test", "test-http-error")
111130
})
112131

113132
It("sets the status condition", func() {
@@ -133,6 +152,9 @@ var _ = Describe("exchange-controller", func() {
133152
BeforeEach(func() {
134153
exchangeName = "test-go-error"
135154
fakeRabbitMQClient.DeclareExchangeReturns(nil, errors.New("a go failure"))
155+
initialiseExchange()
156+
exchange.Labels = map[string]string{"test": "test-go-error"}
157+
initialiseManager("test", "test-go-error")
136158
})
137159

138160
It("sets the status condition to indicate a failure to reconcile", func() {
@@ -162,8 +184,12 @@ var _ = Describe("exchange-controller", func() {
162184
Status: "201 Created",
163185
StatusCode: http.StatusCreated,
164186
}, nil)
187+
initialiseExchange()
188+
exchange.Labels = map[string]string{"test": "test-last-transition-time"}
189+
initialiseManager("test", "test-last-transition-time")
165190
})
166191

192+
// TODO maybe this is a problem because the delete function does not have a fakeClient prepared to return OK for Delete requests
167193
AfterEach(func() {
168194
Expect(k8sClient.Delete(ctx, &exchange)).To(Succeed())
169195
})
@@ -190,7 +216,7 @@ var _ = Describe("exchange-controller", func() {
190216
Status: "204 No Content",
191217
StatusCode: http.StatusNoContent,
192218
}, nil)
193-
exchange.Labels = map[string]string{"k1": "v1"}
219+
exchange.Labels["k1"] = "v1"
194220
Expect(k8sClient.Update(ctx, &exchange)).To(Succeed())
195221
ConsistentlyWithOffset(1, func() []topology.Condition {
196222
_ = k8sClient.Get(
@@ -210,7 +236,7 @@ var _ = Describe("exchange-controller", func() {
210236
Status: "500 Internal Server Error",
211237
StatusCode: http.StatusInternalServerError,
212238
}, errors.New("something went wrong"))
213-
exchange.Labels = map[string]string{"k1": "v2"}
239+
exchange.Labels["k1"] = "v2"
214240
Expect(k8sClient.Update(ctx, &exchange)).To(Succeed())
215241
EventuallyWithOffset(1, func() []topology.Condition {
216242
_ = k8sClient.Get(
@@ -231,6 +257,8 @@ var _ = Describe("exchange-controller", func() {
231257

232258
Context("deletion", func() {
233259
JustBeforeEach(func() {
260+
// Must use a JustBeforeEach to extract this common behaviour
261+
// JustBeforeEach runs AFTER all BeforeEach have completed
234262
fakeRabbitMQClient.DeclareExchangeReturns(&http.Response{
235263
Status: "201 Created",
236264
StatusCode: http.StatusCreated,
@@ -259,6 +287,9 @@ var _ = Describe("exchange-controller", func() {
259287
StatusCode: http.StatusBadGateway,
260288
Body: io.NopCloser(bytes.NewBufferString("Hello World")),
261289
}, nil)
290+
initialiseExchange()
291+
exchange.Labels = map[string]string{"test": "delete-exchange-http-error"}
292+
initialiseManager("test", "delete-exchange-http-error")
262293
})
263294

264295
It("publishes a 'warning' event", func() {
@@ -275,6 +306,9 @@ var _ = Describe("exchange-controller", func() {
275306
BeforeEach(func() {
276307
exchangeName = "delete-go-error"
277308
fakeRabbitMQClient.DeleteExchangeReturns(nil, errors.New("some error"))
309+
initialiseExchange()
310+
exchange.Labels = map[string]string{"test": "delete-go-error"}
311+
initialiseManager("test", "delete-go-error")
278312
})
279313

280314
It("publishes a 'warning' event", func() {

0 commit comments

Comments
 (0)