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.

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

docs: updated self reporting progress example to use WorkflowTaskResults instead of Pod annotations #12089

Merged
merged 5 commits into from
Jun 29, 2024

Conversation

mrbig
Copy link
Contributor

@mrbig mrbig commented Oct 27, 2023

This is a proposed update to the self reporting progress documentation at https://argoproj.github.io/argo-workflows/progress/

The current example is not recommended with version >= 3.4 because the pod metadata has been superseded by the workflowtaskresults resource.

With recent versions if you add the workflows.argoproj.io/progress annotation on the pod the controller will fall back to the
legacy mode. Also the annotation will revert the output of the executor and the progress does not change during the workflow run.

I have also included some hints how the progress update works, because it took me a while to figure out why it takes so long
for the progress to appear on the UI.

I have tested the original and the updated example on argo workflows version 3.5.0

@mrbig mrbig marked this pull request as ready for review October 27, 2023 21:50
Copy link

@agilgur5 agilgur5 left a comment

Choose a reason for hiding this comment

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

Nice catch.

There are some additional changes needed if we want to remove mentions of the pod annotation, see in-line comments

docs/progress.md Outdated Show resolved Hide resolved
docs/progress.md Outdated Show resolved Hide resolved
every 1m.

These values cane be fine tuned with the `ARGO_PROGRESS_PATCH_TICK_DURATION` and `ARGO_PROGRESS_FILE_TICK_DURATION`
environment variables.

Initially the progress of a workflows' pod is always `0/1`. If you want to influence this, make sure to set an initial
progress annotation on the pod:

Choose a reason for hiding this comment

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

here and on line 39 above, "pod annotations" are specifically mentioned. those should be altered as well if we will not be recommending those

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@agilgur5 I'm new to argo workflow, so I'm might not the best person to explain the process. If I understand correctly now the executor is picking up the progress and patches that to the workflowtaskresults resource. Then the controller will pick up the values from there. Am I right?
If you can suggest a new explanation for lines 38-39, I would be very glad.
Thank you

Choose a reason for hiding this comment

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

I believe your understanding is correct. I'm not an expert in the Executor code yet either actually.

Based on your analysis in the PR description, it sounds like workflows.argoproj.io/progress is buggy and actually needs a bugfix for 3.4 to update/initialize workflowtaskresults instead of patching the pod.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure if it's a bug or just some conflict between the old and new behavior that could be avoided by proper configuration on the user side.

My experience on v3.5 showed that the executor automatically falls back to patching the pod resource if it has no permission to patch the workflowtaskresults, and issues a warning about this.

Meanwhile the controller looks at the pod annotations, and if finds the workflows.argoproj.io/progress annotation then the values found in there will override the values from workflowtaskresults, and issues the warning workflow uses legacy/insecure pod patch, see https://argoproj.github.io/argo-workflows/workflow-rbac/

By following the current documentation I easily navigated me into this situation where the executor was running the new behavior while the controller expecting the old one.

My goal with this PR is to help others to avoid the mistake I have made.

I have updated the explanation of the process to match the "new" method, please review it.

Choose a reason for hiding this comment

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

#13260 is potentially related

@agilgur5 agilgur5 added the area/docs Incorrect, missing, or mistakes in docs label Oct 27, 2023
mrbig and others added 2 commits October 28, 2023 00:17
Co-authored-by: Anton Gilgur <[email protected]>
Signed-off-by: Nagy Attila Gábor <[email protected]>
Co-authored-by: Anton Gilgur <[email protected]>
Signed-off-by: Nagy Attila Gábor <[email protected]>
The controller picks this up and writes the progress to the appropriate Status properties.
patch the matching `WorkflowTaskResults` resource with the value `N/M`.
The controller picks this up and writes the progress to the appropriate Status properties
every 1m.
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for this change. It looks like it's both patching annotations and patching WorkflowTaskResults with this progress information, right?

Copy link
Contributor

@juliev0 juliev0 Jun 29, 2024

Choose a reason for hiding this comment

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

Ahh, you're right. I looked at the code too quickly. The Pod Annotations are a backup, as you mention in your other comment.

every 1m.

These values can be fine tuned with the `ARGO_PROGRESS_PATCH_TICK_DURATION` and `ARGO_PROGRESS_FILE_TICK_DURATION`
[environment variables](environment-variables.md).
Copy link
Contributor

Choose a reason for hiding this comment

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

even better if maybe we can clarify what those values represent?

@juliev0 juliev0 merged commit 0322579 into argoproj:main Jun 29, 2024
16 checks passed
@agilgur5 agilgur5 changed the title docs: updated self reporting progress example to match recent conventions docs: updated self reporting progress example to use WorkflowTaskResults instead of Pod annotations Sep 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/docs Incorrect, missing, or mistakes in docs area/executor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants