-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -31,7 +31,9 @@ import ( | |
"kubevirt.io/client-go/log" | ||
|
||
"kubevirt.io/kubevirt/pkg/network/namescheme" | ||
"kubevirt.io/kubevirt/pkg/network/netbinding" | ||
"kubevirt.io/kubevirt/pkg/network/vmispec" | ||
virtconfig "kubevirt.io/kubevirt/pkg/virt-config" | ||
) | ||
|
||
type multusNetworkAnnotationPool struct { | ||
|
@@ -54,11 +56,11 @@ func (mnap multusNetworkAnnotationPool) toString() (string, error) { | |
return string(multusNetworksAnnotation), nil | ||
} | ||
|
||
func GenerateMultusCNIAnnotation(namespace string, interfaces []v1.Interface, networks []v1.Network) (string, error) { | ||
return GenerateMultusCNIAnnotationFromNameScheme(namespace, interfaces, networks, namescheme.CreateHashedNetworkNameScheme(networks)) | ||
func GenerateMultusCNIAnnotation(namespace string, interfaces []v1.Interface, networks []v1.Network, config *virtconfig.ClusterConfig) (string, error) { | ||
return GenerateMultusCNIAnnotationFromNameScheme(namespace, interfaces, networks, namescheme.CreateHashedNetworkNameScheme(networks), config) | ||
} | ||
|
||
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) { | ||
multusNetworkAnnotationPool := multusNetworkAnnotationPool{} | ||
|
||
for _, network := range networks { | ||
|
@@ -67,6 +69,17 @@ func GenerateMultusCNIAnnotationFromNameScheme(namespace string, interfaces []v1 | |
multusNetworkAnnotationPool.add( | ||
newMultusAnnotationData(namespace, interfaces, network, podInterfaceName)) | ||
} | ||
|
||
if config != nil && config.NetworkBindingPlugingsEnabled() { | ||
if iface := vmispec.LookupInterfaceByName(interfaces, network.Name); iface.Binding != nil { | ||
bindingPluginAnnotationData, err := newBindingPluginMultusAnnotationData( | ||
config.GetConfig(), iface.Binding.Name, namespace, network.Name) | ||
if err != nil { | ||
return "", err | ||
} | ||
multusNetworkAnnotationPool.add(*bindingPluginAnnotationData) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.
It seems that it wont work for CNIs that relays on it and config already exising interface with the same name. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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 !multusNetworkAnnotationPool.isEmpty() { | ||
|
@@ -75,6 +88,27 @@ func GenerateMultusCNIAnnotationFromNameScheme(namespace string, interfaces []v1 | |
return "", nil | ||
} | ||
|
||
func newBindingPluginMultusAnnotationData(kvConfig *v1.KubeVirtConfiguration, pluginName, namespace, networkName string) (*networkv1.NetworkSelectionElement, error) { | ||
plugin := netbinding.ReadNetBindingPluginConfiguration(kvConfig, pluginName) | ||
if plugin == nil { | ||
return nil, fmt.Errorf("unable to find the network binding plugin '%s' in Kubevirt configuration", pluginName) | ||
} | ||
|
||
netAttachDefNamespace, netAttachDefName := getNamespaceAndNetworkName(namespace, plugin.NetworkAttachmentDefinition) | ||
|
||
// cniArgNetworkName is the CNI arg name for the VM spec network logical name. | ||
// The binding plugin CNI should read this arg and realize which logical network it should modify. | ||
const cniArgNetworkName = "logic-network-name" | ||
|
||
return &networkv1.NetworkSelectionElement{ | ||
Namespace: netAttachDefNamespace, | ||
Name: netAttachDefName, | ||
CNIArgs: &map[string]interface{}{ | ||
cniArgNetworkName: networkName, | ||
}, | ||
}, nil | ||
} | ||
|
||
func newMultusAnnotationData(namespace string, interfaces []v1.Interface, network v1.Network, podInterfaceName string) networkv1.NetworkSelectionElement { | ||
multusIface := vmispec.LookupInterfaceByName(interfaces, network.Name) | ||
namespace, networkName := getNamespaceAndNetworkName(namespace, network.Multus.NetworkName) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -28,6 +28,9 @@ import ( | |
networkv1 "github.com/k8snetworkplumbingwg/network-attachment-definition-client/pkg/apis/k8s.cni.cncf.io/v1" | ||
|
||
v1 "kubevirt.io/api/core/v1" | ||
|
||
"kubevirt.io/kubevirt/pkg/testutils" | ||
virtconfig "kubevirt.io/kubevirt/pkg/virt-config" | ||
) | ||
|
||
var _ = Describe("Multus annotations", func() { | ||
|
@@ -86,4 +89,81 @@ var _ = Describe("Multus annotations", func() { | |
Expect(multusAnnotationPool.toString()).To(BeIdenticalTo(expectedString)) | ||
}) | ||
}) | ||
|
||
Context("Generate Multus network selection annotation", func() { | ||
When("NetworkBindingPlugins feature enabled", func() { | ||
It("should fail if the specified network binding plugin is not registered (specified in Kubevirt config)", func() { | ||
vmi := &v1.VirtualMachineInstance{ObjectMeta: metav1.ObjectMeta{Name: "testvmi", Namespace: "default"}} | ||
vmi.Spec.Networks = []v1.Network{ | ||
{Name: "default", NetworkSource: v1.NetworkSource{Pod: &v1.PodNetwork{}}}} | ||
vmi.Spec.Domain.Devices.Interfaces = []v1.Interface{ | ||
{Name: "default", Binding: &v1.PluginBinding{Name: "test-binding"}}} | ||
|
||
config := testsClusterConfig(map[string]v1.InterfaceBindingPlugin{ | ||
"another-test-binding": {NetworkAttachmentDefinition: "another-test-binding-net"}, | ||
}) | ||
|
||
_, err := GenerateMultusCNIAnnotation(vmi.Namespace, vmi.Spec.Domain.Devices.Interfaces, vmi.Spec.Networks, config) | ||
|
||
Expect(err).To(HaveOccurred()) | ||
}) | ||
|
||
It("should add network binding plugin net-attach-def to multus annotation", func() { | ||
vmi := &v1.VirtualMachineInstance{ObjectMeta: metav1.ObjectMeta{Name: "testvmi", Namespace: "default"}} | ||
vmi.Spec.Networks = []v1.Network{ | ||
{Name: "default", NetworkSource: v1.NetworkSource{Pod: &v1.PodNetwork{}}}, | ||
{Name: "blue", NetworkSource: v1.NetworkSource{Multus: &v1.MultusNetwork{NetworkName: "test1"}}}, | ||
{Name: "red", NetworkSource: v1.NetworkSource{Multus: &v1.MultusNetwork{NetworkName: "other-namespace/test1"}}}, | ||
} | ||
vmi.Spec.Domain.Devices.Interfaces = []v1.Interface{ | ||
{Name: "default", Binding: &v1.PluginBinding{Name: "test-binding"}}, | ||
{Name: "blue"}, {Name: "red"}} | ||
|
||
config := testsClusterConfig(map[string]v1.InterfaceBindingPlugin{ | ||
"test-binding": {NetworkAttachmentDefinition: "test-binding-net"}, | ||
}) | ||
|
||
Expect(GenerateMultusCNIAnnotation(vmi.Namespace, vmi.Spec.Domain.Devices.Interfaces, vmi.Spec.Networks, config)).To(MatchJSON( | ||
`[ | ||
{"name": "test-binding-net","namespace": "default", "cni-args": {"logic-network-name": "default"}}, | ||
{"name": "test1","namespace": "default","interface": "pod16477688c0e"}, | ||
{"name": "test1","namespace": "other-namespace","interface": "podb1f51a511f1"} | ||
]`, | ||
)) | ||
}) | ||
|
||
DescribeTable("should parse NetworkAttachmentDefinition name and namespace correctly, given", | ||
func(netAttachDefRawName, expectedAnnot string) { | ||
vmi := &v1.VirtualMachineInstance{ObjectMeta: metav1.ObjectMeta{Name: "testvmi", Namespace: "default"}} | ||
vmi.Spec.Networks = []v1.Network{*v1.DefaultPodNetwork()} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done |
||
vmi.Spec.Domain.Devices.Interfaces = []v1.Interface{ | ||
{Name: "default", Binding: &v1.PluginBinding{Name: "test-binding"}}} | ||
|
||
config := testsClusterConfig(map[string]v1.InterfaceBindingPlugin{ | ||
"test-binding": {NetworkAttachmentDefinition: netAttachDefRawName}, | ||
}) | ||
|
||
Expect(GenerateMultusCNIAnnotation(vmi.Namespace, vmi.Spec.Domain.Devices.Interfaces, vmi.Spec.Networks, config)).To(MatchJSON(expectedAnnot)) | ||
}, | ||
Entry("name with no namespace", "my-binding", | ||
`[{"namespace": "default", "name": "my-binding", "cni-args": {"logic-network-name": "default"}}]`), | ||
Entry("name with namespace", "namespace1/my-binding", | ||
`[{"namespace": "namespace1", "name": "my-binding", "cni-args": {"logic-network-name": "default"}}]`), | ||
) | ||
}) | ||
}) | ||
}) | ||
|
||
func testsClusterConfig(plugins map[string]v1.InterfaceBindingPlugin) *virtconfig.ClusterConfig { | ||
kvConfig := &v1.KubeVirtConfiguration{ | ||
DeveloperConfiguration: &v1.DeveloperConfiguration{ | ||
FeatureGates: []string{"NetworkBindingPlugins"}, | ||
}, | ||
NetworkConfiguration: &v1.NetworkConfiguration{ | ||
Binding: plugins, | ||
}, | ||
} | ||
config, _, _ := testutils.NewFakeClusterConfigUsingKVConfig(kvConfig) | ||
|
||
return config | ||
} |
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I am unclear what code we are testing here as a unit. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. Ok, now I understand. |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -73,6 +73,7 @@ type PodVmIfaceStatus struct { | |
} | ||
|
||
var _ = Describe("VirtualMachineInstance watcher", func() { | ||
var config *virtconfig.ClusterConfig | ||
|
||
var ctrl *gomock.Controller | ||
var vmiInterface *kubecli.MockVirtualMachineInstanceInterface | ||
|
@@ -262,7 +263,6 @@ var _ = Describe("VirtualMachineInstance watcher", func() { | |
}, | ||
} | ||
|
||
var config *virtconfig.ClusterConfig | ||
config, _, kvInformer = testutils.NewFakeClusterConfigUsingKVConfig(kubevirtFakeConfig) | ||
pvcInformer, _ = testutils.NewFakeInformerFor(&k8sv1.PersistentVolumeClaim{}) | ||
cdiInformer, _ = testutils.NewFakeInformerFor(&cdiv1.CDIConfig{}) | ||
|
@@ -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 commentThe 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 commentThe 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. |
||
|
||
var err error | ||
pod, err = NewPodForVirtualMachineWithMultusAnnotations(vmi, k8sv1.PodRunning, config) | ||
Expect(err).ToNot(HaveOccurred()) | ||
|
||
expectPodStatusUpdateFailed(pod) | ||
}) | ||
|
||
|
@@ -3359,7 +3362,9 @@ var _ = Describe("VirtualMachineInstance watcher", func() { | |
Name: "blue", | ||
NetworkSource: v1.NetworkSource{Multus: &virtv1.MultusNetwork{NetworkName: "blue-net"}}}} | ||
|
||
pod = NewPodForVirtualMachine(vmi, k8sv1.PodRunning, testPodNetworkStatus...) | ||
pod, err := NewPodForVirtualMachineWithMultusAnnotations(vmi, k8sv1.PodRunning, config, testPodNetworkStatus...) | ||
Expect(err).ToNot(HaveOccurred()) | ||
|
||
prependInjectPodPatch(pod) | ||
|
||
Expect(controller.updateMultusAnnotation(vmi.Namespace, vmi.Spec.Domain.Devices.Interfaces, vmi.Spec.Networks, pod)).To(Succeed()) | ||
|
@@ -3413,8 +3418,10 @@ var _ = Describe("VirtualMachineInstance watcher", func() { | |
vmIfaceStatus = append(vmIfaceStatus, *ifaceStatus[i].vmIfaceStatus) | ||
} | ||
} | ||
pod, err := NewPodForVirtualMachineWithMultusAnnotations(vmi, k8sv1.PodRunning, config, podIfaceStatus...) | ||
Expect(err).ToNot(HaveOccurred()) | ||
|
||
Expect(controller.updateInterfaceStatus(vmi, NewPodForVirtualMachine(vmi, k8sv1.PodRunning, podIfaceStatus...))).To(Succeed()) | ||
Expect(controller.updateInterfaceStatus(vmi, pod)).To(Succeed()) | ||
Expect(vmi.Status.Interfaces).To(ConsistOf(vmIfaceStatus)) | ||
}, | ||
Entry("VMI without interfaces on spec does not generate new interface status", api.NewMinimalVMI(vmName)), | ||
|
@@ -3527,20 +3534,11 @@ func setReadyCondition(vmi *virtv1.VirtualMachineInstance, status k8sv1.Conditio | |
Reason: reason, | ||
}) | ||
} | ||
func NewPodForVirtualMachine(vmi *virtv1.VirtualMachineInstance, phase k8sv1.PodPhase, podNetworkStatus ...networkv1.NetworkStatus) *k8sv1.Pod { | ||
multusAnnotations, _ := services.GenerateMultusCNIAnnotation(vmi.Namespace, vmi.Spec.Domain.Devices.Interfaces, vmi.Spec.Networks) | ||
|
||
func NewPodForVirtualMachine(vmi *virtv1.VirtualMachineInstance, phase k8sv1.PodPhase) *k8sv1.Pod { | ||
podAnnotations := map[string]string{ | ||
virtv1.DomainAnnotation: vmi.Name, | ||
} | ||
if multusAnnotations != "" { | ||
podAnnotations[networkv1.NetworkAttachmentAnnot] = multusAnnotations | ||
} | ||
if len(podNetworkStatus) > 0 { | ||
podCurrentNetworks, err := json.Marshal(podNetworkStatus) | ||
if err == nil { | ||
podAnnotations[networkv1.NetworkStatusAnnot] = string(podCurrentNetworks) | ||
} | ||
} | ||
return &k8sv1.Pod{ | ||
ObjectMeta: metav1.ObjectMeta{ | ||
Name: "test", | ||
|
@@ -3567,6 +3565,26 @@ func NewPodForVirtualMachine(vmi *virtv1.VirtualMachineInstance, phase k8sv1.Pod | |
} | ||
} | ||
|
||
func NewPodForVirtualMachineWithMultusAnnotations(vmi *virtv1.VirtualMachineInstance, phase k8sv1.PodPhase, config *virtconfig.ClusterConfig, podNetworkStatus ...networkv1.NetworkStatus) (*k8sv1.Pod, error) { | ||
pod := NewPodForVirtualMachine(vmi, phase) | ||
|
||
multusAnnotations, err := services.GenerateMultusCNIAnnotation(vmi.Namespace, vmi.Spec.Domain.Devices.Interfaces, vmi.Spec.Networks, config) | ||
if err != nil { | ||
return nil, err | ||
} | ||
pod.Annotations[networkv1.NetworkAttachmentAnnot] = multusAnnotations | ||
|
||
if len(podNetworkStatus) > 0 { | ||
podCurrentNetworksJSON, err := json.Marshal(podNetworkStatus) | ||
if err != nil { | ||
return nil, err | ||
} | ||
pod.Annotations[networkv1.NetworkStatusAnnot] = string(podCurrentNetworksJSON) | ||
} | ||
|
||
return pod, nil | ||
} | ||
|
||
func NewHotplugPVC(name, namespace string, phase k8sv1.PersistentVolumeClaimPhase) *k8sv1.PersistentVolumeClaim { | ||
t := true | ||
return &k8sv1.PersistentVolumeClaim{ | ||
|
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.
Done, I had to do some refactoring around multus annotation generating code to enable more granularity,
see the last commit.
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.
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.
@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.