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

Adjust PipelineRun's StartTime based on TaskRun state. #3461

Merged
merged 3 commits into from
Oct 27, 2020
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
1 change: 1 addition & 0 deletions pkg/reconciler/pipelinerun/pipelinerun.go
Original file line number Diff line number Diff line change
Expand Up @@ -528,6 +528,7 @@ func (c *Reconciler) reconcile(ctx context.Context, pr *v1beta1.PipelineRun, get
}
// Read the condition the way it was set by the Mark* helpers
after = pr.Status.GetCondition(apis.ConditionSucceeded)
pr.Status.StartTime = pipelineRunFacts.State.AdjustStartTime(pr.Status.StartTime)
pr.Status.TaskRuns = pipelineRunFacts.State.GetTaskRunsStatus(pr)
pr.Status.SkippedTasks = pipelineRunFacts.GetSkippedTasks()
logger.Infof("PipelineRun %s status is being set to %s", pr.Name, after)
Expand Down
22 changes: 22 additions & 0 deletions pkg/reconciler/pipelinerun/resources/pipelinerunstate.go
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ import (
"github.com/tektoncd/pipeline/pkg/reconciler/pipeline/dag"
"go.uber.org/zap"
corev1 "k8s.io/api/core/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/util/sets"
"knative.dev/pkg/apis"
)
Expand Down Expand Up @@ -71,6 +72,27 @@ func (state PipelineRunState) IsBeforeFirstTaskRun() bool {
return true
}

// AdjustStartTime adjusts potential drift in the PipelineRun's start time.
Copy link
Member

Choose a reason for hiding this comment

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

In addition to stating why we do this, can you add a sentence describing how we adjust, to make this easier to read?

Copy link
Member Author

Choose a reason for hiding this comment

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

tweaked wording, PTAL 🙏

//
// The StartTime will only adjust earlier, so that the PipelineRun's StartTime
// is no later than any of its constituent TaskRuns.
//
// This drift could be due to us either failing to record the Run's start time
// previously, or our own failure to observe a prior update before reconciling
// the resource again.
func (state PipelineRunState) AdjustStartTime(unadjustedStartTime *metav1.Time) *metav1.Time {
adjustedStartTime := unadjustedStartTime
for _, rprt := range state {
if rprt.TaskRun == nil {
continue
}
if rprt.TaskRun.CreationTimestamp.Time.Before(adjustedStartTime.Time) {
adjustedStartTime = &rprt.TaskRun.CreationTimestamp
}
}
return adjustedStartTime.DeepCopy()
}

// GetTaskRunsStatus returns a map of taskrun name and the taskrun
// ignore a nil taskrun in pipelineRunState, otherwise, capture taskrun object from PipelineRun Status
// update taskrun status based on the pipelineRunState before returning it in the map
Expand Down
82 changes: 82 additions & 0 deletions pkg/reconciler/pipelinerun/resources/pipelinerunstate_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -1049,3 +1049,85 @@ func TestGetPipelineConditionStatus_PipelineTimeouts(t *testing.T) {
t.Fatalf("Expected to get status %s but got %s for state %v", corev1.ConditionFalse, c.Status, oneFinishedState)
}
}

func TestAdjustStartTime(t *testing.T) {
baseline := metav1.Time{Time: time.Now()}

tests := []struct {
name string
prs PipelineRunState
want time.Time
}{{
name: "same times",
prs: PipelineRunState{{
TaskRun: &v1beta1.TaskRun{
ObjectMeta: metav1.ObjectMeta{
Name: "blah",
CreationTimestamp: baseline,
},
},
}},
want: baseline.Time,
}, {
name: "taskrun starts later",
prs: PipelineRunState{{
TaskRun: &v1beta1.TaskRun{
ObjectMeta: metav1.ObjectMeta{
Name: "blah",
CreationTimestamp: metav1.Time{Time: baseline.Time.Add(1 * time.Second)},
},
},
}},
// Stay where you are, you are before the TaskRun.
want: baseline.Time,
}, {
name: "taskrun starts earlier",
prs: PipelineRunState{{
TaskRun: &v1beta1.TaskRun{
ObjectMeta: metav1.ObjectMeta{
Name: "blah",
CreationTimestamp: metav1.Time{Time: baseline.Time.Add(-1 * time.Second)},
},
},
}},
// We expect this to adjust to the earlier time.
want: baseline.Time.Add(-1 * time.Second),
}, {
name: "multiple taskruns, some earlier",
prs: PipelineRunState{{
TaskRun: &v1beta1.TaskRun{
ObjectMeta: metav1.ObjectMeta{
Name: "blah1",
CreationTimestamp: metav1.Time{Time: baseline.Time.Add(-1 * time.Second)},
},
},
}, {
TaskRun: &v1beta1.TaskRun{
ObjectMeta: metav1.ObjectMeta{
Name: "blah2",
CreationTimestamp: metav1.Time{Time: baseline.Time.Add(-2 * time.Second)},
},
},
}, {
TaskRun: nil,
}, {
TaskRun: &v1beta1.TaskRun{
ObjectMeta: metav1.ObjectMeta{
Name: "blah3",
CreationTimestamp: metav1.Time{Time: baseline.Time.Add(2 * time.Second)},
},
},
}},
// We expect this to adjust to the earlier time.
want: baseline.Time.Add(-2 * time.Second),
}}

for _, test := range tests {
t.Run(test.name, func(t *testing.T) {
got := test.prs.AdjustStartTime(&baseline)
if got.Time != test.want {
t.Errorf("AdjustStartTime() = %v, wanted %v", got.Time, test.want)
}
})
}
}