Skip to content
This repository was archived by the owner on Oct 22, 2024. It is now read-only.

Commit 9b17451

Browse files
avalluriavalluri
committed
Revert "test: catch unexpected modifications by operator"
This reverts commit 48a0250.
1 parent d97dbc2 commit 9b17451

File tree

4 files changed

+62
-127
lines changed

4 files changed

+62
-127
lines changed

‎pkg/pmem-csi-operator/controller/deployment/deployment_controller_test.go

+35-36
Original file line numberDiff line numberDiff line change
@@ -185,12 +185,11 @@ func TestDeploymentController(t *testing.T) {
185185

186186
testIt := func(t *testing.T, testK8sVersion version.Version) {
187187
type testContext struct {
188-
c client.Client
189-
cs kubernetes.Interface
190-
rc reconcile.Reconciler
191-
evWatcher watch.Interface
192-
events []*corev1.Event
193-
resourceVersions map[string]string
188+
c client.Client
189+
cs kubernetes.Interface
190+
rc reconcile.Reconciler
191+
evWatcher watch.Interface
192+
events []*corev1.Event
194193
}
195194
const (
196195
testNamespace = "test-namespace"
@@ -210,11 +209,9 @@ func TestDeploymentController(t *testing.T) {
210209
}
211210

212211
setup := func(t *testing.T) *testContext {
213-
tc := &testContext{
214-
c: fake.NewFakeClient(),
215-
cs: cgfake.NewSimpleClientset(),
216-
resourceVersions: map[string]string{},
217-
}
212+
tc := &testContext{}
213+
tc.c = fake.NewFakeClient()
214+
tc.cs = cgfake.NewSimpleClientset()
218215
tc.rc = newReconcileDeployment(tc.c, tc.cs)
219216
tc.evWatcher = tc.rc.(*deployment.ReconcileDeployment).EventBroadcaster().StartEventWatcher(func(ev *corev1.Event) {
220217
// Discard consecutive duplicate events, mimicking the EventAggregator behavior
@@ -248,20 +245,14 @@ func TestDeploymentController(t *testing.T) {
248245
require.ElementsMatch(t, events, expectedEvents, "events must match")
249246
}
250247

251-
validateDriver := func(t *testing.T, tc *testContext, dep *api.Deployment, expectedEvents []string, wasUpdated bool) {
248+
validateDriver := func(t *testing.T, tc *testContext, dep *api.Deployment, expectedEvents []string) {
252249
// We may have to fill in some defaults, so make a copy first.
253250
dep = dep.DeepCopyObject().(*api.Deployment)
254251
if dep.Spec.Image == "" {
255252
dep.Spec.Image = testDriverImage
256253
}
257254

258-
// If the CR was not updated, then objects should still be the same as they were initially.
259-
rv := tc.resourceVersions
260-
if wasUpdated {
261-
rv = nil
262-
}
263-
_, err := validate.DriverDeployment(tc.c, testK8sVersion, testNamespace, *dep, rv)
264-
require.NoError(t, err, "validate deployment")
255+
require.NoError(t, validate.DriverDeployment(tc.c, testK8sVersion, testNamespace, *dep), "validate deployment")
265256
validateEvents(t, tc, dep, expectedEvents)
266257
}
267258

@@ -280,7 +271,7 @@ func TestDeploymentController(t *testing.T) {
280271
require.NoError(t, err, "failed to create deployment")
281272

282273
testReconcilePhase(t, tc.rc, tc.c, d.name, false, false, api.DeploymentPhaseRunning)
283-
validateDriver(t, tc, dep, []string{api.EventReasonNew, api.EventReasonRunning}, false)
274+
validateDriver(t, tc, dep, []string{api.EventReasonNew, api.EventReasonRunning})
284275
})
285276

286277
t.Run("deployment with explicit values", func(t *testing.T) {
@@ -306,7 +297,7 @@ func TestDeploymentController(t *testing.T) {
306297

307298
// Reconcile now should change Phase to running
308299
testReconcilePhase(t, tc.rc, tc.c, d.name, false, false, api.DeploymentPhaseRunning)
309-
validateDriver(t, tc, dep, []string{api.EventReasonNew, api.EventReasonRunning}, false)
300+
validateDriver(t, tc, dep, []string{api.EventReasonNew, api.EventReasonRunning})
310301
})
311302

312303
t.Run("multiple deployments", func(t *testing.T) {
@@ -329,9 +320,9 @@ func TestDeploymentController(t *testing.T) {
329320
require.NoError(t, err, "failed to create deployment2")
330321

331322
testReconcilePhase(t, tc.rc, tc.c, d1.name, false, false, api.DeploymentPhaseRunning)
332-
validateDriver(t, tc, dep1, []string{api.EventReasonNew, api.EventReasonRunning}, false)
323+
validateDriver(t, tc, dep1, []string{api.EventReasonNew, api.EventReasonRunning})
333324
testReconcilePhase(t, tc.rc, tc.c, d2.name, false, false, api.DeploymentPhaseRunning)
334-
validateDriver(t, tc, dep2, []string{api.EventReasonNew, api.EventReasonRunning}, false)
325+
validateDriver(t, tc, dep2, []string{api.EventReasonNew, api.EventReasonRunning})
335326
})
336327

337328
t.Run("invalid device mode", func(t *testing.T) {
@@ -363,7 +354,7 @@ func TestDeploymentController(t *testing.T) {
363354
err := tc.c.Create(context.TODO(), dep)
364355
require.NoError(t, err, "failed to create deployment")
365356
testReconcilePhase(t, tc.rc, tc.c, d.name, false, false, api.DeploymentPhaseRunning)
366-
validateDriver(t, tc, dep, []string{api.EventReasonNew, api.EventReasonRunning}, false)
357+
validateDriver(t, tc, dep, []string{api.EventReasonNew, api.EventReasonRunning})
367358
})
368359

369360
t.Run("direct mode", func(t *testing.T) {
@@ -378,7 +369,7 @@ func TestDeploymentController(t *testing.T) {
378369
err := tc.c.Create(context.TODO(), dep)
379370
require.NoError(t, err, "failed to create deployment")
380371
testReconcilePhase(t, tc.rc, tc.c, d.name, false, false, api.DeploymentPhaseRunning)
381-
validateDriver(t, tc, dep, []string{api.EventReasonNew, api.EventReasonRunning}, false)
372+
validateDriver(t, tc, dep, []string{api.EventReasonNew, api.EventReasonRunning})
382373
})
383374

384375
t.Run("provided private keys", func(t *testing.T) {
@@ -400,7 +391,7 @@ func TestDeploymentController(t *testing.T) {
400391

401392
// First deployment expected to be successful
402393
testReconcilePhase(t, tc.rc, tc.c, d.name, false, false, api.DeploymentPhaseRunning)
403-
validateDriver(t, tc, dep, []string{api.EventReasonNew, api.EventReasonRunning}, false)
394+
validateDriver(t, tc, dep, []string{api.EventReasonNew, api.EventReasonRunning})
404395
})
405396

406397
t.Run("provided private keys and certificates", func(t *testing.T) {
@@ -433,7 +424,7 @@ func TestDeploymentController(t *testing.T) {
433424

434425
// First deployment expected to be successful
435426
testReconcilePhase(t, tc.rc, tc.c, d.name, false, false, api.DeploymentPhaseRunning)
436-
validateDriver(t, tc, dep, []string{api.EventReasonNew, api.EventReasonRunning}, false)
427+
validateDriver(t, tc, dep, []string{api.EventReasonNew, api.EventReasonRunning})
437428
})
438429

439430
t.Run("invalid private keys and certificates", func(t *testing.T) {
@@ -526,13 +517,13 @@ func TestDeploymentController(t *testing.T) {
526517
tc.rc.(*deployment.ReconcileDeployment).AddHook(&hook)
527518

528519
testReconcilePhase(t, tc.rc, tc.c, d.name, false, false, api.DeploymentPhaseRunning)
529-
validateDriver(t, tc, dep, []string{api.EventReasonNew, api.EventReasonRunning}, false)
520+
validateDriver(t, tc, dep, []string{api.EventReasonNew, api.EventReasonRunning})
530521

531522
tc.rc.(*deployment.ReconcileDeployment).RemoveHook(&hook)
532523

533524
// Next reconcile phase should catch the deployment changes
534525
testReconcilePhase(t, tc.rc, tc.c, d.name, false, false, api.DeploymentPhaseRunning)
535-
validateDriver(t, tc, updatedDep, []string{api.EventReasonNew, api.EventReasonRunning}, true)
526+
validateDriver(t, tc, updatedDep, []string{api.EventReasonNew, api.EventReasonRunning})
536527
})
537528

538529
t.Run("updating", func(t *testing.T) {
@@ -552,11 +543,10 @@ func TestDeploymentController(t *testing.T) {
552543
require.NoError(t, err, "create deployment")
553544

554545
testReconcilePhase(t, tc.rc, tc.c, dep.Name, false, false, api.DeploymentPhaseRunning)
555-
validateDriver(t, tc, dep, []string{api.EventReasonNew, api.EventReasonRunning}, false)
546+
validateDriver(t, tc, dep, []string{api.EventReasonNew, api.EventReasonRunning})
556547

557548
// Reconcile now should keep phase as running.
558549
testReconcilePhase(t, tc.rc, tc.c, dep.Name, false, false, api.DeploymentPhaseRunning)
559-
validateDriver(t, tc, dep, []string{api.EventReasonNew, api.EventReasonRunning}, false)
560550
validateEvents(t, tc, dep, []string{api.EventReasonNew, api.EventReasonRunning})
561551

562552
// Retrieve existing object before updating it.
@@ -577,7 +567,7 @@ func TestDeploymentController(t *testing.T) {
577567
testReconcilePhase(t, tc.rc, tc.c, dep.Name, false, false, api.DeploymentPhaseRunning)
578568

579569
// Recheck the container resources are updated
580-
validateDriver(t, tc, dep, []string{api.EventReasonNew, api.EventReasonRunning}, true)
570+
validateDriver(t, tc, dep, []string{api.EventReasonNew, api.EventReasonRunning})
581571
}
582572

583573
t.Run("while running", func(t *testing.T) {
@@ -603,7 +593,7 @@ func TestDeploymentController(t *testing.T) {
603593
err := tc.c.Create(context.TODO(), dep)
604594
require.NoError(t, err, "failed to create deployment")
605595
testReconcilePhase(t, tc.rc, tc.c, d.name, false, false, api.DeploymentPhaseRunning)
606-
validateDriver(t, tc, dep, []string{api.EventReasonNew, api.EventReasonRunning}, false)
596+
validateDriver(t, tc, dep, []string{api.EventReasonNew, api.EventReasonRunning})
607597

608598
err = tc.c.Get(context.TODO(), client.ObjectKey{Name: d.name}, dep)
609599
require.NoError(t, err, "get deployment")
@@ -639,16 +629,25 @@ func TestDeploymentController(t *testing.T) {
639629
err = tc.c.Create(context.TODO(), cm2)
640630
require.NoError(t, err, "create configmap: %s", cm2.Name)
641631

632+
defer func() {
633+
err := tc.c.Delete(context.TODO(), cm1)
634+
if err != nil && !errors.IsNotFound(err) {
635+
require.NoErrorf(t, err, "delete configmap: %s", cm1.Name)
636+
}
637+
err = tc.c.Delete(context.TODO(), cm2)
638+
if err != nil && !errors.IsNotFound(err) {
639+
require.NoErrorf(t, err, "delete configmap: %s", cm2.Name)
640+
}
641+
}()
642+
642643
// Use a fresh reconciler to mimic operator restart
643644
tc.rc = newReconcileDeployment(tc.c, tc.cs)
644645

645646
// A fresh reconcile should delete the newly created above ConfigMap
646647
testReconcilePhase(t, tc.rc, tc.c, d.name, false, false, api.DeploymentPhaseRunning)
647648
err = tc.c.Get(context.TODO(), client.ObjectKey{Name: d.name}, dep)
648649
require.NoError(t, err, "get deployment")
649-
// It is debatable whether the operator should update all objects after
650-
// a restart. Currently it does.
651-
validateDriver(t, tc, dep, []string{api.EventReasonNew, api.EventReasonRunning}, true)
650+
validateDriver(t, tc, dep, []string{api.EventReasonNew, api.EventReasonRunning})
652651

653652
cm := &corev1.ConfigMap{}
654653
err = tc.c.Get(context.TODO(), client.ObjectKey{Name: cm1.Name, Namespace: testNamespace}, cm)

‎test/e2e/operator/deployment_api.go

+12-27
Original file line numberDiff line numberDiff line change
@@ -95,15 +95,7 @@ var _ = deploy.DescribeForSome("API", func(d *deploy.Deployment) bool {
9595
if what == nil {
9696
what = []interface{}{"validate driver"}
9797
}
98-
99-
// We cannot check for unexpected object modifications
100-
// by the operator during E2E testing because the app
101-
// controllers themselves will also modify the same
102-
// objects with status changes. We can only test
103-
// that during unit testing.
104-
initialCreation := false
105-
106-
framework.ExpectNoErrorWithOffset(1, validate.DriverDeploymentEventually(ctx, client, k8sver, d.Namespace, deployment, initialCreation), what...)
98+
framework.ExpectNoErrorWithOffset(1, validate.DriverDeploymentEventually(ctx, client, k8sver, d.Namespace, deployment), what...)
10799
framework.Logf("got expected driver deployment %s", deployment.Name)
108100
}
109101

@@ -205,7 +197,7 @@ var _ = deploy.DescribeForSome("API", func(d *deploy.Deployment) bool {
205197

206198
deployment2 = deploy.CreateDeploymentCR(f, deployment2)
207199
defer deploy.DeleteDeploymentCR(f, deployment2.Name)
208-
validateDriver(deployment1, true /* TODO 2 */, "validate driver #2")
200+
validateDriver(deployment1 /* TODO 2 */, "validate driver #2")
209201
})
210202

211203
It("shall support dots in the name", func() {
@@ -222,7 +214,7 @@ var _ = deploy.DescribeForSome("API", func(d *deploy.Deployment) bool {
222214

223215
deployment = deploy.CreateDeploymentCR(f, deployment)
224216
defer deploy.DeleteDeploymentCR(f, deployment.Name)
225-
validateDriver(deployment, true)
217+
validateDriver(deployment)
226218
})
227219

228220
It("driver deployment shall be running even after operator exit", func() {
@@ -231,21 +223,16 @@ var _ = deploy.DescribeForSome("API", func(d *deploy.Deployment) bool {
231223
deployment = deploy.CreateDeploymentCR(f, deployment)
232224

233225
defer deploy.DeleteDeploymentCR(f, deployment.Name)
234-
validateDriver(deployment, true)
226+
validateDriver(deployment)
235227

236228
// Stop the operator
237229
stopOperator(c, d)
238230
// restore the deployment so that next test should not effect
239231
defer startOperator(c, d)
240232

241233
// Ensure that the driver is running consistently
242-
resourceVersions := map[string]string{}
243234
Consistently(func() error {
244-
final, err := validate.DriverDeployment(client, k8sver, d.Namespace, deployment, resourceVersions)
245-
if final {
246-
framework.Failf("final error during driver validation after restarting: %v", err)
247-
}
248-
return err
235+
return validate.DriverDeployment(client, k8sver, d.Namespace, deployment)
249236
}, "1m", "20s").ShouldNot(HaveOccurred(), "driver validation failure after restarting")
250237
})
251238

@@ -296,7 +283,7 @@ var _ = deploy.DescribeForSome("API", func(d *deploy.Deployment) bool {
296283

297284
// Deleting the existing secret should make the deployment succeed.
298285
deleteSecret(sec.Name)
299-
validateDriver(deployment, true)
286+
validateDriver(deployment)
300287
})
301288
})
302289

@@ -391,7 +378,7 @@ var _ = deploy.DescribeForSome("API", func(d *deploy.Deployment) bool {
391378
deployment = deploy.CreateDeploymentCR(f, deployment)
392379
defer deploy.DeleteDeploymentCR(f, deployment.Name)
393380
deploy.WaitForPMEMDriver(c, deployment.Name, corev1.NamespaceDefault, false /* testing */)
394-
validateDriver(deployment, true)
381+
validateDriver(deployment)
395382

396383
// NOTE(avalluri): As the current operator does not support deploying
397384
// the driver in 'testing' mode, we cannot directely access CSI
@@ -447,10 +434,8 @@ var _ = deploy.DescribeForSome("API", func(d *deploy.Deployment) bool {
447434
validateDriver(deployment, "validate driver before update")
448435

449436
// We have to get a fresh copy before updating it because the
450-
// operator should have modified the status, and only the status.
451-
modifiedDeployment := deploy.GetDeploymentCR(f, deployment.Name)
452-
Expect(modifiedDeployment.Spec).To(Equal(deployment.Spec), "spec unmodified")
453-
Expect(modifiedDeployment.Status.Phase).To(Equal(api.DeploymentPhaseRunning), "deployment phase")
437+
// operator should have modified the status.
438+
deployment = deploy.GetDeploymentCR(f, deployment.Name)
454439

455440
restored := false
456441
if restart {
@@ -462,15 +447,15 @@ var _ = deploy.DescribeForSome("API", func(d *deploy.Deployment) bool {
462447
}()
463448
}
464449

465-
testcase.Mutate(&modifiedDeployment)
466-
deployment = deploy.UpdateDeploymentCR(f, modifiedDeployment)
450+
testcase.Mutate(&deployment)
451+
deployment = deploy.UpdateDeploymentCR(f, deployment)
467452

468453
if restart {
469454
startOperator(c, d)
470455
restored = true
471456
}
472457

473-
validateDriver(modifiedDeployment, "validate driver after update and restart")
458+
validateDriver(deployment, "validate driver after update and restart")
474459
}
475460

476461
It("while running", func() {

‎test/e2e/operator/driver.go

+1-1
Original file line numberDiff line numberDiff line change
@@ -49,7 +49,7 @@ var _ = deploy.DescribeForSome("driver", func(d *deploy.Deployment) bool {
4949
deployment := d.GetDriverDeployment()
5050
deployment = deploy.GetDeploymentCR(f, deployment.Name)
5151

52-
framework.ExpectNoError(validate.DriverDeploymentEventually(ctx, client, *k8sver, d.Namespace, deployment, true), "validate driver")
52+
framework.ExpectNoError(validate.DriverDeploymentEventually(ctx, client, *k8sver, d.Namespace, deployment), "validate driver")
5353
})
5454

5555
// Just very minimal testing at the moment.

0 commit comments

Comments
 (0)