tektoncd
Prevent infinite recursion in skipBecauseResultReferencesAreMissing
Changes
fixes #5271
While I have not been able to determine why setting enable-api-fields to alpha while PipelineRuns using results are in flight hits this, the immediate symptom in the linked issue is that an infinite recursive loop is hit in skipBecauseResultReferencesAreMissing, due to skipBecauseResultReferencesAreMissing calling rpt.Skip(facts), which calls rpt.skip(facts), which ends up calling rpt.skipBecauseResultReferencesAreMissing again, etc. Changing the call to rpt.Skip(facts).SkippingReason == v1beta1.WhenExpressionsSkip to rpt.skipBecauseWhenExpressionsEvaluatedToFalse(facts) not only gets rid of the potential for infinite recursion, it also more directly checks the condition we care about there in the first place.
/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
- [x] Follows the commit message standard
- [x] Meets the Tekton contributor standards (including functionality, content, code)
- [x] 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 - [x] 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
Fixes a potential infinite loop when a Pipeline task is missing needed results.
thanks @abayer, will appreciate some unit tests, its easier to review 😆
I completely failed to reproduce the conditions for the infinite recursion, so there's no new unit test, just the existing ones still passing without any changes, demonstrating that this doesn't result in any behavioral changes.
@abayer: The specified target(s) for /test were not found.
The following commands are available to trigger required jobs:
/test pull-tekton-pipeline-alpha-integration-tests/test pull-tekton-pipeline-build-tests/test pull-tekton-pipeline-integration-tests/test tekton-pipeline-unit-tests
The following commands are available to trigger optional jobs:
/test pull-tekton-pipeline-go-coverage
Use /test all to run all jobs.
In response to this:
/test check-pr-has-kind-label
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.
[APPROVALNOTIFIER] This PR is APPROVED
This pull-request has been approved by: lbernick
The full list of commands accepted by this bot can be found here.
The pull request process is described here
- ~~OWNERS~~ [lbernick]
Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment
Unfortunately, there are no unit tests for skip, skipBecauseWhenExpressionsEvaluatedToFalse, skipBecauseResultReferencesAreMissing, etc.
Unfortunately, there are no unit tests for
skip,skipBecauseWhenExpressionsEvaluatedToFalse,skipBecauseResultReferencesAreMissing, etc.
True, but they are actually well-covered by the tests for Skip - I checked. =)
Yup, I will it take back 🙃 I just ran gocover locally and they are very well covered:
I don't mind merging this change but I haven't got chance to run any additional tests.
I don't think this change is fixing https://github.com/tektoncd/pipeline/issues/5271 since the proposed change does not execute any different with or without alpha flag. Does it?
It will be great to at least know what is the pipeline configuration while running into https://github.com/tektoncd/pipeline/issues/5271.
/hold
I don't mind merging this change but I haven't got chance to run any additional tests.
I'd actually like to hold it a bit - I think I oversimplified this a bit in my head.
I don't think this change is fixing #5271 since the proposed change does not execute any different with or without
alphaflag. Does it?
It's meant to fix the immediate issue in #5271 of the infinite recursion, but not whatever the root cause was.
It will be great to at least know what is the pipeline configuration while running into #5271.
I've got a dump of all PipelineRuns (with inline pipeline specs) running on the user's setup when they changed to alpha, but there are a lot of them, and they're quite big, so trying to narrow down what exactly the trigger is has been...a challenge. I'll come back to that tomorrow and just spend the day trying to reproduce the loop in a canned test case.
thanks a bunch @abayer for all the efforts on this, appreciate your guidance and troubleshooting this 🙏
@abayer: The specified target(s) for /test were not found.
The following commands are available to trigger required jobs:
/test pull-tekton-pipeline-alpha-integration-tests/test pull-tekton-pipeline-build-tests/test pull-tekton-pipeline-integration-tests/test tekton-pipeline-unit-tests
The following commands are available to trigger optional jobs:
/test pull-tekton-pipeline-go-coverage
Use /test all to run all jobs.
In response to this:
/test check-pr-has-kind-label
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.
Closing this because I am 99% sure I figured out the problem in https://github.com/tektoncd/pipeline/issues/5271#issuecomment-1218341861, and this is not the way to prevent the scenario caused by the now-removed implicit parameters behavior from happening again in the future. =)