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
Network plugin binding API: Support CNI plugins #10568
Network plugin binding API: Support CNI plugins #10568
Conversation
9103156
to
de07f0d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed the 1st two commits.
@@ -2675,6 +2675,11 @@ type InterfaceBindingPlugin struct { | |||
// The sidecar handles (libvirt) domain configuration and optional services. | |||
// version: 1alphav1 | |||
SidecarImage string `json:"sidecarImage,omitempty"` | |||
// NetworkAttachmentDefinition References to a NetworkAttachmentDefinition CRD object. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
references to a NetworkAttachmentDefinition CR object.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
@@ -2675,6 +2675,11 @@ type InterfaceBindingPlugin struct { | |||
// The sidecar handles (libvirt) domain configuration and optional services. | |||
// version: 1alphav1 | |||
SidecarImage string `json:"sidecarImage,omitempty"` | |||
// NetworkAttachmentDefinition References to a NetworkAttachmentDefinition CRD object. | |||
// Format: <networkName>, <namespace>/<networkName>. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
networkName
is confusing, I would suggest to make it a simple name
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
if err != nil { | ||
return "", err | ||
} | ||
multusNetworkAnnotationPool.add(*bindingPluginAnnotationData) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is suppose to come immediately after the "base" CNI, not after all of the previous ones.
I am also unsure if it is correct to pass podInterfaceName
in. Have we defined in the design that the way to correlate between the CNI and the network is the pod interface name?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is suppose to come immediately after the "base" CNI, not after all of the previous ones.
I dont think there is difference as long as the "base" CNI invoked before the plugin CNI, right after the "base" or after other interfaces CNIs.
Anyhow I dont mind moving it, fixed.
I am also unsure if it is correct to pass podInterfaceName in
It seems that it wont work for CNIs that relays on it and config already exising interface with the same name.
I changed it to pass CNI args with the pod interface name for now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I dont think there is difference as long as the "base" CNI invoked before the plugin CNI, right after the "base" or after other interfaces CNIs
There is a difference: Resources will not be allocated for the next interface if the current injected CNI fails to configure the pod. It will also allow potential collisions to be detected in the correct order (in case the binding CNI/s touch some shared thing).
@@ -1161,6 +1161,50 @@ var _ = Describe("Template", func() { | |||
}, | |||
), | |||
) | |||
When("NetworkBindingPlugins feature enabled", func() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There has been no logic change in template.go
, so I would not expect these tests to be added here.
All logic seems to have been added to multus_annotations.go
. Can you test it in that context?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tests moved to multus_annotation_test.go
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are missing e2e tests, which IMO are a must when we touch the public API.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am unclear what code we are testing here as a unit.
At the VMI controller logic, I do not think we changed anything. We changed lower level logic which can be validated as a smaller unit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are no new tests, those changes adapt the existing tests to the new code changes, that is GenerateMultusCNIAnnotation
depends on virtconfig.ClusterConfig
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I do not understand why there is a need to update the annotations in a different way due to the changes in this PR. Please explain what did not work, because I would expect everything to keep on working as is without us needing to change anything, except perhaps the function signature.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, now I understand.
I agree with @AlonaKaplan on her comment [1], that would probably solve the distributed changes in these tests and clarify why it was needed.
Tiny typo in the NAd name - |
…gin API Example: Given a bidning name 'dummy' and a NetworkAttachmentDefinition named 'dummy-net-binding', Kubevirt config should be set as follows: --- apiVersion: kubevirt.io/v1 kind: KubeVirt metadata: name: kubevirt namespace: kubevirt spec: configuration: developerConfiguration: featureGates: - NetworkBindingPlugins network: binding: dummy: networkAttachmentDefinition: dummy-net-binding ... In follow-up commits, the registered net-attach-def will be added to the VM virt-launcher pod's Multus network CNI annotation. Signed-off-by: Or Mergi <ormergi@redhat.com>
de07f0d
to
34fae18
Compare
} | ||
|
||
func GenerateMultusCNIAnnotationFromNameScheme(namespace string, interfaces []v1.Interface, networks []v1.Network, networkNameScheme map[string]string) (string, error) { | ||
func GenerateMultusCNIAnnotationFromNameScheme(namespace string, interfaces []v1.Interface, networks []v1.Network, networkNameScheme map[string]string, config *virtconfig.ClusterConfig) (string, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this method is used when an interface is hotplugged as well.
As we don't support hotplugging a side car, I don't think that in this stage we should support hotplugging a CNI.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't the hotplug limited to the core bridge binding?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As we don't support hotplugging a side car, I don't think that in this stage we should support hotplugging a CNI.
Done, I had to do some refactoring around multus annotation generating code to enable more granularity,
see the last commit.
Isn't the hotplug limited to the core bridge binding?
It is, but since the pod may end up with additional interface
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is, but since the pod may end up with additional interface
I do not understand what this means.
What is the reasoning for complicating the code flow at this stage? (I am commenting here before looking at the changes)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In case of interface hotplug where the interface specifies a binding plugin and the plugin has a NAD registered, virt controller VMI controller will the pod Multus annotation and it will have the binding NAD.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't the hotplug limited to the core bridge binding?
@EdDev yes, it is limited. But once you have a bridge interface to hotplug, the whole network annotation for all the interface will be recalculated.
We have special code for SRIOV to avoid adding not plugged SRIOV interfaces to the annotation. We can add the same for plugin binding,
EDIT: On the other hand, virt-controller won't copy binding plugin interface from the VM to the VMI (same as for any other binding that is not bridge or SRIOV) so I think Eddy is right. Nothing should be done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm confused.
We are not doing this for other bindings, e.g. masquerade, macvtap, passt, etc. We are not filtering them, and I do not understand how this is different.
We already depend on something else to filter the requests.
The only reason we had the SR-IOV check is that it depends on the actual state. That is a different logic.
So, if you think this is really needed, I would expect this to be done to all other binding types.
{Name: "blue"}, {Name: "red"}} | ||
}) | ||
|
||
It("should add network binding plugin net-attach-def to multus annotation", func() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would also add a test for a ns/name nad name.
And as Eddy said, the test fit more mutlus_annotations_test.go.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
7215840
to
419e115
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have passed though all commits except the last.
if err != nil { | ||
return "", err | ||
} | ||
multusNetworkAnnotationPool.add(*bindingPluginAnnotationData) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I dont think there is difference as long as the "base" CNI invoked before the plugin CNI, right after the "base" or after other interfaces CNIs
There is a difference: Resources will not be allocated for the next interface if the current injected CNI fails to configure the pod. It will also allow potential collisions to be detected in the correct order (in case the binding CNI/s touch some shared thing).
if iface := vmispec.LookupInterfaceByName(interfaces, network.Name); iface.Binding != nil { | ||
podIfaceName := networkNameScheme[network.Name] | ||
bindingPluginAnnotationData, err := newBindingPluginMultusAnnotationData( | ||
config.GetConfig(), iface.Binding.Name, namespace, podIfaceName) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are still passing in the pod interface name, I do not understand why and how it is helpful for a general binding CNI. Why we are not passing the logical network name?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Following offline discussion I changed it to the "logical" network name.
} | ||
|
||
func GenerateMultusCNIAnnotationFromNameScheme(namespace string, interfaces []v1.Interface, networks []v1.Network, networkNameScheme map[string]string) (string, error) { | ||
func GenerateMultusCNIAnnotationFromNameScheme(namespace string, interfaces []v1.Interface, networks []v1.Network, networkNameScheme map[string]string, config *virtconfig.ClusterConfig) (string, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm confused.
We are not doing this for other bindings, e.g. masquerade, macvtap, passt, etc. We are not filtering them, and I do not understand how this is different.
We already depend on something else to filter the requests.
The only reason we had the SR-IOV check is that it depends on the actual state. That is a different logic.
So, if you think this is really needed, I would expect this to be done to all other binding types.
CNIArgs: &map[string]interface{}{ | ||
"name": podIfaceName, | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This needs to be documented on the commit and PRm it defines an API between kubevirt and the custom binding CNI.
And in this context, I do not think name
is expressing what is provided here. ("name" of what?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This needs to be documented on the commit and PRm it defines an API between kubevirt and the custom binding CNI.
Done
And in this context, I do not think name is expressing what is provided here. ("name" of what?)
Changed to 'vm-network-name' following offline discussion.
It("should fail if the specified network binding plugin is not registered (specified in Kubevirt config)", func() { | ||
kvConfig := &v1.KubeVirtConfiguration{ | ||
DeveloperConfiguration: &v1.DeveloperConfiguration{ | ||
FeatureGates: []string{"NetworkBindingPlugins"}}} | ||
config, _, _ := testutils.NewFakeClusterConfigUsingKVConfig(kvConfig) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you should create a helper that gets a list of bindings and returns the config
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
|
||
DescribeTable("should parse NetworkAttachmentDefinition name and namespace correctly, given", | ||
func(netAttachDefRawName, expectedAnnot string) { | ||
vmi.Spec.Networks = []v1.Network{*v1.DefaultPodNetwork()} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do not think we should use a common setup and then in one incident override it completely.
The common setup should consist of the common things, then you can modify it in each test.
If there is no common setup, then pass it to each test or use helpers to call them from the tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I do not understand why there is a need to update the annotations in a different way due to the changes in this PR. Please explain what did not work, because I would expect everything to keep on working as is without us needing to change anything, except perhaps the function signature.
7182442
to
bb05870
Compare
func newBindingPluginMultusAnnotationData(kvConfig *v1.KubeVirtConfiguration, pluginName, namespace, networkName string) (*networkv1.NetworkSelectionElement, error) { | ||
plugin := netbinding.ReadNetBindingPluginConfiguration(kvConfig, pluginName) | ||
if plugin == nil { | ||
return nil, fmt.Errorf("couldnt find network binding plugin '%s' in Kubevirt configuration", pluginName) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/couldnt/couldn't
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed, rephrased to "unable to find..."
@@ -3359,7 +3361,13 @@ var _ = Describe("VirtualMachineInstance watcher", func() { | |||
Name: "blue", | |||
NetworkSource: v1.NetworkSource{Multus: &virtv1.MultusNetwork{NetworkName: "blue-net"}}}} | |||
|
|||
pod = NewPodForVirtualMachine(vmi, k8sv1.PodRunning, testPodNetworkStatus...) | |||
pod = NewPodForVirtualMachine(vmi, k8sv1.PodRunning) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please introduce NewPodForVirtualMachineWithMutlusAnnotations
method instead of copy pasting it multiple times.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
Namespace: netAttachDefNamespace, | ||
Name: netAttachDefName, | ||
CNIArgs: &map[string]interface{}{ | ||
"vm-network-name": networkName, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will become a formal API between kubevirt and its binding CNI(s).
I think it needs to be a constant with a good description. In the future, it should probably be elevated to a binding API package.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
func newBindingPluginMultusAnnotationData(kvConfig *v1.KubeVirtConfiguration, pluginName, namespace, networkName string) (*networkv1.NetworkSelectionElement, error) { | ||
plugin := netbinding.ReadNetBindingPluginConfiguration(kvConfig, pluginName) | ||
if plugin == nil { | ||
return nil, fmt.Errorf("couldnt find network binding plugin '%s' in Kubevirt configuration", pluginName) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: couldn't
or could not
or my preferred one: unable to find the network ...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
Add the specified network binding plugin NetworkAttachmentDefinition to the VM virt-launcher pod's Multus annotation. If no NetworkAttachmentDefinition is registered (specified in Kubevirt config) its a no-op. This change enable invoking additional CNI chains on virt-launcher pod before it starts along others using network the binding plugin API. Kubevirt adds the registered NetAttachDef to the VM pod Multus network selection annotation, along with CNI args that includes the subject interface logical network name. The binding plugin CNI (specified in the binding plugin NetAttachDef) should read the logical-network-name CNI arg to realize which logical network it should modify. virt-controller vmi unit tests will be adapted to new changes in follow-up commit. Signed-off-by: Or Mergi <ormergi@redhat.com>
Generating multus annotations now depends on cluster config. Passing the tests cluster config to NewPodForVirtualMachine makes it cumbersome and requires changing all the tests that use it. Thus, introduce new factory for creating pod with multus annotations. The tests fake cluster config is moved to global scope to enable access for tests the needs it; tests that generates multus annotations. Signed-off-by: Or Mergi <ormergi@redhat.com>
bb05870
to
9712819
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you!
@@ -3284,8 +3284,11 @@ var _ = Describe("VirtualMachineInstance watcher", func() { | |||
appendNetworkToVMI( | |||
api.NewMinimalVMI(vmName), firstVMNetwork, firstVMInterface), | |||
secondVMNetwork, secondVMInterface) | |||
pod = NewPodForVirtualMachine(vmi, k8sv1.PodRunning) | |||
Expect(pod.Annotations).To(HaveKey(networkv1.NetworkAttachmentAnnot)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need to remove this one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that this test is context of hotplug interface tests, and NewPodForVirtualMachineWithMultusAnnotations() set multus annotation on the pod.
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: AlonaKaplan 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 |
/retest-required |
1 similar comment
/retest-required |
/test pull-kubevirt-e2e-k8s-1.26-sig-compute |
/cherry-pick release-1.1 |
@ormergi: new pull request could not be created: failed to create pull request against kubevirt/kubevirt#release-1.1 from head kubevirt-bot:cherry-pick-10568-to-release-1.1: status code 422 not one of [201], body: {"message":"Validation Failed","errors":[{"resource":"PullRequest","code":"custom","message":"No commits between kubevirt:release-1.1 and kubevirt-bot:cherry-pick-10568-to-release-1.1"}],"documentation_url":"https://docs.github.com/rest/pulls/pulls#create-a-pull-request"} In response to this:
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. |
What this PR does / why we need it:
This PR implement the Network binding plugin API's Pod Creation & Definition integration point [1], enable invoking CNIs to configure virt-launcher pod network namespace.
How it works:
Similar to secondary networks, the binding plugin CNI is invoked by Multus, its necessary to define a
NetworkAttachmentDefiniton
for the bidning plugin.Kubevirt will add the requested binding plugin
NetworkAttachmentDefiniton
to virt-launcher pod's Multus network selection annotation.When the pod is created, its CNI chain will be invoked including the required binding plugin CNI.
How to use:
Given a network binding plugin 'dummy' that require invoking CNI.
Create
NetworkAttachmentDefinition
with the CNI(s) config for the binding plugin:templatecni
source [2]Enable network binding plugins feature and register the plugin and its
NetwrokAttachmentDefinition
in Kubevirt config:Specify the binding in the VM spec:
virt-launcher pod's Multus network selection annotation include the required binding
NetworkAttachmentDefinition
:[1] Network binding plugin API design: https://docs.google.com/document/d/13fLaqZuM8El_CT5XzxPv-1epKsB1-Spv_jtXveEkkdQ#heading=h.qcw5usdbc0nx
[2]
templatecni
source: https://github.com/EdDev/template-cniWhich issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)
format, will close the issue(s) when PR gets merged):Fixes #
Special notes for your reviewer:
Last commit fixes virt-controller vmi_test.go unit tests, it required some more changes thus moved to new commit.
Checklist
This checklist is not enforcing, but it's a reminder of items that could be relevant to every PR.
Approvers are expected to review this list.
Release note: