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

Added proposal with priority but assured concurrency and fairness #930

Closed
wants to merge 2 commits into from
Closed

Added proposal with priority but assured concurrency and fairness #930

wants to merge 2 commits into from

Conversation

MikeSpreitzer
Copy link
Member

No description provided.

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/kep Categorizes KEP tracking issues and PRs modifying the KEP directory sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. labels Apr 2, 2019
@k8s-ci-robot k8s-ci-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Apr 2, 2019
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: MikeSpreitzer
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approver: deads2k

If they are not already assigned, you can assign the PR to them by writing /assign @deads2k in a comment when ready.

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

@MikeSpreitzer
Copy link
Member Author

CC @yue9944882
CC @wojtek-t

@MikeSpreitzer
Exempt priority zero requests from being counted
eb4dd50 
.. to make the management like today's max-in-flight handler.
yue9944882
yue9944882 reviewed Apr 2, 2019
View reviewed changes
keps/sig-api-machinery/20190228-priority-and-fairness.md
queueLengthLimit: 100
```

Some flow schemata.
Copy link
Member

Choose a reason for hiding this comment

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

I believe the api model of flow schema here can be simplified by:

  • removing support for patternMatch and nonPatternMatch. additionally, all controllers are sharing the same user identity as system:kube-controller-manager but differs in user-agent http header.
  • make the field matchingPriority optional and only filled to resolve conflicts among multiple flow schemas. A default value will be working for most cases I suppose.
  • instead of defining explicit boolean tests in the api model. consider make it a bit simpler by costing a bit ambiguity. as the yaml examples in the following shows:
kind: FlowSchema
meta:
  name: system-top
spec:
  matchingPriority: 500 // default to 500, override if necessary
  requestPriority:
    name: system-top
  matchRules:
  - field: groups
     valueSet: [ "system:masters" ]
---
kind: FlowSchema
meta:
  name: system-high-node-heartbeat
spec:
  matchingPriority: 500 // default to 500, override if necessary
  requestPriority:
    name: system-high
  flowDistinguisher:
    source: user
    # no transformation in this case
    matchRules:
  - field: groups
     valueSet: [ "system:nodes" ]
  - field: resource
     valueSet: [ "nodes" ]
---
kind: FlowSchema
meta:
  name: system-high-system-objects
spec:
  matchingPriority: 500 // default to 500, override if necessary
  requestPriority:
    name: system-high
  flowDistinguisher:
    source: user
    # no transformation in this case
    matchRules:
  - field: groups
     valueSet: [ "system:nodes" ]
  - field: namespace
     valueSet: [ "kube-system" ]
---
kind: FlowSchema
meta:
  name: system-high-system-objects
spec:
  matchingPriority: 500 // default to 500, override if necessary
  requestPriority:
    name: system-high
  flowDistinguisher:
    source: user
    # no transformation in this case
    matchRules:
  - field: groups
     valueSet: [ "system:nodes" ]
  - field: namespace
     valueSet: [ "kube-system" ]
---
kind: FlowSchema
meta:
  name: workload-low-serviceaccounts
spec:
  matchingPriority: 500 // default to 500, override if necessary
  requestPriority:
    name: workload-high
  flowDistinguisher:
    source: namespace
    # no transformation in this case
  matchRules:
  - field: groups
     valueSet: [ "system:serviceaccounts" ]
....

Copy link
Member

Choose a reason for hiding this comment

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

  • all controllers are sharing the same user identity as system:kube-controller-manager but differs in user-agent http header.

that's not generally accurate... typically, each controller loop gets a distinct identity

yue9944882
yue9944882 reviewed Apr 2, 2019
View reviewed changes
keps/sig-api-machinery/20190228-priority-and-fairness.md
(ACV), which is calculated as follows.

```
ACV(l) = ceil( SCL * ACS(l) / (100 + sum[priority levels k] ACS(k) ) )
Copy link
Member

Choose a reason for hiding this comment

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

why is it 100? i have a complex feeling about introducing more magic numbers..

Copy link
Member Author

Choose a reason for hiding this comment

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

This number is not particularly magic. It just establishes a magnitude standard to compare the ACS values with. All that matters is their ratios. We could create another configuration parameter to define the share for shared concurrency. But, without loss of generality, we can fix it at a particular number.

@MikeSpreitzer
Copy link
Member Author

In the meeting today we agreed to start simpler, and #933 is an attempt to capture that agreement.
I expect we will soon want to restore borrowing between priority levels.
Nobody was confident that we will want to restore the distinction between mutating and read-only.

@MikeSpreitzer MikeSpreitzer mentioned this pull request Apr 2, 2019
Closed
@MikeSpreitzer MikeSpreitzer mentioned this pull request Apr 12, 2019
Merged
@k8s-ci-robot
Copy link
Contributor

@MikeSpreitzer: PR needs rebase.

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.

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 5, 2019
@wojtek-t
Copy link
Member

wojtek-t commented May 6, 2019

@MikeSpreitzer - should this one be closed now?

@MikeSpreitzer
Copy link
Member Author

It is not something we are trying to merge now. OTOH, it has a more advanced design that I think we will want to look at second.

@fejta-bot
Copy link

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

@k8s-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Aug 4, 2019
@fejta-bot
Copy link

Stale issues rot after 30d of inactivity.
Mark the issue as fresh with /remove-lifecycle rotten.
Rotten issues close after an additional 30d of inactivity.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle rotten

@k8s-ci-robot k8s-ci-robot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Sep 3, 2019
@fejta-bot
Copy link

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/close

@k8s-ci-robot
Copy link
Contributor

@fejta-bot: Closed this PR.

In response to this:

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/close

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.

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

@liggitt liggitt liggitt left review comments

@yue9944882 yue9944882 yue9944882 left review comments

@deads2k deads2k Awaiting requested review from deads2k

@lavalamp lavalamp Awaiting requested review from lavalamp

Assignees
No one assigned
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/kep Categorizes KEP tracking issues and PRs modifying the KEP directory lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@MikeSpreitzer @k8s-ci-robot @wojtek-t @fejta-bot @liggitt @yue9944882