Cleanup: remove potential goroutine leakages in taskrun #4936

kerthcet · 2022-06-03T15:13:45Z

Signed-off-by: kerthcet [email protected]

Changes

Submitter Checklist

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

Docs included if any changes are user facing
Tests included if any functionality added or changed
Follows the commit message standard
Meets the Tekton contributor standards (including
functionality, content, code)
Release notes block below has been filled in
(if there are no user facing changes, use release note "NONE")

Release Notes

NONE

tekton-robot · 2022-06-03T15:14:00Z

Hi @kerthcet. 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.

lbernick

/ok-to-test

Thanks @kerthcet! would you mind adding a commit message body please?

tekton-robot · 2022-06-03T15:36:46Z

[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

Needs approval from an approver in each of these files:

~~OWNERS~~ [lbernick]

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

kerthcet · 2022-06-03T15:48:51Z

/ok-to-test

Thanks @kerthcet! would you mind adding a commit message body please?

Absolutely, done.

abayer · 2022-06-03T21:24:36Z

/retest

abayer · 2022-06-03T21:26:44Z

/lgtm

Good catch!

When we try to record metrics in a forked goroutine, we should inherit the context from upstream to prevent potential goroutine leakages. Signed-off-by: kerthcet <[email protected]>

kerthcet · 2022-06-04T02:08:52Z

@abayer PTAL, fixed the tests error, thanks.

kerthcet · 2022-06-04T02:13:52Z

/hold

kerthcet · 2022-06-04T03:30:11Z

/retest

kerthcet · 2022-06-04T03:35:22Z

/test pull-tekton-pipeline-unit-tests

kerthcet · 2022-06-04T03:45:33Z

Well, unit tests failed for some unknown reason, details here https://tekton-releases.appspot.com/build/tekton-prow/pr-logs/pull/tektoncd_pipeline/4936/pull-tekton-pipeline-unit-tests/1532906825693794306/, but I can't reproduce this, all tests passed in my local env, unhold then.
/unhold

lbernick · 2022-06-06T13:06:32Z

pkg/taskrunmetrics/metrics_test.go

 	}

-	if err := metrics.DurationAndCount(&v1beta1.TaskRun{}, beforeCondition); err == nil {
+	ctx, cancel := context.WithCancel(context.Background())


just curious, what's the need for the cancellation function here?

Make sure to teardown the resources.

lbernick · 2022-06-06T13:07:28Z

Well, unit tests failed for some unknown reason, details here https://tekton-releases.appspot.com/build/tekton-prow/pr-logs/pull/tektoncd_pipeline/4936/pull-tekton-pipeline-unit-tests/1532906825693794306/, but I can't reproduce this

Unfortunately our tests are quite flaky right now :( This error looks a lot like a flake to me so OK to rerun

lbernick · 2022-06-06T14:35:37Z

@abayer I don't think anything has really changed since you LGTMed so I'll go ahead and add that label-- lmk if any concerns

/lgtm

kerthcet · 2022-06-06T15:06:17Z

/unhold

kerthcet · 2022-06-06T15:11:46Z

It seems like I have no privilege to remove the do-not-merge/hold label, need some help @lbernick , thanks a lot.

lbernick · 2022-06-06T15:16:59Z

oh oops sorry-- I don't think you need permissions, it's just a different command

/hold cancel

afrittoli · 2022-06-21T20:53:58Z

/kind bug

tekton-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jun 3, 2022

tekton-robot requested review from dibyom and lbernick June 3, 2022 15:15

lbernick approved these changes Jun 3, 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 Jun 3, 2022

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

kerthcet force-pushed the cleanup/remove-potential-goroutine branch from dbb1eec to 607ab17 Compare June 3, 2022 15:48

tekton-robot assigned abayer Jun 3, 2022

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

Cleanup: remove potential goroutine leakages in metrics recording

341912e

When we try to record metrics in a forked goroutine, we should inherit the context from upstream to prevent potential goroutine leakages. Signed-off-by: kerthcet <[email protected]>

kerthcet force-pushed the cleanup/remove-potential-goroutine branch from 607ab17 to 341912e Compare June 4, 2022 02:06

tekton-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed lgtm Indicates that a PR is ready to be merged. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jun 4, 2022

tekton-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 4, 2022

lbernick reviewed Jun 6, 2022

View reviewed changes

tekton-robot assigned lbernick Jun 6, 2022

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

tekton-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 6, 2022

tekton-robot merged commit c671899 into tektoncd:main Jun 6, 2022

kerthcet deleted the cleanup/remove-potential-goroutine branch June 6, 2022 15:49

tekton-robot added the kind/bug Categorizes issue or PR as related to a bug. label Jun 21, 2022

Cleanup: remove potential goroutine leakages in taskrun #4936

Cleanup: remove potential goroutine leakages in taskrun #4936

Uh oh!

Conversation

kerthcet commented Jun 3, 2022 • edited by afrittoli Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes

Submitter Checklist

Release Notes

Uh oh!

tekton-robot commented Jun 3, 2022

Uh oh!

lbernick left a comment

Choose a reason for hiding this comment

Uh oh!

tekton-robot commented Jun 3, 2022

Uh oh!

kerthcet commented Jun 3, 2022

Uh oh!

abayer commented Jun 3, 2022

Uh oh!

abayer commented Jun 3, 2022

Uh oh!

kerthcet commented Jun 4, 2022

Uh oh!

kerthcet commented Jun 4, 2022

Uh oh!

kerthcet commented Jun 4, 2022

Uh oh!

kerthcet commented Jun 4, 2022

Uh oh!

kerthcet commented Jun 4, 2022

Uh oh!

lbernick Jun 6, 2022

Choose a reason for hiding this comment

Uh oh!

kerthcet Jun 6, 2022

Choose a reason for hiding this comment

Uh oh!

lbernick commented Jun 6, 2022

Uh oh!

lbernick commented Jun 6, 2022

Uh oh!

kerthcet commented Jun 6, 2022

Uh oh!

kerthcet commented Jun 6, 2022

Uh oh!

lbernick commented Jun 6, 2022

Uh oh!

afrittoli commented Jun 21, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

kerthcet commented Jun 3, 2022 •

edited by afrittoli

Loading