Make DefaultThreadsPerController, QPS and Burst configurable via flags #3156

pierretasci · 2020-09-01T18:13:27Z

Changes

Make DefaultThreadsPerController, QPS, and Burst configurable via flags. This helps tune a deployment depending on expected concurrency in production.

Submitter Checklist

These are the criteria that every PR should meet, please check them off as you
review them:

Includes tests (if functionality changed/added)
Includes docs (if user facing)
Commit messages follow commit message best practices
Release notes block has been filled in or deleted (only if no user facing changes)

Reviewer Notes

If API changes are included, additive changes must be approved by at least two OWNERS and backwards incompatible changes must be approved by more than 50% of the OWNERS, and they must first be added in a backwards compatible way.

Release Notes

Allows DefaultThreadsPerController, QPS, and Burst to be configured via flags

tekton-robot · 2020-09-01T18:13:42Z

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

pierretasci · 2020-09-01T18:23:51Z

/kind feature

pierretasci · 2020-09-01T18:35:32Z

/assign @dlorenc

dibyom · 2020-09-01T19:18:08Z

/ok-to-test

dlorenc · 2020-09-02T19:05:00Z

cmd/controller/main.go

+	cfg.Burst = *burst
+
+	ctx := injection.WithNamespaceScope(signals.NewContext(), *namespace)
+	if !*enableLeaderElection {


The leader election part worries me a bit. I think we'd want to setup some end to end testing of this before making it available.

Do you think it's worth trying to include in this change, or separating out to another PR where we can figure out the necessary testing plan?

To my understanding, I think if we remove the option of enabling leader election, it might be a breaking change, as the current release seems to support it via https://github.com/tektoncd/pipeline/blob/master/config/config-leader-election.yaml. I am not too super familiar with knative but looking through PR knative/pkg#1476, looks like one could enable it by setting EnabledComponents in the configmap.

I could break that out in a separate commit though, if we want to add some sort end to end test for it. I might need some guidance with that though, not sure what we might test in addition to what knative is already testing for :)

Either way, I guess we will want to add something in the release notes, if folks are using leader election now and how to use going forward.

Ah, maybe my mistake then. It's not actually clear to me if that works. The comment is "an inactive but valid configuration follows": https://github.com/tektoncd/pipeline/blob/master/config/config-leader-election.yaml#L24

But either way - I'm not quite sure how not including this flag would be a breaking change. It didn't exist before, why do we need it here?

Yeah, me neither as we are not using this feature yet.

But either way - I'm not quite sure how not including this flag would be a breaking change. It didn't exist before, why do we need it here?

I meant from the feature perspective. If it's something user could enable before, and now no longer can if we disabled it.

@mattmoor can you shed some light on leader election config? Would there be any downside here if we remove the flag and enabled leader election by default?

dlorenc · 2020-09-02T19:05:38Z

nit: i think you have a few extra "```" characters in the release notes section as well :)

nishpa214 · 2020-09-02T22:06:43Z

/retest

imjasonh · 2020-09-04T13:25:14Z

/test pull-tekton-pipeline-integration-tests

nishpa214 · 2020-09-04T16:21:29Z

/test pull-tekton-pipeline-integration-tests

vdemeester · 2020-09-08T09:02:47Z

/hold
This bumps some k8s and knative dependencies. I guess the knative dependencies bump are required right ? (and the k8s ones might be a result of that update). We may want to split thoses (either in separate PR and/or commit), and we need to look at the knative changes (like the min support k8s version, …) before merging this.

nishpa214 · 2020-09-08T21:39:46Z

This bumps some k8s and knative dependencies. I guess the knative dependencies bump are required right ? (and the k8s ones might be a result of that update).

@vdemeester yes, knative dependencies bump was required to pick up the change to make QPS and Burst configurable.

We may want to split thoses (either in separate PR and/or commit), and we need to look at the knative changes (like the min support k8s version, …) before merging this.

Cool, will create another PR for bumping knative changes.

ghost · 2020-10-02T16:03:32Z

Since the most recent push is a commit message update I'm going to re-add @vdemeester 's lgtm and add my own approve:

/lgtm
/approve

tekton-robot · 2020-10-02T16:03:40Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: sbwsg

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~~ [sbwsg]

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

ghost · 2020-10-02T17:30:41Z

/kind feature

ghost · 2020-10-02T17:31:16Z

/remove-kind feature

ghost · 2020-10-02T17:31:38Z

/kind feature

afrittoli · 2020-10-06T10:43:46Z

cmd/controller/main.go

+	cfg.Burst = 2 * *burst
+
+	ctx := injection.WithNamespaceScope(signals.NewContext(), *namespace)
+	if !*disableHighAvailability {


This seems to be backwards? If disable is false, we should enable, but we're getting the context with HA disabled.

Oh right, I think the flag used to be enableLeaderElection (or something). cc @nishpa214

The tekton controller has a `disable-ha` flag that was introduced in tektoncd#3156, which can be used to disable support for leader-election-based HA. The default for the disable flag is "false" i.e. support for HA is meant to be enabled by default, however the current check on the flag does the opposite of what it should, resulting in HA being disabled by default. This PR fixes the logic to restore the correct behaviour.

The tekton controller has a `disable-ha` flag that was introduced in #3156, which can be used to disable support for leader-election-based HA. The default for the disable flag is "false" i.e. support for HA is meant to be enabled by default, however the current check on the flag does the opposite of what it should, resulting in HA being disabled by default. This PR fixes the logic to restore the correct behaviour.

The tekton controller has a `disable-ha` flag that was introduced in tektoncd#3156, which can be used to disable support for leader-election-based HA. The default for the disable flag is "false" i.e. support for HA is meant to be enabled by default, however the current check on the flag does the opposite of what it should, resulting in HA being disabled by default. This PR fixes the logic to restore the correct behaviour. (cherry picked from commit 2c76fc1) Signed-off-by: Vincent Demeester <vdemeest@redhat.com>

xiujuan95 · 2020-10-12T09:41:36Z

@pierretasci I very like this PR. But could you pls tell me how can I configure the QPS, thread and burst? By a configMap, deployment or other kubernetes resources? And How can I check the change works? For example, the default QPS is 20, I want to modify it to 30. Then how can know I have changed it successfully? Is one kubernetes resource will show it? Thanks in advance!

The tekton controller has a `disable-ha` flag that was introduced in #3156, which can be used to disable support for leader-election-based HA. The default for the disable flag is "false" i.e. support for HA is meant to be enabled by default, however the current check on the flag does the opposite of what it should, resulting in HA being disabled by default. This PR fixes the logic to restore the correct behaviour. (cherry picked from commit 2c76fc1) Signed-off-by: Vincent Demeester <vdemeest@redhat.com>

xiujuan95 · 2020-10-22T09:26:56Z

@pierretasci Sorry bother you again!

I want to change the default Thread/QPS/Burst to be 32/100/100, not sure if below setting is correct or not?

containers:
      - args:
        - -kube-api-qps=100
        - -kube-api-burst=100
        - -threads-per-controller=32

Because I don't know how to confirm this change takes effect, I need to get a sure answer about how to set the flag from you. Thanks in advance!

pierretasci · 2020-10-22T09:48:37Z

@pierretasci Sorry bother you again!

I want to change the default Thread/QPS/Burst to be 32/100/100, not sure if below setting is correct or not?
containers:
      - args:
        - -kube-api-qps=100
        - -kube-api-burst=100
        - -threads-per-controller=32
Because I don't know how to confirm this change takes effect, I need to get a sure answer about how to set the flag from you. Thanks in advance!

@xiujuan95: @nishpa214 should be able to help with that

nishpa214 · 2020-10-22T16:23:31Z

@xiujuan95 You want to set those parameters for tekton-pipelines-controller container in deployment spec. Here: https://github.com/tektoncd/pipeline/blob/master/config/controller.yaml#L76

You want to set them as following:

...
"-shell-image", "gcr.io/distroless/base:debug@sha256:72a0093a0214e414527a97d359313992534f94a689449615875d922097f0ba62",
"-kube-api-qps", "100",
"-kube-api-burst", "100",
"-threads-per-controller": "32"

xiujuan95 · 2020-10-23T03:03:38Z

@nishpa214 Thanks!
But for the last one -threads-per-controller, I think it should be "-threads-per-controller", "32" rather than "-threads-per-controller": "32", right?

tekton-robot added the do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. label Sep 1, 2020

tekton-robot requested review from afrittoli and bobcatfish September 1, 2020 18:13

tekton-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Sep 1, 2020

tekton-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Sep 1, 2020

tekton-robot added the kind/feature Categorizes issue or PR as related to a new feature. label Sep 1, 2020

tekton-robot assigned dlorenc Sep 1, 2020

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 Sep 1, 2020

pierretasci force-pushed the qps branch 2 times, most recently from 02822f6 to 7f92037 Compare September 2, 2020 18:13

dlorenc reviewed Sep 2, 2020

View reviewed changes

tekton-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 8, 2020

pierretasci mentioned this pull request Sep 8, 2020

Bump knative to enable new features #3181

Merged

4 tasks

tekton-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 15, 2020

pierretasci force-pushed the qps branch from 7f92037 to 37b6580 Compare September 15, 2020 22:52

tekton-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Sep 15, 2020

tekton-robot removed the lgtm Indicates that a PR is ready to be merged. label Oct 2, 2020

tekton-robot assigned ghost Oct 2, 2020

tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Oct 2, 2020

tekton-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 2, 2020

ghost removed the kind/feature Categorizes issue or PR as related to a new feature. label Oct 2, 2020

tekton-robot added the kind/feature Categorizes issue or PR as related to a new feature. label Oct 2, 2020

tekton-robot removed the kind/feature Categorizes issue or PR as related to a new feature. label Oct 2, 2020

tekton-robot added the kind/feature Categorizes issue or PR as related to a new feature. label Oct 2, 2020

tekton-robot merged commit 995e44f into tektoncd:master Oct 2, 2020

pierretasci deleted the qps branch October 2, 2020 20:07

pierDipi mentioned this pull request Oct 6, 2020

Allow override QPS and Burst via flag knative/pkg#1700

Closed

afrittoli reviewed Oct 6, 2020

View reviewed changes

afrittoli mentioned this pull request Oct 6, 2020

Fix the logic for the disable-ha flag #3340

Merged

3 tasks

vdemeester mentioned this pull request Oct 12, 2020

[cherry-pick] Fix the logic for the disable-ha flag #3371

Merged

4 tasks

zhangtbj mentioned this pull request Oct 30, 2020

Deploy tektoncd/pipeline in high concurrent request scenario #1281

Closed

xiujuan95 mentioned this pull request Nov 9, 2020

Add a document to tell users how to configure Thread, QPS and Burst #3508

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make DefaultThreadsPerController, QPS and Burst configurable via flags #3156

Make DefaultThreadsPerController, QPS and Burst configurable via flags #3156

pierretasci commented Sep 1, 2020 •

edited by vdemeester

Loading

tekton-robot commented Sep 1, 2020

pierretasci commented Sep 1, 2020

pierretasci commented Sep 1, 2020

dibyom commented Sep 1, 2020

dlorenc Sep 2, 2020

nishpa214 Sep 2, 2020 •

edited

Loading

dlorenc Sep 8, 2020

nishpa214 Sep 16, 2020

dlorenc commented Sep 2, 2020

nishpa214 commented Sep 2, 2020

imjasonh commented Sep 4, 2020

nishpa214 commented Sep 4, 2020

vdemeester commented Sep 8, 2020

nishpa214 commented Sep 8, 2020

ghost commented Oct 2, 2020

tekton-robot commented Oct 2, 2020

ghost commented Oct 2, 2020

ghost commented Oct 2, 2020

ghost commented Oct 2, 2020

afrittoli Oct 6, 2020

vdemeester Oct 6, 2020

xiujuan95 commented Oct 12, 2020 •

edited

Loading

xiujuan95 commented Oct 22, 2020

pierretasci commented Oct 22, 2020

nishpa214 commented Oct 22, 2020

xiujuan95 commented Oct 23, 2020 •

edited

Loading

Make DefaultThreadsPerController, QPS and Burst configurable via flags #3156

Make DefaultThreadsPerController, QPS and Burst configurable via flags #3156

Conversation

pierretasci commented Sep 1, 2020 • edited by vdemeester Loading

Changes

Submitter Checklist

Reviewer Notes

Release Notes

tekton-robot commented Sep 1, 2020

pierretasci commented Sep 1, 2020

pierretasci commented Sep 1, 2020

dibyom commented Sep 1, 2020

dlorenc Sep 2, 2020

Choose a reason for hiding this comment

nishpa214 Sep 2, 2020 • edited Loading

Choose a reason for hiding this comment

dlorenc Sep 8, 2020

Choose a reason for hiding this comment

nishpa214 Sep 16, 2020

Choose a reason for hiding this comment

dlorenc commented Sep 2, 2020

nishpa214 commented Sep 2, 2020

imjasonh commented Sep 4, 2020

nishpa214 commented Sep 4, 2020

vdemeester commented Sep 8, 2020

nishpa214 commented Sep 8, 2020

ghost commented Oct 2, 2020

tekton-robot commented Oct 2, 2020

ghost commented Oct 2, 2020

ghost commented Oct 2, 2020

ghost commented Oct 2, 2020

afrittoli Oct 6, 2020

Choose a reason for hiding this comment

vdemeester Oct 6, 2020

Choose a reason for hiding this comment

xiujuan95 commented Oct 12, 2020 • edited Loading

xiujuan95 commented Oct 22, 2020

pierretasci commented Oct 22, 2020

nishpa214 commented Oct 22, 2020

xiujuan95 commented Oct 23, 2020 • edited Loading

pierretasci commented Sep 1, 2020 •

edited by vdemeester

Loading

nishpa214 Sep 2, 2020 •

edited

Loading

xiujuan95 commented Oct 12, 2020 •

edited

Loading

xiujuan95 commented Oct 23, 2020 •

edited

Loading