From c8a9e0b3b3144da85e982eda4a8a296db49f7f95 Mon Sep 17 00:00:00 2001 From: Lee Bernick Date: Thu, 24 Feb 2022 08:31:21 -0700 Subject: [PATCH] Add functions to merge step/sidecar overrides This commit adds methods to merge a slice of Step overrides with a slice of Steps. This functionality is necessary for the full implementation of Step and Sidecar overrides. It re-uses the merge process that is used to combine Steps and Step templates; however, the Step is treated as the "template" so that the Step override takes precedence. We do not attempt to validate the resource requirements of the resulting pod; it's possible for the resulting pod to have resource requests > limits, which k8s will reject. This is the same way we currently handle Step template. --- pkg/apis/pipeline/v1beta1/merge.go | 152 +++++++--- pkg/apis/pipeline/v1beta1/merge_test.go | 374 ++++++++++++++++++++++++ 2 files changed, 491 insertions(+), 35 deletions(-) diff --git a/pkg/apis/pipeline/v1beta1/merge.go b/pkg/apis/pipeline/v1beta1/merge.go index 5c7b0ba3b14..f200c99e3ea 100644 --- a/pkg/apis/pipeline/v1beta1/merge.go +++ b/pkg/apis/pipeline/v1beta1/merge.go @@ -23,6 +23,15 @@ import ( "k8s.io/apimachinery/pkg/util/strategicpatch" ) +// mergeData is used to store the intermediate data needed to merge an object +// with a template. It's provided to avoid repeatedly re-serializing the template. +// +k8s:openapi-gen=false +type mergeData struct { + emptyJSON []byte + templateJSON []byte + patchSchema strategicpatch.PatchMetaFromStruct +} + // MergeStepsWithStepTemplate takes a possibly nil container template and a // list of steps, merging each of the steps with the container template, if // it's not nil, and returning the resulting list. @@ -31,61 +40,134 @@ func MergeStepsWithStepTemplate(template *v1.Container, steps []Step) ([]Step, e return steps, nil } - // We need JSON bytes to generate a patch to merge the step containers - // onto the template container, so marshal the template. - templateAsJSON, err := json.Marshal(template) - if err != nil { - return nil, err - } - // We need to do a three-way merge to actually merge the template and - // step containers, so we need an empty container as the "original" - emptyAsJSON, err := json.Marshal(&v1.Container{}) + md, err := getMergeData(template, &v1.Container{}) if err != nil { return nil, err } for i, s := range steps { - // Marshal the step's to JSON - stepAsJSON, err := json.Marshal(s.Container) + merged := v1.Container{} + err := mergeObjWithTemplateBytes(md, &s.Container, &merged) if err != nil { return nil, err } - // Get the patch meta for Container, which is needed for generating and applying the merge patch. - patchSchema, err := strategicpatch.NewPatchMetaFromStruct(template) - - if err != nil { - return nil, err + // If the container's args is nil, reset it to empty instead + if merged.Args == nil && s.Args != nil { + merged.Args = []string{} } - // Create a merge patch, with the empty JSON as the original, the step JSON as the modified, and the template - // JSON as the current - this lets us do a deep merge of the template and step containers, with awareness of - // the "patchMerge" tags. - patch, err := strategicpatch.CreateThreeWayMergePatch(emptyAsJSON, stepAsJSON, templateAsJSON, patchSchema, true) - if err != nil { - return nil, err - } + // Pass through original step Script, for later conversion. + steps[i] = Step{Container: merged, Script: s.Script, OnError: s.OnError, Timeout: s.Timeout} + } + return steps, nil +} - // Actually apply the merge patch to the template JSON. - mergedAsJSON, err := strategicpatch.StrategicMergePatchUsingLookupPatchMeta(templateAsJSON, patch, patchSchema) +// MergeStepsWithOverrides takes a possibly nil list of overrides and a +// list of steps, merging each of the steps with the overrides' resource requirements, if +// it's not nil, and returning the resulting list. +func MergeStepsWithOverrides(steps []Step, overrides []TaskRunStepOverride) ([]Step, error) { + stepNameToOverride := make(map[string]TaskRunStepOverride, len(overrides)) + for _, o := range overrides { + stepNameToOverride[o.Name] = o + } + for i, s := range steps { + o, found := stepNameToOverride[s.Name] + if !found { + continue + } + merged := v1.ResourceRequirements{} + err := mergeObjWithTemplate(&s.Resources, &o.Resources, &merged) if err != nil { return nil, err } + steps[i].Container.Resources = merged + } + return steps, nil +} - // Unmarshal the merged JSON to a Container pointer, and return it. - merged := &v1.Container{} - err = json.Unmarshal(mergedAsJSON, merged) +// MergeSidecarsWithOverrides takes a possibly nil list of overrides and a +// list of sidecars, merging each of the sidecars with the overrides' resource requirements, if +// it's not nil, and returning the resulting list. +func MergeSidecarsWithOverrides(sidecars []Sidecar, overrides []TaskRunSidecarOverride) ([]Sidecar, error) { + if len(overrides) == 0 { + return sidecars, nil + } + sidecarNameToOverride := make(map[string]TaskRunSidecarOverride, len(overrides)) + for _, o := range overrides { + sidecarNameToOverride[o.Name] = o + } + for i, s := range sidecars { + o, found := sidecarNameToOverride[s.Name] + if !found { + continue + } + merged := v1.ResourceRequirements{} + err := mergeObjWithTemplate(&s.Resources, &o.Resources, &merged) if err != nil { return nil, err } + sidecars[i].Container.Resources = merged + } + return sidecars, nil +} - // If the container's args is nil, reset it to empty instead - if merged.Args == nil && s.Args != nil { - merged.Args = []string{} - } +// mergeObjWithTemplate merges obj with template and updates out to reflect the merged result. +// template, obj, and out should point to the same type. out points to the zero value of that type. +func mergeObjWithTemplate(template, obj, out interface{}) error { + md, err := getMergeData(template, out) + if err != nil { + return err + } + return mergeObjWithTemplateBytes(md, obj, out) +} - // Pass through original step Script, for later conversion. - steps[i] = Step{Container: *merged, Script: s.Script, OnError: s.OnError, Timeout: s.Timeout} +// getMergeData serializes the template and empty object to get the intermediate results necessary for +// merging an object of the same type with this template. +// This function is provided to avoid repeatedly serializing an identical template. +func getMergeData(template, empty interface{}) (*mergeData, error) { + // We need JSON bytes to generate a patch to merge the object + // onto the template, so marshal the template. + templateJSON, err := json.Marshal(template) + if err != nil { + return nil, err } - return steps, nil + // We need to do a three-way merge to actually merge the template and + // object, so we need an empty object as the "original" + emptyJSON, err := json.Marshal(empty) + if err != nil { + return nil, err + } + // Get the patch meta, which is needed for generating and applying the merge patch. + patchSchema, err := strategicpatch.NewPatchMetaFromStruct(template) + if err != nil { + return nil, err + } + return &mergeData{templateJSON: templateJSON, emptyJSON: emptyJSON, patchSchema: patchSchema}, nil +} + +// mergeObjWithTemplateBytes merges obj with md's template JSON and updates out to reflect the merged result. +// out is a pointer to the zero value of obj's type. +// This function is provided to avoid repeatedly serializing an identical template. +func mergeObjWithTemplateBytes(md *mergeData, obj, out interface{}) error { + // Marshal the object to JSON + objAsJSON, err := json.Marshal(obj) + if err != nil { + return err + } + // Create a merge patch, with the empty JSON as the original, the object JSON as the modified, and the template + // JSON as the current - this lets us do a deep merge of the template and object, with awareness of + // the "patchMerge" tags. + patch, err := strategicpatch.CreateThreeWayMergePatch(md.emptyJSON, objAsJSON, md.templateJSON, md.patchSchema, true) + if err != nil { + return err + } + + // Actually apply the merge patch to the template JSON. + mergedAsJSON, err := strategicpatch.StrategicMergePatchUsingLookupPatchMeta(md.templateJSON, patch, md.patchSchema) + if err != nil { + return err + } + // Unmarshal the merged JSON to a pointer, and return it. + return json.Unmarshal(mergedAsJSON, out) } diff --git a/pkg/apis/pipeline/v1beta1/merge_test.go b/pkg/apis/pipeline/v1beta1/merge_test.go index 7bf5c3b4940..32814550f8a 100644 --- a/pkg/apis/pipeline/v1beta1/merge_test.go +++ b/pkg/apis/pipeline/v1beta1/merge_test.go @@ -117,3 +117,377 @@ func TestMergeStepsWithStepTemplate(t *testing.T) { }) } } + +func TestMergeStepOverrides(t *testing.T) { + tcs := []struct { + name string + steps []Step + stepOverrides []TaskRunStepOverride + want []Step + }{{ + name: "no overrides", + steps: []Step{{ + Container: corev1.Container{ + Name: "foo", + Resources: corev1.ResourceRequirements{ + Requests: corev1.ResourceList{corev1.ResourceMemory: resource.MustParse("1Gi")}, + }, + }, + }}, + want: []Step{{ + Container: corev1.Container{ + Name: "foo", + Resources: corev1.ResourceRequirements{ + Requests: corev1.ResourceList{corev1.ResourceMemory: resource.MustParse("1Gi")}, + }, + }, + }}, + }, { + name: "not all steps overridden", + steps: []Step{{ + Container: corev1.Container{ + Name: "foo", + Resources: corev1.ResourceRequirements{ + Requests: corev1.ResourceList{corev1.ResourceMemory: resource.MustParse("1Gi")}, + }, + }, + }, { + Container: corev1.Container{ + Name: "bar", + Resources: corev1.ResourceRequirements{ + Requests: corev1.ResourceList{corev1.ResourceMemory: resource.MustParse("1Gi")}, + }, + }, + }}, + stepOverrides: []TaskRunStepOverride{{ + Name: "bar", + Resources: corev1.ResourceRequirements{ + Requests: corev1.ResourceList{corev1.ResourceMemory: resource.MustParse("2Gi")}, + }, + }}, + want: []Step{{ + Container: corev1.Container{ + Name: "foo", + Resources: corev1.ResourceRequirements{ + Requests: corev1.ResourceList{corev1.ResourceMemory: resource.MustParse("1Gi")}, + }, + }, + }, { + Container: corev1.Container{ + Name: "bar", + Resources: corev1.ResourceRequirements{ + Requests: corev1.ResourceList{corev1.ResourceMemory: resource.MustParse("2Gi")}, + }, + }, + }}, + }, { + name: "override memory but not CPU", + steps: []Step{{ + Container: corev1.Container{ + Name: "foo", + Resources: corev1.ResourceRequirements{ + Requests: corev1.ResourceList{ + corev1.ResourceMemory: resource.MustParse("1Gi"), + corev1.ResourceCPU: resource.MustParse("100m"), + }, + }, + }, + }}, + stepOverrides: []TaskRunStepOverride{{ + Name: "foo", + Resources: corev1.ResourceRequirements{ + Requests: corev1.ResourceList{corev1.ResourceMemory: resource.MustParse("2Gi")}, + }, + }}, + want: []Step{{ + Container: corev1.Container{ + Name: "foo", + Resources: corev1.ResourceRequirements{ + Requests: corev1.ResourceList{ + corev1.ResourceCPU: resource.MustParse("100m"), + corev1.ResourceMemory: resource.MustParse("2Gi"), + }, + }, + }, + }}, + }, { + name: "override request but not limit", + steps: []Step{{ + Container: corev1.Container{ + Name: "foo", + Resources: corev1.ResourceRequirements{ + Requests: corev1.ResourceList{corev1.ResourceMemory: resource.MustParse("1Gi")}, + Limits: corev1.ResourceList{corev1.ResourceMemory: resource.MustParse("2Gi")}, + }, + }, + }}, + stepOverrides: []TaskRunStepOverride{{ + Name: "foo", + Resources: corev1.ResourceRequirements{ + Requests: corev1.ResourceList{corev1.ResourceMemory: resource.MustParse("1.5Gi")}, + }, + }}, + want: []Step{{ + Container: corev1.Container{ + Name: "foo", + Resources: corev1.ResourceRequirements{ + Requests: corev1.ResourceList{corev1.ResourceMemory: resource.MustParse("1.5Gi")}, + Limits: corev1.ResourceList{corev1.ResourceMemory: resource.MustParse("2Gi")}, + }, + }, + }}, + }, { + name: "override request and limit", + steps: []Step{{ + Container: corev1.Container{ + Name: "foo", + Resources: corev1.ResourceRequirements{ + Requests: corev1.ResourceList{corev1.ResourceMemory: resource.MustParse("1Gi")}, + Limits: corev1.ResourceList{corev1.ResourceMemory: resource.MustParse("2Gi")}, + }, + }, + }}, + stepOverrides: []TaskRunStepOverride{{ + Name: "foo", + Resources: corev1.ResourceRequirements{ + Requests: corev1.ResourceList{corev1.ResourceMemory: resource.MustParse("1.5Gi")}, + Limits: corev1.ResourceList{corev1.ResourceMemory: resource.MustParse("3Gi")}, + }, + }}, + want: []Step{{ + Container: corev1.Container{ + Name: "foo", + Resources: corev1.ResourceRequirements{ + Requests: corev1.ResourceList{corev1.ResourceMemory: resource.MustParse("1.5Gi")}, + Limits: corev1.ResourceList{corev1.ResourceMemory: resource.MustParse("3Gi")}, + }, + }, + }}, + }, { + // We don't make any effort to reject overrides that would result in invalid pods; + // instead, we let k8s reject the resulting pod. + name: "new request > old limit", + steps: []Step{{ + Container: corev1.Container{ + Name: "foo", + Resources: corev1.ResourceRequirements{ + Requests: corev1.ResourceList{corev1.ResourceMemory: resource.MustParse("1Gi")}, + Limits: corev1.ResourceList{corev1.ResourceMemory: resource.MustParse("2Gi")}, + }, + }, + }}, + stepOverrides: []TaskRunStepOverride{{ + Name: "foo", + Resources: corev1.ResourceRequirements{ + Requests: corev1.ResourceList{corev1.ResourceMemory: resource.MustParse("3Gi")}, + }, + }}, + want: []Step{{ + Container: corev1.Container{ + Name: "foo", + Resources: corev1.ResourceRequirements{ + Requests: corev1.ResourceList{corev1.ResourceMemory: resource.MustParse("3Gi")}, + Limits: corev1.ResourceList{corev1.ResourceMemory: resource.MustParse("2Gi")}, + }, + }, + }}, + }} + for _, tc := range tcs { + t.Run(tc.name, func(t *testing.T) { + steps, err := MergeStepsWithOverrides(tc.steps, tc.stepOverrides) + if err != nil { + t.Errorf("unexpected error merging steps with overrides: %s", err) + } + if d := cmp.Diff(tc.want, steps); d != "" { + t.Errorf("merged steps don't match, diff: %s", diff.PrintWantGot(d)) + } + }) + } +} + +func TestMergeSidecarOverrides(t *testing.T) { + tcs := []struct { + name string + sidecars []Sidecar + sidecarOverrides []TaskRunSidecarOverride + want []Sidecar + }{{ + name: "no overrides", + sidecars: []Sidecar{{ + Container: corev1.Container{ + Name: "foo", + Resources: corev1.ResourceRequirements{ + Requests: corev1.ResourceList{corev1.ResourceMemory: resource.MustParse("1Gi")}, + }, + }, + }}, + want: []Sidecar{{ + Container: corev1.Container{ + Name: "foo", + Resources: corev1.ResourceRequirements{ + Requests: corev1.ResourceList{corev1.ResourceMemory: resource.MustParse("1Gi")}, + }, + }, + }}, + }, { + name: "not all sidecars overridden", + sidecars: []Sidecar{{ + Container: corev1.Container{ + Name: "foo", + Resources: corev1.ResourceRequirements{ + Requests: corev1.ResourceList{corev1.ResourceMemory: resource.MustParse("1Gi")}, + }, + }, + }, { + Container: corev1.Container{ + Name: "bar", + Resources: corev1.ResourceRequirements{ + Requests: corev1.ResourceList{corev1.ResourceMemory: resource.MustParse("1Gi")}, + }, + }, + }}, + sidecarOverrides: []TaskRunSidecarOverride{{ + Name: "bar", + Resources: corev1.ResourceRequirements{ + Requests: corev1.ResourceList{corev1.ResourceMemory: resource.MustParse("2Gi")}, + }, + }}, + want: []Sidecar{{ + Container: corev1.Container{ + Name: "foo", + Resources: corev1.ResourceRequirements{ + Requests: corev1.ResourceList{corev1.ResourceMemory: resource.MustParse("1Gi")}, + }, + }, + }, { + Container: corev1.Container{ + Name: "bar", + Resources: corev1.ResourceRequirements{ + Requests: corev1.ResourceList{corev1.ResourceMemory: resource.MustParse("2Gi")}, + }, + }, + }}, + }, { + name: "override memory but not CPU", + sidecars: []Sidecar{{ + Container: corev1.Container{ + Name: "foo", + Resources: corev1.ResourceRequirements{ + Requests: corev1.ResourceList{ + corev1.ResourceMemory: resource.MustParse("1Gi"), + corev1.ResourceCPU: resource.MustParse("100m"), + }, + }, + }, + }}, + sidecarOverrides: []TaskRunSidecarOverride{{ + Name: "foo", + Resources: corev1.ResourceRequirements{ + Requests: corev1.ResourceList{corev1.ResourceMemory: resource.MustParse("2Gi")}, + }, + }}, + want: []Sidecar{{ + Container: corev1.Container{ + Name: "foo", + Resources: corev1.ResourceRequirements{ + Requests: corev1.ResourceList{ + corev1.ResourceCPU: resource.MustParse("100m"), + corev1.ResourceMemory: resource.MustParse("2Gi"), + }, + }, + }, + }}, + }, { + name: "override request but not limit", + sidecars: []Sidecar{{ + Container: corev1.Container{ + Name: "foo", + Resources: corev1.ResourceRequirements{ + Requests: corev1.ResourceList{corev1.ResourceMemory: resource.MustParse("1Gi")}, + Limits: corev1.ResourceList{corev1.ResourceMemory: resource.MustParse("2Gi")}, + }, + }, + }}, + sidecarOverrides: []TaskRunSidecarOverride{{ + Name: "foo", + Resources: corev1.ResourceRequirements{ + Requests: corev1.ResourceList{corev1.ResourceMemory: resource.MustParse("1.5Gi")}, + }, + }}, + want: []Sidecar{{ + Container: corev1.Container{ + Name: "foo", + Resources: corev1.ResourceRequirements{ + Requests: corev1.ResourceList{corev1.ResourceMemory: resource.MustParse("1.5Gi")}, + Limits: corev1.ResourceList{corev1.ResourceMemory: resource.MustParse("2Gi")}, + }, + }, + }}, + }, { + name: "override request and limit", + sidecars: []Sidecar{{ + Container: corev1.Container{ + Name: "foo", + Resources: corev1.ResourceRequirements{ + Requests: corev1.ResourceList{corev1.ResourceMemory: resource.MustParse("1Gi")}, + Limits: corev1.ResourceList{corev1.ResourceMemory: resource.MustParse("2Gi")}, + }, + }, + }}, + sidecarOverrides: []TaskRunSidecarOverride{{ + Name: "foo", + Resources: corev1.ResourceRequirements{ + Requests: corev1.ResourceList{corev1.ResourceMemory: resource.MustParse("1.5Gi")}, + Limits: corev1.ResourceList{corev1.ResourceMemory: resource.MustParse("3Gi")}, + }, + }}, + want: []Sidecar{{ + Container: corev1.Container{ + Name: "foo", + Resources: corev1.ResourceRequirements{ + Requests: corev1.ResourceList{corev1.ResourceMemory: resource.MustParse("1.5Gi")}, + Limits: corev1.ResourceList{corev1.ResourceMemory: resource.MustParse("3Gi")}, + }, + }, + }}, + }, { + // We don't make any effort to reject overrides that would result in invalid pods; + // instead, we let k8s reject the resulting pod. + name: "new request > old limit", + sidecars: []Sidecar{{ + Container: corev1.Container{ + Name: "foo", + Resources: corev1.ResourceRequirements{ + Requests: corev1.ResourceList{corev1.ResourceMemory: resource.MustParse("1Gi")}, + Limits: corev1.ResourceList{corev1.ResourceMemory: resource.MustParse("2Gi")}, + }, + }, + }}, + sidecarOverrides: []TaskRunSidecarOverride{{ + Name: "foo", + Resources: corev1.ResourceRequirements{ + Requests: corev1.ResourceList{corev1.ResourceMemory: resource.MustParse("3Gi")}, + }, + }}, + want: []Sidecar{{ + Container: corev1.Container{ + Name: "foo", + Resources: corev1.ResourceRequirements{ + Requests: corev1.ResourceList{corev1.ResourceMemory: resource.MustParse("3Gi")}, + Limits: corev1.ResourceList{corev1.ResourceMemory: resource.MustParse("2Gi")}, + }, + }, + }}, + }} + for _, tc := range tcs { + t.Run(tc.name, func(t *testing.T) { + sidecars, err := MergeSidecarsWithOverrides(tc.sidecars, tc.sidecarOverrides) + if err != nil { + t.Errorf("unexpected error merging sidecars with overrides: %s", err) + } + if d := cmp.Diff(tc.want, sidecars); d != "" { + t.Errorf("merged sidecars don't match, diff: %s", diff.PrintWantGot(d)) + } + }) + } +}