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

add SR-IOV GA proposal #6

Merged
merged 1 commit into from
Oct 1, 2019

Conversation

zshi-redhat
Copy link
Contributor

No description provided.

@openshift-ci-robot openshift-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Sep 9, 2019
@stevekuznetsov
Copy link

/uncc


#### Multus

Static IPAM plugin is used to assign IPv4 and IPv6 addresses statically to
Copy link
Contributor

Choose a reason for hiding this comment

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

This is going to be hard, because Multus has to ask for runtimeConfiguration via its CNI configuration file, which is static. We'll probably need to think of a way for NetworkAttachmentDefinitions to ask for CNI capabilites.

cc @dcbw

Copy link
Contributor

@s1061123 s1061123 Sep 30, 2019

Choose a reason for hiding this comment

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

We use network selection element in Pod annotation to describe IP/Mac, not in network attachment definition.
In K8s NPWG, we're already finalize about it and implemented in multus.

See the following Spec/PR for the detail:
Spec proposal: https://docs.google.com/document/d/1j9kSTIIKydjqGiYd8Ni9Tej7gv1sxf9VcWeRYXFe5ko/edit
PR: k8snetworkplumbingwg/multus-cni#387

Copy link
Contributor

Choose a reason for hiding this comment

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

So, the proposal is just about passing the IP runtimeConfig? That makes more sense.

Copy link
Contributor

Choose a reason for hiding this comment

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

Right.


##### Default Route Overwrite

Every pod comes with a default route added via openshift-sdn. It is not
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

@squeed so you think that we should NOT do that? If so, could you please let us know why?

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 Is it because that with default route overwrite, Multus is doing too much other than just calling delegated plugins which sounds like a thick plugin such as danm?

Copy link
Contributor

Choose a reason for hiding this comment

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

@squeed btw, we're going to use https://github.com/redhat-nfvpe/cni-route-override to override CNI route (except default route). For default route, we are proposing 'gateway' feature in NPWG.

If you think that multus includes route manipulation inside it and getting thick plugin, that is not correct, actually. We're still designing as minimum as possible.

Copy link
Contributor

Choose a reason for hiding this comment

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

Okay, that should work as well. Could you provide some example configuration? It's important that we design something that will work with CNI CHECK.

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

@zshi-redhat zshi-redhat Oct 1, 2019

Choose a reason for hiding this comment

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

@squeed I will add a default route overwrite example soon in a separate PR, sth like this:

kind: Pod
metadata:
  name: my-pod
  namespace: my-namespace
  annotations:
    k8s.v1.cni.cncf.io/networks: |
      [
        { "name":"net-a" },
        {
          "name":"net-b",
          "default-route": ["10.0.0.1"]
        }
      ]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

also paste the link for default route overwirte proposal: https://docs.google.com/document/d/1pqbWYtFdEYyXd1cXuyvq_J5SRnhiXCRptXho63gI4A8/edit


### Risks and Mitigations

#### NUMA
Copy link

Choose a reason for hiding this comment

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

This should be part of GA now that we have 1.16 in 4.3.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

- Manage SR-IOV end to end deployment and configuration via Operator
- Sufficient test coverage (upgrade, negative and performance)
- IP and MAC address management for SR-IOV interfaces
- Security and QoS support for SR-IOV interfaces
Copy link

Choose a reason for hiding this comment

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

Security and QoS flags are not a requirements for GA.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed

- Compare SR-IOV network performance w/wo NUMA affinity
- Support SR-IOV QoS flags
- Add SR-IOV Metrics and Alerting
- Support both RHEL and RHCOS worker nodes
Copy link

Choose a reason for hiding this comment

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

Add NUMA support here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added

@fepan
Copy link

fepan commented Oct 1, 2019

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Oct 1, 2019
@squeed
Copy link
Contributor

squeed commented Oct 1, 2019

/approve

As discussed elsewhere, I'd like to see a follow-up PR that outlines how routes will be configured. But that isn't blocking.

@openshift-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: fepan, squeed, zshi-redhat

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

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 1, 2019
@openshift-merge-robot openshift-merge-robot merged commit 5e92b67 into openshift:master Oct 1, 2019
marun added a commit to marun/enhancements that referenced this pull request Jan 21, 2020
openshift-merge-robot pushed a commit that referenced this pull request Oct 18, 2021
Adding risks - mitigations
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. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants