Skip to content

Commit

Permalink
Merge pull request #710 from HeavyWombat/fix/nil-pointer-buildrun-rec…
Browse files Browse the repository at this point in the history
…onciler

Fix nil-pointer dereference of BuildSpec Strategy
  • Loading branch information
openshift-merge-robot authored Apr 2, 2021
2 parents 82c1e01 + b36254c commit 75cdae1
Show file tree
Hide file tree
Showing 3 changed files with 65 additions and 39 deletions.
10 changes: 10 additions & 0 deletions pkg/apis/build/v1alpha1/build_types.go
Original file line number Diff line number Diff line change
Expand Up @@ -105,6 +105,16 @@ type BuildSpec struct {
Timeout *metav1.Duration `json:"timeout,omitempty"`
}

// StrategyName returns the name of the configured strategy, or 'undefined' in
// case the strategy is nil (not set)
func (buildSpec *BuildSpec) StrategyName() string {
if buildSpec.Strategy == nil {
return "undefined (nil strategy)"
}

return buildSpec.Strategy.Name
}

// Image refers to an container image with credentials
type Image struct {
// Image is the reference of the image.
Expand Down
80 changes: 41 additions & 39 deletions pkg/reconciler/buildrun/buildrun.go
Original file line number Diff line number Diff line change
Expand Up @@ -204,11 +204,16 @@ func (r *ReconcileBuildRun) Reconcile(request reconcile.Request) (reconcile.Resu
}

// Increase BuildRun count in metrics
buildmetrics.BuildRunCountInc(buildRun.Status.BuildSpec.Strategy.Name, buildRun.Namespace, buildRun.Spec.BuildRef.Name, buildRun.Name)
buildmetrics.BuildRunCountInc(
buildRun.Status.BuildSpec.StrategyName(),
buildRun.Namespace,
buildRun.Spec.BuildRef.Name,
buildRun.Name,
)

// Report buildrun ramp-up duration (time between buildrun creation and taskrun creation)
buildmetrics.BuildRunRampUpDurationObserve(
buildRun.Status.BuildSpec.Strategy.Name,
buildRun.Status.BuildSpec.StrategyName(),
buildRun.Namespace,
buildRun.Spec.BuildRef.Name,
buildRun.Name,
Expand Down Expand Up @@ -274,7 +279,7 @@ func (r *ReconcileBuildRun) Reconcile(request reconcile.Request) (reconcile.Resu

// Report the buildrun established duration (time between the creation of the buildrun and the start of the buildrun)
buildmetrics.BuildRunEstablishObserve(
buildRun.Status.BuildSpec.Strategy.Name,
buildRun.Status.BuildSpec.StrategyName(),
buildRun.Namespace,
buildRun.Spec.BuildRef.Name,
buildRun.Name,
Expand All @@ -285,46 +290,43 @@ func (r *ReconcileBuildRun) Reconcile(request reconcile.Request) (reconcile.Resu
if lastTaskRun.Status.CompletionTime != nil && buildRun.Status.CompletionTime == nil {
buildRun.Status.CompletionTime = lastTaskRun.Status.CompletionTime

if buildRun.Status.BuildSpec.Strategy != nil {
// buildrun completion duration (total time between the creation of the buildrun and the buildrun completion)
buildmetrics.BuildRunCompletionObserve(
buildRun.Status.BuildSpec.Strategy.Name,
// buildrun completion duration (total time between the creation of the buildrun and the buildrun completion)
buildmetrics.BuildRunCompletionObserve(
buildRun.Status.BuildSpec.StrategyName(),
buildRun.Namespace,
buildRun.Spec.BuildRef.Name,
buildRun.Name,
buildRun.Status.CompletionTime.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.StrategyName(),
buildRun.Namespace,
buildRun.Spec.BuildRef.Name,
buildRun.Name,
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.StrategyName(),
buildRun.Namespace,
buildRun.Spec.BuildRef.Name,
buildRun.Name,
buildRun.Status.CompletionTime.Time.Sub(buildRun.CreationTimestamp.Time),
pod.CreationTimestamp.Time.Sub(lastTaskRun.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.Strategy.Name,
buildRun.Namespace,
buildRun.Spec.BuildRef.Name,
buildRun.Name,
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.Strategy.Name,
buildRun.Namespace,
buildRun.Spec.BuildRef.Name,
buildRun.Name,
pod.CreationTimestamp.Time.Sub(lastTaskRun.CreationTimestamp.Time),
)

}
}
}

Expand Down
14 changes: 14 additions & 0 deletions pkg/reconciler/buildrun/buildrun_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -244,7 +244,21 @@ var _ = Describe("Reconcile BuildRun", func() {
Expect(serviceAccount.Name).To(Equal(buildRunSample.Name + "-sa"))
Expect(serviceAccount.Namespace).To(Equal(buildRunSample.Namespace))
})

It("should not panic in case the build spec strategy is nil", func() {
// As long as the Strategy is a pointer, it can happen that the
// field is nil. During processing, the BuildSpec is copied
// from the respective Build. In case Strategy is nil, the
// controller should not panic.
buildRunSample.Status.BuildSpec.Strategy = nil
Expect(func() {
result, err := reconciler.Reconcile(taskRunRequest)
Expect(err).ToNot(HaveOccurred())
Expect(result).ToNot(BeNil())
}).ShouldNot(Panic())
})
})

Context("from an existing TaskRun with Conditions", func() {
BeforeEach(func() {

Expand Down

0 comments on commit 75cdae1

Please sign in to comment.