Repository: submariner-io/lighthouse
Pull request #218 (2020-07-23 01:24:59)

 Set LastTransitionTime and Reason in ServiceExportStatus
`LastTransitionTime` is defined as the "last time the condition transitioned from one status to another". Set it to the current time.
    
`Reason` is defined as a "a programmatic identifier indicating the reason for the condition's last transition... The value should be a CamelCase string".


submariner-bot: 🤖 Created branch: z_pr218/tpantelis/export_status
submariner-bot: 🤖 Updated branch: z_pr218/tpantelis/export_status
submariner-bot: 🤖 Updated branch: z_pr218/tpantelis/export_status
submariner-bot: 🤖 Updated branch: z_pr218/tpantelis/export_status

Patch:
diff --git a/pkg/agent/controller/agent.go b/pkg/agent/controller/agent.go
index eff83f4..ac40132 100644
--- a/pkg/agent/controller/agent.go
+++ b/pkg/agent/controller/agent.go
@@ -24,6 +24,7 @@ import (
 
 const (
 	submarinerIpamGlobalIp = "submariner.io/globalIp"
+	serviceUnavailable     = "ServiceUnavailable"
 )
 
 func New(spec *AgentSpecification, cfg *rest.Config) (*Controller, error) {
@@ -136,13 +137,13 @@ func (a *Controller) serviceExportToRemoteServiceImport(obj runtime.Object, op s
 	svc, err := a.kubeClientSet.CoreV1().Services(svcExport.Namespace).Get(svcExport.Name, metav1.GetOptions{})
 	if errors.IsNotFound(err) {
 		klog.V(log.DEBUG).Infof("Service to be exported (%s/%s) doesn't exist", svcExport.Namespace, svcExport.Name)
-		a.updateExportedServiceStatus(svcExport, "Service to be exported doesn't exist", lighthousev2a1.ServiceExportInitialized,
-			corev1.ConditionFalse)
+		a.updateExportedServiceStatus(svcExport, lighthousev2a1.ServiceExportInitialized, corev1.ConditionFalse,
+			serviceUnavailable, "Service to be exported doesn't exist")
 		return nil, true
 	} else if err != nil {
 		// some other error. Log and requeue
-		a.updateExportedServiceStatus(svcExport, fmt.Sprintf("Error obtaining service: %v", err), lighthousev2a1.ServiceExportInitialized,
-			corev1.ConditionFalse)
+		a.updateExportedServiceStatus(svcExport, lighthousev2a1.ServiceExportInitialized, corev1.ConditionUnknown,
+			"ServiceRetrievalFailed", fmt.Sprintf("Error retrieving the Service: %v", err))
 		klog.Errorf("Unable to get service for %#v: %v", svc, err)
 		return nil, true
 	}
@@ -150,8 +151,8 @@ func (a *Controller) serviceExportToRemoteServiceImport(obj runtime.Object, op s
 		klog.V(log.DEBUG).Infof("Service to be exported (%s/%s) doesn't have a global IP yet", svcExport.Namespace, svcExport.Name)
 
 		// Globalnet enabled but service doesn't have globalIp yet, Update the status and requeue
-		a.updateExportedServiceStatus(svcExport, "Service doesn't have a global IP yet", lighthousev2a1.ServiceExportInitialized,
-			corev1.ConditionFalse)
+		a.updateExportedServiceStatus(svcExport, lighthousev2a1.ServiceExportInitialized, corev1.ConditionFalse,
+			"ServiceGlobalIPUnavailable", "Service doesn't have a global IP yet")
 		return nil, true
 	}
 	serviceImport.Spec = lighthousev2a1.ServiceImportSpec{
@@ -162,8 +163,8 @@ func (a *Controller) serviceExportToRemoteServiceImport(obj runtime.Object, op s
 			a.newClusterStatus(svc, a.clusterID),
 		},
 	}
-	a.updateExportedServiceStatus(svcExport, "Service was successfully synced to the broker",
-		lighthousev2a1.ServiceExportExported, corev1.ConditionTrue)
+	a.updateExportedServiceStatus(svcExport, lighthousev2a1.ServiceExportExported, corev1.ConditionTrue,
+		"", "Service was successfully synced to the broker")
 
 	return serviceImport, false
 }
@@ -188,14 +189,14 @@ func (a *Controller) serviceToRemoteServiceImport(obj runtime.Object, op syncer.
 	serviceImport := a.newServiceImport(svcExport)
 
 	// Update the status and requeue
-	a.updateExportedServiceStatus(svcExport, "Service to be exported doesn't exist", lighthousev2a1.ServiceExportInitialized,
-		corev1.ConditionFalse)
+	a.updateExportedServiceStatus(svcExport, lighthousev2a1.ServiceExportInitialized, corev1.ConditionFalse,
+		serviceUnavailable, "Service to be exported doesn't exist")
 
 	return serviceImport, false
 }
 
-func (a *Controller) updateExportedServiceStatus(export *lighthousev2a1.ServiceExport, msg string,
-	conditionType lighthousev2a1.ServiceExportConditionType, status corev1.ConditionStatus) {
+func (a *Controller) updateExportedServiceStatus(export *lighthousev2a1.ServiceExport, condType lighthousev2a1.ServiceExportConditionType,
+	status corev1.ConditionStatus, reason, msg string) {
 	retryErr := retry.RetryOnConflict(retry.DefaultRetry, func() error {
 		toUpdate, err := a.lighthouseClient.LighthouseV2alpha1().ServiceExports(export.Namespace).Get(export.Name, metav1.GetOptions{})
 		if errors.IsNotFound(err) {
@@ -204,18 +205,22 @@ func (a *Controller) updateExportedServiceStatus(export *lighthousev2a1.ServiceE
 		} else if err != nil {
 			return err
 		}
+
+		now := metav1.Now()
 		exportCondtion := lighthousev2a1.ServiceExportCondition{
-			Type:               conditionType,
+			Type:               condType,
 			Status:             status,
-			LastTransitionTime: nil,
-			Reason:             nil,
+			LastTransitionTime: &now,
+			Reason:             &reason,
 			Message:            &msg,
 		}
+
 		toUpdate.Status = lighthousev2a1.ServiceExportStatus{
 			Conditions: []lighthousev2a1.ServiceExportCondition{
 				exportCondtion,
 			},
 		}
+
 		_, err = a.lighthouseClient.LighthouseV2alpha1().ServiceExports(toUpdate.Namespace).Update(toUpdate)
 
 		return err


Test patch:
diff --git a/pkg/agent/controller/agent_test.go b/pkg/agent/controller/agent_test.go
index b23f3d7..3d6fefe 100644
--- a/pkg/agent/controller/agent_test.go
+++ b/pkg/agent/controller/agent_test.go
@@ -42,11 +42,7 @@ var _ = Describe("ServiceImport syncing", func() {
 		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.awaitServiceExportStatus(&lighthousev2a1.ServiceExportCondition{
-					Type:   lighthousev2a1.ServiceExportInitialized,
-					Status: corev1.ConditionFalse,
-				})
+				t.awaitServiceUnavailableStatus()
 
 				t.createService()
 				t.awaitServiceExported(t.service.Spec.ClusterIP)
@@ -75,10 +71,7 @@ var _ = Describe("ServiceImport syncing", func() {
 			t.deleteService()
 			t.awaitNoServiceImport(t.brokerServiceImportClient)
 			t.awaitNoServiceImport(t.localServiceImportClient)
-			t.awaitServiceExportStatus(&lighthousev2a1.ServiceExportCondition{
-				Type:   lighthousev2a1.ServiceExportInitialized,
-				Status: corev1.ConditionFalse,
-			})
+			t.awaitServiceUnavailableStatus()
 		})
 	})
 })
@@ -106,9 +99,11 @@ var _ = Describe("Globalnet enabled", func() {
 
 	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() {
+			reason := "ServiceGlobalIPUnavailable"
 			t.awaitServiceExportStatus(&lighthousev2a1.ServiceExportCondition{
 				Type:   lighthousev2a1.ServiceExportInitialized,
 				Status: corev1.ConditionFalse,
+				Reason: &reason,
 			})
 
 			t.service.SetAnnotations(map[string]string{"submariner.io/globalIp": globalIP})
@@ -133,6 +128,7 @@ type testDriver struct {
 	service                   *corev1.Service
 	serviceExport             *lighthousev2a1.ServiceExport
 	stopCh                    chan struct{}
+	now                       time.Time
 }
 
 func newTestDiver() *testDriver {
@@ -143,6 +139,7 @@ func newTestDiver() *testDriver {
 	}}
 
 	BeforeEach(func() {
+		t.now = time.Now()
 		t.initialResources = nil
 		t.stopCh = make(chan struct{})
 
@@ -288,6 +285,9 @@ func (t *testDriver) awaitServiceExportStatus(expCond *lighthousev2a1.ServiceExp
 	Expect(found.Status.Conditions).To(HaveLen(1))
 	Expect(found.Status.Conditions[0].Type).To(Equal(expCond.Type))
 	Expect(found.Status.Conditions[0].Status).To(Equal(expCond.Status))
+	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(Equal(expCond.Reason))
 	Expect(found.Status.Conditions[0].Message).To(Not(BeNil()))
 }
 
@@ -295,8 +295,19 @@ func (t *testDriver) awaitServiceExported(serviceIP string) {
 	t.awaitServiceImport(t.brokerServiceImportClient, serviceIP)
 	t.awaitServiceImport(t.localServiceImportClient, serviceIP)
 
+	empty := ""
 	t.awaitServiceExportStatus(&lighthousev2a1.ServiceExportCondition{
 		Type:   lighthousev2a1.ServiceExportExported,
 		Status: corev1.ConditionTrue,
+		Reason: &empty,
+	})
+}
+
+func (t *testDriver) awaitServiceUnavailableStatus() {
+	reason := "ServiceUnavailable"
+	t.awaitServiceExportStatus(&lighthousev2a1.ServiceExportCondition{
+		Type:   lighthousev2a1.ServiceExportInitialized,
+		Status: corev1.ConditionFalse,
+		Reason: &reason,
 	})
 }