Filter out self-generated leader election claim updates

controller/controller.go

@@ -19,6 +19,7 @@ package controller
 import (
 	"fmt"
 	"os/exec"
+	"reflect"
 	"strings"
 	"sync"
 	"time"
@@ -278,7 +279,35 @@ func (ctrl *ProvisionController) addClaim(obj interface{}) {
 // On update claim, pass the new claim to addClaim. Updates occur at least every
 // resyncPeriod.
 func (ctrl *ProvisionController) updateClaim(oldObj, newObj interface{}) {
-	ctrl.addClaim(newObj)
+	// If they are exactly the same it must be a forced resync (every
+	// resyncPeriod).
+	if reflect.DeepEqual(oldObj, newObj) {
+		ctrl.addClaim(newObj)
+		return
+	}
+
+	// If not a forced resync, we filter out the frequent leader election record
+	// annotation changes by checking if the only update is in the annotation
+	oldClaim, ok := oldObj.(*v1.PersistentVolumeClaim)
+	if !ok {
+		glog.Errorf("Expected PersistentVolumeClaim but handler received %#v", oldObj)
+		return
+	}
+	newClaim, ok := newObj.(*v1.PersistentVolumeClaim)
+	if !ok {
+		glog.Errorf("Expected PersistentVolumeClaim but handler received %#v", newObj)
+		return
+	}
+
+	skipAddClaim, err := ctrl.isOnlyRecordUpdate(oldClaim, newClaim)
+	if err != nil {
+		glog.Errorf("Error checking if only record was updated in claim: %v")
+		return
+	}
+
+	if !skipAddClaim {
+		ctrl.addClaim(newObj)
+	}
 }
 
 // On update volume, check if the updated volume should be deleted and delete if
@@ -298,6 +327,42 @@ func (ctrl *ProvisionController) updateVolume(oldObj, newObj interface{}) {
 	}
 }
 
+// isOnlyRecordUpdate checks if the only update between the old & new claim is
+// the leader election record annotation.
+func (ctrl *ProvisionController) isOnlyRecordUpdate(oldClaim, newClaim *v1.PersistentVolumeClaim) (bool, error) {
+	old, err := ctrl.removeRecord(oldClaim)
+	if err != nil {
+		return false, err
+	}
+	new, err := ctrl.removeRecord(newClaim)
+	if err != nil {
+		return false, err
+	}
+	return reflect.DeepEqual(old, new), nil
+}
+
+// removeRecord returns a claim with its leader election record annotation and
+// ResourceVersion set blank
+func (ctrl *ProvisionController) removeRecord(claim *v1.PersistentVolumeClaim) (*v1.PersistentVolumeClaim, error) {
+	clone, err := api.Scheme.DeepCopy(claim)
+	if err != nil {
+		return nil, fmt.Errorf("Error cloning claim: %v", err)
+	}
+	claimClone, ok := clone.(*v1.PersistentVolumeClaim)
+	if !ok {
+		return nil, fmt.Errorf("Unexpected claim cast error: %v", claimClone)
+	}
+
+	if claimClone.Annotations == nil {
+		claimClone.Annotations = make(map[string]string)
+	}
+	claimClone.Annotations[rl.LeaderElectionRecordAnnotationKey] = ""
+
+	claimClone.ResourceVersion = ""
+
+	return claimClone, nil
+}
+
 func (ctrl *ProvisionController) shouldProvision(claim *v1.PersistentVolumeClaim) bool {
 	if claim.Spec.VolumeName != "" {
 		return false

controller/controller_test.go

@@ -25,6 +25,7 @@ import (
 	"testing"
 	"time"
 
+	rl "github.com/kubernetes-incubator/nfs-provisioner/controller/leaderelection/resourcelock"
 	"k8s.io/client-go/kubernetes"
 	"k8s.io/client-go/kubernetes/fake"
 	fakev1core "k8s.io/client-go/kubernetes/typed/core/v1/fake"
@@ -44,6 +45,7 @@ const (
 	resyncPeriod = 100 * time.Millisecond
 )
 
+// TODO clean this up, e.g. remove redundant params (provisionerName: "foo.bar/baz")
 func TestController(t *testing.T) {
 	tests := []struct {
 		name            string
@@ -405,6 +407,51 @@ func TestShouldDelete(t *testing.T) {
 	}
 }
 
+func TestIsOnlyRecordUpdate(t *testing.T) {
+	tests := []struct {
+		name       string
+		old        *v1.PersistentVolumeClaim
+		new        *v1.PersistentVolumeClaim
+		expectedIs bool
+	}{
+		{
+			name:       "is only record update",
+			old:        newClaim("claim-1", "1-1", "class-1", "", map[string]string{rl.LeaderElectionRecordAnnotationKey: "a"}),
+			new:        newClaim("claim-1", "1-1", "class-1", "", map[string]string{rl.LeaderElectionRecordAnnotationKey: "b"}),
+			expectedIs: true,
+		},
+		{
+			name:       "is seen as only record update, stayed exactly the same",
+			old:        newClaim("claim-1", "1-1", "class-1", "", map[string]string{rl.LeaderElectionRecordAnnotationKey: "a"}),
+			new:        newClaim("claim-1", "1-1", "class-1", "", map[string]string{rl.LeaderElectionRecordAnnotationKey: "a"}),
+			expectedIs: true,
+		},
+		{
+			name:       "isn't only record update, class changed as well",
+			old:        newClaim("claim-1", "1-1", "class-1", "", map[string]string{rl.LeaderElectionRecordAnnotationKey: "a"}),
+			new:        newClaim("claim-1", "1-1", "class-2", "", map[string]string{rl.LeaderElectionRecordAnnotationKey: "b"}),
+			expectedIs: false,
+		},
+		{
+			name:       "isn't only record update, only class changed",
+			old:        newClaim("claim-1", "1-1", "class-1", "", map[string]string{rl.LeaderElectionRecordAnnotationKey: "a"}),
+			new:        newClaim("claim-1", "1-1", "class-2", "", map[string]string{rl.LeaderElectionRecordAnnotationKey: "a"}),
+			expectedIs: false,
+		},
+	}
+	for _, test := range tests {
+		client := fake.NewSimpleClientset()
+		provisioner := newTestProvisioner()
+		ctrl := newTestProvisionController(client, resyncPeriod, "foo.bar/baz", provisioner, "v1.5.0", false)
+
+		is, _ := ctrl.isOnlyRecordUpdate(test.old, test.new)
+		if test.expectedIs != is {
+			t.Logf("test case: %s", test.name)
+			t.Errorf("expected is only record update %v but got %v\n", test.expectedIs, is)
+		}
+	}
+}
+
 func newTestProvisionController(
 	client kubernetes.Interface,
 	resyncPeriod time.Duration,