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

Assign openshift sdn traffic to system priority level #880

Merged
merged 1 commit into from Dec 4, 2020

Conversation

tkashem
Copy link
Contributor

@tkashem tkashem commented Nov 13, 2020

Assign sdn traffic to system priority level, this will bring sdn traffic to the same priority level as kubelet traffic.

Currently the kube-apiserver does not allocate the sdn traffic to any dedicated concurrency pool, so treating it no different than the default cluster workload. This change will allow the kube-apiserver to allocate the sdn traffic to a known priority level system shared by kubelet.

For more information on priority and fairness - https://kubernetes.io/blog/2020/04/06/kubernetes-1-18-feature-api-priority-and-fairness-alpha/

- '*'
subjects:
- kind: ServiceAccount
serviceAccount:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

did i get the service account right?

Copy link
Contributor

Choose a reason for hiding this comment

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

We have two service accounts, sdn-controller and sdn. They're both system priority.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@squeed I added sdn-controller as you suggested.

@tkashem
Copy link
Contributor Author

tkashem commented Nov 13, 2020

/cc @deads2k

@tkashem
Copy link
Contributor Author

tkashem commented Nov 13, 2020

/retest

1 similar comment
@tkashem
Copy link
Contributor Author

tkashem commented Nov 14, 2020

/retest

@squeed
Copy link
Contributor

squeed commented Nov 16, 2020

One small change (both SAs) but otherwise seems reasonable.

@squeed
Copy link
Contributor

squeed commented Nov 16, 2020

Looks reasonable
/approve
I'll leave lgtm to @deads2k, since I can't speak for apiserver QOS.

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 16, 2020
@tkashem
Copy link
Contributor Author

tkashem commented Nov 17, 2020

/retest

@openshift-ci-robot openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 18, 2020
@deads2k
Copy link
Contributor

deads2k commented Nov 20, 2020

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Nov 20, 2020
@deads2k
Copy link
Contributor

deads2k commented Nov 20, 2020

@tkashem this is probably worth a backport.

@dcbw
Copy link
Member

dcbw commented Dec 1, 2020

@tkashem can you rebase? Thanks!

@openshift-ci-robot openshift-ci-robot removed lgtm Indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Dec 2, 2020
@tkashem
Copy link
Contributor Author

tkashem commented Dec 2, 2020

@dcbw rebase completed, it needs re-lgtm.

@tkashem
Copy link
Contributor Author

tkashem commented Dec 2, 2020

/retest

@deads2k
Copy link
Contributor

deads2k commented Dec 2, 2020

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Dec 2, 2020
@openshift-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: deads2k, squeed, tkashem

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

@tkashem
Copy link
Contributor Author

tkashem commented Dec 3, 2020

/retest

1 similar comment
@tkashem
Copy link
Contributor Author

tkashem commented Dec 3, 2020

/retest

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

11 similar comments
@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-merge-robot
Copy link
Contributor

openshift-merge-robot commented Dec 4, 2020

@tkashem: The following tests failed, say /retest to rerun all failed tests:

Test name Commit Details Rerun command
ci/prow/e2e-ovn-hybrid-step-registry 59824d3 link /test e2e-ovn-hybrid-step-registry
ci/prow/e2e-vsphere-ovn 59824d3 link /test e2e-vsphere-ovn
ci/prow/e2e-openstack-ovn 59824d3 link /test e2e-openstack-ovn

Full PR test history. Your PR dashboard.

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. I understand the commands that are listed here.

@openshift-merge-robot openshift-merge-robot merged commit dc4ad31 into openshift:master Dec 4, 2020
@danwinship
Copy link
Contributor

Um, this seems wrong; the sdn and sdn-controller ServiceAccounts are only created if openshift-sdn is selected as the network plugin, so this will be referring to non-existent ServiceAccounts when ovn-kubernetes, kuryr, or a third-party network plugin is in use. Even if that doesn't make things fail, we definitely want this traffic priority thing to be applying at least to both openshift-sdn and ovn-kubernetes...

alexanderConstantinescu added a commit to alexanderConstantinescu/cluster-network-operator that referenced this pull request Dec 16, 2020
PR: openshift#880 added a
deployment stanza for a specific kubernetes feature, but only did that for
openshift-sdn and not for ovn-kubernetes, moreover it added it in the wrong
place independently of the network plugin being deployed

Signed-off-by: Alexander Constantinescu <aconstan@redhat.com>
alexanderConstantinescu added a commit to alexanderConstantinescu/cluster-network-operator that referenced this pull request Dec 16, 2020
PR: openshift#880 added a
deployment stanza for a specific kubernetes feature, but only did that for
openshift-sdn and not for ovn-kubernetes, moreover it added it in the wrong
place independently of the network plugin being deployed

Signed-off-by: Alexander Constantinescu <aconstan@redhat.com>
@tkashem
Copy link
Contributor Author

tkashem commented Dec 16, 2020

Um, this seems wrong; the sdn and sdn-controller ServiceAccounts are only created if openshift-sdn is selected as the network plugin, so this will be referring to non-existent ServiceAccounts when ovn-kubernetes, kuryr, or a third-party network plugin is in use. Even if that doesn't make things fail, we definitely want this traffic priority thing to be applying at least to both openshift-sdn and ovn-kubernetes...

@danwinship so this implies that we have to create a flowschema based on the network stack installed by the network operator.

@squeed
Copy link
Contributor

squeed commented Dec 16, 2020

Yes, @danwinship is right. I shouldn't have approved this.

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. lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants