feat: self-reporting workflow progress. Fixes #1658, #4245 #6714

mweibel · 2021-09-13T14:34:56Z

port of argo-workflow pod progress code to current codebase and extended to handle EOF streams.

most of the code is by @alexec from: https://github.com/argoproj/argo-workflows/pull/4015/files
closes #1658, #4245

I just started to test this code in my cluster, therefore I'd like to wait with considering this PR ready until I'm more confident.
An initial review would be great though!

I didn't implement the part with reporting also a progress message as the whole pod status reporting got changed meanwhile and I'm not sure if we need it anyway.

Thanks :)

alexec

I think my original PoC has some issues:

It requires tailing the pod logs, and that is expensive. Instead we should build on the pattern that the Emissary uses. The process should write a file to a shared volume (e.g. /var/run/argo/progress) to indicate its progress. I know this is not as user friendly as printing to log files, but I think logs files is too expensive.
We want to move away from requiring pod patch permission, to some other mechanism. However, that's a big piece of work, not something to do in this PR. I think we can still run progress on a ticker, but the polling period should be configurable (using env var) maybe starting with 1m rather than 30s. It should only patch if the progress has changed.

mweibel · 2021-09-14T06:10:00Z

Thanks for the comment. I did wonder about what your thoughts were so that clears things up!
Happy to look into doing the progress monitoring in another way and might come back with questions once I get to it.

alexec · 2021-09-28T15:54:31Z

Some additional thoughts on this:

We want to minimise reads and writes to the Kubernetes API. These create churn on etc and can impact the entire cluster's stability. A workflow that has 10k pods that is suddenly doing 10k reads and write every 30s due to this feature would be a problem.
I can see that it is possible to only do writes, and do them as soon as needed. Poll /var/run/argo/progress for changes every 3s, if the file has changed since the last patch, and it is at least 30s since the last pod patch, do the patch. Otherwise, do nothing.

mweibel · 2021-09-29T12:37:54Z

when doing file based progress watching the question is a bit how to do that for other executors than emissary. Should I mount an emptyDir similar to what emissary does when creating the pod (if not emissary executor) and specify using e.g. an env variable what file to watch for progress?

alexec · 2021-09-29T15:34:48Z

Should I mount an emptyDir similar to what emissary does when creating the pod

Yes. It only gets mounted for the emissary today, but we can mount it for all pods.

mweibel · 2021-09-30T07:30:54Z

as I'm continuing working on this I came accross the podReconciliation() function which retrieves all pods and assesses the status for each one.

Would it make sense to move the updating of progress to this function (within performAssessment most likely)?

Reason why I came across this: due to #6647 I need to change the way I access the pods via the informer because they have different keys in the index. Just querying the podInformer using the namespace/nodeID combination doesn't work anymore and I wondered how I could best query the pods. Passing the return value of woc.getAllWorkflowPods() instead of the informer to the updateProgress function is an idea but has some other issues. Maybe adjusting the podReconciliation function makes more sense.

mweibel · 2021-09-30T14:56:16Z

re-requested a review to see if that goes into the right direction.

Several things still need to be done:

make tickers/timeouts configurable. ConfigMap? Env variable? needs to be discussed, not sure what's best for this case
make watching for progress configurable too. It probably adds some load to a system in any case, so I think it should be an optional feature
decide about progress message. Do we want to support that or not? If so, how?
go over variable naming in monitorProgress again, podPatchTickerTime, progressPollDuration are names which I dislike currently
check if there's a better way to discover if pods are running than the approach in monitorProgress currently
unit test fails, check if that's related to progress feature
e2e test fails, will need to fix it. I haven't been able to run the e2e tests locally so that needs to be done first

workflow/controller/workflowpod.go

mweibel · 2021-10-01T11:22:04Z

another TODO (mostly for me, but if anyone has inputs on those todos, please chime in!)

should we stat the file every 3s and check if modified instead of reading it every 3s?

workflow/executor/executor.go

workflow/progress/updater.go

workflow/controller/operator.go

config/config.go

alexec

this is looking great! I'm going to keep the comments coming

cmd/argoexec/commands/root.go

workflow/controller/operator.go

workflow/controller/operator_test.go

workflow/controller/workflowpod.go

workflow/executor/executor.go

codecov · 2021-10-07T13:44:28Z

Codecov Report

Merging #6714 (950f234) into master (02165aa) will increase coverage by 0.16%.
The diff coverage is 59.55%.

❗ Current head 950f234 differs from pull request most recent head 98b8d56. Consider uploading reports for the commit 98b8d56 to get more accurate results

@@            Coverage Diff             @@
##           master    #6714      +/-   ##
==========================================
+ Coverage   48.58%   48.75%   +0.16%     
==========================================
  Files         267      265       -2     
  Lines       19457    19395      -62     
==========================================
+ Hits         9454     9456       +2     
+ Misses       8946     8881      -65     
- Partials     1057     1058       +1

Impacted Files	Coverage Δ
cmd/argoexec/commands/root.go	`2.63% <0.00%> (-0.08%)`	⬇️
workflow/common/common.go	`80.00% <ø> (ø)`
workflow/controller/controller.go	`22.53% <0.00%> (ø)`
workflow/progress/updater.go	`65.78% <21.42%> (-27.76%)`	⬇️
workflow/executor/executor.go	`30.93% <51.25%> (+6.82%)`	⬆️
pkg/apis/workflow/v1alpha1/progress.go	`90.00% <100.00%> (+1.11%)`	⬆️
workflow/controller/operator.go	`71.53% <100.00%> (+0.14%)`	⬆️
workflow/controller/workflowpod.go	`74.45% <100.00%> (+0.55%)`	⬆️
server/auth/gatekeeper.go	`50.00% <0.00%> (-8.38%)`	⬇️
workflow/util/pod_name.go	`46.66% <0.00%> (-1.34%)`	⬇️
... and 15 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 02165aa...98b8d56. Read the comment docs.

mweibel · 2021-10-07T14:39:55Z

To be able to test the progress feature in an e2e test I need to adjust the workflow-controller-configmap for that specific test. I couldn't find an example how to do that, is this not yet a case covered?

docs/progress.md

workflow/controller/workflowpod.go

docs/progress.md

workflow/executor/executor.go

workflow/executor/executor_test.go

alexec · 2021-10-26T15:42:48Z

the e2e failure is due to the fact that I had to adjust argosay to be able to specify a custom sleep duration when doing an echo. This would need a new build of argosay before doing the e2e test I believe. Could anyone do that or recommend a different approach?

I would just use a script to do this rather then force a new argosay image. Argosay does contain a shell.

alexec

I'm going to push you to simplify.

Try syncing with master to fix your builds.

workflow/controller/workflowpod_test.go

workflow/executor/executor.go

alexec

look great btw!

workflow/controller/workflowpod.go

alexec

One comment on what looks like a bug.
Please sync with master.
Please dismiss this review when done.

workflow/controller/workflowpod.go

original code from: https://github.com/argoproj/argo-workflows/pull/4015/files closes argoproj#1658, argoproj#4245 Signed-off-by: Michael Weibel <[email protected]>

alexec · 2021-11-03T15:24:42Z

workflow/executor/executor.go

@@ -77,6 +79,12 @@ type WorkflowExecutor struct {
 // list of errors that occurred during execution.
 // the first of these is used as the overall message of the node
 errors []error
+
+ // current progress which is synced every `annotationPatchTickDuration` to the pods annotations.
+ progress string


minor - could be Progress type?

I can make a PR to change this, probably tomorrow 👍

simster7 · 2021-11-03T15:28:33Z

This is pretty cool! 🚀

alexec · 2021-11-03T15:30:32Z

I think this is a neat feature, as esp useful for CI use cases, or long running tasks.

mweibel · 2021-11-03T16:41:09Z

amazing, thanks for merging and the great reviews @alexec! We started using this already and it works well so far (though we used a to version 3.2 backported branch of it). Looking forward to the release :)

alexec reviewed Sep 13, 2021

View reviewed changes

mweibel force-pushed the custom-workflow-progress branch from a0bb1e5 to b13a971 Compare September 28, 2021 12:28

mweibel force-pushed the custom-workflow-progress branch 2 times, most recently from e708e5e to c31bbcd Compare September 30, 2021 14:48

mweibel requested a review from alexec September 30, 2021 14:48

alexec reviewed Sep 30, 2021

View reviewed changes

workflow/controller/workflowpod.go Outdated Show resolved Hide resolved

mweibel force-pushed the custom-workflow-progress branch 2 times, most recently from 9b6f526 to 64987e3 Compare October 1, 2021 15:23

alexec reviewed Oct 1, 2021

View reviewed changes

workflow/executor/executor.go Outdated Show resolved Hide resolved

workflow/progress/updater.go Outdated Show resolved Hide resolved

workflow/controller/operator.go Outdated Show resolved Hide resolved

mweibel force-pushed the custom-workflow-progress branch 3 times, most recently from 634bbad to 2accf4c Compare October 5, 2021 14:30

alexec reviewed Oct 5, 2021

View reviewed changes

config/config.go Outdated Show resolved Hide resolved

alexec reviewed Oct 5, 2021

View reviewed changes

mweibel force-pushed the custom-workflow-progress branch 4 times, most recently from 784bd4c to 63a2da0 Compare October 7, 2021 13:34

alexec reviewed Oct 7, 2021

View reviewed changes

docs/progress.md Outdated Show resolved Hide resolved

alexec requested changes Oct 7, 2021

View reviewed changes

workflow/controller/workflowpod.go Outdated Show resolved Hide resolved

docs/progress.md Outdated Show resolved Hide resolved

workflow/executor/executor.go Show resolved Hide resolved

workflow/executor/executor_test.go Show resolved Hide resolved

mweibel force-pushed the custom-workflow-progress branch from aa58fe2 to e89c7a2 Compare October 8, 2021 11:56

mweibel force-pushed the custom-workflow-progress branch 2 times, most recently from 94a412a to 11a7e34 Compare October 26, 2021 14:58

alexec requested changes Oct 26, 2021

View reviewed changes

alexec reviewed Oct 26, 2021

View reviewed changes

alexec changed the title ~~feat: self reporting workflow progress~~ feat: self-reporting workflow progress. Fixes #1658, #4245 Oct 26, 2021

mweibel force-pushed the custom-workflow-progress branch from 30f918e to d2c7f65 Compare October 28, 2021 09:40

mweibel commented Oct 28, 2021

View reviewed changes

workflow/controller/workflowpod.go Outdated Show resolved Hide resolved

mweibel requested a review from alexec October 28, 2021 10:12

mweibel force-pushed the custom-workflow-progress branch 2 times, most recently from 51fee1b to be986ab Compare October 28, 2021 12:26

alexec requested changes Oct 31, 2021

View reviewed changes

workflow/controller/workflowpod.go Outdated Show resolved Hide resolved

mweibel force-pushed the custom-workflow-progress branch 2 times, most recently from 83dc731 to 966e677 Compare November 1, 2021 07:41

mweibel requested a review from alexec November 1, 2021 08:47

alexec reviewed Nov 1, 2021

View reviewed changes

workflow/controller/workflowpod.go Outdated Show resolved Hide resolved

mweibel mentioned this pull request Nov 2, 2021

fix(controller): default volume/mount to emissary. Fixes #7118 #7125

Merged

1 task

mweibel force-pushed the custom-workflow-progress branch 4 times, most recently from 2ccc723 to 747e2e8 Compare November 3, 2021 10:27

feat: self reporting workflow progress

98b8d56

original code from: https://github.com/argoproj/argo-workflows/pull/4015/files closes argoproj#1658, argoproj#4245 Signed-off-by: Michael Weibel <[email protected]>

mweibel force-pushed the custom-workflow-progress branch from 747e2e8 to 98b8d56 Compare November 3, 2021 10:50

mweibel requested a review from alexec November 3, 2021 12:15

alexec approved these changes Nov 3, 2021

View reviewed changes

alexec merged commit 7886a2b into argoproj:master Nov 3, 2021

alexec mentioned this pull request Nov 8, 2021

Patching pod status too often #7184

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: self-reporting workflow progress. Fixes #1658, #4245 #6714

feat: self-reporting workflow progress. Fixes #1658, #4245 #6714

mweibel commented Sep 13, 2021

alexec left a comment •

edited

Loading

mweibel commented Sep 14, 2021

alexec commented Sep 28, 2021

mweibel commented Sep 29, 2021

alexec commented Sep 29, 2021

mweibel commented Sep 30, 2021 •

edited

Loading

mweibel commented Sep 30, 2021 •

edited

Loading

mweibel commented Oct 1, 2021 •

edited

Loading

alexec left a comment

codecov bot commented Oct 7, 2021 •

edited

Loading

mweibel commented Oct 7, 2021

alexec commented Oct 26, 2021

alexec left a comment

alexec left a comment

alexec left a comment •

edited

Loading

alexec Nov 3, 2021

mweibel Nov 3, 2021

simster7 commented Nov 3, 2021

alexec commented Nov 3, 2021

mweibel commented Nov 3, 2021

feat: self-reporting workflow progress. Fixes #1658, #4245 #6714

feat: self-reporting workflow progress. Fixes #1658, #4245 #6714

Conversation

mweibel commented Sep 13, 2021

alexec left a comment • edited Loading

Choose a reason for hiding this comment

mweibel commented Sep 14, 2021

alexec commented Sep 28, 2021

mweibel commented Sep 29, 2021

alexec commented Sep 29, 2021

mweibel commented Sep 30, 2021 • edited Loading

mweibel commented Sep 30, 2021 • edited Loading

mweibel commented Oct 1, 2021 • edited Loading

alexec left a comment

Choose a reason for hiding this comment

codecov bot commented Oct 7, 2021 • edited Loading

Codecov Report

mweibel commented Oct 7, 2021

alexec commented Oct 26, 2021

alexec left a comment

Choose a reason for hiding this comment

alexec left a comment

Choose a reason for hiding this comment

alexec left a comment • edited Loading

Choose a reason for hiding this comment

alexec Nov 3, 2021

Choose a reason for hiding this comment

mweibel Nov 3, 2021

Choose a reason for hiding this comment

simster7 commented Nov 3, 2021

alexec commented Nov 3, 2021

mweibel commented Nov 3, 2021

alexec left a comment •

edited

Loading

mweibel commented Sep 30, 2021 •

edited

Loading

mweibel commented Sep 30, 2021 •

edited

Loading

mweibel commented Oct 1, 2021 •

edited

Loading

codecov bot commented Oct 7, 2021 •

edited

Loading

alexec left a comment •

edited

Loading