Use standard value type for k8s.v1.cni.cncf.io/networks annotation #4146
Conversation
|
@arunvelayutham I cannot add you as a reviewer at the moment. But I invited you as an antrea-io member (you should have received an invitation from Github), which will enable me to do that. |
Codecov Report
@@ Coverage Diff @@
## main #4146 +/- ##
==========================================
+ Coverage 65.92% 66.48% +0.55%
==========================================
Files 304 304
Lines 46626 46614 -12
==========================================
+ Hits 30737 30989 +252
+ Misses 13487 13213 -274
- Partials 2402 2412 +10
|
antoninbas
force-pushed
the
secondarynetwork-switch-to-standard-annotation
branch
2 times, most recently
from
536fc6b
to
5fef5a1
Compare
Thanks @antoninbas . I just got to see the invite and accepted it. Please add me to this PR review. |
antoninbas
force-pushed
the
secondarynetwork-switch-to-standard-annotation
branch
from
5fef5a1
to
aad9225
Compare
| // Set type to "antrea" | ||
| Type string `json:"type,omitempty"` | ||
| // Set mode to "sriov" | ||
| Mode string `json:"mode,omitempty"` |
There was a problem hiding this comment.
Do you think NetworkType works better? I am not certain. Mode works for me too.
There was a problem hiding this comment.
that works for me. I was inspired by macvlan, but I agree that the macvlan mode is a totally different concept :) Let's see if @arunvelayutham likes networkType too.
There was a problem hiding this comment.
NetworkType may be confusing as we are referring to the physical/logical "interface" as Mode (SRIOV or Veth?).
There was a problem hiding this comment.
I think it is not about interface type (SRIOV or veth), but network type - SRIOV, Overlay, MACVLAN, IPVLAN, etc.
|
@antoninbas To clarify on the interface type at Annotation, The reason we kept interface type as part of Pod Annotation is to make it flexible at the Pod/CNF level to decide which interface they wanted to use based on the traffic needs/usage (for example, CNF with higher throughput needs can use SRIOV VFs). So, if we choose "mode" option on CR to decide what type of network interface to use, we may need to have additional logic to handle Pod Scheduling (on a server with SRIOV capability) and a fallback option if there is an issue to get the VF assigned?. |
| // See the License for the specific language governing permissions and | ||
| // limitations under the License. | ||
|
|
||
| package podwatch |
There was a problem hiding this comment.
Thank you very much for getting the controller_test added!!.
| } | ||
| if netinfo.InterfaceType == sriovInterfaceType { |
There was a problem hiding this comment.
@antoninbas To clarify on the interface type at Annotation, The reason we kept interface type as part of Pod Annotation is to make it flexible at the Pod/CNF level to decide which interface they wanted to use based on the traffic needs/usage (for example, CNF with higher throughput needs can use SRIOV VFs).
As per the PR change, we moved it to the CR instance. Which means, any CNF pod which is getting configured to be part of this network will use SRIOV. for example, if the CNF gets scheduled in a server with no SRIOV NIC support (though its unlikely scenario in DC env?) secondary network config will fail (in current implementation, if the interface type is not provided, it will fall back to veth pair - Note: veth pair code is not merged yet).
So, if we choose "mode" option on CR to decide what type of network interface to use, we may need to have additional logic to handle Pod Scheduling (on a server with SRIOV capability) and a fallback option if there is an issue to get the VF assigned?.
|
@arunvelayutham you're correct, but in the current version (the one in main) it looks like we are using the standard upstream annotation, when in practice we use the standard annotation name but with a custom annotation type which is confusing. I also initially believed that there was value in choosing the "interface type" on a per interface basis, but this is not the upstream approach / the Multus approach. Yet, Multus is a standard choice for enterprise users. So is there really an interest in being able to make that choice on a per-interface basis, as opposed to defining 2 networks / NetworkAttachmentDefinition (one with SRIOV and one without)? Curious to hear what @jianjuns thinks here. I don't think the question is about having an automatic fallback from SRIOV to veth. When using SRIOV, the Pod needs to request a VF and this is taken into account during scheduling. It's more about having 2 NetworkAttachmentDefinition CRs (one for SRIOV, one for veth) with separate subnets, or being able to share the same NetworkAttachmentDefinition CR. |
|
Sure @antoninbas got it. I'm good either ways. separate subnet/net-attach-def per mode (sriov and veth) vs Pod specific interface choice. I Just wanted to ensure that the use case is clear from the edge/cnf perspective. @jianjuns please share your inputs as well. b/w, are these changes tested?. If not, I can help with that (just need time till Friday COB, if that is ok). I call pull in your changes and do a local build and test it at my cluster env. |
|
I am confused on the sriov or veth "interface type" or "mode" discussion. There should be only network concept. SR-IOV is a network type. "veth" is not a network type, but just an interface type, and itself defines nothing. We need not expose veth to user configuration at all in my mind, which is just an implementation choice of some types of network (e.g. overlay with OVS, Linux bridge, MACVLAN, etc.). And I do not feel on one network (NetworkAttachmentDefinition) we should have both SR-IOV and veth interfaces - how can that work? Neither SR-IOV should fallback to veth (I do not understand what does that mean - veth connects to what? What network?). Back to the original topic, yes, I also feel we should stick to the upstream annotation/CRD behaviors. |
|
@jianjuns I agree that the SRIOV vs veth discussion was confusing. |
|
@antoninbas : sure, you can have that case. But 1) it is not supported by any other plugin I know; 2) it is not very useful in my mind, as you be able to achieve the same with two network definitions; 3) if we want to support that, we need to introduce more parameters (for MACVLAN, or other ways to connect veth) than just a "veth" mode. 4) Probably we should introduce another annotation, not the existing one. And I personally feel no need to support that case, at least not for now. |
antoninbas
force-pushed
the
secondarynetwork-switch-to-standard-annotation
branch
from
aad9225
to
a8a4391
Compare
|
@jianjuns I agree with you. I updated the PR to replace 'mode' with 'networkType' as discussed above. |
I would appreciate your help with that, since we don't have the e2e test CI job running for this feature yet. That's also why I tried to add a comprehensive suite of unit tests. |
antoninbas
added
the
action/release-note
Sure, I will do it today and get back @antoninbas |
@antoninbas CRD Instance: kubectl get network-attachment-definitions.k8s.cni.cncf.io -n kube-system Pod Spec with Annotation defined: kubectl get pods -A Antrea-agent logs: |
For SecondaryNetwork support, we switch to using the standard value type for the k8s.v1.cni.cncf.io/networks annotation: https://pkg.go.dev/github.com/k8snetworkplumbingwg/[email protected]/pkg/apis/k8s.cni.cncf.io/v1#NetworkSelectionElement By switching to the standard type: * we avoid user confusion * we ensure compatibility with other solutions such as Multus * we support "edge" cases that were not supported before: 1) using a NetworkAttachmentDefinition CR defined in a different Namespace than the Pod, 2) providing the NetworkAttachmentDefinition CR name as `[namespace/]name[@interface]` * we can use the standard ParseNetworkAnnotation function and avoid potential bugs or inconsistencies. This means that it is no longer possible to provide an "interface type" as part of the annotation value. However, this is not really a standard concept, as this information is typically provided in the NetworkAttachmentDefinition CR. For example, "bridge" mode for "macvlan". And in our case we use "sriov" mode (for the "antrea" network type). In addition to this change, we add a good amount of unit tests, in the context of antrea-io#4142. Signed-off-by: Antonin Bas <[email protected]>
Signed-off-by: Antonin Bas <[email protected]>
Signed-off-by: Antonin Bas <[email protected]>
antoninbas
force-pushed
the
secondarynetwork-switch-to-standard-annotation
branch
from
a8a4391
to
560558a
Compare
|
@arunvelayutham thanks for testing and confirming that it works fine |
|
@jianjuns could you give this another review? |
|
/test-all |
|
LGTM |
…ntrea-io#4146) For SecondaryNetwork support, we switch to using the standard value type for the k8s.v1.cni.cncf.io/networks annotation: https://pkg.go.dev/github.com/k8snetworkplumbingwg/[email protected]/pkg/apis/k8s.cni.cncf.io/v1#NetworkSelectionElement By switching to the standard type: * we avoid user confusion * we ensure compatibility with other solutions such as Multus * we support "edge" cases that were not supported before: 1) using a NetworkAttachmentDefinition CR defined in a different Namespace than the Pod, 2) providing the NetworkAttachmentDefinition CR name as `[namespace/]name[@interface]` * we can use the standard ParseNetworkAnnotation function and avoid potential bugs or inconsistencies. This means that it is no longer possible to provide an "interface type" as part of the annotation value. However, this is not really a standard concept, as this information is typically provided in the NetworkAttachmentDefinition CR. For example, "bridge" mode for "macvlan". And in our case we use "sriov" networkType. In addition to this change, we add a good amount of unit tests, in the context of antrea-io#4142. Signed-off-by: Antonin Bas <[email protected]>
For SecondaryNetwork support, we switch to using the standard value type
for the k8s.v1.cni.cncf.io/networks annotation:
https://pkg.go.dev/github.com/k8snetworkplumbingwg/[email protected]/pkg/apis/k8s.cni.cncf.io/v1#NetworkSelectionElement
By switching to the standard type:
NetworkAttachmentDefinition CR defined in a different Namespace than
the Pod, 2) providing the NetworkAttachmentDefinition CR name as
[namespace/]name[@interface]potential bugs or inconsistencies.
This means that it is no longer possible to provide an "interface type"
as part of the annotation value. However, this is not really a standard
concept, as this information is typically provided in the
NetworkAttachmentDefinition CR. For example, "bridge" mode for
"macvlan". And in our case we use "sriov" mode (for the "antrea" network
type).
In addition to this change, we add a good amount of unit tests, in the
context of #4142.
Signed-off-by: Antonin Bas [email protected]