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: Apply podSpecPatch in woc.execWf.Spec and template to pod sequentially #12476

Merged
merged 1 commit into from
Jun 11, 2024
Merged

fix: Apply podSpecPatch in woc.execWf.Spec and template to pod sequentially #12476

merged 1 commit into from
Jun 11, 2024

Conversation

jswxstw
Copy link
Member

@jswxstw jswxstw commented Jan 6, 2024 •
edited
Loading

Fixes #11130
Fixes #13122

Motivation

  1. podSpecPatch missed keywords null after merging(wf/tmpl), so removing fields in map/slice by podSpecPatch does not work.
  2. workflow-level podSpecPatch does not work when workflow is submitted from template by workflowTemplateRef.

Modifications

  1. Apply the workflow/template podSpecPatch to the pod spec sequentially avoiding the merge operation.
  2. Apply podSpecPatch in woc.execWf.Spec instead of woc.wf.

Verification

UT and local test

@agilgur5 agilgur5 added type/security Security related area/spec Changes to the workflow specification. labels Jan 7, 2024
@agilgur5
Copy link

agilgur5 commented Jan 7, 2024

I haven't looked at it in detail, but a quick observation that it does look like a different approach from #11222 (not necessarily good or bad)

Also DCO check is still failing as it's expecting a different/real email address

@jswxstw
Copy link
Member Author

jswxstw commented Jan 7, 2024

#11222

Yes, this PR try to fix bug in pod spec patch, but #11222 try to change the merge logic of mainContainer which I think it is an incompatible change. For example, mainContainer default values in configmap may be missed after merge.
b14c2c3edf7991e3927d4f75e044b905

@agilgur5
Copy link

agilgur5 commented Jan 8, 2024 •
edited
Loading

Ah so this allows overriding with podSpecPatch with null, which is a workaround, but does not touch mainContainer etc merge. Different kind of fix. Thanks for the elaboration!

@jswxstw
Copy link
Member Author

jswxstw commented Mar 25, 2024 •
edited
Loading

argo-workflows/workflow/controller/workflowpod.go

Line 363 in 7286d49

tmpl.PodSpecPatch, err = util.PodSpecPatchMerge(woc.wf, tmpl)

There is a new bug in the original logic here: see #12729 (comment) and it is also fixed in this PR.

@tczhao tczhao self-assigned this May 31, 2024
@jswxstw jswxstw changed the title fix: podSpecPatch missed keywords null after merge(wf/tmpl).Fixes #… fix: Apply podSpecPatch in woc.execWf.Spec and template to pod sequentially May 31, 2024
tczhao
tczhao requested changes Jun 2, 2024
View reviewed changes
Copy link
Member

@tczhao tczhao left a comment

Choose a reason for hiding this comment

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

looks good

workflow/controller/workflowpod_test.go Outdated Show resolved Hide resolved
@jswxstw jswxstw mentioned this pull request Jun 3, 2024
Open
4 tasks
@jswxstw
fix: Apply podSpecPatch in woc.execWf.Spec and template to pod sequen…
82420c6 
…tially. Fixes argoproj#11130 argoproj#13122

Signed-off-by: oninowang <oninowang@tencent.com>
@tczhao tczhao enabled auto-merge (squash) June 3, 2024 03:40
@tczhao
Copy link
Member

tczhao commented Jun 3, 2024

Hi @terrytangyuan , could you have a look at this PR again

This was referenced Jun 4, 2024
Open
Closed
Open
@tczhao tczhao merged commit 6c8393f into argoproj:main Jun 11, 2024
28 checks passed
@tczhao tczhao mentioned this pull request Jun 12, 2024
Open
@agilgur5 agilgur5 added this to the v3.5.x patches milestone Jun 14, 2024
agilgur5 pushed a commit that referenced this pull request Jun 14, 2024
@jswxstw
fix: Apply podSpecPatch in woc.execWf.Spec and template to pod sequ…
e8f0cae 
…entially (#12476)

Signed-off-by: oninowang <oninowang@tencent.com>
Co-authored-by: jswxstw <jswxstw@gmail.com>
(cherry picked from commit 6c8393f)
@tczhao tczhao mentioned this pull request Jun 18, 2024
Closed
This was referenced Jun 18, 2024
Open
Open
Open
@tczhao tczhao mentioned this pull request Jun 28, 2024
Open
@jswxstw jswxstw mentioned this pull request Nov 1, 2024
Closed
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@terrytangyuan terrytangyuan terrytangyuan approved these changes

@tczhao tczhao tczhao approved these changes

Assignees

@terrytangyuan terrytangyuan

@tczhao tczhao

Labels
area/spec Changes to the workflow specification. type/security Security related
Projects
None yet
Milestone
v3.5.x patches
4 participants
@jswxstw @agilgur5 @tczhao @terrytangyuan