Skip to content

Commit

Permalink
test: catch unexpected modifications by operator
Browse files Browse the repository at this point in the history 
The operator had a bug where its change detection caused it to update
objects unnecessarily. This wasn't caught before by the tests because
the modified objects still had the expected content (as far as we
know, at least - intel#742 still
lacks a proper explanation).

Now the update unit test catches that bug:

    TestDeploymentController/Kubernetes_1.18/updating/pmemPercentage_in_default_deployment: deployment_controller_test.go:264:
        	Error Trace:	deployment_controller_test.go:264
        	            				deployment_controller_test.go:559
        	            				deployment_controller_test.go:584
        	Error:      	Received unexpected error:
        	            	deployed driver different from expected deployment:
        	            	object was modified unnecessarily: "pmem-csi-with-defaults-node" of type "apps/v1, Kind=DaemonSet" in namespace "test-namespace"
        	            	object was modified unnecessarily: "pmem-csi-with-defaults-controller" of type "apps/v1, Kind=StatefulSet" in namespace "test-namespace"
        	Test:       	TestDeploymentController/Kubernetes_1.18/updating/pmemPercentage_in_default_deployment
        	Messages:   	validate deployment

We cannot use the same validation during E2E testing because the app
controllers also modify the objects by setting their status.
  • Loading branch information
@pohly
pohly committed Sep 24, 2020
1 parent 9d3db60 commit 48a0250
Show file tree
Hide file tree
Showing 4 changed files with 127 additions and 62 deletions.
71 changes: 36 additions & 35 deletions pkg/pmem-csi-operator/controller/deployment/deployment_controller_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -185,11 +185,12 @@ func TestDeploymentController(t *testing.T) {

testIt := func(t *testing.T, testK8sVersion version.Version) {
type testContext struct {
c client.Client
cs kubernetes.Interface
rc reconcile.Reconciler
evWatcher watch.Interface
events []*corev1.Event
c client.Client
cs kubernetes.Interface
rc reconcile.Reconciler
evWatcher watch.Interface
events []*corev1.Event
resourceVersions map[string]string
}
const (
testNamespace = "test-namespace"
Expand All @@ -209,9 +210,11 @@ func TestDeploymentController(t *testing.T) {
}

setup := func(t *testing.T) *testContext {
tc := &testContext{}
tc.c = fake.NewFakeClient()
tc.cs = cgfake.NewSimpleClientset()
tc := &testContext{
c: fake.NewFakeClient(),
cs: cgfake.NewSimpleClientset(),
resourceVersions: map[string]string{},
}
tc.rc = newReconcileDeployment(tc.c, tc.cs)
tc.evWatcher = tc.rc.(*deployment.ReconcileDeployment).EventBroadcaster().StartEventWatcher(func(ev *corev1.Event) {
// Discard consecutive duplicate events, mimicking the EventAggregator behavior
Expand Down Expand Up @@ -245,14 +248,20 @@ func TestDeploymentController(t *testing.T) {
require.ElementsMatch(t, events, expectedEvents, "events must match")
}

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

require.NoError(t, validate.DriverDeployment(tc.c, testK8sVersion, testNamespace, *dep), "validate deployment")
// If the CR was not updated, then objects should still be the same as they were initially.
rv := tc.resourceVersions
if wasUpdated {
rv = nil
}
_, err := validate.DriverDeployment(tc.c, testK8sVersion, testNamespace, *dep, rv)
require.NoError(t, err, "validate deployment")
validateEvents(t, tc, dep, expectedEvents)
}

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

testReconcilePhase(t, tc.rc, tc.c, d.name, false, false, api.DeploymentPhaseRunning)
validateDriver(t, tc, dep, []string{api.EventReasonNew, api.EventReasonRunning})
validateDriver(t, tc, dep, []string{api.EventReasonNew, api.EventReasonRunning}, false)
})

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

// Reconcile now should change Phase to running
testReconcilePhase(t, tc.rc, tc.c, d.name, false, false, api.DeploymentPhaseRunning)
validateDriver(t, tc, dep, []string{api.EventReasonNew, api.EventReasonRunning})
validateDriver(t, tc, dep, []string{api.EventReasonNew, api.EventReasonRunning}, false)
})

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

testReconcilePhase(t, tc.rc, tc.c, d1.name, false, false, api.DeploymentPhaseRunning)
validateDriver(t, tc, dep1, []string{api.EventReasonNew, api.EventReasonRunning})
validateDriver(t, tc, dep1, []string{api.EventReasonNew, api.EventReasonRunning}, false)
testReconcilePhase(t, tc.rc, tc.c, d2.name, false, false, api.DeploymentPhaseRunning)
validateDriver(t, tc, dep2, []string{api.EventReasonNew, api.EventReasonRunning})
validateDriver(t, tc, dep2, []string{api.EventReasonNew, api.EventReasonRunning}, false)
})

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

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

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

// First deployment expected to be successful
testReconcilePhase(t, tc.rc, tc.c, d.name, false, false, api.DeploymentPhaseRunning)
validateDriver(t, tc, dep, []string{api.EventReasonNew, api.EventReasonRunning})
validateDriver(t, tc, dep, []string{api.EventReasonNew, api.EventReasonRunning}, false)
})

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

// First deployment expected to be successful
testReconcilePhase(t, tc.rc, tc.c, d.name, false, false, api.DeploymentPhaseRunning)
validateDriver(t, tc, dep, []string{api.EventReasonNew, api.EventReasonRunning})
validateDriver(t, tc, dep, []string{api.EventReasonNew, api.EventReasonRunning}, false)
})

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

testReconcilePhase(t, tc.rc, tc.c, d.name, false, false, api.DeploymentPhaseRunning)
validateDriver(t, tc, dep, []string{api.EventReasonNew, api.EventReasonRunning})
validateDriver(t, tc, dep, []string{api.EventReasonNew, api.EventReasonRunning}, false)

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

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

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

testReconcilePhase(t, tc.rc, tc.c, dep.Name, false, false, api.DeploymentPhaseRunning)
validateDriver(t, tc, dep, []string{api.EventReasonNew, api.EventReasonRunning})
validateDriver(t, tc, dep, []string{api.EventReasonNew, api.EventReasonRunning}, false)

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

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

// Recheck the container resources are updated
validateDriver(t, tc, dep, []string{api.EventReasonNew, api.EventReasonRunning})
validateDriver(t, tc, dep, []string{api.EventReasonNew, api.EventReasonRunning}, true)
}

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

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

defer func() {
err := tc.c.Delete(context.TODO(), cm1)
if err != nil && !errors.IsNotFound(err) {
require.NoErrorf(t, err, "delete configmap: %s", cm1.Name)
}
err = tc.c.Delete(context.TODO(), cm2)
if err != nil && !errors.IsNotFound(err) {
require.NoErrorf(t, err, "delete configmap: %s", cm2.Name)
}
}()

// Use a fresh reconciler to mimic operator restart
tc.rc = newReconcileDeployment(tc.c, tc.cs)

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

cm := &corev1.ConfigMap{}
err = tc.c.Get(context.TODO(), client.ObjectKey{Name: cm1.Name, Namespace: testNamespace}, cm)
Expand Down
39 changes: 27 additions & 12 deletions test/e2e/operator/deployment_api.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,15 @@ var _ = deploy.DescribeForSome("API", func(d *deploy.Deployment) bool {
if what == nil {
what = []interface{}{"validate driver"}
}
framework.ExpectNoErrorWithOffset(1, validate.DriverDeploymentEventually(ctx, client, k8sver, d.Namespace, deployment), what...)

// We cannot check for unexpected object modifications
// by the operator during E2E testing because the app
// controllers themselves will also modify the same
// objects with status changes. We can only test
// that during unit testing.
initialCreation := false

framework.ExpectNoErrorWithOffset(1, validate.DriverDeploymentEventually(ctx, client, k8sver, d.Namespace, deployment, initialCreation), what...)
framework.Logf("got expected driver deployment %s", deployment.Name)
}

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

deployment2 = deploy.CreateDeploymentCR(f, deployment2)
defer deploy.DeleteDeploymentCR(f, deployment2.Name)
validateDriver(deployment1 /* TODO 2 */, "validate driver #2")
validateDriver(deployment1, true /* TODO 2 */, "validate driver #2")
})

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

deployment = deploy.CreateDeploymentCR(f, deployment)
defer deploy.DeleteDeploymentCR(f, deployment.Name)
validateDriver(deployment)
validateDriver(deployment, true)
})

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

defer deploy.DeleteDeploymentCR(f, deployment.Name)
validateDriver(deployment)
validateDriver(deployment, true)

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

// Ensure that the driver is running consistently
resourceVersions := map[string]string{}
Consistently(func() error {
return validate.DriverDeployment(client, k8sver, d.Namespace, deployment)
final, err := validate.DriverDeployment(client, k8sver, d.Namespace, deployment, resourceVersions)
if final {
framework.Failf("final error during driver validation after restarting: %v", err)
}
return err
}, "1m", "20s").ShouldNot(HaveOccurred(), "driver validation failure after restarting")
})

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

// Deleting the existing secret should make the deployment succeed.
deleteSecret(sec.Name)
validateDriver(deployment)
validateDriver(deployment, true)
})
})

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

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

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

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

testcase.Mutate(&deployment)
deployment = deploy.UpdateDeploymentCR(f, deployment)
testcase.Mutate(&modifiedDeployment)
deployment = deploy.UpdateDeploymentCR(f, modifiedDeployment)

if restart {
startOperator(c, d)
restored = true
}

validateDriver(deployment, "validate driver after update and restart")
validateDriver(modifiedDeployment, "validate driver after update and restart")
}

It("while running", func() {
Expand Down
2 changes: 1 addition & 1 deletion test/e2e/operator/driver.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ var _ = deploy.DescribeForSome("driver", func(d *deploy.Deployment) bool {
deployment := d.GetDriverDeployment()
deployment = deploy.GetDeploymentCR(f, deployment.Name)

framework.ExpectNoError(validate.DriverDeploymentEventually(ctx, client, *k8sver, d.Namespace, deployment), "validate driver")
framework.ExpectNoError(validate.DriverDeploymentEventually(ctx, client, *k8sver, d.Namespace, deployment, true), "validate driver")
})

// Just very minimal testing at the moment.
Expand Down
Loading

0 comments on commit 48a0250

Please sign in to comment.