K8SGCSReporter: Defer handling of aborted jobs until they are complete

kubernetes · Aug 28, 2020 · 2db2d93 · 2db2d93
1 parent 0ff0a30
commit 2db2d93
Show file tree

Hide file tree

Showing 3 changed files with 61 additions and 18 deletions.
diff --git a/prow/crier/reporters/gcs/kubernetes/BUILD.bazel b/prow/crier/reporters/gcs/kubernetes/BUILD.bazel
@@ -36,6 +36,7 @@ go_test(
         "@io_k8s_api//core/v1:go_default_library",
         "@io_k8s_apimachinery//pkg/apis/meta/v1:go_default_library",
         "@io_k8s_apimachinery//pkg/types:go_default_library",
+        "@io_k8s_sigs_controller_runtime//pkg/reconcile:go_default_library",
     ],
 )
 

diff --git a/prow/crier/reporters/gcs/kubernetes/reporter.go b/prow/crier/reporters/gcs/kubernetes/reporter.go
@@ -94,27 +94,33 @@ func (rg k8sResourceGetter) GetEvents(cluster, namespace string, pod *v1.Pod) ([
 }
 
 func (gr *gcsK8sReporter) Report(log *logrus.Entry, pj *prowv1.ProwJob) ([]*prowv1.ProwJob, *reconcile.Result, error) {
-	return []*prowv1.ProwJob{pj}, nil, gr.report(log, pj)
+	result, err := gr.report(log, pj)
+	return []*prowv1.ProwJob{pj}, result, err
 }
 
-func (gr *gcsK8sReporter) report(log *logrus.Entry, pj *prowv1.ProwJob) error {
+func (gr *gcsK8sReporter) report(log *logrus.Entry, pj *prowv1.ProwJob) (*reconcile.Result, error) {
 	ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) // TODO: pass through a global context?
 	defer cancel()
 
-	if !pj.Complete() {
+	if !pj.Complete() && pj.Status.State != prowv1.AbortedState {
 		if err := gr.addFinalizer(pj); err != nil {
-			return fmt.Errorf("failed to add finalizer to pod: %w", err)
+			return nil, fmt.Errorf("failed to add finalizer to pod: %w", err)
 		}
-		return nil
+		return nil, nil
+	}
+
+	// Aborted jobs are not completed initially
+	if !pj.Complete() {
+		return &reconcile.Result{RequeueAfter: 10 * time.Second}, nil
 	}
 
 	_, _, err := util.GetJobDestination(gr.cfg, pj)
 	if err != nil {
 		log.WithError(err).Warn("Not uploading because we couldn't find a destination")
-		return nil
+		return nil, nil
 	}
 
-	return gr.reportPodInfo(ctx, log, pj)
+	return nil, gr.reportPodInfo(ctx, log, pj)
 }
 
 func (gr *gcsK8sReporter) addFinalizer(pj *prowv1.ProwJob) error {

diff --git a/prow/crier/reporters/gcs/kubernetes/reporter_test.go b/prow/crier/reporters/gcs/kubernetes/reporter_test.go
@@ -30,6 +30,7 @@ import (
 	"k8s.io/apimachinery/pkg/types"
 	"k8s.io/test-infra/prow/config"
 	"k8s.io/test-infra/prow/crier/reporters/gcs/internal/testutil"
+	"sigs.k8s.io/controller-runtime/pkg/reconcile"
 
 	prowv1 "k8s.io/test-infra/prow/apis/prowjobs/v1"
 )
@@ -176,16 +177,18 @@ func (rg testResourceGetter) PatchPod(cluster, namespace, name string, pt types.
 
 func TestReportPodInfo(t *testing.T) {
 	tests := []struct {
-		name          string
-		pjName        string
-		pjComplete    bool
-		pjPending     bool
-		pod           *v1.Pod
-		events        []v1.Event
-		dryRun        bool
-		expectReport  bool
-		expectErr     bool
-		expectedPatch string
+		name                    string
+		pjName                  string
+		pjComplete              bool
+		pjPending               bool
+		pjState                 prowv1.ProwJobState
+		pod                     *v1.Pod
+		events                  []v1.Event
+		dryRun                  bool
+		expectReport            bool
+		expectErr               bool
+		expectedPatch           string
+		expectedReconcileResult *reconcile.Result
 	}{
 		{
 			name:       "prowjob picks up pod and events",
@@ -276,6 +279,32 @@ func TestReportPodInfo(t *testing.T) {
 			expectReport:  true,
 			expectedPatch: `{"metadata":{"finalizers":null}}`,
 		},
+		{
+			name:                    "RequeueAfter is returned for incomplete aborted job and nothing happens",
+			pjName:                  "ba123965-4fd4-421f-8509-7590c129ab69",
+			pjState:                 prowv1.AbortedState,
+			pjPending:               false,
+			pjComplete:              false,
+			expectReport:            false,
+			expectedReconcileResult: &reconcile.Result{RequeueAfter: 10 * time.Second},
+		},
+		{
+			name:       "Completed aborted job is reported",
+			pjName:     "ba123965-4fd4-421f-8509-7590c129ab69",
+			pjState:    prowv1.AbortedState,
+			pjPending:  false,
+			pjComplete: true,
+			pod: &v1.Pod{
+				ObjectMeta: metav1.ObjectMeta{
+					Finalizers: []string{"gcsk8sreporter"},
+					Name:       "ba123965-4fd4-421f-8509-7590c129ab69",
+					Namespace:  "test-pods",
+					Labels:     map[string]string{"created-by-prow": "true"},
+				},
+			},
+			expectReport:  true,
+			expectedPatch: `{"metadata":{"finalizers":null}}`,
+		},
 	}
 
 	for _, tc := range tests {
@@ -301,6 +330,9 @@ func TestReportPodInfo(t *testing.T) {
 			if tc.pjPending {
 				pj.Status.PendingTime = &metav1.Time{}
 			}
+			if tc.pjState != "" {
+				pj.Status.State = tc.pjState
+			}
 
 			fca := testutil.Fca{C: config.Config{ProwConfig: config.ProwConfig{
 				PodNamespace: "test-pods",
@@ -327,7 +359,7 @@ func TestReportPodInfo(t *testing.T) {
 			}
 			author := &testutil.TestAuthor{}
 			reporter := internalNew(fca.Config, author, rg, 1.0, tc.dryRun)
-			err := reporter.report(logrus.NewEntry(logrus.StandardLogger()), pj)
+			reconcileResult, err := reporter.report(logrus.NewEntry(logrus.StandardLogger()), pj)
 
 			if tc.expectErr {
 				if err == nil {
@@ -338,6 +370,10 @@ func TestReportPodInfo(t *testing.T) {
 				t.Fatalf("Unexpected error: %v", err)
 			}
 
+			if diff := cmp.Diff(reconcileResult, tc.expectedReconcileResult); diff != "" {
+				t.Errorf("reconcileResult differs from expected reconcileResult: %s", diff)
+			}
+
 			if !tc.expectReport {
 				if author.AlreadyUsed {
 					t.Fatalf("Expected nothing to be written, but something was written to %q:\n\n%s", author.Path, string(author.Content))