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.

Sign up for GitHub

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

Jump to bottom

fix: merge podTemplates filed more specific, such as tolerations, nodeselector and so on. #7851

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

fix: merge podTemplates filed more specific, such as tolerations, nodeselector and so on. #7851

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
9736dfb
merge podTemplates more specific
Ma-YuXin Apr 6, 2024
a1b85c4
refmt the files to satify go code style with gofmt
Ma-YuXin Apr 8, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
82 changes: 82 additions & 0 deletions pkg/apis/pipeline/pod/template.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -223,6 +223,62 @@ func MergePodTemplateWithDefault(tpl, defaultTpl *PodTemplate) *PodTemplate {
}
}

// Similar to MergePodTemplateWithDefault, but more fine-grained. When the value
// type is map or slice, we will merge their values first. If there is the same
// value, the value from tpl will overwrite the value from defaultTpl.
func ExpandedMergePodTemplateWithDefault(tpl, defaultTpl *PodTemplate) *PodTemplate {
switch {
case defaultTpl == nil:
// No configured default, just return the template
return tpl
case tpl == nil:
// No template, just return the default template
return defaultTpl
default:
// Otherwise, merge fields
if tpl.NodeSelector == nil {
tpl.NodeSelector = defaultTpl.NodeSelector
} else {
tpl.NodeSelector = mergeMaps(defaultTpl.NodeSelector, tpl.NodeSelector)
}

tpl.Env = mergeByName(defaultTpl.Env, tpl.Env)
tpl.Tolerations = mergeByName(defaultTpl.Tolerations, tpl.Tolerations)
if tpl.SecurityContext == nil {
tpl.SecurityContext = defaultTpl.SecurityContext
}
tpl.Volumes = mergeByName(defaultTpl.Volumes, tpl.Volumes)
if tpl.RuntimeClassName == nil {
tpl.RuntimeClassName = defaultTpl.RuntimeClassName
}
if tpl.AutomountServiceAccountToken == nil {
tpl.AutomountServiceAccountToken = defaultTpl.AutomountServiceAccountToken
}
if tpl.DNSPolicy == nil {
tpl.DNSPolicy = defaultTpl.DNSPolicy
}
if tpl.DNSConfig == nil {
tpl.DNSConfig = defaultTpl.DNSConfig
}
if tpl.EnableServiceLinks == nil {
tpl.EnableServiceLinks = defaultTpl.EnableServiceLinks
}
if tpl.PriorityClassName == nil {
tpl.PriorityClassName = defaultTpl.PriorityClassName
}
if tpl.SchedulerName == "" {
tpl.SchedulerName = defaultTpl.SchedulerName
}
tpl.ImagePullSecrets = mergeByName(defaultTpl.ImagePullSecrets, tpl.ImagePullSecrets)
tpl.HostAliases = mergeByName(defaultTpl.HostAliases, tpl.HostAliases)
if !tpl.HostNetwork && defaultTpl.HostNetwork {
tpl.HostNetwork = true
}
tpl.TopologySpreadConstraints = mergeByName(defaultTpl.TopologySpreadConstraints, tpl.TopologySpreadConstraints)
return tpl
}
}

// AAPodTemplate holds pod specific configuration for the affinity-assistant
type AAPodTemplate = AffinityAssistantTemplate

Expand Down Expand Up @@ -251,6 +307,24 @@ func MergeAAPodTemplateWithDefault(tpl, defaultTpl *AAPodTemplate) *AAPodTemplat
}
}

// mergeMaps takes two maps with the same key and value types
// and merges them, giving priority to the items in the override map.
func mergeMaps[K comparable, V any](base, overrides map[K]V) map[K]V {
result := make(map[K]V)
Copy link
Contributor

Choose a reason for hiding this comment

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

may be we can just add the overwrite to the base and done with it, instead of creating new and return that

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think overwriting the base is safe. The base/default can be reused. We should only change tpl.


// Add all elements from base to the result
for key, value := range base {
result[key] = value
}

// Add all elements from overrides to the result, overwriting any existing keys
for key, value := range overrides {
result[key] = value
}

return result
}

// mergeByName merges two slices of items with names based on the getName
// function, giving priority to the items in the override slice.
func mergeByName[T any](base, overrides []T) []T {
Expand Down Expand Up @@ -292,6 +366,14 @@ func getName(item interface{}) string {
return item.Name
case corev1.Volume:
return item.Name
case corev1.Toleration:
return item.Key
case corev1.TopologySpreadConstraint:
return item.TopologyKey
case corev1.HostAlias:
return item.IP
case corev1.LocalObjectReference:
return item.Name
default:
return ""
}
Expand Down
104 changes: 104 additions & 0 deletions pkg/apis/pipeline/pod/template_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -168,3 +168,107 @@ func TestMergePodTemplateWithDefault(t *testing.T) {
})
}
}

func TestExpandedMergePodTemplateWithDefault(t *testing.T) {
type testCase struct {
name string
tpl *PodTemplate
defaultTpl *PodTemplate
expected *PodTemplate
}

testCases := []testCase{
{
name: "defaultTpl is nil",
tpl: &PodTemplate{
NodeSelector: map[string]string{"foo": "bar"},
},
defaultTpl: nil,
expected: &PodTemplate{
NodeSelector: map[string]string{"foo": "bar"},
},
},
{
name: "tpl is nil",
tpl: nil,
defaultTpl: &PodTemplate{
NodeSelector: map[string]string{"foo": "bar"},
},
expected: &PodTemplate{
NodeSelector: map[string]string{"foo": "bar"},
},
},
{
name: "override default env",
tpl: &PodTemplate{
Env: []corev1.EnvVar{{Name: "foo", Value: "bar"}},
},
defaultTpl: &PodTemplate{
Env: []corev1.EnvVar{{Name: "foo", Value: "baz"}},
},
expected: &PodTemplate{
Env: []corev1.EnvVar{{Name: "foo", Value: "bar"}},
},
},
{
name: "merge envs",
tpl: &PodTemplate{
Env: []corev1.EnvVar{{Name: "foo", Value: "bar"}},
},
defaultTpl: &PodTemplate{
Env: []corev1.EnvVar{{Name: "bar", Value: "bar"}},
},
expected: &PodTemplate{
Env: []corev1.EnvVar{
{Name: "foo", Value: "bar"},
{Name: "bar", Value: "bar"},
},
},
},
{
name: "override default nodeselector",
tpl: &PodTemplate{
NodeSelector: map[string]string{"disktype": "ssd"},
},
defaultTpl: &PodTemplate{
NodeSelector: map[string]string{"disktype": "hdd"},
},
expected: &PodTemplate{
NodeSelector: map[string]string{"disktype": "ssd"},
},
},
{
name: "merge nodeselector",
tpl: &PodTemplate{
NodeSelector: map[string]string{"kubernetes.io/hostname": "node-1"},
},
defaultTpl: &PodTemplate{
NodeSelector: map[string]string{"kubernetes.io/os": "linux"},
},
expected: &PodTemplate{
NodeSelector: map[string]string{"kubernetes.io/hostname": "node-1", "kubernetes.io/os": "linux"},
},
},
{
name: "update host network",
tpl: &PodTemplate{
HostNetwork: false,
},
defaultTpl: &PodTemplate{
HostNetwork: true,
},
expected: &PodTemplate{
HostNetwork: true,
},
},
}

for _, tc := range testCases {
t.Run(tc.name, func(t *testing.T) {
result := ExpandedMergePodTemplateWithDefault(tc.tpl, tc.defaultTpl)
if !reflect.DeepEqual(result, tc.expected) {
t.Errorf("mergeByName(%v, %v) = %v, want %v", tc.tpl, tc.defaultTpl, result, tc.expected)
}
})
}
}
2 changes: 1 addition & 1 deletion pkg/apis/pipeline/v1/pipelinerun_types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -660,7 +660,7 @@ func (pr *PipelineRun) GetTaskRunSpec(pipelineTaskName string) PipelineTaskRunSp
if task.PipelineTaskName == pipelineTaskName {
// merge podTemplates specified in pipelineRun.spec.taskRunSpecs[].podTemplate and pipelineRun.spec.podTemplate
// with taskRunSpecs taking higher precedence
s.PodTemplate = pod.MergePodTemplateWithDefault(task.PodTemplate, s.PodTemplate)
s.PodTemplate = pod.ExpandedMergePodTemplateWithDefault(task.PodTemplate, s.PodTemplate)
if task.ServiceAccountName != "" {
s.ServiceAccountName = task.ServiceAccountName
}
Expand Down
110 changes: 110 additions & 0 deletions pkg/apis/pipeline/v1/pipelinerun_types_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -689,6 +689,116 @@ func TestPipelineRun_GetTaskRunSpec(t *testing.T) {
SchedulerName: "task-2-schedule",
},
},
}, {
name: "verify whether tolerations are merged correctly",
pr: &v1.PipelineRun{
ObjectMeta: metav1.ObjectMeta{Name: "pr"},
Spec: v1.PipelineRunSpec{
TaskRunTemplate: v1.PipelineTaskRunTemplate{
PodTemplate: &pod.Template{
Tolerations: []corev1.Toleration{
{
Key: "arch",
Operator: corev1.TolerationOpEqual,
Effect: corev1.TaintEffectPreferNoSchedule,
Value: "arm64",
},
},
},
ServiceAccountName: "defaultSA",
},
PipelineRef: &v1.PipelineRef{Name: "prs"},
TaskRunSpecs: []v1.PipelineTaskRunSpec{{
PipelineTaskName: "different-tolerations",
ServiceAccountName: "different-tolerations-service-account",
PodTemplate: &pod.Template{
Tolerations: []corev1.Toleration{
{
Key: "pipeline-priority",
Operator: corev1.TolerationOpEqual,
Effect: corev1.TaintEffectNoSchedule,
Value: "low",
},
},
},
}, {
PipelineTaskName: "same-tolerations",
ServiceAccountName: "same-tolerations-service-account",
PodTemplate: &pod.Template{
Tolerations: []corev1.Toleration{
{
Key: "arch",
Operator: corev1.TolerationOpEqual,
Effect: corev1.TaintEffectPreferNoSchedule,
Value: "amd64",
},
},
},
}},
},
},
expectedPodTemplates: map[string]*pod.PodTemplate{
"different-tolerations": {
Tolerations: []corev1.Toleration{
{
Key: "pipeline-priority",
Operator: corev1.TolerationOpEqual,
Effect: corev1.TaintEffectNoSchedule,
Value: "low",
}, {
Key: "arch",
Operator: corev1.TolerationOpEqual,
Effect: corev1.TaintEffectPreferNoSchedule,
Value: "arm64",
},
},
},
"same-tolerations": {
Tolerations: []corev1.Toleration{
{
Key: "arch",
Operator: corev1.TolerationOpEqual,
Effect: corev1.TaintEffectPreferNoSchedule,
Value: "amd64",
},
},
},
},
}, {
name: "verify whether nodeselector are merged correctly",
pr: &v1.PipelineRun{
ObjectMeta: metav1.ObjectMeta{Name: "pr"},
Spec: v1.PipelineRunSpec{
TaskRunTemplate: v1.PipelineTaskRunTemplate{
PodTemplate: &pod.Template{
NodeSelector: map[string]string{"disktype": "ssd"},
},
ServiceAccountName: "defaultSA",
},
PipelineRef: &v1.PipelineRef{Name: "prs"},
TaskRunSpecs: []v1.PipelineTaskRunSpec{{
PipelineTaskName: "different-nodeselector",
ServiceAccountName: "different-nodeselector-service-account",
PodTemplate: &pod.Template{
NodeSelector: map[string]string{"disktype": "hdd"},
},
}, {
PipelineTaskName: "same-nodeselector",
ServiceAccountName: "same-nodeselector-service-account",
PodTemplate: &pod.Template{
NodeSelector: map[string]string{"kubernetes.io/os": "linux"},
},
}},
},
},
expectedPodTemplates: map[string]*pod.PodTemplate{
"different-nodeselector": {
NodeSelector: map[string]string{"disktype": "hdd"},
},
"same-nodeselector": {
NodeSelector: map[string]string{"disktype": "ssd", "kubernetes.io/os": "linux"},
},
},
},
} {
for taskName := range tt.expectedPodTemplates {
Expand Down
2 changes: 1 addition & 1 deletion pkg/apis/pipeline/v1beta1/pipelinerun_types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -624,7 +624,7 @@ func (pr *PipelineRun) GetTaskRunSpec(pipelineTaskName string) PipelineTaskRunSp
if task.PipelineTaskName == pipelineTaskName {
// merge podTemplates specified in pipelineRun.spec.taskRunSpecs[].podTemplate and pipelineRun.spec.podTemplate
// with taskRunSpecs taking higher precedence
s.TaskPodTemplate = pod.MergePodTemplateWithDefault(task.TaskPodTemplate, s.TaskPodTemplate)
s.TaskPodTemplate = pod.ExpandedMergePodTemplateWithDefault(task.TaskPodTemplate, s.TaskPodTemplate)
if task.TaskServiceAccountName != "" {
s.TaskServiceAccountName = task.TaskServiceAccountName
}
Expand Down