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

Add a new mechanism for requeuing a key. #2201

Merged
merged 1 commit into from
Jul 30, 2021

Conversation

mattmoor
Copy link
Member

Changes

This is modeled after some of the semantics available in controller-runtime's Result type, which allows reconcilers to indicate a desire to reprocess a key either immediately or after a delay.

I worked around our lack of this when I reworked Tekton's timeout handling logic by exposing a "snooze" function to the Reconciler that wrapped EnqueueAfter, but this feels like a cleaner API for folks to use in general, and is consistent with our NewPermanentError and NewSkipKey functions in terms of influencing the queuing behaviors in our reconcilers via wrapped errors.

You can see some discussion of this here: https://knative.slack.com/archives/CA4DNJ9A4/p1627524921161100

/kind enhancement

Release Note

Introduce a way for Knative-style controllers to requeue keys by returning a wrapped error from Reconcile[Kind].  See NewRequeue{Immediately,After} for more details.

Docs

Shouldn't have user-facing docs impact.

This is modelled after some of the semantics available in controller-runtime's `Result` type, which allows reconcilers to indicate a desire to reprocess a key either immediately or after a delay.

I worked around our lack of this when I reworked Tekton's timeout handling logic by exposing a "snooze" function to the Reconciler that wrapped `EnqueueAfter`, but this feels like a cleaner API for folks to use in general, and is consistent with our `NewPermanentError` and `NewSkipKey` functions in terms of influencing the queuing behaviors in our reconcilers via wrapped errors.

You can see some discussion of this here: https://knative.slack.com/archives/CA4DNJ9A4/p1627524921161100
@google-cla google-cla bot added the cla: yes Indicates the PR's author has signed the CLA. label Jul 29, 2021
@knative-prow-robot knative-prow-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Jul 29, 2021
@vaikas
Copy link
Contributor

vaikas commented Jul 29, 2021

UT failed with:

--- FAIL: TestAdmissionValidResponseForResource (0.21s)
    logger.go:130: 2021-07-29T21:15:09.052Z	ERROR	webhook/webhook.go:196	failed to fetch secret	{"error": "secret \"webhook-certs\" not found"}
    admission_integration_test.go:139: Failed to get response Get "https://0.0.0.0:43137/bazinga": remote error: tls: unrecognized name
    admission_integration_test.go:176: Admit was called before informers had synced.
    logger.go:130: 2021-07-29T21:15:09.052Z	INFO	webhook/webhook.go:235	Starting to fail readiness probes...
    ```

@mattmoor
Copy link
Member Author

I haven't touched that and can't rerun it because I'm not an editor.

@vaikas
Copy link
Contributor

vaikas commented Jul 29, 2021

Yeah, I'm rerunning them :)
/lgtm
/approve
/hold

Just adding a hold so the other folks from the thread have a chance to take a look. If they don't have concerns, I'll remove the hold tmw morning.
@markusthoemmes

@knative-prow-robot knative-prow-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 29, 2021
@knative-prow-robot knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Jul 29, 2021
@knative-prow-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mattmoor, vaikas

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

@codecov
Copy link

codecov bot commented Jul 29, 2021

Codecov Report

Merging #2201 (083056d) into main (889b567) will increase coverage by 0.05%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #2201      +/-   ##
==========================================
+ Coverage   65.98%   66.04%   +0.05%     
==========================================
  Files         220      220              
  Lines        9207     9220      +13     
==========================================
+ Hits         6075     6089      +14     
+ Misses       2861     2860       -1     
  Partials      271      271              
Impacted Files Coverage Δ
controller/controller.go 87.59% <100.00%> (+1.03%) ⬆️
test/gcs/mock/mock.go 91.39% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 889b567...083056d. Read the comment docs.

@vaikas
Copy link
Contributor

vaikas commented Jul 30, 2021

/unhold

@knative-prow-robot knative-prow-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 30, 2021
@knative-prow-robot knative-prow-robot merged commit 3826bb2 into knative:main Jul 30, 2021
@mattmoor mattmoor deleted the requeue-after branch July 30, 2021 15:45
@mattmoor
Copy link
Member Author

Looks like we want codegen to not emit an event for these too, so I'll have a small follow-up shortly.

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. cla: yes Indicates the PR's author has signed the CLA. kind/enhancement lgtm Indicates that a PR is ready to be merged. 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.

3 participants