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

Adding HPA for eventing-webhook #4792

Merged
merged 2 commits into from
Jan 26, 2021
Merged

Adding HPA for eventing-webhook #4792

merged 2 commits into from
Jan 26, 2021

Conversation

matzew
Copy link
Member

@matzew matzew commented Jan 26, 2021 •
edited
Loading

Signed-off-by: Matthias Wessendorf mwessend@redhat.com

Proposed Changes

  • Set up HPA for eventing-webhook:
    • Scale Webhook based on CPU
    • Add initial PodDisruptionBudget

Similar to this PR on the serving component from September 2020: knative/serving#9444

Adding HorizontalPodAutoscaler and PodDisruptionBudget for the eventing webhook

@google-cla google-cla bot added the cla: yes Indicates the PR's author has signed the CLA. label Jan 26, 2021
@knative-prow-robot knative-prow-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Jan 26, 2021
@knative-prow-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: matzew

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

@knative-prow-robot knative-prow-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 26, 2021
@codecov
Copy link

codecov bot commented Jan 26, 2021 •
edited
Loading

Codecov Report

Merging #4792 (3c3f351) into master (81ce898) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #4792   +/-   ##
=======================================
  Coverage   81.25%   81.25%           
=======================================
  Files         291      291           
  Lines        8281     8281           
=======================================
  Hits         6729     6729           
  Misses       1143     1143           
  Partials      409      409

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 81ce898...028f423. Read the comment docs.

@devguyio
Copy link
Contributor

/assign

@devguyio devguyio self-requested a review January 26, 2021 13:43
@devguyio
Copy link
Contributor

devguyio commented Jan 26, 2021 •
edited
Loading

  • I understand serving has it, and the only reasoning I saw was to not have a SPOF. However, given that this is a control plane component, I'm not really sure how valuable is the replication based on CPU. I can imagine it might starve on memory more, I think @mattmoor had such an opinion? also, if we want to achieve high availability, shouldn't the min be 2?
  • I don't have a strong opinion, other than the question stated above.
  • @matzew I think it might be useful to have a release note? something like "The eventing webhook has now a horizontal pod autoscaler based on ......etc"

lgtm if folks wanna have it in , I'll leave this for someone with a stronger opinion about it. @vaikas @grantr @lionelvillard

@matzew
Copy link
Member Author

matzew commented Jan 26, 2021

Related I will be also working introducing high-availability field on the KnativeEventing spec, of the operator.

There you set the replicas. This is more ground work, and adding a HPA/scaling for the stateless webhook (exactly like we have on serving)

pierDipi
pierDipi reviewed Jan 26, 2021
View reviewed changes
config/core/deployments/webhook-hpa.yaml Outdated
minAvailable: 80%
selector:
matchLabels:
app: webhook
Copy link
Member

Choose a reason for hiding this comment

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

This should be eventing-webhook.

config/core/deployments/webhook-hpa.yaml Outdated
# See the License for the specific language governing permissions and
# limitations under the License.

apiVersion: autoscaling/v2beta1
Copy link
Member

Choose a reason for hiding this comment

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

Can't we use v2beta2?

@matzew
Adding HPA for eventing-webhook:
083ed81 
- Scale Webhook based on CPU
- Add initial PodDisruptionBudget
@matzew
Copy link
Member Author

matzew commented Jan 26, 2021

@pierDipi true: I updated to v2beta2 and fixed the eventing-webhook

This matches now the HPAs we have already in for the MT Broker, which also goes for the cpu resource

@matzew
😱 we have already 2021
028f423 
Signed-off-by: Matthias Wessendorf <mwessend@redhat.com>
@devguyio
Copy link
Contributor

lgtm

leaving it for @pierDipi

pierDipi
pierDipi reviewed Jan 26, 2021
View reviewed changes
Copy link
Member

@pierDipi pierDipi left a comment

Choose a reason for hiding this comment

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

/lgtm

@knative-prow-robot knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Jan 26, 2021
@pierDipi
Copy link
Member

/kind enhancement

@knative-prow-robot knative-prow-robot merged commit ae17ca3 into knative:master Jan 26, 2021
@matzew matzew mentioned this pull request Feb 4, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pierDipi pierDipi pierDipi left review comments

@aliok aliok Awaiting requested review from aliok

@n3wscott n3wscott Awaiting requested review from n3wscott

@devguyio devguyio Awaiting requested review from devguyio

Assignees

@devguyio devguyio

@pierDipi pierDipi

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/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@matzew @knative-prow-robot @devguyio @pierDipi