From a72eaf320606b7025e58cd4a8594ad407f1e7513 Mon Sep 17 00:00:00 2001 From: Nikolaos Moraitis Date: Tue, 30 Apr 2019 12:56:04 +0200 Subject: [PATCH 1/4] DecorationConfig: add pointer to Duration to apply the omitempty --- prow/apis/prowjobs/v1/types.go | 15 +++++++++++---- prow/cmd/build/controller.go | 2 +- prow/pod-utils/decorate/podspec.go | 2 +- 3 files changed, 13 insertions(+), 6 deletions(-) diff --git a/prow/apis/prowjobs/v1/types.go b/prow/apis/prowjobs/v1/types.go index f3f31382fe1d..5f7113272564 100644 --- a/prow/apis/prowjobs/v1/types.go +++ b/prow/apis/prowjobs/v1/types.go @@ -198,11 +198,11 @@ func (d *Duration) MarshalJSON() ([]byte, error) { type DecorationConfig struct { // Timeout is how long the pod utilities will wait // before aborting a job with SIGINT. - Timeout Duration `json:"timeout,omitempty"` + Timeout *Duration `json:"timeout,omitempty"` // GracePeriod is how long the pod utilities will wait // after sending SIGINT to send SIGKILL when aborting // a job. Only applicable if decorating the PodSpec. - GracePeriod Duration `json:"grace_period,omitempty"` + GracePeriod *Duration `json:"grace_period,omitempty"` // UtilityImages holds pull specs for utility container // images used to decorate a PodSpec. @@ -246,10 +246,10 @@ func (d *DecorationConfig) ApplyDefault(def *DecorationConfig) *DecorationConfig merged.UtilityImages = merged.UtilityImages.ApplyDefault(def.UtilityImages) merged.GCSConfiguration = merged.GCSConfiguration.ApplyDefault(def.GCSConfiguration) - if merged.Timeout.Duration == 0 { + if merged.Timeout == nil { merged.Timeout = def.Timeout } - if merged.GracePeriod.Duration == 0 { + if merged.GracePeriod == nil { merged.GracePeriod = def.GracePeriod } if merged.GCSCredentialsSecret == "" { @@ -305,6 +305,13 @@ func (d *DecorationConfig) Validate() error { return nil } +func (d *Duration) Get() time.Duration { + if d == nil { + return 0 + } + return d.Duration +} + // UtilityImages holds pull specs for the utility images // to be used for a job type UtilityImages struct { diff --git a/prow/cmd/build/controller.go b/prow/cmd/build/controller.go index 1dee8d9cc042..00a2ee94f962 100644 --- a/prow/cmd/build/controller.go +++ b/prow/cmd/build/controller.go @@ -686,7 +686,7 @@ func decorateSteps(steps []coreapi.Container, dc prowjobv1.DecorationConfig, too previousMarker = entries[i-1].MarkerFile } // TODO(fejta): consider refactoring entrypoint to accept --expire=time.Now.Add(dc.Timeout) so we timeout each step correctly (assuming a good clock) - opt, err := decorate.InjectEntrypoint(&steps[i], dc.Timeout.Duration, dc.GracePeriod.Duration, steps[i].Name, previousMarker, alwaysPass, logMount, toolsMount) + opt, err := decorate.InjectEntrypoint(&steps[i], dc.Timeout.Get(), dc.GracePeriod.Get(), steps[i].Name, previousMarker, alwaysPass, logMount, toolsMount) if err != nil { return nil, fmt.Errorf("inject entrypoint into %s: %v", steps[i].Name, err) } diff --git a/prow/pod-utils/decorate/podspec.go b/prow/pod-utils/decorate/podspec.go index 4c7460709f59..530b5ef1e700 100644 --- a/prow/pod-utils/decorate/podspec.go +++ b/prow/pod-utils/decorate/podspec.go @@ -535,7 +535,7 @@ func decorate(spec *coreapi.PodSpec, pj *prowapi.ProwJob, rawEnv map[string]stri previous = "" exitZero = false ) - wrapperOptions, err := InjectEntrypoint(&spec.Containers[0], pj.Spec.DecorationConfig.Timeout.Duration, pj.Spec.DecorationConfig.GracePeriod.Duration, prefix, previous, exitZero, logMount, toolsMount) + wrapperOptions, err := InjectEntrypoint(&spec.Containers[0], pj.Spec.DecorationConfig.Timeout.Get(), pj.Spec.DecorationConfig.GracePeriod.Get(), prefix, previous, exitZero, logMount, toolsMount) if err != nil { return fmt.Errorf("wrap container: %v", err) } From 9a58b5e232f7b3c502e7bad0539664663e0d3bae Mon Sep 17 00:00:00 2001 From: Nikolaos Moraitis Date: Tue, 30 Apr 2019 12:58:03 +0200 Subject: [PATCH 2/4] fix tests for the changed DecorationConfig --- prow/apis/prowjobs/v1/types_test.go | 8 ++++---- prow/cmd/build/controller_test.go | 11 +++++++++-- prow/config/config_test.go | 8 ++++---- prow/pod-utils/decorate/podspec_test.go | 24 ++++++++++++------------ 4 files changed, 29 insertions(+), 22 deletions(-) diff --git a/prow/apis/prowjobs/v1/types_test.go b/prow/apis/prowjobs/v1/types_test.go index 20e3297bd589..c0b6f8f55752 100644 --- a/prow/apis/prowjobs/v1/types_test.go +++ b/prow/apis/prowjobs/v1/types_test.go @@ -42,7 +42,7 @@ func TestDecorationDefaulting(t *testing.T) { { name: "timeout provided", provided: &DecorationConfig{ - Timeout: Duration{Duration: 10 * time.Minute}, + Timeout: &Duration{Duration: 10 * time.Minute}, }, expected: func(orig, def *DecorationConfig) *DecorationConfig { def.Timeout = orig.Timeout @@ -52,7 +52,7 @@ func TestDecorationDefaulting(t *testing.T) { { name: "grace period provided", provided: &DecorationConfig{ - GracePeriod: Duration{Duration: 10 * time.Hour}, + GracePeriod: &Duration{Duration: 10 * time.Hour}, }, expected: func(orig, def *DecorationConfig) *DecorationConfig { def.GracePeriod = orig.GracePeriod @@ -163,8 +163,8 @@ func TestDecorationDefaulting(t *testing.T) { tc := testCase t.Run(tc.name, func(t *testing.T) { defaults := &DecorationConfig{ - Timeout: Duration{Duration: 1 * time.Minute}, - GracePeriod: Duration{Duration: 10 * time.Second}, + Timeout: &Duration{Duration: 1 * time.Minute}, + GracePeriod: &Duration{Duration: 10 * time.Second}, UtilityImages: &UtilityImages{ CloneRefs: "clonerefs", InitUpload: "initupload", diff --git a/prow/cmd/build/controller_test.go b/prow/cmd/build/controller_test.go index fa292e1e6c1d..fddb7edaa0ec 100644 --- a/prow/cmd/build/controller_test.go +++ b/prow/cmd/build/controller_test.go @@ -1469,6 +1469,8 @@ func TestMakeBuild(t *testing.T) { pj.Spec.ExtraRefs = []prowjobv1.Refs{{Org: "bonus"}} pj.Spec.DecorationConfig = &prowjobv1.DecorationConfig{ UtilityImages: &prowjobv1.UtilityImages{}, + Timeout: &prowjobv1.Duration{Duration: 0}, + GracePeriod: &prowjobv1.Duration{Duration: 0}, } return pj }, @@ -1479,6 +1481,8 @@ func TestMakeBuild(t *testing.T) { pj.Spec.ExtraRefs = []prowjobv1.Refs{{Org: "bonus"}} pj.Spec.DecorationConfig = &prowjobv1.DecorationConfig{ UtilityImages: &prowjobv1.UtilityImages{}, + Timeout: &prowjobv1.Duration{Duration: 0}, + GracePeriod: &prowjobv1.Duration{Duration: 0}, } pj.Spec.BuildSpec.Source = &buildv1alpha1.SourceSpec{} return pj @@ -1542,8 +1546,8 @@ func TestMakeBuild(t *testing.T) { func TestDecorateSteps(t *testing.T) { var dc prowjobv1.DecorationConfig - dc.Timeout = prowjobv1.Duration{Duration: 10 * time.Minute} - dc.GracePeriod = prowjobv1.Duration{Duration: 5 * time.Minute} + dc.Timeout = &prowjobv1.Duration{Duration: 10 * time.Minute} + dc.GracePeriod = &prowjobv1.Duration{Duration: 5 * time.Minute} _, tm := tools() tm.Name += "not-static" tm.MountPath += "fancy" @@ -1739,6 +1743,9 @@ func TestInjectTimeout(t *testing.T) { dc := prowjobv1.DecorationConfig{ UtilityImages: &prowjobv1.UtilityImages{}, + Timeout: &prowjobv1.Duration{ + Duration: defaultTimeout, + }, } if tc.decoratedTimeout != nil { dc.Timeout.Duration = *tc.decoratedTimeout diff --git a/prow/config/config_test.go b/prow/config/config_test.go index 372ee492a952..df3be19454cb 100644 --- a/prow/config/config_test.go +++ b/prow/config/config_test.go @@ -323,8 +323,8 @@ periodics: - "test" - "./..."`, expected: &prowapi.DecorationConfig{ - Timeout: prowapi.Duration{Duration: 2 * time.Hour}, - GracePeriod: prowapi.Duration{Duration: 15 * time.Second}, + Timeout: &prowapi.Duration{Duration: 2 * time.Hour}, + GracePeriod: &prowapi.Duration{Duration: 15 * time.Second}, UtilityImages: &prowapi.UtilityImages{ CloneRefs: "clonerefs:default", InitUpload: "initupload:default", @@ -382,8 +382,8 @@ periodics: - "test" - "./..."`, expected: &prowapi.DecorationConfig{ - Timeout: prowapi.Duration{Duration: 1 * time.Nanosecond}, - GracePeriod: prowapi.Duration{Duration: 1 * time.Nanosecond}, + Timeout: &prowapi.Duration{Duration: 1 * time.Nanosecond}, + GracePeriod: &prowapi.Duration{Duration: 1 * time.Nanosecond}, UtilityImages: &prowapi.UtilityImages{ CloneRefs: "clonerefs:explicit", InitUpload: "initupload:explicit", diff --git a/prow/pod-utils/decorate/podspec_test.go b/prow/pod-utils/decorate/podspec_test.go index 256d532dc5c3..23096df76a52 100644 --- a/prow/pod-utils/decorate/podspec_test.go +++ b/prow/pod-utils/decorate/podspec_test.go @@ -456,8 +456,8 @@ func TestProwJobToPod(t *testing.T) { Type: prowapi.PresubmitJob, Job: "job-name", DecorationConfig: &prowapi.DecorationConfig{ - Timeout: prowapi.Duration{Duration: 120 * time.Minute}, - GracePeriod: prowapi.Duration{Duration: 10 * time.Second}, + Timeout: &prowapi.Duration{Duration: 120 * time.Minute}, + GracePeriod: &prowapi.Duration{Duration: 10 * time.Second}, UtilityImages: &prowapi.UtilityImages{ CloneRefs: "clonerefs:tag", InitUpload: "initupload:tag", @@ -677,8 +677,8 @@ func TestProwJobToPod(t *testing.T) { Type: prowapi.PresubmitJob, Job: "job-name", DecorationConfig: &prowapi.DecorationConfig{ - Timeout: prowapi.Duration{Duration: 120 * time.Minute}, - GracePeriod: prowapi.Duration{Duration: 10 * time.Second}, + Timeout: &prowapi.Duration{Duration: 120 * time.Minute}, + GracePeriod: &prowapi.Duration{Duration: 10 * time.Second}, UtilityImages: &prowapi.UtilityImages{ CloneRefs: "clonerefs:tag", InitUpload: "initupload:tag", @@ -898,8 +898,8 @@ func TestProwJobToPod(t *testing.T) { Type: prowapi.PresubmitJob, Job: "job-name", DecorationConfig: &prowapi.DecorationConfig{ - Timeout: prowapi.Duration{Duration: 120 * time.Minute}, - GracePeriod: prowapi.Duration{Duration: 10 * time.Second}, + Timeout: &prowapi.Duration{Duration: 120 * time.Minute}, + GracePeriod: &prowapi.Duration{Duration: 10 * time.Second}, UtilityImages: &prowapi.UtilityImages{ CloneRefs: "clonerefs:tag", InitUpload: "initupload:tag", @@ -1145,8 +1145,8 @@ func TestProwJobToPod(t *testing.T) { Type: prowapi.PresubmitJob, Job: "job-name", DecorationConfig: &prowapi.DecorationConfig{ - Timeout: prowapi.Duration{Duration: 120 * time.Minute}, - GracePeriod: prowapi.Duration{Duration: 10 * time.Second}, + Timeout: &prowapi.Duration{Duration: 120 * time.Minute}, + GracePeriod: &prowapi.Duration{Duration: 10 * time.Second}, UtilityImages: &prowapi.UtilityImages{ CloneRefs: "clonerefs:tag", InitUpload: "initupload:tag", @@ -1391,8 +1391,8 @@ func TestProwJobToPod(t *testing.T) { Type: prowapi.PeriodicJob, Job: "job-name", DecorationConfig: &prowapi.DecorationConfig{ - Timeout: prowapi.Duration{Duration: 120 * time.Minute}, - GracePeriod: prowapi.Duration{Duration: 10 * time.Second}, + Timeout: &prowapi.Duration{Duration: 120 * time.Minute}, + GracePeriod: &prowapi.Duration{Duration: 10 * time.Second}, UtilityImages: &prowapi.UtilityImages{ CloneRefs: "clonerefs:tag", InitUpload: "initupload:tag", @@ -1554,8 +1554,8 @@ func TestProwJobToPod(t *testing.T) { Type: prowapi.PresubmitJob, Job: "job-name", DecorationConfig: &prowapi.DecorationConfig{ - Timeout: prowapi.Duration{Duration: 120 * time.Minute}, - GracePeriod: prowapi.Duration{Duration: 10 * time.Second}, + Timeout: &prowapi.Duration{Duration: 120 * time.Minute}, + GracePeriod: &prowapi.Duration{Duration: 10 * time.Second}, UtilityImages: &prowapi.UtilityImages{ CloneRefs: "clonerefs:tag", InitUpload: "initupload:tag", From f333f2038617c65a592f4d15b452149377f377b2 Mon Sep 17 00:00:00 2001 From: Nikolaos Moraitis Date: Tue, 30 Apr 2019 12:59:01 +0200 Subject: [PATCH 3/4] update prowjobs v1 generated code --- prow/apis/prowjobs/v1/zz_generated.deepcopy.go | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/prow/apis/prowjobs/v1/zz_generated.deepcopy.go b/prow/apis/prowjobs/v1/zz_generated.deepcopy.go index a6cdb850e13e..be2fa4c0964d 100644 --- a/prow/apis/prowjobs/v1/zz_generated.deepcopy.go +++ b/prow/apis/prowjobs/v1/zz_generated.deepcopy.go @@ -30,8 +30,16 @@ import ( // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *DecorationConfig) DeepCopyInto(out *DecorationConfig) { *out = *in - out.Timeout = in.Timeout - out.GracePeriod = in.GracePeriod + if in.Timeout != nil { + in, out := &in.Timeout, &out.Timeout + *out = new(Duration) + **out = **in + } + if in.GracePeriod != nil { + in, out := &in.GracePeriod, &out.GracePeriod + *out = new(Duration) + **out = **in + } if in.UtilityImages != nil { in, out := &in.UtilityImages, &out.UtilityImages *out = new(UtilityImages) From 3645be7e3955ece88ea5bca982d7bea82351c897 Mon Sep 17 00:00:00 2001 From: Nikolaos Moraitis Date: Fri, 3 May 2019 14:21:49 +0200 Subject: [PATCH 4/4] update remaining entries --- prow/cmd/build/controller.go | 2 +- prow/cmd/build/controller_test.go | 6 +++--- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/prow/cmd/build/controller.go b/prow/cmd/build/controller.go index 00a2ee94f962..64ce44b092d9 100644 --- a/prow/cmd/build/controller.go +++ b/prow/cmd/build/controller.go @@ -723,7 +723,7 @@ func determineTimeout(spec *buildv1alpha1.BuildSpec, dc *prowjobv1.DecorationCon switch { case spec.Timeout != nil: return spec.Timeout.Duration - case dc != nil && dc.Timeout.Duration > 0: + case dc != nil && dc.Timeout.Get() > 0: return dc.Timeout.Duration default: return defaultTimeout diff --git a/prow/cmd/build/controller_test.go b/prow/cmd/build/controller_test.go index fddb7edaa0ec..613793955904 100644 --- a/prow/cmd/build/controller_test.go +++ b/prow/cmd/build/controller_test.go @@ -1574,15 +1574,15 @@ func TestDecorateSteps(t *testing.T) { }, } expected[1].Name = "step-1" - o1, err := decorate.InjectEntrypoint(&expected[0], dc.Timeout.Duration, dc.GracePeriod.Duration, expected[0].Name, "", true, logMount, tm) + o1, err := decorate.InjectEntrypoint(&expected[0], dc.Timeout.Get(), dc.GracePeriod.Get(), expected[0].Name, "", true, logMount, tm) if err != nil { t.Fatalf("inject expected 0: %v", err) } - o2, err := decorate.InjectEntrypoint(&expected[1], dc.Timeout.Duration, dc.GracePeriod.Duration, expected[1].Name, o1.MarkerFile, true, logMount, tm) + o2, err := decorate.InjectEntrypoint(&expected[1], dc.Timeout.Get(), dc.GracePeriod.Get(), expected[1].Name, o1.MarkerFile, true, logMount, tm) if err != nil { t.Fatalf("inject expected 1: %v", err) } - o3, err := decorate.InjectEntrypoint(&expected[2], dc.Timeout.Duration, dc.GracePeriod.Duration, expected[2].Name, o2.MarkerFile, true, logMount, tm) + o3, err := decorate.InjectEntrypoint(&expected[2], dc.Timeout.Get(), dc.GracePeriod.Get(), expected[2].Name, o2.MarkerFile, true, logMount, tm) if err != nil { t.Fatalf("inject expected 2: %v", err) }