de-dupe order and resource dependencies #5446

pritidesai · 2022-09-07T00:45:15Z

Changes

In case of a pipeline with a huge list of redundant dependencies, the list of dependencies is growing exponentially. This way of calculating the list of dependencies is causing extra delay in the validation cycle. This delay sometimes hit the webhook timeout during validation. This is one of the changes being proposed to make the validation cycle efficient and avoid unnecessary delay.

Introducing a test case which was running until a timeout was hit (without @rafalbigaj's changes in #5421). Thanks @rafalbigaj for the efficient implementation of building a graph. This test now runs to competition, on average taking 3 minutes. With the changes in this PR, this test runs under two seconds.

/kind bug

Submitter Checklist

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

Has Docs included if any changes are user facing
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)
Release notes contains the string "action required" if the change requires additional action from users switching to the new release

Release Notes

De-dupe task dependencies - order and resource dependencies all together. It's very common to have a task with multiple when expressions referring to the same task but different results. Maintain a set of dependencies and add only a new parent.

tekton-robot · 2022-09-07T00:49:43Z

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/apis/pipeline/v1beta1/pipeline_types.go	97.0%	96.2%	-0.7

tekton-robot · 2022-09-07T05:59:45Z

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/apis/pipeline/v1beta1/pipeline_types.go	97.0%	96.8%	-0.2

rafalbigaj

I like this simplification very much! Thanks for catching the next bottleneck 🙏

abayer · 2022-09-07T13:15:47Z

/retest
/approve

tekton-robot · 2022-09-07T13:15:55Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: abayer, rafalbigaj

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~~ [abayer]

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

afrittoli · 2022-09-08T06:23:29Z

Thanks @pritidesai - could you apply the change to the v1 API as well?

In case of a pipeline with a huge list of redundant dependencies, the list is growing exponentially. This way of calculating the list of dependencies is causing extra delay in the validation cycle. This delay sometimes hit the webhook timeout during validation. This is one of the changes being proposed to make the validation cycle efficient and avoid unneccesary delay.

pritidesai · 2022-09-08T16:00:18Z

thank you @afrittoli for pointing that out 👍 I have applied these changes to v1. Thanks!

tekton-robot · 2022-09-08T16:02:39Z

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/apis/pipeline/v1/pipeline_types.go	96.0%	95.7%	-0.3
pkg/apis/pipeline/v1beta1/pipeline_types.go	97.0%	96.8%	-0.2

vdemeester · 2022-09-08T16:03:58Z

/lgtm

afrittoli · 2022-09-08T20:08:37Z

Do we need to cherry-pick this one too? @pritidesai

pritidesai · 2022-09-12T17:41:39Z

/cherrypick release-v0.37.x

pritidesai · 2022-09-12T17:41:57Z

/cherrypick release-v0.38.x

pritidesai · 2022-09-12T17:42:08Z

/cherrypick release-v0.39.x

tekton-robot · 2022-09-12T17:42:26Z

@pritidesai: #5446 failed to apply on top of branch "release-v0.37.x":

Applying: de-dup order and resource dependencies
Using index info to reconstruct a base tree...
A	pkg/apis/pipeline/v1/pipeline_types.go
A	pkg/apis/pipeline/v1/pipeline_types_test.go
M	pkg/apis/pipeline/v1beta1/pipeline_types.go
M	pkg/apis/pipeline/v1beta1/pipeline_types_test.go
M	pkg/reconciler/pipelinerun/resources/pipelinerunstate_test.go
Falling back to patching base and 3-way merge...
Auto-merging pkg/reconciler/pipelinerun/resources/pipelinerunstate_test.go
CONFLICT (content): Merge conflict in pkg/reconciler/pipelinerun/resources/pipelinerunstate_test.go
Auto-merging pkg/apis/pipeline/v1beta1/pipeline_types_test.go
CONFLICT (content): Merge conflict in pkg/apis/pipeline/v1beta1/pipeline_types_test.go
Auto-merging pkg/apis/pipeline/v1beta1/pipeline_types.go
CONFLICT (modify/delete): pkg/apis/pipeline/v1/pipeline_types_test.go deleted in HEAD and modified in de-dup order and resource dependencies. Version de-dup order and resource dependencies of pkg/apis/pipeline/v1/pipeline_types_test.go left in tree.
CONFLICT (modify/delete): pkg/apis/pipeline/v1/pipeline_types.go deleted in HEAD and modified in de-dup order and resource dependencies. Version de-dup order and resource dependencies of pkg/apis/pipeline/v1/pipeline_types.go left in tree.
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 de-dup order and resource dependencies
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.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.

tekton-robot · 2022-09-12T17:42:35Z

@pritidesai: #5446 failed to apply on top of branch "release-v0.38.x":

Applying: de-dup order and resource dependencies
Using index info to reconstruct a base tree...
A	pkg/apis/pipeline/v1/pipeline_types.go
A	pkg/apis/pipeline/v1/pipeline_types_test.go
M	pkg/apis/pipeline/v1beta1/pipeline_types.go
M	pkg/apis/pipeline/v1beta1/pipeline_types_test.go
M	pkg/reconciler/pipelinerun/resources/pipelinerunstate_test.go
Falling back to patching base and 3-way merge...
Auto-merging pkg/reconciler/pipelinerun/resources/pipelinerunstate_test.go
CONFLICT (content): Merge conflict in pkg/reconciler/pipelinerun/resources/pipelinerunstate_test.go
Auto-merging pkg/apis/pipeline/v1beta1/pipeline_types_test.go
Auto-merging pkg/apis/pipeline/v1beta1/pipeline_types.go
CONFLICT (modify/delete): pkg/apis/pipeline/v1/pipeline_types_test.go deleted in HEAD and modified in de-dup order and resource dependencies. Version de-dup order and resource dependencies of pkg/apis/pipeline/v1/pipeline_types_test.go left in tree.
CONFLICT (modify/delete): pkg/apis/pipeline/v1/pipeline_types.go deleted in HEAD and modified in de-dup order and resource dependencies. Version de-dup order and resource dependencies of pkg/apis/pipeline/v1/pipeline_types.go left in tree.
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 de-dup order and resource dependencies
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.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-12T17:42:44Z

@pritidesai: #5446 failed to apply on top of branch "release-v0.39.x":

Applying: de-dup order and resource dependencies
Using index info to reconstruct a base tree...
M	pkg/apis/pipeline/v1/pipeline_types.go
M	pkg/apis/pipeline/v1/pipeline_types_test.go
M	pkg/apis/pipeline/v1beta1/pipeline_types.go
M	pkg/apis/pipeline/v1beta1/pipeline_types_test.go
M	pkg/reconciler/pipelinerun/resources/pipelinerunstate_test.go
Falling back to patching base and 3-way merge...
Auto-merging pkg/reconciler/pipelinerun/resources/pipelinerunstate_test.go
CONFLICT (content): Merge conflict in pkg/reconciler/pipelinerun/resources/pipelinerunstate_test.go
Auto-merging pkg/apis/pipeline/v1beta1/pipeline_types_test.go
Auto-merging pkg/apis/pipeline/v1beta1/pipeline_types.go
Auto-merging pkg/apis/pipeline/v1/pipeline_types_test.go
CONFLICT (content): Merge conflict in pkg/apis/pipeline/v1/pipeline_types_test.go
Auto-merging pkg/apis/pipeline/v1/pipeline_types.go
CONFLICT (content): Merge conflict in pkg/apis/pipeline/v1/pipeline_types.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 de-dup order and resource dependencies
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.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 release-note Denotes a PR that will be considered when it comes time to generate release notes. kind/bug Categorizes issue or PR as related to a bug. labels Sep 7, 2022

tekton-robot requested review from dlorenc and lbernick September 7, 2022 00:45

tekton-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Sep 7, 2022

pritidesai force-pushed the dependencies branch from 2759512 to 09c49f9 Compare September 7, 2022 05:55

rafalbigaj approved these changes Sep 7, 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 7, 2022

pritidesai force-pushed the dependencies branch from 09c49f9 to d2fe86e Compare September 8, 2022 15:57

tekton-robot assigned vdemeester Sep 8, 2022

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

tekton-robot merged commit 74aa8fe into tektoncd:main Sep 8, 2022

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

pritidesai mentioned this pull request Sep 12, 2022

[release-v0.36.x] de-dupe order and resource dependencies #5479

Merged

7 tasks

This was referenced Sep 12, 2022

[release-v0.37.x] de-dupe order and resource dependencies #5482

Merged

[release-v0.38.x] de-dupe order and resource dependencies #5483

Merged

[release-v0.39.x] de-dupe order and resource dependencies #5484

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

de-dupe order and resource dependencies #5446

de-dupe order and resource dependencies #5446

pritidesai commented Sep 7, 2022 •

edited

Loading

tekton-robot commented Sep 7, 2022

tekton-robot commented Sep 7, 2022

rafalbigaj left a comment

abayer commented Sep 7, 2022

tekton-robot commented Sep 7, 2022

afrittoli commented Sep 8, 2022

pritidesai commented Sep 8, 2022

tekton-robot commented Sep 8, 2022

vdemeester commented Sep 8, 2022

afrittoli commented Sep 8, 2022

pritidesai commented Sep 12, 2022

pritidesai commented Sep 12, 2022

pritidesai commented Sep 12, 2022

tekton-robot commented Sep 12, 2022

tekton-robot commented Sep 12, 2022

tekton-robot commented Sep 12, 2022

de-dupe order and resource dependencies #5446

de-dupe order and resource dependencies #5446

Conversation

pritidesai commented Sep 7, 2022 • edited Loading

Changes

Submitter Checklist

Release Notes

tekton-robot commented Sep 7, 2022

tekton-robot commented Sep 7, 2022

rafalbigaj left a comment

Choose a reason for hiding this comment

abayer commented Sep 7, 2022

tekton-robot commented Sep 7, 2022

afrittoli commented Sep 8, 2022

pritidesai commented Sep 8, 2022

tekton-robot commented Sep 8, 2022

vdemeester commented Sep 8, 2022

afrittoli commented Sep 8, 2022

pritidesai commented Sep 12, 2022

pritidesai commented Sep 12, 2022

pritidesai commented Sep 12, 2022

tekton-robot commented Sep 12, 2022

tekton-robot commented Sep 12, 2022

tekton-robot commented Sep 12, 2022

pritidesai commented Sep 7, 2022 •

edited

Loading