Check for quorom group instance changes during PlanUpdate (#567)

Signed-off-by: Steven Kaufer <[email protected]>

pkg/plugin/group/quorum.go

@@ -3,12 +3,13 @@ package group
 import (
 	"errors"
 	"fmt"
-	log "github.com/Sirupsen/logrus"
-	"github.com/docker/infrakit/pkg/spi/group"
-	"github.com/docker/infrakit/pkg/spi/instance"
 	"reflect"
 	"sync"
 	"time"
+
+	log "github.com/Sirupsen/logrus"
+	"github.com/docker/infrakit/pkg/spi/group"
+	"github.com/docker/infrakit/pkg/spi/instance"
 )
 
 // TODO(wfarner): Converge this implementation with scaler.go, they share a lot of behavior.
@@ -38,6 +39,11 @@ func (q *quorum) PlanUpdate(scaled Scaled, settings groupSettings, newSettings g
 		return nil, errors.New("Logical ID changes to a quorum is not currently supported")
 	}
 
+	if settings.config.InstanceHash() == newSettings.config.InstanceHash() {
+		// This is a no-op update because the instance configuration is unchanged
+		return &noopUpdate{}, nil
+	}
+
 	return &rollingupdate{
 		desc: fmt.Sprintf(
 			"Performing a rolling update on %d instances",

pkg/plugin/group/quorum_test.go

@@ -1,12 +1,16 @@
 package group
 
 import (
+	"testing"
+	"time"
+
 	mock_group "github.com/docker/infrakit/pkg/mock/plugin/group"
+	mock_instance "github.com/docker/infrakit/pkg/mock/spi/instance"
+	"github.com/docker/infrakit/pkg/plugin/group/types"
 	"github.com/docker/infrakit/pkg/spi/group"
 	"github.com/docker/infrakit/pkg/spi/instance"
 	"github.com/golang/mock/gomock"
-	"testing"
-	"time"
+	"github.com/stretchr/testify/require"
 )
 
 var (
@@ -95,3 +99,96 @@ func TestRemoveUnknown(t *testing.T) {
 
 	quorum.Run()
 }
+
+func TestQuorumPlanUpdateNoChanges(t *testing.T) {
+	ctrl := gomock.NewController(t)
+	defer ctrl.Finish()
+
+	groupID := group.ID("quorum")
+	scaled := mock_group.NewMockScaled(ctrl)
+	instancePlugin := mock_instance.NewMockPlugin(ctrl)
+	settings := groupSettings{
+		instancePlugin: instancePlugin,
+		config: types.Spec{
+			Allocation: types.AllocationMethod{
+				LogicalIDs: []instance.LogicalID{
+					*a.LogicalID,
+				},
+			},
+		},
+	}
+	quorum := NewQuorum(groupID, scaled, logicalIDs, 1*time.Millisecond)
+	plan, err := quorum.PlanUpdate(scaled, settings, settings)
+	require.NoError(t, err)
+	require.IsType(t, &noopUpdate{}, plan)
+}
+
+func TestQuorumPlanUpdateLogicalIDChange(t *testing.T) {
+	ctrl := gomock.NewController(t)
+	defer ctrl.Finish()
+
+	groupID := group.ID("quorum")
+	scaled := mock_group.NewMockScaled(ctrl)
+	instancePlugin := mock_instance.NewMockPlugin(ctrl)
+	settingsOld := groupSettings{
+		instancePlugin: instancePlugin,
+		config: types.Spec{
+			Allocation: types.AllocationMethod{
+				LogicalIDs: []instance.LogicalID{
+					*a.LogicalID,
+				},
+			},
+		},
+	}
+	settingsNew := groupSettings{
+		instancePlugin: instancePlugin,
+		config: types.Spec{
+			Allocation: types.AllocationMethod{
+				LogicalIDs: []instance.LogicalID{
+					*b.LogicalID,
+				},
+			},
+		},
+	}
+	quorum := NewQuorum(groupID, scaled, logicalIDs, 1*time.Millisecond)
+	_, err := quorum.PlanUpdate(scaled, settingsOld, settingsNew)
+	require.Error(t, err)
+}
+
+func TestQuorumPlanUpdateRollingUpdate(t *testing.T) {
+	ctrl := gomock.NewController(t)
+	defer ctrl.Finish()
+
+	groupID := group.ID("quorum")
+	scaled := mock_group.NewMockScaled(ctrl)
+	instancePlugin := mock_instance.NewMockPlugin(ctrl)
+	instanceOld := types.InstancePlugin{
+		Plugin: "name-old",
+	}
+	instanceNew := types.InstancePlugin{
+		Plugin: "name-new",
+	}
+	allocation := types.AllocationMethod{
+		LogicalIDs: []instance.LogicalID{
+			*b.LogicalID,
+		},
+	}
+	settingsOld := groupSettings{
+		instancePlugin: instancePlugin,
+		config: types.Spec{
+			Allocation: allocation,
+			Instance:   instanceOld,
+		},
+	}
+	settingsNew := groupSettings{
+		instancePlugin: instancePlugin,
+		config: types.Spec{
+			Allocation: allocation,
+			Instance:   instanceNew,
+		},
+	}
+	quorum := NewQuorum(groupID, scaled, logicalIDs, 1*time.Millisecond)
+	plan, err := quorum.PlanUpdate(scaled, settingsOld, settingsNew)
+	require.NoError(t, err)
+	require.IsType(t, &rollingupdate{}, plan)
+}

pkg/plugin/group/scaler.go

@@ -89,7 +89,7 @@ func (s *scaler) PlanUpdate(scaled Scaled, settings groupSettings, newSettings g
 				newSettings.config.Allocation.Size)
 		} else {
 			plan.desc = fmt.Sprintf(
-				"Terminating %d instances to reduce the group size to %d, "+
+				"Terminating %d instances to reduce the group size to %d,"+
 					" then performing a rolling update on %d instances",
 				int(sizeChange)*-1,
 				newSettings.config.Allocation.Size,