feat: Emit WorkflowNodeRunning Event #5531
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
kennytrytek
force-pushed
the
kgt/workflowrunning-event-issue-5320
branch
from
1afc23a
to
4d40abe
Compare
Codecov Report
@@ Coverage Diff @@
## master #5531 +/- ##
==========================================
- Coverage 46.91% 46.84% -0.08%
==========================================
Files 244 244
Lines 15209 15286 +77
==========================================
+ Hits 7135 7160 +25
- Misses 7169 7218 +49
- Partials 905 908 +3
Continue to review full report at Codecov.
|
kennytrytek
force-pushed
the
kgt/workflowrunning-event-issue-5320
branch
from
4d40abe
to
b1ef536
Compare
alexec
reviewed
Mar 30, 2021
alexec
requested changes
Mar 30, 2021
alexec
reviewed
Mar 31, 2021
Hmm, that does look like a potentially interesting approach. I'll take a look tonight and see if I can make that work. My initial thought is if it does work, then we won't need node event logic anywhere else, but I may be misunderstanding what |
|
I have an unstated secondary goal here. The existing code is not robust. If we can fix it in this PR by a little redesign, that’s a win. |
|
@alexec, I have updated the implementation to do all I don't think this is a problem, as event consumers must assume that the order may not be totally deterministic (and Argo or Kubernetes don't make any guarantees along those lines). |
|
Shoot, that functional test failure looks related to these changes. I'll have to take another look on Friday. |
|
Running those tests locally succeeded. Closing/reopening to try again. |
|
Functional test failed in a timeout waiting for events, probably waiting for the |
alexec
requested changes
Apr 1, 2021
|
Implemented your suggestions in my local checkout, but it appears the functional test has revealed a real problem with these changes. This is what I see running locally: Still trying to find a solution to that issue... UPDATE: There is a Slack conversation about this for anyone looking to provide assistance. :) |
|
I checkout out your code changes and found the failed unit tests passes locally. I would sync with master and retry. |
kennytrytek
force-pushed
the
kgt/workflowrunning-event-issue-5320
branch
from
1818a70
to
b4f8f1d
Compare
|
Rebased onto latest master, but still see the same error running the functional test: Full error output from the controller logs: |
- Issue argoproj#5320 Signed-off-by: Kenny Trytek <kenneth.g.trytek@gmail.com>
- Remove redundant onNodeComplete function. - Separate recording event in assess_node_status to its own conditional statement. - Rename function for clarity. Signed-off-by: Kenny Trytek <kenneth.g.trytek@gmail.com>
- Consolidate all WorkflowNode* events to one code path. Signed-off-by: Kenny Trytek <kenneth.g.trytek@gmail.com>
- Remove extra loop in event-sending logic. - Move phase event recording to immediately after woc is updated. Signed-off-by: Kenny Trytek <kenneth.g.trytek@gmail.com>
- Reassign woc.wf.TypeMeta during persistUpdates, since the Workflow representation returned from the server does not add this field. Signed-off-by: Kenny Trytek <kenneth.g.trytek@gmail.com>
kennytrytek
force-pushed
the
kgt/workflowrunning-event-issue-5320
branch
from
fafdef0
to
26ba3cd
Compare
alexec
reviewed
Apr 18, 2021
|
@alexec This is ready for review whenever you are available. |
alexec
requested changes
Apr 25, 2021
- Use DeepCopy for event reporting to avoid modifying the active node. Signed-off-by: Kenny Trytek <kenneth.g.trytek@gmail.com>
alexec
reviewed
Apr 28, 2021
- Use objects instead of pointers in the method signature to simplify things. Signed-off-by: Kenny Trytek <kenneth.g.trytek@gmail.com>
|
@kennytrytek I have installed Argo version 3.0.7 but I still don't see WorkflowNodeRunning event, is it expected? |
|
@Sushant20, I believe it was released in Argo 3.1.0. I know it's mentioned in the v3.0.3 cherry-pick, but that does not accurately reflect the release version. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Signed-off-by: Kenny Trytek kenneth.g.trytek@gmail.com
Checklist
Feature Request
This pull request adds a
Runningevent to WorkflowNode execution, outlined in #5320.Testing This Change
Besides the updated unit and functional tests, I ran this change locally with Argo Events installed and a sensor and trigger set up to deliver webhooks to another locally-running server.
Below is a running log of my testing steps as I iterated on this change. Discrete thoughts/iterations are separated by a horizontal line.
Running the hello-world example, I have so far not seen any
WorkflowNodeRunningevent anywhere. I'm not sure why this is the case, but I will continue to look and see what I am missing. I thought writing towoc.eventRecorderwould be sufficient to create these events, but maybe there is another step that I am not aware of.Running this example from the
operator_test.go, I get a different result locally than in the test.Workflow:
Test result in
operator_test.go:Local result from
argo submitof that workflow with an Argo Events sensor:Note that
"Normal WorkflowNodeRunning Running node steps-events[0].a"is missing. I will continue to investigate why this occurs.Simplifying this test, I see the same results.
Workflow:
operator_test.goresult:Local Argo result:
Okay, found it. It's this case where the node completes very quickly and is never registered as "running."
argo-workflows/workflow/controller/operator.go
Line 1598 in d964fe4
Now to decide how to handle it. Should Argo always emit a
WorkflowNodeRunningevent in the case the node ran to completion, even if the event is artificial? I think it should, so that the events emitted are predictable and not susceptible to race conditions.Okay, force-pushed my changes since this PR is still in draft.
Latest results running this workflow:
With an Argo event source configured to send events to a locally-running web server, I get these results:
The results here are exactly what was expected: WorkflowRunning, WorkflowNodeRunning, WorkflowNodeSucceeded, WorkflowSucceeded.
✅