Repository: submariner-io/lighthouse (go)

Issue:
Append to the ServiceExportCondition list 
When we update the `ServiceExport` status, we always overwrite the entire in the `ServiceExportCondition`. Since it's a list, we should append if the new `ServiceExportCondition` is different than the previous entry. Also we probably want to truncate the list to keep the last 10 or so entries.

Pull request description:
Append to the ServiceExportCondition list
Fixes #221

Patch:
diff --git a/go.sum b/go.sum
index 8c5d96d..7ad0a43 100644
--- a/go.sum
+++ b/go.sum
@@ -343,8 +343,6 @@ github.com/stretchr/testify v1.2.2/go.mod h1:a8OnRcib4nhh0OaRAV+Yts87kKdq0PP7pXf
 github.com/stretchr/testify v1.3.0/go.mod h1:M5WIy9Dh21IEIfnGCwXGc5bZfKNJtfHm1UVUgZn+9EI=
 github.com/stretchr/testify v1.4.0 h1:2E4SXV/wtOkTonXsotYi4li6zVWxYlZuYNCXe9XRJyk=
 github.com/stretchr/testify v1.4.0/go.mod h1:j7eGeouHqKxXV5pUuKE4zz7dFj8WfuZ+81PSLYec5m4=
-github.com/submariner-io/admiral v0.6.0-pre0 h1:/f/cs1qZPNHRQMl4yQxYMEZzOlKeZguGM9HSP84Rt/w=
-github.com/submariner-io/admiral v0.6.0-pre0/go.mod h1:cBv1Zvfai117idhYv5WPxTZEsgWQfeSqwPpxo0EMZXg=
 github.com/submariner-io/admiral v0.6.0-pre1 h1:v3KBPSQlfFovfkpyXnrwmWeAcSdbJ5zGBgslsLNSAMY=
 github.com/submariner-io/admiral v0.6.0-pre1/go.mod h1:cBv1Zvfai117idhYv5WPxTZEsgWQfeSqwPpxo0EMZXg=
 github.com/submariner-io/shipyard v0.5.0 h1:o4kTCubLFLeh+PaBDac1J7zNY3JKWwj7QrMLNb4g/p8=
diff --git a/pkg/agent/controller/agent.go b/pkg/agent/controller/agent.go
index 3712c9f..b2b57d8 100644
--- a/pkg/agent/controller/agent.go
+++ b/pkg/agent/controller/agent.go
@@ -2,6 +2,7 @@ package controller
 
 import (
 	"fmt"
+	"reflect"
 
 	"github.com/submariner-io/admiral/pkg/log"
 	"github.com/submariner-io/admiral/pkg/syncer"
@@ -29,6 +30,8 @@ const (
 	originNamespace        = "origin-namespace"
 )
 
+var MaxExportStatusConditions = 10
+
 func New(spec *AgentSpecification, cfg *rest.Config) (*Controller, error) {
 	kubeClientSet, err := kubernetes.NewForConfig(cfg)
 	if err != nil {
@@ -223,7 +226,7 @@ func (a *Controller) updateExportedServiceStatus(name, namespace string, condTyp
 		}
 
 		now := metav1.Now()
-		exportCondtion := lighthousev2a1.ServiceExportCondition{
+		exportCondition := lighthousev2a1.ServiceExportCondition{
 			Type:               condType,
 			Status:             status,
 			LastTransitionTime: &now,
@@ -231,10 +234,19 @@ func (a *Controller) updateExportedServiceStatus(name, namespace string, condTyp
 			Message:            &msg,
 		}
 
-		toUpdate.Status = lighthousev2a1.ServiceExportStatus{
-			Conditions: []lighthousev2a1.ServiceExportCondition{
-				exportCondtion,
-			},
+		numCond := len(toUpdate.Status.Conditions)
+		if numCond > 0 && serviceExportConditionEqual(&toUpdate.Status.Conditions[numCond-1], &exportCondition) {
+			klog.V(log.DEBUG).Infof("Last ServiceExportCondition for (%s/%s) is equal - not updating status: %#v",
+				namespace, name, toUpdate.Status.Conditions[numCond-1])
+			return nil
+		}
+
+		if numCond >= MaxExportStatusConditions {
+			copy(toUpdate.Status.Conditions[0:], toUpdate.Status.Conditions[1:])
+			toUpdate.Status.Conditions = toUpdate.Status.Conditions[:MaxExportStatusConditions]
+			toUpdate.Status.Conditions[MaxExportStatusConditions-1] = exportCondition
+		} else {
+			toUpdate.Status.Conditions = append(toUpdate.Status.Conditions, exportCondition)
 		}
 
 		_, err = a.lighthouseClient.LighthouseV2alpha1().ServiceExports(toUpdate.Namespace).Update(toUpdate)
@@ -246,6 +258,11 @@ func (a *Controller) updateExportedServiceStatus(name, namespace string, condTyp
 	}
 }
 
+func serviceExportConditionEqual(c1, c2 *lighthousev2a1.ServiceExportCondition) bool {
+	return c1.Type == c2.Type && c1.Status == c2.Status && reflect.DeepEqual(c1.Reason, c2.Reason) &&
+		reflect.DeepEqual(c1.Message, c2.Message)
+}
+
 func (a *Controller) newClusterStatus(service *corev1.Service, clusterID string) lighthousev2a1.ClusterStatus {
 	mcsIp := getGlobalIpFromService(service)
 	if mcsIp == "" {


Test patch:
diff --git a/pkg/agent/controller/agent_test.go b/pkg/agent/controller/agent_test.go
index 6e2a5f2..004c40d 100644
--- a/pkg/agent/controller/agent_test.go
+++ b/pkg/agent/controller/agent_test.go
@@ -6,6 +6,7 @@ import (
 
 	. "github.com/onsi/ginkgo"
 	. "github.com/onsi/gomega"
+	"github.com/onsi/gomega/format"
 	"github.com/submariner-io/admiral/pkg/fake"
 	"github.com/submariner-io/admiral/pkg/syncer/broker"
 	"github.com/submariner-io/admiral/pkg/syncer/test"
@@ -37,17 +38,17 @@ var _ = Describe("ServiceImport syncing", func() {
 			It("should correctly sync a ServiceImport and update the ServiceExport status", func() {
 				t.createService()
 				t.createServiceExport()
-				t.awaitServiceExported(t.service.Spec.ClusterIP)
+				t.awaitServiceExported(t.service.Spec.ClusterIP, 0)
 			})
 		})
 
 		When("the Service doesn't initially exist", func() {
 			It("should initially update the ServiceExport status to Initialized and eventually sync a ServiceImport", func() {
 				t.createServiceExport()
-				t.awaitServiceUnavailableStatus()
+				t.awaitServiceUnavailableStatus(0)
 
 				t.createService()
-				t.awaitServiceExported(t.service.Spec.ClusterIP)
+				t.awaitServiceExported(t.service.Spec.ClusterIP, 1)
 			})
 		})
 	})
@@ -56,7 +57,7 @@ var _ = Describe("ServiceImport syncing", func() {
 		It("should delete the ServiceImport", func() {
 			t.createService()
 			t.createServiceExport()
-			t.awaitServiceExported(t.service.Spec.ClusterIP)
+			t.awaitServiceExported(t.service.Spec.ClusterIP, 0)
 
 			t.deleteServiceExport()
 			t.awaitNoServiceImport(t.brokerServiceImportClient)
@@ -68,12 +69,12 @@ var _ = Describe("ServiceImport syncing", func() {
 		It("should delete the ServiceImport", func() {
 			t.createService()
 			t.createServiceExport()
-			t.awaitServiceExported(t.service.Spec.ClusterIP)
+			nextStatusIndex := t.awaitServiceExported(t.service.Spec.ClusterIP, 0)
 
 			t.deleteService()
 			t.awaitNoServiceImport(t.brokerServiceImportClient)
 			t.awaitNoServiceImport(t.localServiceImportClient)
-			t.awaitServiceUnavailableStatus()
+			t.awaitServiceUnavailableStatus(nextStatusIndex)
 		})
 	})
 
@@ -86,7 +87,8 @@ var _ = Describe("ServiceImport syncing", func() {
 			t.createService()
 			t.createServiceExport()
 
-			t.awaitServiceExportStatus(lighthousev2a1.ServiceExportInitialized, corev1.ConditionTrue, "AwaitingSync")
+			t.awaitServiceExportStatus(0, newServiceExportCondition(lighthousev2a1.ServiceExportInitialized,
+				corev1.ConditionTrue, "AwaitingSync"))
 
 			t.awaitNotServiceExportStatus(&lighthousev2a1.ServiceExportCondition{
 				Type:   lighthousev2a1.ServiceExportExported,
@@ -94,7 +96,28 @@ var _ = Describe("ServiceImport syncing", func() {
 			})
 
 			t.brokerServiceImportClient.PersistentFailOnCreate.Store("")
-			t.awaitServiceExported(t.service.Spec.ClusterIP)
+			t.awaitServiceExported(t.service.Spec.ClusterIP, 0)
+		})
+	})
+
+	When("the ServiceExportCondition list count reaches MaxExportStatusConditions", func() {
+		var oldMaxExportStatusConditions int
+
+		BeforeEach(func() {
+			oldMaxExportStatusConditions = controller.MaxExportStatusConditions
+			controller.MaxExportStatusConditions = 1
+		})
+
+		AfterEach(func() {
+			controller.MaxExportStatusConditions = oldMaxExportStatusConditions
+		})
+
+		It("should correctly truncate the ServiceExportCondition list", func() {
+			t.createService()
+			t.createServiceExport()
+
+			t.awaitServiceExportStatus(0, newServiceExportCondition(lighthousev2a1.ServiceExportExported,
+				corev1.ConditionTrue, ""))
 		})
 	})
 })
@@ -116,19 +139,20 @@ var _ = Describe("Globalnet enabled", func() {
 		})
 
 		It("should sync a ServiceImport with the global IP of the Service", func() {
-			t.awaitServiceExported(globalIP)
+			t.awaitServiceExported(globalIP, 0)
 		})
 	})
 
 	When("a local ServiceExport is created and the Service does not initially have a global IP", func() {
 		It("should eventually sync a ServiceImport with the global IP of the Service", func() {
-			t.awaitServiceExportStatus(lighthousev2a1.ServiceExportInitialized, corev1.ConditionFalse, "ServiceGlobalIPUnavailable")
+			t.awaitServiceExportStatus(0, newServiceExportCondition(lighthousev2a1.ServiceExportInitialized,
+				corev1.ConditionFalse, "ServiceGlobalIPUnavailable"))
 
 			t.service.SetAnnotations(map[string]string{"submariner.io/globalIp": globalIP})
 			_, err := t.localServiceClient.CoreV1().Services(t.service.Namespace).Update(t.service)
 			Expect(err).To(Succeed())
 
-			t.awaitServiceExported(globalIP)
+			t.awaitServiceExported(globalIP, 1)
 		})
 	})
 })
@@ -273,9 +297,8 @@ func (t *testDriver) awaitNoServiceImport(client dynamic.ResourceInterface) {
 	test.WaitForNoResource(client, t.service.Name+"-"+t.service.Namespace+"-"+clusterID)
 }
 
-func (t *testDriver) awaitServiceExportStatus(expType lighthousev2a1.ServiceExportConditionType,
-	expStatus corev1.ConditionStatus, expReason string) {
-	var found *lighthousev2a1.ServiceExport
+func (t *testDriver) awaitServiceExportStatus(atIndex int, expCond ...*lighthousev2a1.ServiceExportCondition) {
+	var found []lighthousev2a1.ServiceExportCondition
 
 	err := wait.PollImmediate(50*time.Millisecond, 5*time.Second, func() (bool, error) {
 		se, err := t.lighthouseClient.LighthouseV2alpha1().ServiceExports(t.service.Namespace).Get(t.service.Name, metav1.GetOptions{})
@@ -287,28 +310,37 @@ func (t *testDriver) awaitServiceExportStatus(expType lighthousev2a1.ServiceExpo
 			return false, err
 		}
 
-		found = se
+		found = se.Status.Conditions
 
-		return len(se.Status.Conditions) > 0 && se.Status.Conditions[0].Type == expType &&
-			se.Status.Conditions[0].Status == expStatus, nil
+		if (atIndex + len(expCond)) != len(se.Status.Conditions) {
+			return false, nil
+		}
+
+		j := atIndex
+		for _, exp := range expCond {
+			actual := se.Status.Conditions[j]
+			j++
+			Expect(actual.Type).To(Equal(exp.Type))
+			Expect(actual.Status).To(Equal(exp.Status))
+			Expect(actual.LastTransitionTime).To(Not(BeNil()))
+			Expect(actual.LastTransitionTime.After(t.now)).To(BeTrue())
+			Expect(actual.Reason).To(Not(BeNil()))
+			Expect(*actual.Reason).To(Equal(*exp.Reason))
+			Expect(actual.Message).To(Not(BeNil()))
+		}
+
+		return true, nil
 	})
 
 	if err == wait.ErrWaitTimeout {
 		if found == nil {
 			Fail("ServiceExport not found")
 		}
+
+		Fail(format.Message(found, fmt.Sprintf("to contain at index %d", atIndex), expCond))
 	} else {
 		Expect(err).To(Succeed())
 	}
-
-	Expect(found.Status.Conditions).To(HaveLen(1))
-	Expect(found.Status.Conditions[0].Type).To(Equal(expType))
-	Expect(found.Status.Conditions[0].Status).To(Equal(expStatus))
-	Expect(found.Status.Conditions[0].LastTransitionTime).To(Not(BeNil()))
-	Expect(found.Status.Conditions[0].LastTransitionTime.After(t.now)).To(BeTrue())
-	Expect(found.Status.Conditions[0].Reason).To(Not(BeNil()))
-	Expect(*found.Status.Conditions[0].Reason).To(Equal(expReason))
-	Expect(found.Status.Conditions[0].Message).To(Not(BeNil()))
 }
 
 func (t *testDriver) awaitNotServiceExportStatus(notCond *lighthousev2a1.ServiceExportCondition) {
@@ -339,13 +371,27 @@ func (t *testDriver) awaitNotServiceExportStatus(notCond *lighthousev2a1.Service
 	}
 }
 
-func (t *testDriver) awaitServiceExported(serviceIP string) {
+func (t *testDriver) awaitServiceExported(serviceIP string, statusIndex int) int {
 	t.awaitServiceImport(t.brokerServiceImportClient, serviceIP)
 	t.awaitServiceImport(t.localServiceImportClient, serviceIP)
 
-	t.awaitServiceExportStatus(lighthousev2a1.ServiceExportExported, corev1.ConditionTrue, "")
+	t.awaitServiceExportStatus(statusIndex, newServiceExportCondition(lighthousev2a1.ServiceExportInitialized,
+		corev1.ConditionTrue, "AwaitingSync"), newServiceExportCondition(lighthousev2a1.ServiceExportExported,
+		corev1.ConditionTrue, ""))
+
+	return statusIndex + 2
 }
 
-func (t *testDriver) awaitServiceUnavailableStatus() {
-	t.awaitServiceExportStatus(lighthousev2a1.ServiceExportInitialized, corev1.ConditionFalse, "ServiceUnavailable")
+func (t *testDriver) awaitServiceUnavailableStatus(atIndex int) {
+	t.awaitServiceExportStatus(atIndex, newServiceExportCondition(lighthousev2a1.ServiceExportInitialized,
+		corev1.ConditionFalse, "ServiceUnavailable"))
+}
+
+func newServiceExportCondition(cType lighthousev2a1.ServiceExportConditionType,
+	status corev1.ConditionStatus, reason string) *lighthousev2a1.ServiceExportCondition {
+	return &lighthousev2a1.ServiceExportCondition{
+		Type:   cType,
+		Status: status,
+		Reason: &reason,
+	}
 }
diff --git a/test/e2e/discovery/service_discovery.go b/test/e2e/discovery/service_discovery.go
index 1571303..e8ce1a8 100644
--- a/test/e2e/discovery/service_discovery.go
+++ b/test/e2e/discovery/service_discovery.go
@@ -6,7 +6,6 @@ import (
 	"strings"
 
 	. "github.com/onsi/ginkgo"
-	. "github.com/onsi/gomega"
 	lhframework "github.com/submariner-io/lighthouse/test/e2e/framework"
 	"github.com/submariner-io/shipyard/test/e2e/framework"
 	corev1 "k8s.io/api/core/v1"
@@ -52,9 +51,7 @@ func RunServiceDiscoveryTest(f *lhframework.Framework) {
 	f.AwaitGlobalnetIP(framework.ClusterB, nginxServiceClusterB.Name, nginxServiceClusterB.Namespace)
 	f.NewServiceExport(framework.ClusterB, nginxServiceClusterB.Name, nginxServiceClusterB.Namespace)
 
-	se := f.AwaitServiceExportStatusCondition(framework.ClusterB, nginxServiceClusterB.Name, nginxServiceClusterB.Namespace)
-	Expect(se.Status.Conditions[0].Status).To(Equal(corev1.ConditionTrue))
-	Expect(*se.Status.Conditions[0].Message).To(Equal("Service was successfully synced to the broker"))
+	f.AwaitServiceExportedStatusCondition(framework.ClusterB, nginxServiceClusterB.Name, nginxServiceClusterB.Namespace)
 
 	By(fmt.Sprintf("Creating a Netshoot Deployment on %q", clusterAName))
 	netshootPodList := f.NewNetShootDeployment(framework.ClusterA)
@@ -93,9 +90,7 @@ func RunServiceDiscoveryLocalTest(f *lhframework.Framework) {
 	f.AwaitGlobalnetIP(framework.ClusterB, nginxServiceClusterB.Name, nginxServiceClusterB.Namespace)
 	f.NewServiceExport(framework.ClusterB, nginxServiceClusterB.Name, nginxServiceClusterB.Namespace)
 
-	se := f.AwaitServiceExportStatusCondition(framework.ClusterB, nginxServiceClusterB.Name, nginxServiceClusterB.Namespace)
-	Expect(se.Status.Conditions[0].Status).To(Equal(corev1.ConditionTrue))
-	Expect(*se.Status.Conditions[0].Message).To(Equal("Service was successfully synced to the broker"))
+	f.AwaitServiceExportedStatusCondition(framework.ClusterB, nginxServiceClusterB.Name, nginxServiceClusterB.Namespace)
 
 	By(fmt.Sprintf("Creating a Netshoot Deployment on %q", clusterAName))
 	netshootPodList := f.NewNetShootDeployment(framework.ClusterA)
diff --git a/test/e2e/framework/framework.go b/test/e2e/framework/framework.go
index bcc8dfb..e87adff 100644
--- a/test/e2e/framework/framework.go
+++ b/test/e2e/framework/framework.go
@@ -64,20 +64,28 @@ func (f *Framework) NewServiceExport(cluster framework.ClusterIndex, name, names
 	return serviceExport
 }
 
-func (f *Framework) AwaitServiceExportStatusCondition(cluster framework.ClusterIndex, name,
-	namespace string) *lighthousev2a1.ServiceExport {
+func (f *Framework) AwaitServiceExportedStatusCondition(cluster framework.ClusterIndex, name, namespace string) {
 	se := LighthouseClients[cluster].LighthouseV2alpha1().ServiceExports(namespace)
-	By(fmt.Sprintf("Retrieving serviceExport %s.%s on %q", name, namespace, framework.TestContext.ClusterIDs[cluster]))
-	serviceExport := framework.AwaitUntil("retrieve serviceExport", func() (interface{}, error) {
+	By(fmt.Sprintf("Retrieving ServiceExport %s.%s on %q", name, namespace, framework.TestContext.ClusterIDs[cluster]))
+	framework.AwaitUntil("retrieve ServiceExport", func() (interface{}, error) {
 		return se.Get(name, metav1.GetOptions{})
 	}, func(result interface{}) (bool, string, error) {
 		se := result.(*lighthousev2a1.ServiceExport)
 		if len(se.Status.Conditions) == 0 {
-			return false, "Status.Conditions is empty", nil
+			return false, "No ServiceExportConditions", nil
 		}
+
+		last := se.Status.Conditions[len(se.Status.Conditions)-1]
+		if last.Type != lighthousev2a1.ServiceExportExported {
+			return false, fmt.Sprintf("ServiceExportCondition Type is %v", last.Type), nil
+		}
+
+		if last.Status != v1.ConditionTrue {
+			return false, fmt.Sprintf("ServiceExportCondition Status is %v", last.Status), nil
+		}
+
 		return true, "", nil
-	}).(*lighthousev2a1.ServiceExport)
-	return serviceExport
+	})
 }
 
 func (f *Framework) DeleteServiceExport(cluster framework.ClusterIndex, name, namespace string) {