-
Notifications
You must be signed in to change notification settings - Fork 3.1k
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.
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
fix(controller): set pod name version annotation when no lock #12965
base: main
Are you sure you want to change the base?
fix(controller): set pod name version annotation when no lock #12965
Conversation
- `setWfPodNamesAnnotation` was added to the `Unknown` phase check a few lines below this, but not to this line - these are the two places where `Unknown` is handled - without this, the annotation wouldn't be set if the workflow couldn't grab a lock, creating a mismatch when using synchronization - also simplify the conditional into one with an `&&` rather than nesting - reducing nesting reduces code complexity - slightly simplify `setWfPodNamesAnnotation` as well Signed-off-by: Anton Gilgur <agilgur5@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, but we better have the test case added before merge right?
phase := woc.wf.Status.Phase | ||
if phase == wfv1.WorkflowUnknown { | ||
phase = wfv1.WorkflowPending | ||
setWfPodNamesAnnotation(woc.wf) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hate to be picky but wondering if we could just set this in one place at the top of the function? like maybe logic like "if annotation isn't set, then set it". Is there any reason it should ever not be set? That would handle any future cases where we preemptively exit for whatever reason.
Fixes #10178, Fixes #6982 (comment), Fixes #10735 (comment)
Partly addresses #6356 (comment), but not entirely, as the PDB is a more complicated case. Will file a separate issue for that. EDIT: filed #12966
Motivation
setWfPodNamesAnnotation
was added to theUnknown
phase check a few lines below this, but not to this lineUnknown
is handledModifications
add
setWfPodNamesAnnotation
to the otherUnknown
phase block&&
rather than nestingslightly simplify
setWfPodNamesAnnotation
as wellVerification
Tested with the same Workflow from #12966, creating two in quick succession (one waits on the mutex lock from the other),
workflows.argoproj.io/pod-name-format: v2
annotations present in both.Should probably add a case to
operator_concurrency_test.go
, TBD