From f082e1463a14640ad4f85a7d1296daa63572898a Mon Sep 17 00:00:00 2001 From: ENCALADA Date: Wed, 16 Sep 2020 16:48:24 +0200 Subject: [PATCH] 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. --- .../buildrun/buildrun_controller.go | 83 ++++++----- .../buildrun/buildrun_controller_test.go | 30 +++- pkg/metrics/metrics.go | 24 +++- test/catalog.go | 136 ++++++++++++++++-- 4 files changed, 214 insertions(+), 59 deletions(-) diff --git a/pkg/controller/buildrun/buildrun_controller.go b/pkg/controller/buildrun/buildrun_controller.go index ef7ff509b..67e0d0628 100644 --- 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 index 011ec0739..f6ea80bbd 100644 --- 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 index 53dc3602d..45dd80c3d 100644 --- 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()) + } } diff --git a/test/catalog.go b/test/catalog.go index 6218baed3..219bb1bf1 100644 --- a/test/catalog.go +++ b/test/catalog.go @@ -1,5 +1,5 @@ // Copyright The Shipwright Contributors -// +// // SPDX-License-Identifier: Apache-2.0 package test @@ -7,10 +7,7 @@ package test import ( "context" "strconv" - - "knative.dev/pkg/apis" - knativev1beta1 "knative.dev/pkg/apis/duck/v1beta1" - "sigs.k8s.io/yaml" + "time" . "github.com/onsi/gomega" build "github.com/shipwright-io/build/pkg/apis/build/v1alpha1" @@ -22,7 +19,10 @@ import ( "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/apimachinery/pkg/types" + "knative.dev/pkg/apis" + knativev1beta1 "knative.dev/pkg/apis/duck/v1beta1" crc "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/yaml" ) // Catalog allows you to access helper functions @@ -149,13 +149,10 @@ func (c *Catalog) FakeClusterBuildStrategyList() *build.ClusterBuildStrategyList } } - // FakeNoClusterBuildStrategyList to support tests func (c *Catalog) FakeNoClusterBuildStrategyList() *build.ClusterBuildStrategyList { return &build.ClusterBuildStrategyList{ - Items: []build.ClusterBuildStrategy{ - - }, + Items: []build.ClusterBuildStrategy{}, } } @@ -189,8 +186,7 @@ func (c *Catalog) FakeBuildStrategyList() *build.BuildStrategyList { // FakeNoBuildStrategyList to support tests func (c *Catalog) FakeNoBuildStrategyList() *build.BuildStrategyList { return &build.BuildStrategyList{ - Items: []build.BuildStrategy{ - }, + Items: []build.BuildStrategy{}, } } @@ -210,8 +206,7 @@ func (c *Catalog) FakeSecretList() corev1.SecretList { // FakeSecretListInNamespace to support test func (c *Catalog) FakeNoSecretListInNamespace() corev1.SecretList { return corev1.SecretList{ - Items: []corev1.Secret{ - }, + Items: []corev1.Secret{}, } } @@ -449,6 +444,45 @@ func (c *Catalog) StubBuildRunGetWithSAandStrategies( } } +// StubBuildCRDsPodAndTaskRun stubs different objects in case a client +// GET call is executed against them +func (c *Catalog) StubBuildCRDsPodAndTaskRun( + b *build.Build, + br *build.BuildRun, + sa *corev1.ServiceAccount, + cb *build.ClusterBuildStrategy, + bs *build.BuildStrategy, + tr *v1beta1.TaskRun, + pod *corev1.Pod, +) func(context context.Context, nn types.NamespacedName, object runtime.Object) error { + return func(context context.Context, nn types.NamespacedName, object runtime.Object) error { + switch object := object.(type) { + case *build.Build: + b.DeepCopyInto(object) + return nil + case *build.BuildRun: + br.DeepCopyInto(object) + return nil + case *corev1.ServiceAccount: + sa.DeepCopyInto(object) + return nil + case *build.ClusterBuildStrategy: + cb.DeepCopyInto(object) + return nil + case *build.BuildStrategy: + bs.DeepCopyInto(object) + return nil + case *v1beta1.TaskRun: + tr.DeepCopyInto(object) + return nil + case *corev1.Pod: + pod.DeepCopyInto(object) + return nil + } + return errors.NewNotFound(schema.GroupResource{}, nn.Name) + } +} + // DefaultTaskRunWithStatus returns a minimal tekton TaskRun with an Status func (c *Catalog) DefaultTaskRunWithStatus(trName string, buildRunName string, ns string, status corev1.ConditionStatus, reason string) *v1beta1.TaskRun { return &v1beta1.TaskRun{ @@ -472,6 +506,40 @@ func (c *Catalog) DefaultTaskRunWithStatus(trName string, buildRunName string, n } } +// TaskRunWithCompletionAndStartTime provides a TaskRun object with a +// Completion and StartTime +func (c *Catalog) TaskRunWithCompletionAndStartTime(trName string, buildRunName string, ns string) *v1beta1.TaskRun { + return &v1beta1.TaskRun{ + ObjectMeta: metav1.ObjectMeta{ + Name: trName, + Namespace: ns, + Labels: map[string]string{"buildrun.build.dev/name": buildRunName}, + }, + Spec: v1beta1.TaskRunSpec{}, + Status: v1beta1.TaskRunStatus{ + TaskRunStatusFields: v1beta1.TaskRunStatusFields{ + CompletionTime: &metav1.Time{ + Time: time.Now(), + }, + StartTime: &metav1.Time{ + Time: time.Now(), + }, + PodName: "foobar", + }, + Status: knativev1beta1.Status{ + Conditions: knativev1beta1.Conditions{ + { + Type: apis.ConditionSucceeded, + Reason: "something bad happened", + Status: corev1.ConditionFalse, + Message: "some message", + }, + }, + }, + }, + } +} + // DefaultTaskRunWithFalseStatus returns a minimal tektont TaskRun with a FALSE status func (c *Catalog) DefaultTaskRunWithFalseStatus(trName string, buildRunName string, ns string) *v1beta1.TaskRun { return &v1beta1.TaskRun{ @@ -547,6 +615,48 @@ func (c *Catalog) DefaultBuildRun(buildRunName string, buildName string) *build. } } +// PodWithInitContainerStatus returns a pod with a single +// entry under the Status field for InitContainer Status +func (c *Catalog) PodWithInitContainerStatus(podName string, initContainerName string) *corev1.Pod { + return &corev1.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Name: podName, + }, + Status: corev1.PodStatus{ + InitContainerStatuses: []corev1.ContainerStatus{ + { + Name: initContainerName, + }, + }, + }, + } +} + +// BuildRunWithBuildSnapshot returns BuildRun Object with a populated +// BuildSpec in the Status field +func (c *Catalog) BuildRunWithBuildSnapshot(buildRunName string, buildName string) *build.BuildRun { + return &build.BuildRun{ + ObjectMeta: metav1.ObjectMeta{ + Name: buildRunName, + CreationTimestamp: metav1.Time{ + Time: time.Now(), + }, + }, + Status: build.BuildRunStatus{ + BuildSpec: &build.BuildSpec{ + StrategyRef: &build.StrategyRef{ + Name: "foobar", + }, + }, + }, + Spec: build.BuildRunSpec{ + BuildRef: &build.BuildRef{ + Name: buildName, + }, + }, + } +} + // DefaultTaskRun returns a minimal TaskRun object func (c *Catalog) DefaultTaskRun(taskRunName string, ns string) *v1beta1.TaskRun { return &v1beta1.TaskRun{