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.

Sign up for GitHub

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

Jump to bottom

Replace snooze with NewRequeueKey #4131

Merged
merged 1 commit into from
Aug 16, 2021
Merged

Replace snooze with NewRequeueKey #4131

merged 1 commit into from
Aug 16, 2021

Conversation

mattmoor
Copy link
Member

Changes

Adopt the NewRequeueKey to replace the snooze functions.

/kind cleanup

WIP until #4129 lands

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 or deleted (only if no user facing changes)

Release Notes

NONE

@tekton-robot tekton-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. release-note-none Denotes a PR that doesnt merit a release note. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. labels Jul 30, 2021
@tekton-robot tekton-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jul 30, 2021
@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/reconciler/taskrun/controller.go 96.3% 96.0% -0.3

@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/reconciler/taskrun/controller.go 96.3% 96.0% -0.3

@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/reconciler/taskrun/controller.go 96.3% 96.0% -0.3

@mattmoor
Copy link
Member Author

/retest

I think this is a flake, let's see

@mattmoor mattmoor mentioned this pull request Aug 2, 2021
Merged
5 tasks
@mattmoor
Copy link
Member Author

mattmoor commented Aug 4, 2021

@ScrapCodes Probably not the best place to discuss. Is there an issue or some place that I should read through for more context about what you are trying to do? If you link it here or DM it to me on slack, I can try to weigh in. In general, I see the enqueue functions as fairly low-level mechanisms, so we should probably only reach around the controller abstractions we have in place fairly intentionally, and consider whether it's possible to do the same within the abstraction or extend the abstraction for the use case you have in mind.

@afrittoli
Copy link
Member

/easycla

@ScrapCodes
Copy link
Contributor

Can I schedule some function, without creating a goroutine inside a reconciler?

e.g. if I use time.AfterFunc it creates a goroutine.

A reconciler may maintain its thread pool, in order to schedule something e.g. after 1 min check if there is update otherwise timeout. Do we have this from knative?

In my PR, I am trying to timeout a run (i.e. it does not support retries) if the custom task controller does not respond within a short timeout. This can be achieved by scheduling a reconciling event for that PipelineRun and then checking if the last update time has elapsed the timeout duration.

@ScrapCodes ScrapCodes mentioned this pull request Aug 6, 2021
Closed
5 tasks
@mattmoor mattmoor changed the title [WIP] Replace snooze with NewRequeueKey Replace snooze with NewRequeueKey Aug 6, 2021
@tekton-robot tekton-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 6, 2021
@mattmoor
Copy link
Member Author

mattmoor commented Aug 6, 2021

cc @imjasonh @vdemeester

I've rebased this and it should be RFAL.

@CI-Gods 👇
/sacrifice 🐐

@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/reconciler/taskrun/controller.go 96.3% 96.0% -0.3

@mattmoor mattmoor mentioned this pull request Aug 6, 2021
Merged
3 tasks
@mattmoor
Copy link
Member Author

mattmoor commented Aug 6, 2021

Hmm, let's try another 🐐 . This looks like a flake.

/retest

@mattmoor
Copy link
Member Author

mattmoor commented Aug 6, 2021

I'm seeing this in the detailed logs, which seems very unlikely to be related to my change:

googleapi: Error 400: The disk resource 'projects/tekton-prow-1/zones/us-central1-a/disks/gke-tpipeline-e2e-cls1-pvc-1e50c020-13f7-4269-90d2-01c6ae94703b' is already being used by 'projects/tekton-prow-1/zones/us-central1-a/instances/gke-tpipeline-e2e-cls142-default-pool-6e83aa5f-n4cm', resourceInUseByAnotherResource

/retest

@mattmoor
Copy link
Member Author

mattmoor commented Aug 6, 2021

Weird, this one's reporting 0 Failed. I wonder if this is somehow related to the large log files @sbwsg noticed.

@vdemeester
Copy link
Member

/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 Aug 9, 2021
@vdemeester
Copy link
Member

/retest

@dlorenc
Copy link
Contributor

dlorenc commented Aug 16, 2021

/lgtm

@tekton-robot tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Aug 16, 2021
@tekton-robot tekton-robot merged commit 0bf24ba into tektoncd:main Aug 16, 2021
@mattmoor mattmoor deleted the use-requeue-key branch August 16, 2021 16:09
@bobcatfish bobcatfish mentioned this pull request Aug 19, 2021
Merged
@bobcatfish
Copy link
Collaborator

@mattmoor btw this is the PR I was referring to when I started that whole thread about asking reviewers check look at the checklist requirements b/c I didnt feel the commit message had enough context - In this specific case looking at the PR it's hard for me to understand what NewRequeueKey is and why it replaces snooze (i think id have to go digging into the knative/pkg code to figure that out?)

@mattmoor
Copy link
Member Author

Yeah, this is a bit better: knative/pkg#2201

@bobcatfish
Copy link
Collaborator

Ah that's great, thanks for the extra conext @mattmoor

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vdemeester vdemeester vdemeester approved these changes

@imjasonh imjasonh Awaiting requested review from imjasonh

Assignees

@dlorenc dlorenc

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. lgtm Indicates that a PR is ready to be merged. release-note-none Denotes a PR that doesnt merit a release note. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@mattmoor @tekton-robot @afrittoli @ScrapCodes @vdemeester @dlorenc @bobcatfish