Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Validate nil pointer exceptions during BuildRun Reconcile #396

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
83 changes: 45 additions & 38 deletions pkg/controller/buildrun/buildrun_controller.go
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
// Copyright The Shipwright Contributors
//
//
// SPDX-License-Identifier: Apache-2.0

package buildrun
Expand Down Expand Up @@ -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 {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, I think you are uncovering a bug in the BuildSpec. The StrategyRef is not optional and therefore should not be a reference - similar to Source and Output that are also mandatory. Can you change this in build_types.go and remove this nil check here again ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removed, thats a good point. Unit tests know nothing about the tags. Also, when you say "Can you change this in build_types.go" , what do you meant?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean this loc:

StrategyRef *StrategyRef `json:"strategy"`
. It should not be *StrategyRef but StrategyRef. But, I just looked around, there are more similar problems like the BuildRef in the build run. So, you can also leave it as is and I open an issue to separately address all this problems. If you quickly confirm, I will approve this PR and address the wrong references separately.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, pls open the issue first. I would like to have the discussion around the pointer or not, interesting.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Issue opened: #397

Let's provide feed back there. This PR is fine then.

// 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),
)

}
}
}

Expand Down
30 changes: 28 additions & 2 deletions pkg/controller/buildrun/buildrun_controller_test.go
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
// Copyright The Shipwright Contributors
//
//
// SPDX-License-Identifier: Apache-2.0

package buildrun_test
Expand Down Expand Up @@ -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
)

Expand Down Expand Up @@ -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 (
Expand Down
24 changes: 18 additions & 6 deletions pkg/metrics/metrics.go
Original file line number Diff line number Diff line change
Expand Up @@ -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())
}
}
Loading