Improve DAG validation for pipelines with hundreds of tasks #5421

rafalbigaj · 2022-09-02T09:24:01Z

DAG validation rewritten using Kahn's algorithm to find cycles in task dependencies.

Original implementation, as pointed at #5420 is a root cause of poor validation webhook performance, which fails on default timeout (10s).

Changes

Submitter Checklist

As the author of this PR, please check off the items in this checklist:

Has Tests included if any functionality added or changed
Follows the commit message standard
Meets the Tekton contributor standards (including
functionality, content, code)
Has a kind label. You can add one by adding a comment on this PR that contains /kind <type>. Valid types are bug, cleanup, design, documentation, feature, flake, misc, question, tep
Release notes block below has been updated with any user facing changes (API changes, bug fixes, changes requiring upgrade notices or deprecation warnings)

/kind bug

Release Notes

bug fixes:
- https://github.com/tektoncd/pipeline/issues/5420 - Improve DAG validation for pipelines with hundreds of tasks (validation wehbook performance)

tekton-robot · 2022-09-02T09:24:12Z

Hi @rafalbigaj. Thanks for your PR.

I'm waiting for a tektoncd member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

pkg/reconciler/pipeline/dag/dag.go

Udiknedormin · 2022-09-02T10:37:25Z

pkg/reconciler/pipeline/dag/dag.go

+			if len(deps[dep]) == 0 {
+				independentTasks.Insert(dep)
+			}


I can see it's an optimization originating from the fact that no topo-sort order is actually built or printed (as "independent" tasks that are also "final" ones --- are not even put into the independentTasks set).

pkg/reconciler/pipeline/dag/dag.go

Udiknedormin · 2022-09-02T10:45:58Z

pkg/reconciler/pipeline/dag/dag_test.go

@@ -549,6 +550,78 @@ func TestBuild_InvalidDAG(t *testing.T) {
 	}
 }

+func TestBuildGraphWithHundredsOfTasks_Success(t *testing.T) {


Maybe some performance check too?

yes please, so that we do not run into it in future, thanks!

The problem with performance checks is that they could be inherently flaky because of the variable performance of the test nodes. If we do add performance checks, I would suggest for now to only log the execution time.
We can collect such timings over time for some time and then decide on an acceptable bar for the execution time.

That's a great idea @afrittoli, where do we store those timings?

I was running the test you had introduced #3524 (a huge thanks to you for introducing such test):

pipeline/pkg/reconciler/pipelinerun/resources/pipelinerunstate_test.go

Line 1404 in 99404d5

func buildPipelineStateWithLargeDepencyGraph(t *testing.T) PipelineRunState {

I ran it locally yesterday multiple times which took 60 seconds 😲 (without this PR).

At the time introducing this test, it took less than the default timeout 30 seconds based on the PR description (if I am reading it right):

This changes adds a unit test that reproduces the issue in https://github.com/tektoncd/pipeline/issues/3521, which used to fail (with timeout 30s) and now succeedes for pipelines roughly up to 120 tasks / 120 links. On my laptop, going beyond 120 tasks/links takes longer than 30s, so I left the unit test at 80 to avoid introducing a flaky test in CI. There is still work to do to improve this further, some profiling / tracing work might help.

There has been many changes introduced since then, we really need a way to flag us (nightly performance tests) when we introduce any delay.

Nightly performance test or like you are suggesting to collect timings over time. I am fine with either option.

Yes. The TestBuildGraphWithHundredsOfTasks_Success in a way is a kind of performance test (like the one I added at the time), because if things slow down significantly, the tests will eventually timeout.
If we collected test execution times and graphed them over time - or if we had some dedicated nightly performance test, we would be able to see a change in execution time sooner than just waiting for the tests to timeout.

That is something that we would need to setup as part of the CI infrastructure. Would you like to create an issue about that?

yup definitely, we had a PR from @guillaumerose #4378 which didn't materialize but at least most of us including @vdemeester and @imjasonh were on board with the idea of running nightly performance tests.

vdemeester · 2022-09-02T11:18:41Z

/ok-to-test

tekton-robot · 2022-09-02T11:23:21Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/reconciler/pipeline/dag/dag.go	98.8%	97.9%	-0.8

tekton-robot · 2022-09-02T12:16:58Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/reconciler/pipeline/dag/dag.go	98.8%	99.0%	0.3

tekton-robot · 2022-09-02T12:32:06Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/reconciler/pipeline/dag/dag.go	98.8%	99.0%	0.3

tekton-robot · 2022-09-02T12:36:58Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/reconciler/pipeline/dag/dag.go	98.8%	99.0%	0.3

Udiknedormin

Looks ok to me now.

afrittoli · 2022-09-02T13:18:15Z

pkg/reconciler/pipeline/dag/export_test.go

@@ -0,0 +1,21 @@
+/*
+Copyright 2019 The Tekton Authors


afrittoli · 2022-09-02T13:21:22Z

Thanks @rafalbigaj for this! Do you have any number about the improvement in performance with the new algorithm?
This change adds complexity to the code, which is fine as long as it is justified by the performance improvement.

DAG validation rewritten using Kahn's algorithm to find cycles in task dependencies. Original implementation, as pointed at tektoncd#5420 is a root cause of poor validation webhook performance, which fails on default timeout (10s).

rafalbigaj · 2022-09-02T14:49:21Z

@afrittoli let me provide as an example results from TestBuildGraphWithHundredsOfTasks_Success test in both implementations:

0.04s - new implementation based on cycles detection using Kahn's algorithm (findCyclesInDependencies)
did not finish in 1h! - old implementation based on cycle detection on every link addition (lookForNode in linkPipelineTasks)

Benchmarked on: 2,3 GHz 8-Core Intel Core i9; 16 GB 2400 MHz DDR4

afrittoli

Thanks @rafalbigaj for this. The code and test coverage looks good to me.
Just one possible NIT, but nothing blocking for this PR.
/approve

afrittoli · 2022-09-02T14:58:40Z

pkg/reconciler/pipeline/dag/export_test.go

+
+// exports for tests
+
+var FindCyclesInDependencies = findCyclesInDependencies


We have a policy of testing exported functions only (generally), but I think in this case it makes sense to test findCyclesInDependencies directly!

NIT: I wonder if instead of exporting for tests, we could have the test for findCyclesInDependencies in a dedicated test module in the dag package?
Not asking to change this yet, I'd like to see what others think - and we could also change this in a different PR in case.

@tektoncd/core-maintainers wdyt?

tekton-robot · 2022-09-02T15:26:22Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: afrittoli, Udiknedormin

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [afrittoli]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

pritidesai · 2022-09-02T15:34:29Z

pkg/reconciler/pipeline/dag/dag.go

@@ -163,9 +200,7 @@ func addLink(pt string, previousTask string, nodes map[string]*Node) error {
 		return fmt.Errorf("task %s depends on %s but %s wasn't present in Pipeline", pt, previousTask, previousTask)
 	}
 	next := nodes[pt]
-	if err := linkPipelineTasks(prev, next); err != nil {


I had troubleshooted till this point, thanks for taking it further, appreciate your efforts 🙏

pritidesai · 2022-09-02T15:44:29Z

pkg/reconciler/pipeline/dag/dag.go

@@ -163,9 +200,7 @@ func addLink(pt string, previousTask string, nodes map[string]*Node) error {
 		return fmt.Errorf("task %s depends on %s but %s wasn't present in Pipeline", pt, previousTask, previousTask)
 	}
 	next := nodes[pt]


This is holding a huge struct (object) in real world applications. It is set to the entire pipelineTask specification along with a list of params, when expressions, the entire specification when taskSpec is specified, etc. The pipelineTask specification is not required at this point since all the dependencies (runAfter, task results in params and when expressions, and from) are calculated before calling dag.Build. All we need is just the pipelineTask.Name i.e. HashKey(). We have a room of improvement in future to avoid passing the entire blob around.

afrittoli · 2022-09-02T16:10:43Z

@tektoncd/core-maintainers I added the "needs-cherry-pick" label for this item. While there was no explicit functional regression, there has been a performance degradation over time already since v0.36.x, which is significantly eased (solved) by this PR. I would propose doing a new series of minor releases to include this.

abayer · 2022-09-05T19:15:32Z

/lgtm

afrittoli · 2022-09-06T11:17:14Z

/cherrypick release-v0.36.x

tekton-robot · 2022-09-06T11:18:00Z

@afrittoli: #5421 failed to apply on top of branch "release-v0.36.x":

Applying: Improve DAG validation for pipelines with hundreds of tasks
Using index info to reconstruct a base tree...
A	pkg/apis/pipeline/v1/pipeline_validation_test.go
M	pkg/reconciler/pipeline/dag/dag_test.go
Falling back to patching base and 3-way merge...
Auto-merging pkg/reconciler/pipeline/dag/dag_test.go
Auto-merging pkg/apis/pipeline/v1beta1/pipeline_validation_test.go
CONFLICT (content): Merge conflict in pkg/apis/pipeline/v1beta1/pipeline_validation_test.go
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 Improve DAG validation for pipelines with hundreds of tasks
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

In response to this:

/cherrypick release-v0.36.x

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

afrittoli · 2022-09-06T11:19:16Z

/cherrypick release-v0.37.x

tekton-robot · 2022-09-06T11:19:53Z

@afrittoli: new pull request created: #5430

In response to this:

/cherrypick release-v0.37.x

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

afrittoli · 2022-09-06T11:22:07Z

/cherrypick release-v0.38.x

afrittoli · 2022-09-06T11:22:18Z

/cherrypick release-v0.39.x

tekton-robot · 2022-09-06T11:22:44Z

@afrittoli: new pull request created: #5431

In response to this:

/cherrypick release-v0.38.x

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

tekton-robot · 2022-09-06T11:22:55Z

@afrittoli: new pull request created: #5432

In response to this:

/cherrypick release-v0.39.x

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

tekton-robot added kind/bug Categorizes issue or PR as related to a bug. do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Sep 2, 2022

tekton-robot requested review from dibyom and imjasonh September 2, 2022 09:24

tekton-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Sep 2, 2022

Udiknedormin suggested changes Sep 2, 2022

View reviewed changes

tekton-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Sep 2, 2022

rafalbigaj force-pushed the main branch from 18a6a2d to 171d89a Compare September 2, 2022 12:12

rafalbigaj requested review from Udiknedormin and removed request for imjasonh and dibyom September 2, 2022 12:18

rafalbigaj force-pushed the main branch from 171d89a to db0c6ad Compare September 2, 2022 12:27

rafalbigaj force-pushed the main branch from db0c6ad to cf2f32d Compare September 2, 2022 12:32

Udiknedormin approved these changes Sep 2, 2022

View reviewed changes

afrittoli requested review from imjasonh and dibyom September 2, 2022 13:14

afrittoli reviewed Sep 2, 2022

View reviewed changes

tekton-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Sep 2, 2022

rafalbigaj force-pushed the main branch from cf2f32d to 6895508 Compare September 2, 2022 13:45

afrittoli reviewed Sep 2, 2022

View reviewed changes

tekton-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 2, 2022

afrittoli added this to the Pipelines v0.40 milestone Sep 2, 2022

pritidesai reviewed Sep 2, 2022

View reviewed changes

afrittoli added the needs-cherry-pick Indicates a PR needs to be cherry-pick to a release branch label Sep 2, 2022

tekton-robot assigned abayer Sep 5, 2022

tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Sep 5, 2022

tekton-robot merged commit 7b00bd2 into tektoncd:main Sep 5, 2022

vdemeester mentioned this pull request Sep 6, 2022

Validation of pipelines with hundreds of tasks and links fails on default webhook timeout (10s) #5420

Closed

tekton-robot mentioned this pull request Sep 6, 2022

[release-v0.37.x] Improve DAG validation for pipelines with hundreds of tasks #5430

Merged

tekton-robot mentioned this pull request Sep 6, 2022

[release-v0.38.x] Improve DAG validation for pipelines with hundreds of tasks #5431

Merged

tekton-robot mentioned this pull request Sep 6, 2022

[release-v0.39.x] Improve DAG validation for pipelines with hundreds of tasks #5432

Merged

This was referenced Sep 6, 2022

Improve DAG validation for pipelines with hundreds of tasks - v1beta1 tests #5433

Closed

Improve DAG validation - v1beta1 tests #5434

Merged

lbernick added the area/performance Issues or PRs that are related to performance aspects. label Sep 6, 2022

pritidesai mentioned this pull request Sep 7, 2022

de-dupe order and resource dependencies #5446

Merged

7 tasks

afrittoli mentioned this pull request Sep 9, 2022

[release-v0.36.x] Improve DAG validation for pipelines with hundreds of tasks #5461

Merged

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve DAG validation for pipelines with hundreds of tasks #5421

Improve DAG validation for pipelines with hundreds of tasks #5421

rafalbigaj commented Sep 2, 2022 •

edited by afrittoli

Loading

tekton-robot commented Sep 2, 2022

Udiknedormin Sep 2, 2022

Udiknedormin Sep 2, 2022

pritidesai Sep 2, 2022 •

edited

Loading

afrittoli Sep 2, 2022

pritidesai Sep 2, 2022 •

edited

Loading

pritidesai Sep 2, 2022

afrittoli Sep 2, 2022

pritidesai Sep 2, 2022

vdemeester commented Sep 2, 2022

tekton-robot commented Sep 2, 2022

tekton-robot commented Sep 2, 2022

tekton-robot commented Sep 2, 2022

tekton-robot commented Sep 2, 2022

Udiknedormin left a comment

afrittoli Sep 2, 2022

afrittoli commented Sep 2, 2022

rafalbigaj commented Sep 2, 2022

afrittoli left a comment

afrittoli Sep 2, 2022

tekton-robot commented Sep 2, 2022

pritidesai Sep 2, 2022

pritidesai Sep 2, 2022

afrittoli commented Sep 2, 2022

abayer commented Sep 5, 2022

afrittoli commented Sep 6, 2022

tekton-robot commented Sep 6, 2022

afrittoli commented Sep 6, 2022

tekton-robot commented Sep 6, 2022

afrittoli commented Sep 6, 2022

afrittoli commented Sep 6, 2022

tekton-robot commented Sep 6, 2022

tekton-robot commented Sep 6, 2022


		// exports for tests

		var FindCyclesInDependencies = findCyclesInDependencies

Improve DAG validation for pipelines with hundreds of tasks #5421

Improve DAG validation for pipelines with hundreds of tasks #5421

Conversation

rafalbigaj commented Sep 2, 2022 • edited by afrittoli Loading

Changes

Submitter Checklist

Release Notes

tekton-robot commented Sep 2, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pritidesai Sep 2, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pritidesai Sep 2, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vdemeester commented Sep 2, 2022

tekton-robot commented Sep 2, 2022

tekton-robot commented Sep 2, 2022

tekton-robot commented Sep 2, 2022

tekton-robot commented Sep 2, 2022

Udiknedormin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

afrittoli commented Sep 2, 2022

rafalbigaj commented Sep 2, 2022

afrittoli left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tekton-robot commented Sep 2, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

afrittoli commented Sep 2, 2022

abayer commented Sep 5, 2022

afrittoli commented Sep 6, 2022

tekton-robot commented Sep 6, 2022

afrittoli commented Sep 6, 2022

tekton-robot commented Sep 6, 2022

afrittoli commented Sep 6, 2022

afrittoli commented Sep 6, 2022

tekton-robot commented Sep 6, 2022

tekton-robot commented Sep 6, 2022

rafalbigaj commented Sep 2, 2022 •

edited by afrittoli

Loading

pritidesai Sep 2, 2022 •

edited

Loading

pritidesai Sep 2, 2022 •

edited

Loading