fix(controller): Updates GetTaskAncestry to skip visited nod. Fixes #1907

[StoneRelax]

Checklist:

• [b] Either (a) I've created an enhancement proposal and discussed it with the community, (b) this is a bug fix, or (c) this does not need to be in the release notes.
• [c] The title of the PR is (a) conventional, (b) states what changed, and (c) suffixes the related issues number. E.g. "fix(controller): Updates such and such. Fixes #1234". Why? for the release notes.
• Optional. I've added My organization is added to the README.
• I've signed the CLA and required builds are green.

Fixes #1907

[codecov]

Codecov Report

Merging #1908 into master will increase coverage by 1.8%.
The diff coverage is 100%.

@@            Coverage Diff            @@
##           master    #1908     +/-   ##
=========================================
+ Coverage   11.34%   13.14%   +1.8%     
=========================================
  Files          75       72      -3     
  Lines       31291    25087   -6204     
=========================================
- Hits         3549     3297    -252     
+ Misses      27266    21375   -5891     
+ Partials      476      415     -61

Impacted Files	Coverage Δ
workflow/common/ancestry.go	97.36% <100%> (+0.22%)	⬆️
workflow/common/convert.go	0% <0%> (-94.29%)	⬇️
workflow/controller/scope.go	21.42% <0%> (-30.96%)	⬇️
workflow/controller/config_controller.go	14.72% <0%> (-11.67%)	⬇️
workflow/controller/steps.go	60.46% <0%> (-4.77%)	⬇️
pkg/apis/workflow/v1alpha1/workflow_types.go	8.33% <0%> (-3.22%)	⬇️
server/cronworkflow/cron_workflow_server.go	51.61% <0%> (-2.68%)	⬇️
server/workflow/workflow_server.go	33.33% <0%> (-2.32%)	⬇️
workflow/executor/executor.go	4.09% <0%> (-0.42%)	⬇️
cmd/argo/commands/get.go	9.59% <0%> (-0.26%)	⬇️
... and 28 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 183a29e...6fa95db. Read the comment docs.

[alexec]

@jessesuen @sarabala1979 is there a good reason to revisit these?

[StoneRelax]

is this being reviewed ? please let me know if I missed something

[CLAassistant]

All committers have signed the CLA.

[StoneRelax]

finally resolved CLA issue. Is there any update on this plz?

[alexec]

Couple of things need doing:

1. CI stalled. If you git merge master && git push that should fix it.
2. I think tests need to be added.
3. I think @sarabala1979 should have the opportunity to review this.

[simster7]

The outputs of this change should be identical to the output before this PR.

@StoneRelax do you see a significant performance improvement here? Seems like the code skipped is not expensive, so curious about how much improvement this brings.

Regardless, I agree that this function should be covered by tests. I have also merged to master for you

[StoneRelax]

@simster7 thanks for reviewing.
GetTaskAncestry is not expensive but it is recursively called, so if the DAG has too many tasks within nested dependencies it will result controller spending forever to do DAG validation like described in issues/1907 (wf spec to reproduce is provided). Since the task dependencies is unlikely to change during a single DAG validation cycle, it should skip visited node just like what do when ensuring DAG has no loop.
@alexec CI is now passing. For test case I think workflow/common/ancestry_test.TestGetTaskAncestryForValidation is ensuring the output of this method is the same with before. Let me know if you think more test is needed.

[simster7]

Thanks @StoneRelax!