From 5715bb3c2ed8702a50531ea41b4ebe7a52693143 Mon Sep 17 00:00:00 2001 From: Jerome Ju Date: Fri, 2 Jun 2023 19:17:44 +0000 Subject: [PATCH] Sync v1 apis with v1beta1 changes This commit syncs v1 apis with v1beta1 changes that are required by v1 storage swap. - it adds the function required ie. IsTimeoutConditionSet() - it includes the changes to param_types --- pkg/apis/pipeline/v1/param_types.go | 16 +- pkg/apis/pipeline/v1/pipelinerun_types.go | 30 +++ .../pipeline/v1/pipelinerun_types_test.go | 223 ++++++++++++++++++ pkg/apis/pipeline/v1/result_validation.go | 6 - pkg/apis/pipeline/v1/resultref.go | 11 +- 5 files changed, 264 insertions(+), 22 deletions(-) diff --git a/pkg/apis/pipeline/v1/param_types.go b/pkg/apis/pipeline/v1/param_types.go index 14598b96e11..b47e45f72b3 100644 --- a/pkg/apis/pipeline/v1/param_types.go +++ b/pkg/apis/pipeline/v1/param_types.go @@ -20,7 +20,6 @@ import ( "context" "encoding/json" "fmt" - "regexp" "strings" "github.com/tektoncd/pipeline/pkg/substitution" @@ -29,12 +28,6 @@ import ( "knative.dev/pkg/apis" ) -// exactVariableSubstitutionFormat matches strings that only contain a single reference to result or param variables, but nothing else -// i.e. `$(result.resultname)` is a match, but `foo $(result.resultname)` is not. -const exactVariableSubstitutionFormat = `^\$\([_a-zA-Z0-9.-]+(\.[_a-zA-Z0-9.-]+)*(\[([0-9]+|\*)\])?\)$` - -var exactVariableSubstitutionRegex = regexp.MustCompile(exactVariableSubstitutionFormat) - // ParamsPrefix is the prefix used in $(...) expressions referring to parameters const ParamsPrefix = "params" @@ -473,7 +466,7 @@ func (paramValues ParamValue) MarshalJSON() ([]byte, error) { func (paramValues *ParamValue) ApplyReplacements(stringReplacements map[string]string, arrayReplacements map[string][]string, objectReplacements map[string]map[string]string) { switch paramValues.Type { case ParamTypeArray: - var newArrayVal []string + newArrayVal := []string{} for _, v := range paramValues.ArrayVal { newArrayVal = append(newArrayVal, substitution.ApplyArrayReplacements(v, stringReplacements, arrayReplacements)...) } @@ -505,7 +498,7 @@ func (paramValues *ParamValue) applyOrCorrect(stringReplacements map[string]stri // trim the head "$(" and the tail ")" or "[*])" // i.e. get "params.name" from "$(params.name)" or "$(params.name[*])" - trimedStringVal := StripStarVarSubExpression(stringVal) + trimedStringVal := substitution.StripStarVarSubExpression(stringVal) // if the stringVal is a reference to a string param if _, ok := stringReplacements[trimedStringVal]; ok { @@ -527,11 +520,6 @@ func (paramValues *ParamValue) applyOrCorrect(stringReplacements map[string]stri } } -// StripStarVarSubExpression strips "$(target[*])"" to get "target" -func StripStarVarSubExpression(s string) string { - return strings.TrimSuffix(strings.TrimSuffix(strings.TrimPrefix(s, "$("), ")"), "[*]") -} - // NewStructuredValues creates an ParamValues of type ParamTypeString or ParamTypeArray, based on // how many inputs are given (>1 input will create an array, not string). func NewStructuredValues(value string, values ...string) *ParamValue { diff --git a/pkg/apis/pipeline/v1/pipelinerun_types.go b/pkg/apis/pipeline/v1/pipelinerun_types.go index 4f4e33b3a18..9b07d17e96c 100644 --- a/pkg/apis/pipeline/v1/pipelinerun_types.go +++ b/pkg/apis/pipeline/v1/pipelinerun_types.go @@ -18,6 +18,7 @@ package v1 import ( "context" + "fmt" "time" "github.com/tektoncd/pipeline/pkg/apis/config" @@ -153,6 +154,22 @@ func (pr *PipelineRun) GetNamespacedName() types.NamespacedName { return types.NamespacedName{Namespace: pr.Namespace, Name: pr.Name} } +// IsTimeoutConditionSet returns true when the pipelinerun has the pipelinerun timed out reason +func (pr *PipelineRun) IsTimeoutConditionSet() bool { + condition := pr.Status.GetCondition(apis.ConditionSucceeded) + return condition.IsFalse() && condition.Reason == PipelineRunReasonTimedOut.String() +} + +// SetTimeoutCondition sets the status of the PipelineRun to timed out. +func (pr *PipelineRun) SetTimeoutCondition(ctx context.Context) { + pr.Status.SetCondition(&apis.Condition{ + Type: apis.ConditionSucceeded, + Status: corev1.ConditionFalse, + Reason: PipelineRunReasonTimedOut.String(), + Message: fmt.Sprintf("PipelineRun %q failed to finish within %q", pr.Name, pr.PipelineTimeout(ctx).String()), + }) +} + // HasTimedOut returns true if a pipelinerun has exceeded its spec.Timeout based on its status.Timeout func (pr *PipelineRun) HasTimedOut(ctx context.Context, c clock.PassiveClock) bool { timeout := pr.PipelineTimeout(ctx) @@ -170,6 +187,19 @@ func (pr *PipelineRun) HasTimedOut(ctx context.Context, c clock.PassiveClock) bo return false } +// HasTimedOutForALongTime returns true if a pipelinerun has exceeed its spec.Timeout based its status.StartTime +// by a large margin +func (pr *PipelineRun) HasTimedOutForALongTime(ctx context.Context, c clock.PassiveClock) bool { + if !pr.HasTimedOut(ctx, c) { + return false + } + timeout := pr.PipelineTimeout(ctx) + startTime := pr.Status.StartTime + runtime := c.Since(startTime.Time) + // We are arbitrarily defining large margin as doubling the spec.timeout + return runtime >= 2*timeout +} + // HaveTasksTimedOut returns true if a pipelinerun has exceeded its spec.Timeouts.Tasks func (pr *PipelineRun) HaveTasksTimedOut(ctx context.Context, c clock.PassiveClock) bool { timeout := pr.TasksTimeout() diff --git a/pkg/apis/pipeline/v1/pipelinerun_types_test.go b/pkg/apis/pipeline/v1/pipelinerun_types_test.go index 8ca1e7b0fe1..7420347ac4b 100644 --- a/pkg/apis/pipeline/v1/pipelinerun_types_test.go +++ b/pkg/apis/pipeline/v1/pipelinerun_types_test.go @@ -17,16 +17,20 @@ limitations under the License. package v1_test import ( + "context" "testing" "time" "github.com/google/go-cmp/cmp" + "github.com/google/go-cmp/cmp/cmpopts" + "github.com/tektoncd/pipeline/pkg/apis/config" "github.com/tektoncd/pipeline/pkg/apis/pipeline/pod" v1 "github.com/tektoncd/pipeline/pkg/apis/pipeline/v1" "github.com/tektoncd/pipeline/test/diff" corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "knative.dev/pkg/apis" + duckv1 "knative.dev/pkg/apis/duck/v1" ) func TestPipelineRunStatusConditions(t *testing.T) { @@ -211,6 +215,225 @@ func TestPipelineRunHasStarted(t *testing.T) { } } +func TestPipelineRunIsTimeoutConditionSet(t *testing.T) { + tcs := []struct { + name string + condition apis.Condition + want bool + }{{ + name: "should return true when reason is timeout", + condition: apis.Condition{ + Type: apis.ConditionSucceeded, + Status: corev1.ConditionFalse, + Reason: v1.PipelineRunReasonTimedOut.String(), + }, + want: true, + }, { + name: "should return false if status is not false", + condition: apis.Condition{ + Type: apis.ConditionSucceeded, + Status: corev1.ConditionUnknown, + Reason: v1.PipelineRunReasonTimedOut.String(), + }, + want: false, + }, { + name: "should return false if the reason is not timeout", + condition: apis.Condition{ + Type: apis.ConditionSucceeded, + Status: corev1.ConditionFalse, + Reason: v1.PipelineRunReasonFailed.String(), + }, + want: false, + }} + + for _, tc := range tcs { + t.Run(tc.name, func(t *testing.T) { + pr := &v1.PipelineRun{ + ObjectMeta: metav1.ObjectMeta{Name: "test-pipeline-run"}, + Status: v1.PipelineRunStatus{ + Status: duckv1.Status{ + Conditions: duckv1.Conditions{tc.condition}, + }, + }, + } + if got := pr.IsTimeoutConditionSet(); got != tc.want { + t.Errorf("pr.IsTimeoutConditionSet() (-want, +got):\n- %t\n+ %t", tc.want, got) + } + }) + } +} + +func TestPipelineRunSetTimeoutCondition(t *testing.T) { + ctx := config.ToContext(context.Background(), &config.Config{ + Defaults: &config.Defaults{ + DefaultTimeoutMinutes: 120, + }, + }) + + tcs := []struct { + name string + pipelineRun *v1.PipelineRun + want *apis.Condition + }{{ + name: "set condition to default timeout", + pipelineRun: &v1.PipelineRun{ObjectMeta: metav1.ObjectMeta{Name: "test-pipeline-run"}}, + want: &apis.Condition{ + Type: "Succeeded", + Status: "False", + Reason: "PipelineRunTimeout", + Message: `PipelineRun "test-pipeline-run" failed to finish within "2h0m0s"`, + }, + }, { + name: "set condition to spec.timeouts.pipeline value", + pipelineRun: &v1.PipelineRun{ + ObjectMeta: metav1.ObjectMeta{Name: "test-pipeline-run"}, + Spec: v1.PipelineRunSpec{ + Timeouts: &v1.TimeoutFields{ + Pipeline: &metav1.Duration{Duration: time.Hour}, + }, + }, + }, + want: &apis.Condition{ + Type: "Succeeded", + Status: "False", + Reason: "PipelineRunTimeout", + Message: `PipelineRun "test-pipeline-run" failed to finish within "1h0m0s"`, + }, + }} + + for _, tc := range tcs { + t.Run(tc.name, func(t *testing.T) { + tc.pipelineRun.SetTimeoutCondition(ctx) + + got := tc.pipelineRun.Status.GetCondition(apis.ConditionSucceeded) + if d := cmp.Diff(tc.want, got, cmpopts.IgnoreFields(apis.Condition{}, "LastTransitionTime")); d != "" { + t.Errorf("Unexpected PipelineRun condition: %v", diff.PrintWantGot(d)) + } + }) + } +} + +func TestPipelineRunHasTimedOutForALongTime(t *testing.T) { + tcs := []struct { + name string + timeout time.Duration + starttime time.Time + expected bool + }{{ + name: "has timed out for a long time", + timeout: 1 * time.Hour, + starttime: now.Add(-2 * time.Hour), + expected: true, + }, { + name: "has timed out for not a long time", + timeout: 1 * time.Hour, + starttime: now.Add(-90 * time.Minute), + expected: false, + }, { + name: "has not timed out", + timeout: 1 * time.Hour, + starttime: now.Add(-30 * time.Minute), + expected: false, + }, { + name: "has no timeout specified", + timeout: 0 * time.Second, + starttime: now.Add(-24 * time.Hour), + expected: false, + }} + + for _, tc := range tcs { + t.Run("pipeline.timeouts.pipeline "+tc.name, func(t *testing.T) { + pr := &v1.PipelineRun{ + ObjectMeta: metav1.ObjectMeta{Name: "foo"}, + Spec: v1.PipelineRunSpec{ + Timeouts: &v1.TimeoutFields{Pipeline: &metav1.Duration{Duration: tc.timeout}}, + }, + Status: v1.PipelineRunStatus{PipelineRunStatusFields: v1.PipelineRunStatusFields{ + StartTime: &metav1.Time{Time: tc.starttime}, + }}, + } + + if pr.HasTimedOutForALongTime(context.Background(), testClock) != tc.expected { + t.Errorf("Expected HasTimedOut to be %t when using pipeline.timeouts.pipeline", tc.expected) + } + }) + } +} + +func TestPipelineRunHasTimedOut(t *testing.T) { + tcs := []struct { + name string + timeout time.Duration + starttime time.Time + expected bool + }{{ + name: "timedout", + timeout: 1 * time.Second, + starttime: now.AddDate(0, 0, -1), + expected: true, + }, { + name: "nottimedout", + timeout: 25 * time.Hour, + starttime: now.AddDate(0, 0, -1), + expected: false, + }, { + name: "notimeoutspecified", + timeout: 0 * time.Second, + starttime: now.AddDate(0, 0, -1), + expected: false, + }, + } + + for _, tc := range tcs { + t.Run("pipeline.timeouts.pipeline "+tc.name, func(t *testing.T) { + pr := &v1.PipelineRun{ + ObjectMeta: metav1.ObjectMeta{Name: "foo"}, + Spec: v1.PipelineRunSpec{ + Timeouts: &v1.TimeoutFields{Pipeline: &metav1.Duration{Duration: tc.timeout}}, + }, + Status: v1.PipelineRunStatus{PipelineRunStatusFields: v1.PipelineRunStatusFields{ + StartTime: &metav1.Time{Time: tc.starttime}, + }}, + } + + if pr.HasTimedOut(context.Background(), testClock) != tc.expected { + t.Errorf("Expected HasTimedOut to be %t when using pipeline.timeouts.pipeline", tc.expected) + } + }) + t.Run("pipeline.timeouts.tasks "+tc.name, func(t *testing.T) { + pr := &v1.PipelineRun{ + ObjectMeta: metav1.ObjectMeta{Name: "foo"}, + Spec: v1.PipelineRunSpec{ + Timeouts: &v1.TimeoutFields{Tasks: &metav1.Duration{Duration: tc.timeout}}, + }, + Status: v1.PipelineRunStatus{PipelineRunStatusFields: v1.PipelineRunStatusFields{ + StartTime: &metav1.Time{Time: tc.starttime}, + }}, + } + + if pr.HaveTasksTimedOut(context.Background(), testClock) != tc.expected { + t.Errorf("Expected HasTimedOut to be %t when using pipeline.timeouts.pipeline", tc.expected) + } + }) + t.Run("pipeline.timeouts.finally "+tc.name, func(t *testing.T) { + pr := &v1.PipelineRun{ + ObjectMeta: metav1.ObjectMeta{Name: "foo"}, + Spec: v1.PipelineRunSpec{ + Timeouts: &v1.TimeoutFields{Finally: &metav1.Duration{Duration: tc.timeout}}, + }, + Status: v1.PipelineRunStatus{PipelineRunStatusFields: v1.PipelineRunStatusFields{ + StartTime: &metav1.Time{Time: tc.starttime}, + FinallyStartTime: &metav1.Time{Time: tc.starttime}, + }}, + } + + if pr.HasFinallyTimedOut(context.Background(), testClock) != tc.expected { + t.Errorf("Expected HasTimedOut to be %t when using pipeline.timeouts.pipeline", tc.expected) + } + }) + } +} + func TestPipelineRunTimeouts(t *testing.T) { tcs := []struct { name string diff --git a/pkg/apis/pipeline/v1/result_validation.go b/pkg/apis/pipeline/v1/result_validation.go index e9f2c9d303f..0d19c2ab0af 100644 --- a/pkg/apis/pipeline/v1/result_validation.go +++ b/pkg/apis/pipeline/v1/result_validation.go @@ -16,16 +16,10 @@ package v1 import ( "context" "fmt" - "regexp" "knative.dev/pkg/apis" ) -// ResultNameFormat Constant used to define the regex Result.Name should follow -const ResultNameFormat = `^([A-Za-z0-9][-A-Za-z0-9_.]*)?[A-Za-z0-9]$` - -var resultNameFormatRegex = regexp.MustCompile(ResultNameFormat) - // Validate implements apis.Validatable func (tr TaskResult) Validate(ctx context.Context) (errs *apis.FieldError) { if !resultNameFormatRegex.MatchString(tr.Name) { diff --git a/pkg/apis/pipeline/v1/resultref.go b/pkg/apis/pipeline/v1/resultref.go index 1dcd06e6e20..5a7610e0325 100644 --- a/pkg/apis/pipeline/v1/resultref.go +++ b/pkg/apis/pipeline/v1/resultref.go @@ -39,19 +39,26 @@ const ( objectResultExpressionFormat = "tasks..results.." // ResultTaskPart Constant used to define the "tasks" part of a pipeline result reference ResultTaskPart = "tasks" - // ResultFinallyPart Constant used to define the "finally" part of a task result reference + // ResultFinallyPart Constant used to define the "finally" part of a pipeline result reference ResultFinallyPart = "finally" // ResultResultPart Constant used to define the "results" part of a pipeline result reference ResultResultPart = "results" // TODO(#2462) use one regex across all substitutions // variableSubstitutionFormat matches format like $result.resultname, $result.resultname[int] and $result.resultname[*] variableSubstitutionFormat = `\$\([_a-zA-Z0-9.-]+(\.[_a-zA-Z0-9.-]+)*(\[([0-9]+|\*)\])?\)` + // exactVariableSubstitutionFormat matches strings that only contain a single reference to result or param variables, but nothing else + // i.e. `$(result.resultname)` is a match, but `foo $(result.resultname)` is not. + exactVariableSubstitutionFormat = `^\$\([_a-zA-Z0-9.-]+(\.[_a-zA-Z0-9.-]+)*(\[([0-9]+|\*)\])?\)$` // arrayIndexing will match all `[int]` and `[*]` for parseExpression arrayIndexing = `\[([0-9])*\*?\]` + // ResultNameFormat Constant used to define the regex Result.Name should follow + ResultNameFormat = `^([A-Za-z0-9][-A-Za-z0-9_.]*)?[A-Za-z0-9]$` ) // VariableSubstitutionRegex is a regex to find all result matching substitutions var VariableSubstitutionRegex = regexp.MustCompile(variableSubstitutionFormat) +var exactVariableSubstitutionRegex = regexp.MustCompile(exactVariableSubstitutionFormat) +var resultNameFormatRegex = regexp.MustCompile(ResultNameFormat) // arrayIndexingRegex is used to match `[int]` and `[*]` var arrayIndexingRegex = regexp.MustCompile(arrayIndexing) @@ -180,7 +187,7 @@ func parseExpression(substitutionExpression string) (string, string, int, string return subExpressions[1], subExpressions[3], 0, subExpressions[4], nil } } - return "", "", 0, "", fmt.Errorf("Must be one of the form 1). %q; 2). %q", resultExpressionFormat, objectResultExpressionFormat) + return "", "", 0, "", fmt.Errorf("must be one of the form 1). %q; 2). %q", resultExpressionFormat, objectResultExpressionFormat) } // ParseResultName parse the input string to extract resultName and result index.