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.

Sign up for GitHub

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

Jump to bottom

SRIOV migration based hotplug #10185

Merged
merged 6 commits into from
Aug 6, 2023
Merged

SRIOV migration based hotplug #10185

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
94a51b5
SRIOV hotplug, virt-controller: Allow hot plugging SRIOV interface
AlonaKaplan Jul 27, 2023
91e1a36
SRIOV hotplug, virt-controller: Don't update existing pod annotation
AlonaKaplan Jul 27, 2023
deefa19
SRIOV hotplug, virt-controller: ignore not plugged ifaces on PCI Map
AlonaKaplan Jul 27, 2023
c9f53f1
SRIOV hotplug, virt-handler: Ignore SRIOV ifaces with no multus status
AlonaKaplan Jul 27, 2023
38a3bd5
e2e: Rename hotplug test file to hotplug_bridge
AlonaKaplan Jul 31, 2023
560e4cb
SRIOV hotplug, e2e: add SRIOV hotplug test
AlonaKaplan Jul 27, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion pkg/network/sriov/sriov.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@ func mapNetworkNameToPCIAddress(networks []v1.Network, interfaces []v1.Interface
multusInterfaceName := networkNameScheme[sriovIface.Name]
AlonaKaplan marked this conversation as resolved.
Show resolved Hide resolved
networkStatusEntry, exist := multusInterfaceNameToNetworkStatusMap[multusInterfaceName]
if !exist {
return nil, fmt.Errorf("failed to find network-status entry with interface %q", multusInterfaceName)
continue // The interface is not plugged yet
}
if networkStatusEntry.DeviceInfo == nil || networkStatusEntry.DeviceInfo.Pci == nil {
return nil, fmt.Errorf("failed to find device-info/pci-address in network-status annotation for SR-IOV interface %q", sriovIface.Name)
Expand Down
34 changes: 18 additions & 16 deletions pkg/network/sriov/sriov_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -200,22 +200,6 @@ var _ = Describe("SRIOV", func() {
networkPCIAnnotationValue := sriov.CreateNetworkPCIAnnotationValue(networkList, interfaceList, networkStatusAnnotationValue)
Expect(networkPCIAnnotationValue).To(Equal("{}"))
},
Entry("when pod's networkStatus Annotation does not exist",
[]virtv1.Network{newMultusNetwork("foo", "default/nad1")},
[]virtv1.Interface{newSRIOVInterface("foo")},
"",
),
Entry("when networkStatusAnnotation is valid but one SR-IOV entry is missing",
[]virtv1.Network{
newMultusNetwork("foo", "default/nad1"),
newMultusNetwork("boo", "default/nad2"),
},
[]virtv1.Interface{
newSRIOVInterface("foo"),
newSRIOVInterface("boo"),
},
fmt.Sprintf(networkStatusWithOneSRIOVNetworkFmt, fooHashedIfaceName),
),
Entry("when networkStatusAnnotation is valid but with no pci data on one of the SRIOV interfaces",
[]virtv1.Network{
newMultusNetwork("foo", "default/nad1"),
Expand Down Expand Up @@ -308,6 +292,24 @@ var _ = Describe("SRIOV", func() {
fmt.Sprintf(networkStatusWithOneBridgeOneSRIOVNetworksFmt, fooOrdinalIfaceName, booOrdinalIfaceName),
`{"foo":"0000:65:00.2"}`,
),
Entry("when pod's networkStatus Annotation does not exist",
[]virtv1.Network{newMultusNetwork("foo", "default/nad1")},
[]virtv1.Interface{newSRIOVInterface("foo")},
"",
`{}`,
),
Entry("when networkStatusAnnotation is valid but one SR-IOV entry is missing",
[]virtv1.Network{
newMultusNetwork("foo", "default/nad1"),
newMultusNetwork("boo", "default/nad2"),
},
[]virtv1.Interface{
newSRIOVInterface("foo"),
newSRIOVInterface("boo"),
},
fmt.Sprintf(networkStatusWithOneSRIOVNetworkFmt, fooHashedIfaceName),
`{"foo":"0000:04:02.5"}`,
),
)
})

Expand Down
2 changes: 1 addition & 1 deletion pkg/network/vmispec/hotplug.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ func NetworksToHotplug(networks []v1.Network, interfaceStatus []v1.VirtualMachin
func IndexInterfacesFromStatus(interfaces []v1.VirtualMachineInstanceNetworkInterface, p func(ifaceStatus v1.VirtualMachineInstanceNetworkInterface) bool) map[string]v1.VirtualMachineInstanceNetworkInterface {
indexedInterfaceStatus := map[string]v1.VirtualMachineInstanceNetworkInterface{}
for _, iface := range interfaces {
if p(iface) {
if p == nil || p(iface) {
indexedInterfaceStatus[iface.Name] = iface
}
}
Expand Down
25 changes: 15 additions & 10 deletions pkg/virt-controller/watch/network.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,23 +24,28 @@ import (
"kubevirt.io/kubevirt/pkg/network/vmispec"
)

func calculateDynamicInterfaces(vmi *v1.VirtualMachineInstance) ([]v1.Interface, []v1.Network, bool) {
vmiSpecIfaces := vmispec.FilterInterfacesSpec(vmi.Spec.Domain.Devices.Interfaces, func(iface v1.Interface) bool {
func calculateInterfacesAndNetworksForMultusAnnotationUpdate(vmi *v1.VirtualMachineInstance) ([]v1.Interface, []v1.Network, bool) {
vmiNonAbsentSpecIfaces := vmispec.FilterInterfacesSpec(vmi.Spec.Domain.Devices.Interfaces, func(iface v1.Interface) bool {
return iface.State != v1.InterfaceStateAbsent
})
ifacesToHotUnplugExist := len(vmi.Spec.Domain.Devices.Interfaces) > len(vmiSpecIfaces)
ifacesToHotUnplugExist := len(vmi.Spec.Domain.Devices.Interfaces) > len(vmiNonAbsentSpecIfaces)

vmiSpecNets := vmi.Spec.Networks
if ifacesToHotUnplugExist {
vmiSpecNets = vmispec.FilterNetworksByInterfaces(vmi.Spec.Networks, vmiSpecIfaces)
}
ifacesToHotplugExist := len(vmispec.NetworksToHotplug(vmiSpecNets, vmi.Status.Interfaces)) > 0
ifacesStatusByName := vmispec.IndexInterfacesFromStatus(vmi.Status.Interfaces, nil)
ifacesToAnnotate := vmispec.FilterInterfacesSpec(vmiNonAbsentSpecIfaces, func(iface v1.Interface) bool {
_, ifaceInStatus := ifacesStatusByName[iface.Name]
sriovIfaceNotPlugged := iface.SRIOV != nil && !ifaceInStatus
return !sriovIfaceNotPlugged
Comment on lines +36 to +37
Copy link
Contributor

Choose a reason for hiding this comment

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

IMO the intermediate variable doesn't help understanding what will be the result, the reader still need to interpret what !sriovIfaceNotPlugged means.
How about using the direct condition with better descriptive name?

nonSriovDesiredIfaces := iface.SRIOV == nil || ifaceInStatus
return nonSriovDesiredIfaces

We can address it later though.

})

networksToAnnotate := vmispec.FilterNetworksByInterfaces(vmi.Spec.Networks, ifacesToAnnotate)

ifacesToHotplugExist := len(vmispec.NetworksToHotplug(networksToAnnotate, vmi.Status.Interfaces)) > 0

isIfaceChangeRequired := ifacesToHotplugExist || ifacesToHotUnplugExist
if !isIfaceChangeRequired {
return nil, nil, false
}
return vmiSpecIfaces, vmiSpecNets, isIfaceChangeRequired
return ifacesToAnnotate, networksToAnnotate, isIfaceChangeRequired
}

func applyDynamicIfaceRequestOnVMI(vm *v1.VirtualMachine, vmi *v1.VirtualMachineInstance, hasOrdinalIfaces bool) *v1.VirtualMachineInstanceSpec {
Expand All @@ -49,7 +54,7 @@ func applyDynamicIfaceRequestOnVMI(vm *v1.VirtualMachine, vmi *v1.VirtualMachine
vmIndexedNetworks := vmispec.IndexNetworkSpecByName(vm.Spec.Template.Spec.Networks)
for _, vmIface := range vm.Spec.Template.Spec.Domain.Devices.Interfaces {
_, existsInVMISpec := vmiIndexedInterfaces[vmIface.Name]
shouldBeHotPlug := !existsInVMISpec && vmIface.State != v1.InterfaceStateAbsent && vmIface.InterfaceBindingMethod.Bridge != nil
shouldBeHotPlug := !existsInVMISpec && vmIface.State != v1.InterfaceStateAbsent && (vmIface.InterfaceBindingMethod.Bridge != nil || vmIface.InterfaceBindingMethod.SRIOV != nil)
shouldBeHotUnplug := !hasOrdinalIfaces && existsInVMISpec && vmIface.State == v1.InterfaceStateAbsent
if shouldBeHotPlug {
vmiSpecCopy.Networks = append(vmiSpecCopy.Networks, vmIndexedNetworks[vmIface.Name])
Expand Down
60 changes: 53 additions & 7 deletions pkg/virt-controller/watch/network_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,13 +41,15 @@ var _ = Describe("Network interface hot{un}plug", func() {

testNetworkName1 = "testnet1"
testNetworkName2 = "testnet2"
testNetworkName3 = "testnet3"
testNetworkName4 = "testnet4"

ordinal = true
)
DescribeTable("calculate if changes are required",

func(vmi *v1.VirtualMachineInstance, pod *k8sv1.Pod, expIfaces []v1.Interface, expNets []v1.Network, expToChange bool) {
ifaces, nets, exists := calculateDynamicInterfaces(vmi)
ifaces, nets, exists := calculateInterfacesAndNetworksForMultusAnnotationUpdate(vmi)
Expect(ifaces).To(Equal(expIfaces))
Expect(nets).To(Equal(expNets))
Expect(exists).To(Equal(expToChange))
Expand All @@ -69,23 +71,52 @@ var _ = Describe("Network interface hot{un}plug", func() {
),
Entry("when vmi interfaces have an extra interface which requires hotplug",
libvmi.New(
libvmi.WithInterface(v1.Interface{Name: testNetworkName1}),
libvmi.WithInterface(v1.Interface{Name: testNetworkName1, InterfaceBindingMethod: v1.InterfaceBindingMethod{SRIOV: &v1.InterfaceSRIOV{}}}),
libvmi.WithInterface(v1.Interface{Name: testNetworkName2}),
libvmi.WithInterface(v1.Interface{Name: testNetworkName3}),
libvmi.WithInterface(v1.Interface{Name: testNetworkName4, InterfaceBindingMethod: v1.InterfaceBindingMethod{SRIOV: &v1.InterfaceSRIOV{}}}),
libvmi.WithNetwork(&v1.Network{Name: testNetworkName1}),
libvmi.WithNetwork(&v1.Network{Name: testNetworkName2}),
libvmi.WithNetwork(&v1.Network{Name: testNetworkName3}),
libvmi.WithNetwork(&v1.Network{Name: testNetworkName4}),
withInterfaceStatus(v1.VirtualMachineInstanceNetworkInterface{Name: testNetworkName1}),
withInterfaceStatus(v1.VirtualMachineInstanceNetworkInterface{Name: testNetworkName2}),
),
&k8sv1.Pod{
ObjectMeta: metav1.ObjectMeta{Annotations: map[string]string{
networkv1.NetworkStatusAnnot: `[
{"interface":"net1", "name":"red-net", "namespace": "default"}
{"interface":"net1", "name":"red-net", "namespace": "default"},
{"interface":"net2", "name":"blue-net", "namespace": "default"}
]`,
}},
},
[]v1.Interface{{Name: testNetworkName1}, {Name: testNetworkName2}},
[]v1.Network{{Name: testNetworkName1}, {Name: testNetworkName2}},
[]v1.Interface{{Name: testNetworkName1, InterfaceBindingMethod: v1.InterfaceBindingMethod{SRIOV: &v1.InterfaceSRIOV{}}}, {Name: testNetworkName2}, {Name: testNetworkName3}},
[]v1.Network{{Name: testNetworkName1}, {Name: testNetworkName2}, {Name: testNetworkName3}},
expectToChange,
),
Entry("when vmi interfaces have an extra SRIOV interface which requires hotplug, change is not required since SRIOV hotplug to a pod is not supported",
libvmi.New(
libvmi.WithInterface(v1.Interface{Name: testNetworkName1}),
libvmi.WithInterface(v1.Interface{Name: testNetworkName2, InterfaceBindingMethod: v1.InterfaceBindingMethod{SRIOV: &v1.InterfaceSRIOV{}}}),
libvmi.WithInterface(v1.Interface{Name: testNetworkName3, InterfaceBindingMethod: v1.InterfaceBindingMethod{SRIOV: &v1.InterfaceSRIOV{}}}),
libvmi.WithNetwork(&v1.Network{Name: testNetworkName1}),
libvmi.WithNetwork(&v1.Network{Name: testNetworkName2}),
libvmi.WithNetwork(&v1.Network{Name: testNetworkName3}),
withInterfaceStatus(v1.VirtualMachineInstanceNetworkInterface{Name: testNetworkName1}),
withInterfaceStatus(v1.VirtualMachineInstanceNetworkInterface{Name: testNetworkName2}),
),
&k8sv1.Pod{
ObjectMeta: metav1.ObjectMeta{Annotations: map[string]string{
networkv1.NetworkStatusAnnot: `[
{"interface":"net1", "name":"red-net", "namespace": "default"},
{"interface":"net2", "name":"blue-net", "namespace": "default"}
]`,
}},
},
nil,
nil,
expectNoChange,
),
Copy link
Member

Choose a reason for hiding this comment

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

We are missing a case where an extra SR-IOV and an extra non-SR-IOV exist, then we should see only the existing interface (that is already there) and the new non-SR-IOV interface added, but not the SR-IOV one.

Copy link
Member Author

Choose a reason for hiding this comment

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

Added existing SRIOV interface to test "Entry("when vmi interfaces have an extra SRIOV interface which requires hotplug, change is not required since SRIOV hotplug to a pod is not supported","

Copy link
Member

Choose a reason for hiding this comment

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

I was looking for "expectToChange" + new SR-IOV not added.
I do not think we have it.

Entry("when a vmi interface has state set to `absent`, requiring hotunplug",
libvmi.New(
libvmi.WithInterface(v1.Interface{Name: testNetworkName1}),
Expand Down Expand Up @@ -148,7 +179,7 @@ var _ = Describe("Network interface hot{un}plug", func() {
libvmi.WithNetwork(&v1.Network{Name: testNetworkName1}),
),
false),
Entry("when an interface has to be hotplugged",
Entry("when a bridge binding interface has to be hotplugged",
libvmi.New(
libvmi.WithInterface(bridgeInterface(testNetworkName1)),
libvmi.WithNetwork(&v1.Network{Name: testNetworkName1}),
Expand All @@ -159,7 +190,18 @@ var _ = Describe("Network interface hot{un}plug", func() {
libvmi.WithNetwork(&v1.Network{Name: testNetworkName1}),
),
!ordinal),
Entry("when an interface has to be hotplugged but it has no bridge binding",
Entry("when an SRIOV binding interface has to be hotplugged",
libvmi.New(
libvmi.WithInterface(sriovInterface(testNetworkName1)),
libvmi.WithNetwork(&v1.Network{Name: testNetworkName1}),
),
libvmi.New(),
libvmi.New(
libvmi.WithInterface(sriovInterface(testNetworkName1)),
libvmi.WithNetwork(&v1.Network{Name: testNetworkName1}),
),
!ordinal),
Entry("when an interface has to be hotplugged but it has no SRIOV or bridge binding",
libvmi.New(
libvmi.WithInterface(v1.Interface{Name: testNetworkName1, InterfaceBindingMethod: v1.InterfaceBindingMethod{Macvtap: &v1.InterfaceMacvtap{}}}),
libvmi.WithNetwork(&v1.Network{Name: testNetworkName1}),
Expand Down Expand Up @@ -275,6 +317,10 @@ func bridgeInterface(name string) v1.Interface {
return v1.Interface{Name: name, InterfaceBindingMethod: v1.InterfaceBindingMethod{Bridge: &v1.InterfaceBridge{}}}
}

func sriovInterface(name string) v1.Interface {
return v1.Interface{Name: name, InterfaceBindingMethod: v1.InterfaceBindingMethod{SRIOV: &v1.InterfaceSRIOV{}}}
}

func bridgeAbsentInterface(name string) v1.Interface {
iface := bridgeInterface(name)
iface.State = v1.InterfaceStateAbsent
Expand Down
6 changes: 3 additions & 3 deletions pkg/virt-controller/watch/vmi.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1267,8 +1267,8 @@ func (c *VMIController) sync(vmi *virtv1.VirtualMachineInstance, pod *k8sv1.Pod,
}
}

if vmiSpecIfaces, vmiSpecNets, dynamicIfacesExist := calculateDynamicInterfaces(vmi); dynamicIfacesExist {
if err := c.handleDynamicInterfaceRequests(vmi.Namespace, vmiSpecIfaces, vmiSpecNets, pod); err != nil {
if vmiSpecIfaces, vmiSpecNets, dynamicIfacesExist := calculateInterfacesAndNetworksForMultusAnnotationUpdate(vmi); dynamicIfacesExist {
if err := c.updateMultusAnnotation(vmi.Namespace, vmiSpecIfaces, vmiSpecNets, pod); err != nil {
return &syncErrorImpl{
err: fmt.Errorf("failed to hot{un}plug network interfaces for vmi [%s/%s]: %w", vmi.GetNamespace(), vmi.GetName(), err),
reason: FailedHotplugSyncReason,
Expand Down Expand Up @@ -2275,7 +2275,7 @@ func (c *VMIController) getVolumePhaseMessageReason(volume *virtv1.Volume, names
return virtv1.VolumePending, PVCNotReadyReason, "PVC is in phase Lost"
}

func (c *VMIController) handleDynamicInterfaceRequests(namespace string, interfaces []virtv1.Interface, networks []virtv1.Network, pod *k8sv1.Pod) error {
func (c *VMIController) updateMultusAnnotation(namespace string, interfaces []virtv1.Interface, networks []virtv1.Network, pod *k8sv1.Pod) error {
podAnnotations := pod.GetAnnotations()

indexedMultusStatusIfaces := services.NonDefaultMultusNetworksIndexedByIfaceName(pod)
Expand Down