Validate nil pointer exceptions during BuildRun Reconcile

When generating the rampup metrics, we needed to validate for nil pointers in order to avoid some panic() scenarios. Adding a test case for this validation. When metrics are not used, some variables are not initialized, leading to panic() scenarios when using them later in the reconciliation of the BuildRun controller. Adding some if conditionals to validate nils.
shipwright-io · Sep 16, 2020 · f082e14 · f082e14
1 parent a7787b3
commit f082e14
Show file tree

Hide file tree

Showing 4 changed files with 214 additions and 59 deletions.
diff --git a/pkg/controller/buildrun/buildrun_controller.go b/pkg/controller/buildrun/buildrun_controller.go
@@ -1,5 +1,5 @@
 // Copyright The Shipwright Contributors
-// 
+//
 // SPDX-License-Identifier: Apache-2.0
 
 package buildrun
@@ -367,53 +367,60 @@ func (r *ReconcileBuildRun) Reconcile(request reconcile.Request) (reconcile.Resu
 
 			buildRun.Status.LatestTaskRunRef = &lastTaskRun.Name
 			buildRun.Status.StartTime = lastTaskRun.Status.StartTime
-
 			if lastTaskRun.Status.CompletionTime != nil && buildRun.Status.CompletionTime == nil {
 				buildRun.Status.CompletionTime = lastTaskRun.Status.CompletionTime
 
-				// Increase BuildRun count in metrics
-				buildmetrics.BuildRunCountInc(buildRun.Status.BuildSpec.StrategyRef.Name)
-
-				// buildrun established duration (time between the creation of the buildrun and the start of the buildrun)
-				buildmetrics.BuildRunEstablishObserve(
-					buildRun.Status.BuildSpec.StrategyRef.Name,
-					buildRun.Namespace,
-					buildRun.Status.StartTime.Time.Sub(buildRun.CreationTimestamp.Time),
-				)
-
-				// buildrun completetion duration (total time between the creation of the buildrun and the buildrun completion)
-				buildmetrics.BuildRunCompletionObserve(
-					buildRun.Status.BuildSpec.StrategyRef.Name,
-					buildRun.Namespace,
-					buildRun.Status.CompletionTime.Time.Sub(buildRun.CreationTimestamp.Time),
-				)
-
-				// buildrun ramp-up duration (time between buildrun creation and taskrun creation)
-				buildmetrics.BuildRunRampUpDurationObserve(
-					buildRun.Status.BuildSpec.StrategyRef.Name,
-					buildRun.Namespace,
-					lastTaskRun.CreationTimestamp.Time.Sub(buildRun.CreationTimestamp.Time),
-				)
-
-				// Look for the pod created by the taskrun
-				var pod = &corev1.Pod{}
-				if err := r.client.Get(ctx, types.NamespacedName{Namespace: request.Namespace, Name: lastTaskRun.Status.PodName}, pod); err == nil {
-					lastInitPodIdx := len(pod.Status.InitContainerStatuses) - 1
-					lastInitPod := pod.Status.InitContainerStatuses[lastInitPodIdx]
-
-					// taskrun ramp-up duration (time between taskrun creation and taskrun pod creation)
-					buildmetrics.TaskRunRampUpDurationObserve(
+				if buildRun.Status.BuildSpec.StrategyRef != nil {
+					// Increase BuildRun count in metrics
+					buildmetrics.BuildRunCountInc(buildRun.Status.BuildSpec.StrategyRef.Name)
+
+					// buildrun established duration (time between the creation of the buildrun and the start of the buildrun)
+					buildmetrics.BuildRunEstablishObserve(
 						buildRun.Status.BuildSpec.StrategyRef.Name,
 						buildRun.Namespace,
-						pod.CreationTimestamp.Time.Sub(lastTaskRun.CreationTimestamp.Time),
+						buildRun.Status.StartTime.Time.Sub(buildRun.CreationTimestamp.Time),
 					)
 
-					// taskrun pod ramp-up (time between pod creation and last init container completion)
-					buildmetrics.TaskRunPodRampUpDurationObserve(
+					// buildrun completetion duration (total time between the creation of the buildrun and the buildrun completion)
+					buildmetrics.BuildRunCompletionObserve(
 						buildRun.Status.BuildSpec.StrategyRef.Name,
 						buildRun.Namespace,
-						lastInitPod.State.Terminated.FinishedAt.Sub(pod.CreationTimestamp.Time),
+						buildRun.Status.CompletionTime.Time.Sub(buildRun.CreationTimestamp.Time),
 					)
+
+					// buildrun ramp-up duration (time between buildrun creation and taskrun creation)
+					buildmetrics.BuildRunRampUpDurationObserve(
+						buildRun.Status.BuildSpec.StrategyRef.Name,
+						buildRun.Namespace,
+						lastTaskRun.CreationTimestamp.Time.Sub(buildRun.CreationTimestamp.Time),
+					)
+
+					// Look for the pod created by the taskrun
+					var pod = &corev1.Pod{}
+					if err := r.client.Get(ctx, types.NamespacedName{Namespace: request.Namespace, Name: lastTaskRun.Status.PodName}, pod); err == nil {
+						if len(pod.Status.InitContainerStatuses) > 0 {
+
+							lastInitPodIdx := len(pod.Status.InitContainerStatuses) - 1
+							lastInitPod := pod.Status.InitContainerStatuses[lastInitPodIdx]
+
+							if lastInitPod.State.Terminated != nil {
+								// taskrun pod ramp-up (time between pod creation and last init container completion)
+								buildmetrics.TaskRunPodRampUpDurationObserve(
+									buildRun.Status.BuildSpec.StrategyRef.Name,
+									buildRun.Namespace,
+									lastInitPod.State.Terminated.FinishedAt.Sub(pod.CreationTimestamp.Time),
+								)
+							}
+						}
+
+						// taskrun ramp-up duration (time between taskrun creation and taskrun pod creation)
+						buildmetrics.TaskRunRampUpDurationObserve(
+							buildRun.Status.BuildSpec.StrategyRef.Name,
+							buildRun.Namespace,
+							pod.CreationTimestamp.Time.Sub(lastTaskRun.CreationTimestamp.Time),
+						)
+
+					}
 				}
 			}
 

diff --git a/pkg/controller/buildrun/buildrun_controller_test.go b/pkg/controller/buildrun/buildrun_controller_test.go
@@ -1,5 +1,5 @@
 // Copyright The Shipwright Contributors
-// 
+//
 // SPDX-License-Identifier: Apache-2.0
 
 package buildrun_test
@@ -43,7 +43,7 @@ var _ = Describe("Reconcile BuildRun", func() {
 		buildRunSample                                         *build.BuildRun
 		taskRunSample                                          *v1beta1.TaskRun
 		statusWriter                                           *fakes.FakeStatusWriter
-		fakeBuildStrategyKind build.BuildStrategyKind
+		fakeBuildStrategyKind                                  build.BuildStrategyKind
 		taskRunName, buildRunName, buildName, strategyName, ns string
 	)
 
@@ -337,6 +337,32 @@ var _ = Describe("Reconcile BuildRun", func() {
 				Expect(err).ToNot(HaveOccurred())
 				Expect(reconcile.Result{}).To(Equal(result))
 			})
+
+			It("does not break the reconcile when a taskrun pod initcontainers are not ready", func() {
+				taskRunSample = ctl.TaskRunWithCompletionAndStartTime(taskRunName, buildRunName, ns)
+
+				buildRunSample = ctl.BuildRunWithBuildSnapshot(buildRunName, buildName)
+
+				// Override Stub get calls to include a completed TaskRun
+				// and a Pod with one initContainer Status
+				client.GetCalls(ctl.StubBuildCRDsPodAndTaskRun(
+					buildSample,
+					buildRunSample,
+					ctl.DefaultServiceAccount("foobar"),
+					ctl.DefaultClusterBuildStrategy(),
+					ctl.DefaultNamespacedBuildStrategy(),
+					taskRunSample,
+					ctl.PodWithInitContainerStatus("foobar", "init-foobar")),
+				)
+
+				result, err := reconciler.Reconcile(taskRunRequest)
+				Expect(err).ToNot(HaveOccurred())
+				Expect(reconcile.Result{}).To(Equal(result))
+
+				// Three client calls because based on the Stub, we should
+				// trigger a call to get the related TaskRun pod.
+				Expect(client.GetCallCount()).To(Equal(3))
+			})
 		})
 		Context("from an existing BuildRun resource", func() {
 			var (

diff --git a/pkg/metrics/metrics.go b/pkg/metrics/metrics.go
@@ -144,30 +144,42 @@ func BuildCountInc(buildStrategy string) {
 
 // BuildRunCountInc increases a number of the existing build run total count
 func BuildRunCountInc(buildStrategy string) {
-	buildRunCount.WithLabelValues(buildStrategy).Inc()
+	if buildRunCount != nil {
+		buildRunCount.WithLabelValues(buildStrategy).Inc()
+	}
 }
 
 // BuildRunEstablishObserve sets the build run establish time
 func BuildRunEstablishObserve(buildStrategy string, namespace string, duration time.Duration) {
-	buildRunEstablishDuration.With(createHistogramLabels(buildStrategy, namespace)).Observe(duration.Seconds())
+	if buildRunEstablishDuration != nil {
+		buildRunEstablishDuration.With(createHistogramLabels(buildStrategy, namespace)).Observe(duration.Seconds())
+	}
 }
 
 // BuildRunCompletionObserve sets the build run completion time
 func BuildRunCompletionObserve(buildStrategy string, namespace string, duration time.Duration) {
-	buildRunCompletionDuration.With(createHistogramLabels(buildStrategy, namespace)).Observe(duration.Seconds())
+	if buildRunCompletionDuration != nil {
+		buildRunCompletionDuration.With(createHistogramLabels(buildStrategy, namespace)).Observe(duration.Seconds())
+	}
 }
 
 // BuildRunRampUpDurationObserve processes the observation of a new buildrun ramp-up duration
 func BuildRunRampUpDurationObserve(buildStrategy string, namespace string, duration time.Duration) {
-	buildRunRampUpDuration.With(createHistogramLabels(buildStrategy, namespace)).Observe(duration.Seconds())
+	if buildRunRampUpDuration != nil {
+		buildRunRampUpDuration.With(createHistogramLabels(buildStrategy, namespace)).Observe(duration.Seconds())
+	}
 }
 
 // TaskRunRampUpDurationObserve processes the observation of a new taskrun ramp-up duration
 func TaskRunRampUpDurationObserve(buildStrategy string, namespace string, duration time.Duration) {
-	taskRunRampUpDuration.With(createHistogramLabels(buildStrategy, namespace)).Observe(duration.Seconds())
+	if taskRunRampUpDuration != nil {
+		taskRunRampUpDuration.With(createHistogramLabels(buildStrategy, namespace)).Observe(duration.Seconds())
+	}
 }
 
 // TaskRunPodRampUpDurationObserve processes the observation of a new taskrun pod ramp-up duration
 func TaskRunPodRampUpDurationObserve(buildStrategy string, namespace string, duration time.Duration) {
-	taskRunPodRampUpDuration.With(createHistogramLabels(buildStrategy, namespace)).Observe(duration.Seconds())
+	if taskRunPodRampUpDuration != nil {
+		taskRunPodRampUpDuration.With(createHistogramLabels(buildStrategy, namespace)).Observe(duration.Seconds())
+	}
 }