OCPBUGS-16017: update DataGather CR status in case of job failure (openshift#809)

Author: tremes

pkg/controller/gather_commands.go

@@ -141,7 +141,7 @@ func (d *GatherJob) GatherAndUpload(kubeConfig, protoKubeConfig *rest.Config) er
 		return err
 	}
 
-	dataGatherCR, err = updateDataGatherStatus(ctx, *insightClient, dataGatherCR.DeepCopy(), insightsv1alpha1.Pending, nil)
+	dataGatherCR, err = status.UpdateDataGatherStatus(ctx, insightClient, dataGatherCR.DeepCopy(), insightsv1alpha1.Pending, nil)
 	if err != nil {
 		klog.Error("failed to update coresponding DataGather custom resource: %v", err)
 		return err
@@ -180,7 +180,7 @@ func (d *GatherJob) GatherAndUpload(kubeConfig, protoKubeConfig *rest.Config) er
 	)
 	uploader := insightsuploader.New(nil, insightsClient, configObserver, nil, nil, 0)
 
-	dataGatherCR, err = updateDataGatherStatus(ctx, *insightClient, dataGatherCR, insightsv1alpha1.Running, nil)
+	dataGatherCR, err = status.UpdateDataGatherStatus(ctx, insightClient, dataGatherCR, insightsv1alpha1.Running, nil)
 	if err != nil {
 		klog.Error("failed to update coresponding DataGather custom resource: %v", err)
 		return err
@@ -214,7 +214,7 @@ func (d *GatherJob) GatherAndUpload(kubeConfig, protoKubeConfig *rest.Config) er
 	if err != nil {
 		conditions = append(conditions, status.DataRecordedCondition(metav1.ConditionFalse, "RecordingFailed",
 			fmt.Sprintf("Failed to record data: %v", err)))
-		_, recErr := updateDataGatherStatus(ctx, *insightClient, dataGatherCR, insightsv1alpha1.Failed, conditions)
+		_, recErr := status.UpdateDataGatherStatus(ctx, insightClient, dataGatherCR, insightsv1alpha1.Failed, conditions)
 		if recErr != nil {
 			klog.Error("data recording failed and the update of DataGaher resource status failed as well: %v", recErr)
 		}
@@ -229,7 +229,7 @@ func (d *GatherJob) GatherAndUpload(kubeConfig, protoKubeConfig *rest.Config) er
 		klog.Error(err)
 		conditions = append(conditions, status.DataUploadedCondition(metav1.ConditionFalse, reason,
 			fmt.Sprintf("Failed to upload data: %v", err)))
-		_, updateErr := updateDataGatherStatus(ctx, *insightClient, dataGatherCR, insightsv1alpha1.Failed, conditions)
+		_, updateErr := status.UpdateDataGatherStatus(ctx, insightClient, dataGatherCR, insightsv1alpha1.Failed, conditions)
 		if updateErr != nil {
 			klog.Error("data upload failed and the update of DataGaher resource status failed as well: %v", updateErr)
 		}
@@ -240,7 +240,7 @@ func (d *GatherJob) GatherAndUpload(kubeConfig, protoKubeConfig *rest.Config) er
 	dataGatherCR.Status.InsightsRequestID = insightsRequestID
 	conditions = append(conditions, status.DataUploadedCondition(metav1.ConditionTrue, reason, ""))
 
-	_, err = updateDataGatherStatus(ctx, *insightClient, dataGatherCR, insightsv1alpha1.Completed, conditions)
+	_, err = status.UpdateDataGatherStatus(ctx, insightClient, dataGatherCR, insightsv1alpha1.Completed, conditions)
 	if err != nil {
 		klog.Error(err)
 		return err
@@ -271,26 +271,3 @@ func record(functionReports []gather.GathererFunctionReport,
 	}
 	return recdriver.LastArchive()
 }
-
-// updateDataGatherStatus updates status' time attributes, state and conditions
-// of the provided DataGather resource
-func updateDataGatherStatus(ctx context.Context,
-	insightsClient insightsv1alpha1cli.InsightsV1alpha1Client,
-	dataGatherCR *insightsv1alpha1.DataGather,
-	newState insightsv1alpha1.DataGatherState, conditions []metav1.Condition) (*insightsv1alpha1.DataGather, error) {
-	switch newState {
-	case insightsv1alpha1.Completed:
-		dataGatherCR.Status.FinishTime = metav1.Now()
-	case insightsv1alpha1.Failed:
-		dataGatherCR.Status.FinishTime = metav1.Now()
-	case insightsv1alpha1.Running:
-		dataGatherCR.Status.StartTime = metav1.Now()
-	case insightsv1alpha1.Pending:
-		// no op
-	}
-	dataGatherCR.Status.State = newState
-	if conditions != nil {
-		dataGatherCR.Status.Conditions = append(dataGatherCR.Status.Conditions, conditions...)
-	}
-	return insightsClient.DataGathers().UpdateStatus(ctx, dataGatherCR, metav1.UpdateOptions{})
-}

pkg/controller/periodic/periodic.go

@@ -304,6 +304,14 @@ func (c *Controller) GatherJob() {
 	updateMetrics(dataGatherFinished)
 	if !dataGatheredOK {
 		klog.Errorf("Last data gathering %v was not successful", dataGatherFinished.Name)
+		conditions := []metav1.Condition{
+			status.DataRecordedCondition(metav1.ConditionFalse, "JobNotSucceeded", ""),
+			status.DataUploadedCondition(metav1.ConditionFalse, "JobNotSucceeded", ""),
+		}
+		_, err = status.UpdateDataGatherStatus(ctx, c.dataGatherClient, dataGatherFinished, insightsv1alpha1.Failed, conditions)
+		if err != nil {
+			klog.Error("Failed to update failed DataGather resource %s: %v ", dataGatherFinished.Name, err)
+		}
 		return
 	}
 
@@ -511,7 +519,7 @@ func (c *Controller) createDataGatherAttributeValues() ([]string, insightsv1alph
 // wasDataGatherSuccessful reads status conditions of the provided "dataGather" "datagather.insights.openshift.io"
 // custom resource and checks whether the data was successfully uploaded or not and updates status accordingly
 func (c *Controller) wasDataGatherSuccessful(dataGather *insightsv1alpha1.DataGather) bool {
-	dataUploadedCon := getConditionByStatus(dataGather, status.DataUploaded)
+	dataUploadedCon := status.GetConditionByStatus(dataGather, status.DataUploaded)
 	statusSummary := controllerstatus.Summary{
 		Operation: controllerstatus.Uploading,
 		Healthy:   true,
@@ -533,24 +541,11 @@ func (c *Controller) wasDataGatherSuccessful(dataGather *insightsv1alpha1.DataGa
 	return statusSummary.Healthy
 }
 
-// getConditionByStatus tries to get the condition with the provided condition status
-// from the provided "datagather" resource. Returns nil when no condition is found.
-func getConditionByStatus(dataGather *insightsv1alpha1.DataGather, conStatus string) *metav1.Condition {
-	var dataUploadedCon *metav1.Condition
-	for i := range dataGather.Status.Conditions {
-		con := dataGather.Status.Conditions[i]
-		if con.Type == conStatus {
-			dataUploadedCon = &con
-		}
-	}
-	return dataUploadedCon
-}
-
 // updateMetrics reads the HTTP status code from the reason of the "DataUploaded" condition
 // from the provided "datagather" resource and increments
 // the "insightsclient_request_send_total" Prometheus metric accordingly.
 func updateMetrics(dataGather *insightsv1alpha1.DataGather) {
-	dataUploadedCon := getConditionByStatus(dataGather, status.DataUploaded)
+	dataUploadedCon := status.GetConditionByStatus(dataGather, status.DataUploaded)
 	var statusCode int
 	if dataUploadedCon == nil {
 		statusCode = 0

pkg/controller/status/datagather_conditions.go

@@ -0,0 +1,72 @@
+package status
+
+import (
+	"context"
+
+	insightsv1alpha1 "github.com/openshift/api/insights/v1alpha1"
+	insightsv1alpha1cli "github.com/openshift/client-go/insights/clientset/versioned/typed/insights/v1alpha1"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+)
+
+const (
+	DataUploaded = "DataUploaded"
+	DataRecorded = "DataRecorded"
+)
+
+// DataUploadedCondition returns new "DataUploaded" status condition with provided status, reason and message
+func DataUploadedCondition(status metav1.ConditionStatus, reason, message string) metav1.Condition {
+	return metav1.Condition{
+		Type:               DataUploaded,
+		LastTransitionTime: metav1.Now(),
+		Status:             status,
+		Reason:             reason,
+		Message:            message,
+	}
+}
+
+// DataRecordedCondition returns new "DataRecorded" status condition with provided status, reason and message
+func DataRecordedCondition(status metav1.ConditionStatus, reason, message string) metav1.Condition {
+	return metav1.Condition{
+		Type:               DataRecorded,
+		LastTransitionTime: metav1.Now(),
+		Status:             status,
+		Reason:             reason,
+		Message:            message,
+	}
+}
+
+// updateDataGatherStatus updates status' time attributes, state and conditions
+// of the provided DataGather resource
+func UpdateDataGatherStatus(ctx context.Context,
+	insightsClient insightsv1alpha1cli.InsightsV1alpha1Interface,
+	dataGatherCR *insightsv1alpha1.DataGather,
+	newState insightsv1alpha1.DataGatherState, conditions []metav1.Condition) (*insightsv1alpha1.DataGather, error) {
+	switch newState {
+	case insightsv1alpha1.Completed:
+		dataGatherCR.Status.FinishTime = metav1.Now()
+	case insightsv1alpha1.Failed:
+		dataGatherCR.Status.FinishTime = metav1.Now()
+	case insightsv1alpha1.Running:
+		dataGatherCR.Status.StartTime = metav1.Now()
+	case insightsv1alpha1.Pending:
+		// no op
+	}
+	dataGatherCR.Status.State = newState
+	if conditions != nil {
+		dataGatherCR.Status.Conditions = append(dataGatherCR.Status.Conditions, conditions...)
+	}
+	return insightsClient.DataGathers().UpdateStatus(ctx, dataGatherCR, metav1.UpdateOptions{})
+}
+
+// GetConditionByStatus tries to get the condition with the provided condition status
+// from the provided "datagather" resource. Returns nil when no condition is found.
+func GetConditionByStatus(dataGather *insightsv1alpha1.DataGather, conStatus string) *metav1.Condition {
+	var dataUploadedCon *metav1.Condition
+	for i := range dataGather.Status.Conditions {
+		con := dataGather.Status.Conditions[i]
+		if con.Type == conStatus {
+			dataUploadedCon = &con
+		}
+	}
+	return dataUploadedCon
+}

pkg/controller/status/datagather_status.go

@@ -0,0 +1,97 @@
+package status
+
+import (
+	"context"
+	"testing"
+
+	"github.com/openshift/api/insights/v1alpha1"
+	insightsFakeCli "github.com/openshift/client-go/insights/clientset/versioned/fake"
+	"github.com/stretchr/testify/assert"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+)
+
+func TestUpdateDataGatherStatus(t *testing.T) {
+	tests := []struct {
+		name                    string
+		dataGather              *v1alpha1.DataGather
+		dgState                 v1alpha1.DataGatherState
+		updatingConditions      []metav1.Condition
+		expectedDataRecordedCon metav1.Condition
+		expectedDataUploadedCon metav1.Condition
+	}{
+		{
+			name: "updating DataGather to completed state",
+			dataGather: &v1alpha1.DataGather{
+				ObjectMeta: metav1.ObjectMeta{
+					Name: "test-datagather-1",
+				},
+			},
+			dgState: v1alpha1.Completed,
+			updatingConditions: []metav1.Condition{
+				DataRecordedCondition(metav1.ConditionTrue, "AsExpected", ""),
+				DataUploadedCondition(metav1.ConditionTrue, "HttpStatus200", "testing message"),
+			},
+			expectedDataRecordedCon: metav1.Condition{
+				Type:   DataRecorded,
+				Status: metav1.ConditionTrue,
+				Reason: "AsExpected",
+			},
+			expectedDataUploadedCon: metav1.Condition{
+				Type:    DataUploaded,
+				Status:  metav1.ConditionTrue,
+				Reason:  "HttpStatus200",
+				Message: "testing message",
+			},
+		},
+		{
+			name: "updating DataGather to failed state",
+			dataGather: &v1alpha1.DataGather{
+				ObjectMeta: metav1.ObjectMeta{
+					Name: "test-datagather-1",
+				},
+			},
+			dgState: v1alpha1.Failed,
+			updatingConditions: []metav1.Condition{
+				DataRecordedCondition(metav1.ConditionFalse, "Failure", "testing error message"),
+				DataUploadedCondition(metav1.ConditionFalse, "HttpStatus403", "testing message"),
+			},
+			expectedDataRecordedCon: metav1.Condition{
+				Type:    DataRecorded,
+				Status:  metav1.ConditionFalse,
+				Reason:  "Failure",
+				Message: "testing error message",
+			},
+			expectedDataUploadedCon: metav1.Condition{
+				Type:    DataUploaded,
+				Status:  metav1.ConditionFalse,
+				Reason:  "HttpStatus403",
+				Message: "testing message",
+			},
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			cs := insightsFakeCli.NewSimpleClientset()
+			createdDG, err := cs.InsightsV1alpha1().DataGathers().Create(context.Background(), tt.dataGather, metav1.CreateOptions{})
+			assert.NoError(t, err)
+			updatedDG, err := UpdateDataGatherStatus(context.Background(), cs.InsightsV1alpha1(),
+				createdDG, tt.dgState, tt.updatingConditions)
+			assert.NoError(t, err)
+			assert.NotNil(t, updatedDG.Status.StartTime)
+			assert.NotNil(t, updatedDG.Status.FinishTime)
+
+			dataRecordedCon := GetConditionByStatus(updatedDG, DataRecorded)
+			assert.NotNil(t, dataRecordedCon)
+			assert.Equal(t, tt.expectedDataRecordedCon.Reason, dataRecordedCon.Reason)
+			assert.Equal(t, tt.expectedDataRecordedCon.Status, dataRecordedCon.Status)
+			assert.Equal(t, tt.expectedDataRecordedCon.Message, dataRecordedCon.Message)
+
+			dataUploadedCon := GetConditionByStatus(updatedDG, DataUploaded)
+			assert.NotNil(t, dataRecordedCon)
+			assert.Equal(t, tt.expectedDataUploadedCon.Reason, dataUploadedCon.Reason)
+			assert.Equal(t, tt.expectedDataUploadedCon.Status, dataUploadedCon.Status)
+			assert.Equal(t, tt.expectedDataUploadedCon.Message, dataUploadedCon.Message)
+		})
+	}
+}