Merge pull request #361 from alecmerdler/ALM-581

Tests for Runaway Control Loops

test/e2e/csv_e2e_test.go

@@ -295,13 +295,18 @@ func TestCreateCSVRequirementsMet(t *testing.T) {
 	require.NoError(t, err)
 	defer cleanupCSV()
 
-	_, err = fetchCSV(t, c, csv.Name, csvSucceededChecker)
+	fetchedCSV, err := fetchCSV(t, c, csv.Name, csvSucceededChecker)
 	require.NoError(t, err)
 
 	// Should create deployment
 	dep, err := c.GetDeployment(testNamespace, strategy.DeploymentSpecs[0].Name)
 	require.NoError(t, err)
 	require.Equal(t, strategy.DeploymentSpecs[0].Name, dep.Name)
+
+	// Fetch cluster service version again to check for unnecessary control loops
+	sameCSV, err := fetchCSV(t, c, csv.Name, csvSucceededChecker)
+	require.NoError(t, err)
+	compareResources(t, fetchedCSV, sameCSV)
 }
 
 func TestUpdateCSVSameDeploymentName(t *testing.T) {
@@ -471,7 +476,7 @@ func TestUpdateCSVSameDeploymentName(t *testing.T) {
 	defer cleanupNewCSV()
 
 	// Wait for updated CSV to succeed
-	_, err = fetchCSV(t, c, csvNew.Name, csvSucceededChecker)
+	fetchedCSV, err := fetchCSV(t, c, csvNew.Name, csvSucceededChecker)
 	require.NoError(t, err)
 
 	// should have csv-sa and old-csv-sa
@@ -499,6 +504,11 @@ func TestUpdateCSVSameDeploymentName(t *testing.T) {
 	// csv-sa shouldn't have been GC'd
 	_, err = c.GetServiceAccount(testNamespace, "csv-sa")
 	require.NoError(t, err)
+
+	// Fetch cluster service version again to check for unnecessary control loops
+	sameCSV, err := fetchCSV(t, c, csvNew.Name, csvSucceededChecker)
+	require.NoError(t, err)
+	compareResources(t, fetchedCSV, sameCSV)
 }
 
 func TestUpdateCSVDifferentDeploymentName(t *testing.T) {
@@ -621,8 +631,13 @@ func TestUpdateCSVDifferentDeploymentName(t *testing.T) {
 	defer cleanupNewCSV()
 
 	// Wait for updated CSV to succeed
-	_, err = fetchCSV(t, c, csvNew.Name, csvSucceededChecker)
+	fetchedCSV, err := fetchCSV(t, c, csvNew.Name, csvSucceededChecker)
+	require.NoError(t, err)
+
+	// Fetch cluster service version again to check for unnecessary control loops
+	sameCSV, err := fetchCSV(t, c, csvNew.Name, csvSucceededChecker)
 	require.NoError(t, err)
+	compareResources(t, fetchedCSV, sameCSV)
 
 	// Should have created new deployment and deleted old
 	depNew, err := c.GetDeployment(testNamespace, strategyNew.DeploymentSpecs[0].Name)

test/e2e/alm_e2e_test.go → test/e2e/installplan_e2e_test.go

@@ -181,9 +181,15 @@ func TestCreateInstallPlanManualApproval(t *testing.T) {
 	t.Logf("%d Vault Resources present", vaultResourcesPresent)
 	require.NotZero(t, vaultResourcesPresent)
 
+	// Fetch installplan again to check for unnecessary control loops
+	_, err = fetchInstallPlan(t, c, approvedInstallPlan.GetName(), func(fip *installplanv1alpha1.InstallPlan) bool {
+		compareResources(t, approvedInstallPlan, fip)
+		return true
+	})
+	require.NoError(t, err)
+
 }
 
-// This captures the current state of OLM where Failed InstallPlans aren't implemented and should be removed in the future
 func TestCreateInstallPlanFromInvalidClusterServiceVersionNameExistingBehavior(t *testing.T) {
 	c := newKubeClient(t)
 
@@ -213,7 +219,14 @@ func TestCreateInstallPlanFromInvalidClusterServiceVersionNameExistingBehavior(t
 			fip.Status.Conditions[0].Reason == installplanv1alpha1.InstallPlanReasonDependencyConflict
 	})
 
-	// InstallPlans don't have a failed status, they end up in a Planning state with a "false" resolved state
+	// Fetch installplan again to check for unnecessary control loops
+	_, err = fetchInstallPlan(t, c, installPlan.GetName(), func(fip *installplanv1alpha1.InstallPlan) bool {
+		compareResources(t, fetchedInstallPlan, fip)
+		return true
+	})
+	require.NoError(t, err)
+
+	require.Equal(t, installplanv1alpha1.InstallPlanPhaseFailed, fetchedInstallPlan.Status.Phase)
 	require.Equal(t, installplanv1alpha1.InstallPlanResolved, fetchedInstallPlan.Status.Conditions[0].Type)
 	require.Equal(t, corev1.ConditionFalse, fetchedInstallPlan.Status.Conditions[0].Status)
 	require.Equal(t, installplanv1alpha1.InstallPlanReasonInstallCheckFailed, fetchedInstallPlan.Status.Conditions[0].Reason)
@@ -248,4 +261,11 @@ func TestCreateInstallPlanFromInvalidClusterServiceVersionName(t *testing.T) {
 	require.NoError(t, err)
 
 	require.Equal(t, installplanv1alpha1.InstallPlanPhaseFailed, fetchedInstallPlan.Status.Phase)
+
+	// Fetch installplan again to check for unnecessary control loops
+	_, err = fetchInstallPlan(t, c, fetchedInstallPlan.GetName(), func(fip *installplanv1alpha1.InstallPlan) bool {
+		compareResources(t, fetchedInstallPlan, fip)
+		return true
+	})
+	require.NoError(t, err)
 }

test/e2e/subscription_e2e_test.go

@@ -292,6 +292,10 @@ func TestCreateNewSubscription(t *testing.T) {
 	require.NoError(t, err)
 	require.NotNil(t, subscription)
 
+	// Fetch subscription again to check for unnecessary control loops
+	sameSubscription, err := fetchSubscription(t, c, testSubscriptionName)
+	compareResources(t, subscription, sameSubscription)
+
 	// Deleting subscription / installplan doesn't clean up the CSV
 	cleanupCustomResource(t, c, csvv1alpha1.GroupVersion,
 		csvv1alpha1.ClusterServiceVersionKind, csv.GetName())()
@@ -343,4 +347,8 @@ func TestCreateNewSubscriptionManualApproval(t *testing.T) {
 
 	require.Equal(t, v1alpha1.ApprovalManual, installPlan.Spec.Approval)
 	require.Equal(t, v1alpha1.InstallPlanPhaseRequiresApproval, installPlan.Status.Phase)
+
+	// Fetch subscription again to check for unnecessary control loops
+	sameSubscription, err := fetchSubscription(t, c, "manual-subscription")
+	compareResources(t, subscription, sameSubscription)
 }

test/e2e/util_test.go

@@ -8,9 +8,11 @@ import (
 
 	"github.com/stretchr/testify/require"
 	corev1 "k8s.io/api/core/v1"
+	"k8s.io/apimachinery/pkg/api/equality"
 	"k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
+	"k8s.io/apimachinery/pkg/util/diff"
 	"k8s.io/apimachinery/pkg/util/wait"
 	"k8s.io/apiserver/pkg/storage/names"
 
@@ -146,3 +148,11 @@ func cleanupCustomResource(t *testing.T, c operatorclient.ClientInterface, group
 		require.NoError(t, c.DeleteCustomResource(apis.GroupName, group, testNamespace, kind, name))
 	}
 }
+
+// compareResources compares resource equality then prints a diff for easier debugging
+func compareResources(t *testing.T, expected, actual interface{}) {
+	if eq := equality.Semantic.DeepEqual(expected, actual); !eq {
+		t.Fatalf("Resource does not match expected value: %s",
+			diff.ObjectDiff(expected, actual))
+	}
+}