Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: support to produce results from a failed task #6510

Merged
merged 1 commit into from
May 19, 2023

Conversation

pritidesai
Copy link
Member

@pritidesai pritidesai commented Apr 7, 2023

Changes

If the task fails or succeeds, as long as the task result is initialized, it can be successfully parsed and can be referenced by the final task.

This commit enables the failed task to produce the task results, and the final task can reference it.

Closes #5749

This is a duplicate of #5750 but with conflicts resolved and e2e test. I was reviewing PR #5750 and decided to create an e2e test. After noticing it had merge conflicts, decided to resolve them and create this PR. @cugykw is now a co-author of this commit.

/kind feature

Submitter Checklist

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

  • Has Docs if any changes are user facing, including updates to minimum requirements e.g. Kubernetes version bumps
  • 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

PipelineRun can produce task results from the failed tasks, and the final task can reference those results.

@tekton-robot tekton-robot added the release-note Denotes a PR that will be considered when it comes time to generate release notes. label Apr 7, 2023
@tekton-robot tekton-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Apr 7, 2023
@tekton-robot
Copy link
Collaborator

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/pod/status.go 91.2% 91.6% 0.4

@tekton-robot
Copy link
Collaborator

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

File Old Coverage New Coverage Delta
pkg/pod/status.go 91.2% 91.6% 0.4

@pritidesai
Copy link
Member Author

/kind feature

assigning to the reviewers of original pr:

/assign @vdemeester
/assign @chitrangpatel

@tekton-robot tekton-robot added the kind/feature Categorizes issue or PR as related to a new feature. label Apr 7, 2023
@tekton-robot
Copy link
Collaborator

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/pod/status.go 91.2% 91.6% 0.4

@tekton-robot
Copy link
Collaborator

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

File Old Coverage New Coverage Delta
pkg/pod/status.go 91.2% 91.6% 0.4

@tekton-robot
Copy link
Collaborator

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/pod/status.go 91.2% 91.6% 0.4

@tekton-robot
Copy link
Collaborator

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

File Old Coverage New Coverage Delta
pkg/pod/status.go 91.2% 91.6% 0.4

@pritidesai
Copy link
Member Author

looks like a flake, at least one of them didn't fail earlier:

test: TestTaskResultsFromFailedTasks expand_more | 6s
test: TestPipelineTaskTimeout expand_more

Was able to run TestTaskResultsFromFailedTasks with alpha flag.

@pritidesai
Copy link
Member Author

/retest

@tekton-robot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: vdemeester

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:

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

@tekton-robot tekton-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 11, 2023
@QuanZhang-William
Copy link
Member

/assign

@pritidesai
Copy link
Member Author

/retest

@pritidesai
Copy link
Member Author

So the alpha test failure is not a flake 😞

But it is interesting that the same test succeeds with stable and beta integration test check.

The logs of tests with stable: https://storage.googleapis.com/tekton-prow/pr-logs/pull/tektoncd_pipeline/6510/pull-tekton-pipeline-integration-tests/1644479150448185344/build-log.txt

The logs of tests with alpha: https://storage.googleapis.com/tekton-prow/pr-logs/pull/tektoncd_pipeline/6510/pull-tekton-pipeline-alpha-integration-tests/1659248641593839616/build-log.txt

The difference is:

With stable only one task is skipped as expected:

Message:            "Tasks Completed: 2 (Failed: 1, Cancelled 0), Skipped: 1",

With alpha, the final task which was expected to execute is skipped with the reason "Results were missing":

Message:            "Tasks Completed: 1 (Failed: 1, Cancelled 0), Skipped: 2",

@pritidesai
Copy link
Member Author

hey @chitrangpatel, do you think the configuration of RESULTS_FROM=sidecar-logs could be causing a final task to skip if the dependent task fails?

@@ -208,7 +208,7 @@ func setTaskRunStatusBasedOnStepStatus(ctx context.Context, logger *zap.SugaredL
}

taskResults, filteredResults := filterResults(results, specResults)
if tr.IsSuccessful() {
if tr.IsDone() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you need to do the same thing in line 185 as well.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That will account for the same change when fetching results from sidecar logs too.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks a bunch @chitrangpatel for a quick response 🙏 hopefully works now 🤞

If the task fails or succeeds, as long as the task result is initialized,
it can be successfully parsed and can be referenced by the final task.

This commit enables the failed task to produce the task results, and
the final task can reference it.

Please see detailed description in issue tektoncd#5749

Co-authored-by: yang kewei <2524563635@qq.com>

Signed-off-by: pritidesai <pdesai@us.ibm.com>
Signed-off-by: Priti Desai <pdesai@us.ibm.com>
@tekton-robot
Copy link
Collaborator

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/pod/status.go 91.2% 91.6% 0.4

@tekton-robot
Copy link
Collaborator

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

File Old Coverage New Coverage Delta
pkg/pod/status.go 91.2% 91.6% 0.4

@chitrangpatel
Copy link
Member

/lgtm

@tekton-robot tekton-robot added the lgtm Indicates that a PR is ready to be merged. label May 18, 2023
@chitrangpatel
Copy link
Member

/retest

@pritidesai
Copy link
Member Author

/test pull-tekton-pipeline-go-coverage-df

@tekton-robot
Copy link
Collaborator

@pritidesai: 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-beta-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 pull-tekton-pipeline-go-coverage-df

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.

@pritidesai
Copy link
Member Author

/retest

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. kind/feature Categorizes issue or PR as related to a new feature. lgtm Indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

able to get task results from failed tasks
6 participants