From 2d43e4549e76ab1444f6febbecdd159d3dc0c40b Mon Sep 17 00:00:00 2001
From: Jan Safranek <jsafrane@redhat.com>
Date: Wed, 1 Jun 2016 08:35:33 +0200
Subject: [PATCH] Fix data race in volume controller unit test.

Reactor must be locked when fiddling with reactor.volumes and reactor.claims.
Therefore add new functions to add/delete volume/claim with sending an event.
---
 .../persistentvolume/controller_test.go       | 22 +-------
 .../persistentvolume/framework_test.go        | 56 +++++++++++++++++++
 2 files changed, 59 insertions(+), 19 deletions(-)

diff --git a/pkg/controller/persistentvolume/controller_test.go b/pkg/controller/persistentvolume/controller_test.go
index ef7a17d3a89..f34aeca38dd 100644
--- a/pkg/controller/persistentvolume/controller_test.go
+++ b/pkg/controller/persistentvolume/controller_test.go
@@ -25,7 +25,6 @@ import (
 	"k8s.io/kubernetes/pkg/client/cache"
 	"k8s.io/kubernetes/pkg/client/clientset_generated/internalclientset/fake"
 	"k8s.io/kubernetes/pkg/controller/framework"
-	"k8s.io/kubernetes/pkg/conversion"
 )
 
 // Test the real controller methods (add/update/delete claim/volume) with
@@ -52,8 +51,7 @@ func TestControllerSync(t *testing.T) {
 			// Custom test function that generates an add event
 			func(ctrl *PersistentVolumeController, reactor *volumeReactor, test controllerTest) error {
 				claim := newClaim("claim5-2", "uid5-2", "1Gi", "", api.ClaimPending)
-				reactor.claims[claim.Name] = claim
-				reactor.claimSource.Add(claim)
+				reactor.addClaimEvent(claim)
 				return nil
 			},
 		},
@@ -69,14 +67,7 @@ func TestControllerSync(t *testing.T) {
 			func(ctrl *PersistentVolumeController, reactor *volumeReactor, test controllerTest) error {
 				obj := ctrl.claims.List()[0]
 				claim := obj.(*api.PersistentVolumeClaim)
-				// Remove the claim from list of resulting claims.
-				delete(reactor.claims, claim.Name)
-				// Poke the controller with deletion event. Cloned claim is
-				// needed to prevent races (and we would get a clone from etcd
-				// too).
-				clone, _ := conversion.NewCloner().DeepCopy(claim)
-				claimClone := clone.(*api.PersistentVolumeClaim)
-				reactor.claimSource.Delete(claimClone)
+				reactor.deleteClaimEvent(claim)
 				return nil
 			},
 		},
@@ -92,14 +83,7 @@ func TestControllerSync(t *testing.T) {
 			func(ctrl *PersistentVolumeController, reactor *volumeReactor, test controllerTest) error {
 				obj := ctrl.volumes.store.List()[0]
 				volume := obj.(*api.PersistentVolume)
-				// Remove the volume from list of resulting volumes.
-				delete(reactor.volumes, volume.Name)
-				// Poke the controller with deletion event. Cloned volume is
-				// needed to prevent races (and we would get a clone from etcd
-				// too).
-				clone, _ := conversion.NewCloner().DeepCopy(volume)
-				volumeClone := clone.(*api.PersistentVolume)
-				reactor.volumeSource.Delete(volumeClone)
+				reactor.deleteVolumeEvent(volume)
 				return nil
 			},
 		},
diff --git a/pkg/controller/persistentvolume/framework_test.go b/pkg/controller/persistentvolume/framework_test.go
index 5a7a2ed7779..4ce1d29087e 100644
--- a/pkg/controller/persistentvolume/framework_test.go
+++ b/pkg/controller/persistentvolume/framework_test.go
@@ -472,6 +472,62 @@ func (r *volumeReactor) waitTest() {
 	}
 }
 
+// deleteVolumeEvent simulates that a volume has been deleted in etcd and
+// the controller receives 'volume deleted' event.
+func (r *volumeReactor) deleteVolumeEvent(volume *api.PersistentVolume) {
+	r.lock.Lock()
+	defer r.lock.Unlock()
+
+	// Remove the volume from list of resulting volumes.
+	delete(r.volumes, volume.Name)
+
+	// Generate deletion event. Cloned volume is needed to prevent races (and we
+	// would get a clone from etcd too).
+	clone, _ := conversion.NewCloner().DeepCopy(volume)
+	volumeClone := clone.(*api.PersistentVolume)
+	r.volumeSource.Delete(volumeClone)
+}
+
+// deleteClaimEvent simulates that a claim has been deleted in etcd and the
+// controller receives 'claim deleted' event.
+func (r *volumeReactor) deleteClaimEvent(claim *api.PersistentVolumeClaim) {
+	r.lock.Lock()
+	defer r.lock.Unlock()
+
+	// Remove the claim from list of resulting claims.
+	delete(r.claims, claim.Name)
+
+	// Generate deletion event. Cloned volume is needed to prevent races (and we
+	// would get a clone from etcd too).
+	clone, _ := conversion.NewCloner().DeepCopy(claim)
+	claimClone := clone.(*api.PersistentVolumeClaim)
+	r.claimSource.Delete(claimClone)
+}
+
+// addVolumeEvent simulates that a volume has been added in etcd and the
+// controller receives 'volume added' event.
+func (r *volumeReactor) addVolumeEvent(volume *api.PersistentVolume) {
+	r.lock.Lock()
+	defer r.lock.Unlock()
+
+	r.volumes[volume.Name] = volume
+	// Generate event. No cloning is needed, this claim is not stored in the
+	// controller cache yet.
+	r.volumeSource.Add(volume)
+}
+
+// addClaimEvent simulates that a claim has been deleted in etcd and the
+// controller receives 'claim added' event.
+func (r *volumeReactor) addClaimEvent(claim *api.PersistentVolumeClaim) {
+	r.lock.Lock()
+	defer r.lock.Unlock()
+
+	r.claims[claim.Name] = claim
+	// Generate event. No cloning is needed, this claim is not stored in the
+	// controller cache yet.
+	r.claimSource.Add(claim)
+}
+
 func newVolumeReactor(client *fake.Clientset, ctrl *PersistentVolumeController, volumeSource, claimSource *framework.FakeControllerSource, errors []reactorError) *volumeReactor {
 	reactor := &volumeReactor{
 		volumes:      make(map[string]*api.PersistentVolume),