这是indexloc提供的服务，不要输入任何密码
Skip to content

Remove an unnecessary start delay when resolving a tag to digest #12668

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
Merged
merged 1 commit into from
Feb 25, 2022
Merged

Remove an unnecessary start delay when resolving a tag to digest #12668

merged 1 commit into from
Feb 25, 2022

Conversation

dprotaso
Copy link
Member

@dprotaso dprotaso commented Feb 25, 2022 •
edited
Loading

Part of #12667

Proposed Changes

ItemExponentialFailureRateLimiter when adding any item will have an initial delay before processing starts. For tag to digest resolution that delay was 1s. Rather than change our timings we just skip the delay on the first AddRateLimited

This PR doesn't fix #12667 since we don't want to carry this long term and want to drop this once K8s adopts the change and we pick it up

Release Note

Remove an unnecessary start delay when resolving a tag to digest

@knative-prow-robot knative-prow-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. area/API API objects and controllers approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Feb 25, 2022
@codecov
Copy link

codecov bot commented Feb 25, 2022 •
edited
Loading

Codecov Report

Merging #12668 (5e56e72) into main (d846cbb) will decrease coverage by 0.17%.
The diff coverage is 20.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main   #12668      +/-   ##
==========================================
- Coverage   87.47%   87.30%   -0.18%     
==========================================
  Files         195      196       +1     
  Lines        9712     9736      +24     
==========================================
+ Hits         8496     8500       +4     
- Misses        932      951      +19     
- Partials      284      285       +1
Impacted Files Coverage Δ
pkg/reconciler/revision/rate_limiter.go 16.66% <16.66%> (ø)
pkg/reconciler/revision/controller.go 86.00% <100.00%> (ø)
pkg/autoscaler/statforwarder/leases.go 75.53% <0.00%> (-1.44%) ⬇️
pkg/activator/net/revision_backends.go 92.60% <0.00%> (+0.86%) ⬆️

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 d846cbb...5e56e72. Read the comment docs.

@dprotaso dprotaso force-pushed the tag-to-digest-delay branch 2 times, most recently from 1a9f33f to 3c3727e Compare February 25, 2022 02:21
@dprotaso dprotaso changed the title Use a modified copy of ItemExponentialFailureRateLimiter to elide the unnecessary delay Use a modified copy of ItemExponentialFailureRateLimiter to elide an unnecessary start delay Feb 25, 2022
@dprotaso dprotaso force-pushed the tag-to-digest-delay branch from 3c3727e to 27e83d7 Compare February 25, 2022 02:23
@dprotaso dprotaso force-pushed the tag-to-digest-delay branch from 27e83d7 to 5e56e72 Compare February 25, 2022 02:23
@dprotaso dprotaso changed the title Use a modified copy of ItemExponentialFailureRateLimiter to elide an unnecessary start delay Remove an unnecessary start delay when resolving tag to digests Feb 25, 2022
@dprotaso dprotaso changed the title Remove an unnecessary start delay when resolving tag to digests Remove an unnecessary start delay when resolving tags to digests Feb 25, 2022
@dprotaso dprotaso changed the title Remove an unnecessary start delay when resolving tags to digests Remove an unnecessary start delay when resolving a tag to digest Feb 25, 2022
@dprotaso
Copy link
Member Author

/assign @julz

mattmoor
mattmoor reviewed Feb 25, 2022
View reviewed changes
pkg/reconciler/revision/rate_limiter.go
// From: https://github.com/kubernetes/client-go/blob/master/util/workqueue/default_rate_limiters.go
//
// TODO - drop this file when we pick up the following upstream change
// https://github.com/kubernetes/kubernetes/pull/108343
Copy link
Member

Choose a reason for hiding this comment

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

Is this file effectively a fork of upstream at this PR?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yup

mattmoor
mattmoor approved these changes Feb 25, 2022
View reviewed changes
Copy link
Member

@mattmoor mattmoor left a comment

Choose a reason for hiding this comment

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

/lgtm
/approve
/hold

Can we open an issue to track removing this at a particular K8s version (once we have one)?

@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 Feb 25, 2022
@knative-prow-robot knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Feb 25, 2022
@knative-prow-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

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

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

@dprotaso
Copy link
Member Author

Yup the original issue is going to be repurposed to track the removal

/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 Feb 25, 2022
@dprotaso
Copy link
Member Author

/cherry-pick release-1.2

@knative-prow-robot
Copy link
Contributor

@dprotaso: once the present PR merges, I will cherry-pick it on top of release-1.2 in a new PR and assign it to you.

In response to this:

/cherry-pick release-1.2

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.

@dprotaso
Copy link
Member Author

/cherry-pick release-1.1

@knative-prow-robot
Copy link
Contributor

@dprotaso: once the present PR merges, I will cherry-pick it on top of release-1.1 in a new PR and assign it to you.

In response to this:

/cherry-pick release-1.1

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.

@knative-prow-robot knative-prow-robot merged commit d48569e into knative:main Feb 25, 2022
@dprotaso dprotaso deleted the tag-to-digest-delay branch February 25, 2022 03:27
@knative-prow-robot knative-prow-robot mentioned this pull request Feb 25, 2022
Merged
@knative-prow-robot
Copy link
Contributor

@dprotaso: new pull request created: #12669

In response to this:

/cherry-pick release-1.2

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.

@knative-prow-robot knative-prow-robot mentioned this pull request Feb 25, 2022
Merged
@knative-prow-robot
Copy link
Contributor

@dprotaso: new pull request created: #12670

In response to this:

/cherry-pick release-1.1

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.

@dprotaso dprotaso added this to the v1.3.0 milestone Mar 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mattmoor mattmoor mattmoor approved these changes

Assignees

@julz julz

@mattmoor mattmoor

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/API API objects and controllers lgtm Indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
v1.3.0
Development

Successfully merging this pull request may close these issues.
4 participants
@dprotaso @knative-prow-robot @mattmoor @julz