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

Pods Egress DSCP QoS proposal #1035

Merged
merged 1 commit into from
Apr 25, 2022

Conversation

oribon
Copy link
Contributor

@oribon oribon commented Feb 16, 2022

Signed-off-by: Ori Braunshtein obraunsh@redhat.com

@oribon oribon changed the title Pods Egress DSCP QoS proposal WIP: Pods Egress DSCP QoS proposal Feb 16, 2022
@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 16, 2022
@oribon oribon force-pushed the egress_qos branch 4 times, most recently from 7679258 to e0a1867 Compare February 16, 2022 13:57
@oribon oribon changed the title WIP: Pods Egress DSCP QoS proposal Pods Egress DSCP QoS proposal Feb 16, 2022
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 16, 2022
enhancements/network/egress-qos.md Outdated Show resolved Hide resolved
enhancements/network/egress-qos.md Outdated Show resolved Hide resolved
enhancements/network/egress-qos.md Outdated Show resolved Hide resolved
enhancements/network/egress-qos.md Outdated Show resolved Hide resolved
enhancements/network/egress-qos.md Outdated Show resolved Hide resolved
enhancements/network/egress-qos.md Outdated Show resolved Hide resolved
Copy link
Contributor

@tssurya tssurya left a comment

Choose a reason for hiding this comment

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

/cc

enhancements/network/egress-qos.md Outdated Show resolved Hide resolved
@openshift-ci openshift-ci bot requested a review from tssurya February 23, 2022 10:45
@oribon oribon force-pushed the egress_qos branch 2 times, most recently from 0569749 to 80c8ad1 Compare February 23, 2022 12:29
enhancements/network/egress-qos.md Outdated Show resolved Hide resolved
enhancements/network/egress-qos.md Outdated Show resolved Hide resolved
enhancements/network/egress-qos.md Show resolved Hide resolved
enhancements/network/egress-qos.md Outdated Show resolved Hide resolved
enhancements/network/egress-qos.md Outdated Show resolved Hide resolved
enhancements/network/egress-qos.md Outdated Show resolved Hide resolved
enhancements/network/egress-qos.md Outdated Show resolved Hide resolved
enhancements/network/egress-qos.md Show resolved Hide resolved
enhancements/network/egress-qos.md Show resolved Hide resolved
enhancements/network/egress-qos.md Outdated Show resolved Hide resolved
@oribon oribon force-pushed the egress_qos branch 3 times, most recently from fcc5c1a to 7acd7ed Compare February 27, 2022 13:00
oribon added a commit to oribon/ovn-kubernetes that referenced this pull request Apr 6, 2022
Add the EgressQoS controller as described in openshift/enhancements#1035

Signed-off-by: Ori Braunshtein <obraunsh@redhat.com>
oribon added a commit to oribon/ovn-kubernetes that referenced this pull request Apr 6, 2022
Add the EgressQoS controller as described in openshift/enhancements#1035

Signed-off-by: Ori Braunshtein <obraunsh@redhat.com>
oribon added a commit to oribon/ovn-kubernetes that referenced this pull request Apr 12, 2022
Adding the API necessary for the EgressQoS controller
implementation as described in openshift/enhancements#1035

Signed-off-by: Ori Braunshtein <obraunsh@redhat.com>
oribon added a commit to oribon/ovn-kubernetes that referenced this pull request Apr 12, 2022
Add the EgressQoS controller as described in openshift/enhancements#1035

Signed-off-by: Ori Braunshtein <obraunsh@redhat.com>
oribon added a commit to oribon/ovn-kubernetes that referenced this pull request Apr 12, 2022
Add the EgressQoS controller as described in openshift/enhancements#1035

Signed-off-by: Ori Braunshtein <obraunsh@redhat.com>
oribon added a commit to oribon/ovn-kubernetes that referenced this pull request Apr 12, 2022
Add the EgressQoS controller as described in openshift/enhancements#1035

Signed-off-by: Ori Braunshtein <obraunsh@redhat.com>
A new API `EgressQoS` under the `k8s.ovn.org/v1` version will be added to `pkg/crd`.

A new controller in OVN-K will watch `EgressQoS`, `Pod` and `Node` objects, which will create the relevant QoS objects in OVN and result in the necessary flows to be programmed in OVS. To which logical switches these QoS objects are attached to differs a bit between SGW/LGW modes:
* In SGW mode it is enough to attach the QoS objects only to the single distributed `join` switch, as all pods egress traffic goes through it.
Copy link
Contributor

Choose a reason for hiding this comment

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

do we need to put it on 2 different switches (depending on gateway mode)? It would be easier to just do this on the node switch for both modes I think. Also, is the qos evaluation done after load balancer DNAT so that your dst match would be post service DNAT?

Copy link
Contributor

Choose a reason for hiding this comment

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

I know I previously made a comment that steered you towards splitting to worker and join switch for different modes, but I think since we match on dest ip its OK to do them both on the worker switch.

@dceara pointed out that the qos eval is done pre-LB DNAT, so matching on destinations that are k8s endpoints woudln't work. Maybe its worth just noting that here?

Copy link

Choose a reason for hiding this comment

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

Is there any reason why the traffic can't be dscp-marked in the "egress" pipeline (egress from OVN perspective)? If we use "to-lport" QOS rules then there's no issue, those are applied after LB DNAT, so they could match on endpoint IPs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I know I previously made a comment that steered you towards splitting to worker and join switch for different modes, but I think since we match on dest ip its OK to do them both on the worker switch.

@dceara pointed out that the qos eval is done pre-LB DNAT, so matching on destinations that are k8s endpoints woudln't work. Maybe its worth just noting that here?

changed it to use node switches regardless of sgw/lgw.

Is there any reason why the traffic can't be dscp-marked in the "egress" pipeline (egress from OVN perspective)? If we use "to-lport" QOS rules then there's no issue, those are applied after LB DNAT, so they could match on endpoint IPs.

no reason, changed it to to-lport as it seems that it still works

Signed-off-by: Ori Braunshtein <obraunsh@redhat.com>
oribon added a commit to oribon/ovn-kubernetes that referenced this pull request Apr 17, 2022
Add the EgressQoS controller as described in openshift/enhancements#1035

Signed-off-by: Ori Braunshtein <obraunsh@redhat.com>
oribon added a commit to oribon/ovn-kubernetes that referenced this pull request Apr 20, 2022
Adding the API necessary for the EgressQoS controller
implementation as described in openshift/enhancements#1035

Signed-off-by: Ori Braunshtein <obraunsh@redhat.com>
oribon added a commit to oribon/ovn-kubernetes that referenced this pull request Apr 20, 2022
Add the EgressQoS controller as described in openshift/enhancements#1035

Signed-off-by: Ori Braunshtein <obraunsh@redhat.com>
oribon added a commit to oribon/ovn-kubernetes that referenced this pull request Apr 24, 2022
Adding the API necessary for the EgressQoS controller
implementation as described in openshift/enhancements#1035

Signed-off-by: Ori Braunshtein <obraunsh@redhat.com>
oribon added a commit to oribon/ovn-kubernetes that referenced this pull request Apr 24, 2022
Add the EgressQoS controller as described in openshift/enhancements#1035

Signed-off-by: Ori Braunshtein <obraunsh@redhat.com>
@trozet
Copy link
Contributor

trozet commented Apr 25, 2022

/hold cancel
/lgtm

@openshift-ci openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 25, 2022
@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Apr 25, 2022
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Apr 25, 2022

@oribon: all tests passed!

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 9c808b2 into openshift:master Apr 25, 2022
oribon added a commit to oribon/ovn-kubernetes that referenced this pull request Apr 26, 2022
Adding the API necessary for the EgressQoS controller
implementation as described in openshift/enhancements#1035

Signed-off-by: Ori Braunshtein <obraunsh@redhat.com>
oribon added a commit to oribon/ovn-kubernetes that referenced this pull request Apr 26, 2022
Add the EgressQoS controller as described in openshift/enhancements#1035

Signed-off-by: Ori Braunshtein <obraunsh@redhat.com>
oribon added a commit to oribon/ovn-kubernetes that referenced this pull request Apr 26, 2022
Add the EgressQoS controller as described in openshift/enhancements#1035

Signed-off-by: Ori Braunshtein <obraunsh@redhat.com>
oribon added a commit to oribon/ovn-kubernetes that referenced this pull request Apr 26, 2022
Add the EgressQoS controller as described in openshift/enhancements#1035

Signed-off-by: Ori Braunshtein <obraunsh@redhat.com>
oribon added a commit to oribon/ovn-kubernetes that referenced this pull request Apr 26, 2022
Add the EgressQoS controller as described in openshift/enhancements#1035

Signed-off-by: Ori Braunshtein <obraunsh@redhat.com>
tssurya pushed a commit to tssurya/ovn-kubernetes-1 that referenced this pull request May 1, 2022
Adding the API necessary for the EgressQoS controller
implementation as described in openshift/enhancements#1035

Signed-off-by: Ori Braunshtein <obraunsh@redhat.com>
tssurya pushed a commit to tssurya/ovn-kubernetes-1 that referenced this pull request May 1, 2022
Add the EgressQoS controller as described in openshift/enhancements#1035

Signed-off-by: Ori Braunshtein <obraunsh@redhat.com>
tssurya pushed a commit to tssurya/ovn-kubernetes-1 that referenced this pull request May 2, 2022
Adding the API necessary for the EgressQoS controller
implementation as described in openshift/enhancements#1035

Signed-off-by: Ori Braunshtein <obraunsh@redhat.com>
tssurya pushed a commit to tssurya/ovn-kubernetes-1 that referenced this pull request May 2, 2022
Add the EgressQoS controller as described in openshift/enhancements#1035

Signed-off-by: Ori Braunshtein <obraunsh@redhat.com>
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.

10 participants