Skip to content

Commit c1c9887

Browse files
almaslennikovalmaslennikov
committed
fix: getCurrentBridgeState to return all uplinks for grouped bridges
The getCurrentBridgeState() function was only processing the first uplink (Uplinks[0]) and ignoring the rest. This caused bridges with multiple uplinks (created via groupingPolicy: all) to constantly reconfigure because the current state comparison always showed a mismatch. Additionally, NeedToUpdateBridges was comparing the entire Bridges struct including the GroupingPolicy field. However, GroupingPolicy is a policy directive used during configuration and is not stored in OVS, so it will never appear in the discovered status. This caused another source of constant mismatch detection. Changes: - Modify getCurrentBridgeState() to iterate through all uplinks in knownConfig.Uplinks instead of only processing the first one - Modify NeedToUpdateBridges to only compare OVS configurations, ignoring the GroupingPolicy field - Add test cases for bridge with multiple uplinks - Add test case for partial uplinks when some interfaces are missing - Add test case for groupingPolicy difference being ignored Fixes infinite reconciliation loop where bridge ports kept being removed and re-added on every sync cycle. Signed-off-by: Alexander Maslennikov <amaslennikov@nvidia.com>
1 parent ee112ab commit c1c9887

File tree

4 files changed

+252
-39
lines changed

4 files changed

+252
-39
lines changed

‎api/v1/helper.go‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1039,7 +1039,9 @@ func GenerateBridgeName(iface *InterfaceExt) string {
10391039

10401040
// NeedToUpdateBridges returns true if bridge for the host requires update
10411041
func NeedToUpdateBridges(bridgeSpec, bridgeStatus *Bridges) bool {
1042-
return !equality.Semantic.DeepEqual(bridgeSpec, bridgeStatus)
1042+
// Compare only OVS configurations, not GroupingPolicy which is a policy directive
1043+
// and not something that exists in the actual OVS state
1044+
return !equality.Semantic.DeepEqual(bridgeSpec.OVS, bridgeStatus.OVS)
10431045
}
10441046

10451047
// SetKeepUntilTime sets an annotation to hold the "keep until time" for the node’s state.

‎api/v1/helper_test.go‎

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1688,6 +1688,18 @@ func TestNeedToUpdateBridges(t *testing.T) {
16881688
statusBridge: &v1.Bridges{OVS: []v1.OVSConfigExt{}},
16891689
expectedResult: true,
16901690
},
1691+
{
1692+
tname: "no update required - groupingPolicy difference should be ignored",
1693+
specBridge: &v1.Bridges{
1694+
GroupingPolicy: "all",
1695+
OVS: []v1.OVSConfigExt{{Name: "br-test", Bridge: v1.OVSBridgeConfig{DatapathType: "netdev"}}},
1696+
},
1697+
statusBridge: &v1.Bridges{
1698+
// Status doesn't have GroupingPolicy since it's not stored in OVS
1699+
OVS: []v1.OVSConfigExt{{Name: "br-test", Bridge: v1.OVSBridgeConfig{DatapathType: "netdev"}}},
1700+
},
1701+
expectedResult: false,
1702+
},
16911703
}
16921704
for _, tc := range testtable {
16931705
t.Run(tc.tname, func(t *testing.T) {

‎pkg/host/internal/bridge/ovs/ovs.go‎

Lines changed: 44 additions & 38 deletions
Original file line numberDiff line numberDiff line change
@@ -651,47 +651,53 @@ func (o *ovs) getCurrentBridgeState(ctx context.Context, dbClient client.Client,
651651
if len(knownConfig.Uplinks) == 0 {
652652
return currentConfig, nil
653653
}
654-
knownConfigUplink := knownConfig.Uplinks[0]
655-
iface, err := o.getInterfaceByName(ctx, dbClient, knownConfigUplink.Name)
656-
if err != nil {
657-
return nil, err
658-
}
659-
if iface == nil {
660-
return currentConfig, nil
661-
}
662654

663-
if iface.Error != nil {
664-
funcLog.V(2).Info("getCurrentBridgeState(): interface has an error, remove it from the bridge state", "interface", iface.Name, "error", iface.Error)
665-
// interface has an error, do not report info about it to let the operator try to recreate it
666-
return currentConfig, nil
667-
}
655+
// Process all uplinks
656+
for _, knownConfigUplink := range knownConfig.Uplinks {
657+
iface, err := o.getInterfaceByName(ctx, dbClient, knownConfigUplink.Name)
658+
if err != nil {
659+
return nil, err
660+
}
661+
if iface == nil {
662+
// Interface not found, skip it (bridge needs reconfiguration)
663+
continue
664+
}
668665

669-
port, err := o.getPortByInterface(ctx, dbClient, iface)
670-
if err != nil {
671-
return nil, err
672-
}
673-
if port == nil {
674-
return currentConfig, nil
675-
}
666+
if iface.Error != nil {
667+
funcLog.V(2).Info("getCurrentBridgeState(): interface has an error, skip it", "interface", iface.Name, "error", iface.Error)
668+
// interface has an error, do not report info about it to let the operator try to recreate it
669+
continue
670+
}
676671

677-
if !bridge.HasPort(port.UUID) {
678-
// interface belongs to a wrong bridge, do not include uplink config to
679-
// the current bridge state to let the operator try to fix this
680-
return currentConfig, nil
681-
}
682-
currentConfig.Uplinks = []sriovnetworkv1.OVSUplinkConfigExt{{
683-
PciAddress: knownConfigUplink.PciAddress,
684-
Name: knownConfigUplink.Name,
685-
Interface: sriovnetworkv1.OVSInterfaceConfig{
686-
Type: iface.Type,
687-
ExternalIDs: updateMap(knownConfigUplink.Interface.ExternalIDs, iface.ExternalIDs),
688-
Options: updateMap(knownConfigUplink.Interface.Options, iface.Options),
689-
OtherConfig: updateMap(knownConfigUplink.Interface.OtherConfig, iface.OtherConfig),
690-
},
691-
}}
692-
if iface.MTURequest != nil {
693-
mtu := *iface.MTURequest
694-
currentConfig.Uplinks[0].Interface.MTURequest = &mtu
672+
port, err := o.getPortByInterface(ctx, dbClient, iface)
673+
if err != nil {
674+
return nil, err
675+
}
676+
if port == nil {
677+
// Port not found, skip it
678+
continue
679+
}
680+
681+
if !bridge.HasPort(port.UUID) {
682+
// interface belongs to a wrong bridge, skip it
683+
continue
684+
}
685+
686+
uplinkConfig := sriovnetworkv1.OVSUplinkConfigExt{
687+
PciAddress: knownConfigUplink.PciAddress,
688+
Name: knownConfigUplink.Name,
689+
Interface: sriovnetworkv1.OVSInterfaceConfig{
690+
Type: iface.Type,
691+
ExternalIDs: updateMap(knownConfigUplink.Interface.ExternalIDs, iface.ExternalIDs),
692+
Options: updateMap(knownConfigUplink.Interface.Options, iface.Options),
693+
OtherConfig: updateMap(knownConfigUplink.Interface.OtherConfig, iface.OtherConfig),
694+
},
695+
}
696+
if iface.MTURequest != nil {
697+
mtu := *iface.MTURequest
698+
uplinkConfig.Interface.MTURequest = &mtu
699+
}
700+
currentConfig.Uplinks = append(currentConfig.Uplinks, uplinkConfig)
695701
}
696702
return currentConfig, nil
697703
}

‎pkg/host/internal/bridge/ovs/ovs_test.go‎

Lines changed: 193 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -593,6 +593,199 @@ var _ = Describe("OVS", func() {
593593
Expect(ret[0].Bridge.OtherConfig).To(BeEmpty())
594594
Expect(ret[0].Uplinks[0].Interface.MTURequest).To(BeNil())
595595
})
596+
It("Should return all uplinks for bridge with multiple uplinks", func() {
597+
mtu := 1500
598+
// Create interfaces for multiple uplinks
599+
iface1 := &InterfaceEntry{
600+
Name: "p0",
601+
UUID: uuid.NewString(),
602+
Type: "dpdk",
603+
MTURequest: &mtu,
604+
}
605+
iface2 := &InterfaceEntry{
606+
Name: "p1",
607+
UUID: uuid.NewString(),
608+
Type: "dpdk",
609+
MTURequest: &mtu,
610+
}
611+
iface3 := &InterfaceEntry{
612+
Name: "p2",
613+
UUID: uuid.NewString(),
614+
Type: "dpdk",
615+
MTURequest: &mtu,
616+
}
617+
iface4 := &InterfaceEntry{
618+
Name: "p3",
619+
UUID: uuid.NewString(),
620+
Type: "dpdk",
621+
MTURequest: &mtu,
622+
}
623+
// Create ports for each interface
624+
port1 := &PortEntry{
625+
Name: "p0",
626+
UUID: uuid.NewString(),
627+
Interfaces: []string{iface1.UUID},
628+
}
629+
port2 := &PortEntry{
630+
Name: "p1",
631+
UUID: uuid.NewString(),
632+
Interfaces: []string{iface2.UUID},
633+
}
634+
port3 := &PortEntry{
635+
Name: "p2",
636+
UUID: uuid.NewString(),
637+
Interfaces: []string{iface3.UUID},
638+
}
639+
port4 := &PortEntry{
640+
Name: "p3",
641+
UUID: uuid.NewString(),
642+
Interfaces: []string{iface4.UUID},
643+
}
644+
// Create bridge with all ports
645+
br := &BridgeEntry{
646+
Name: "br-rail-1",
647+
UUID: uuid.NewString(),
648+
Ports: []string{port1.UUID, port2.UUID, port3.UUID, port4.UUID},
649+
DatapathType: "netdev",
650+
}
651+
ovsEntry := &OpenvSwitchEntry{
652+
UUID: uuid.NewString(),
653+
Bridges: []string{br.UUID},
654+
}
655+
initialDBContent := &testDBEntries{
656+
OpenVSwitch: []*OpenvSwitchEntry{ovsEntry},
657+
Bridge: []*BridgeEntry{br},
658+
Port: []*PortEntry{port1, port2, port3, port4},
659+
Interface: []*InterfaceEntry{iface1, iface2, iface3, iface4},
660+
}
661+
createInitialDBContent(ctx, ovsClient, initialDBContent)
662+
663+
// Create managed bridges config with multiple uplinks
664+
conf := map[string]*sriovnetworkv1.OVSConfigExt{
665+
"br-rail-1": {
666+
Name: "br-rail-1",
667+
Bridge: sriovnetworkv1.OVSBridgeConfig{
668+
DatapathType: "netdev",
669+
},
670+
Uplinks: []sriovnetworkv1.OVSUplinkConfigExt{
671+
{
672+
PciAddress: "0000:08:00.0",
673+
Name: "p0",
674+
Interface: sriovnetworkv1.OVSInterfaceConfig{
675+
Type: "dpdk",
676+
MTURequest: &mtu,
677+
},
678+
},
679+
{
680+
PciAddress: "0000:08:00.1",
681+
Name: "p1",
682+
Interface: sriovnetworkv1.OVSInterfaceConfig{
683+
Type: "dpdk",
684+
MTURequest: &mtu,
685+
},
686+
},
687+
{
688+
PciAddress: "0000:08:00.2",
689+
Name: "p2",
690+
Interface: sriovnetworkv1.OVSInterfaceConfig{
691+
Type: "dpdk",
692+
MTURequest: &mtu,
693+
},
694+
},
695+
{
696+
PciAddress: "0000:08:00.3",
697+
Name: "p3",
698+
Interface: sriovnetworkv1.OVSInterfaceConfig{
699+
Type: "dpdk",
700+
MTURequest: &mtu,
701+
},
702+
},
703+
},
704+
},
705+
}
706+
store.EXPECT().GetManagedOVSBridges().Return(conf, nil)
707+
ret, err := ovs.GetOVSBridges(ctx)
708+
Expect(err).NotTo(HaveOccurred())
709+
Expect(ret).To(HaveLen(1))
710+
Expect(ret[0].Name).To(Equal("br-rail-1"))
711+
Expect(ret[0].Uplinks).To(HaveLen(4))
712+
// Verify all uplinks are returned
713+
uplinkNames := make([]string, 0, 4)
714+
for _, uplink := range ret[0].Uplinks {
715+
uplinkNames = append(uplinkNames, uplink.Name)
716+
}
717+
Expect(uplinkNames).To(ContainElements("p0", "p1", "p2", "p3"))
718+
})
719+
It("Should return partial uplinks when some interfaces are missing", func() {
720+
mtu := 1500
721+
// Create only 2 of 4 interfaces
722+
iface1 := &InterfaceEntry{
723+
Name: "p0",
724+
UUID: uuid.NewString(),
725+
Type: "dpdk",
726+
MTURequest: &mtu,
727+
}
728+
iface2 := &InterfaceEntry{
729+
Name: "p1",
730+
UUID: uuid.NewString(),
731+
Type: "dpdk",
732+
MTURequest: &mtu,
733+
}
734+
port1 := &PortEntry{
735+
Name: "p0",
736+
UUID: uuid.NewString(),
737+
Interfaces: []string{iface1.UUID},
738+
}
739+
port2 := &PortEntry{
740+
Name: "p1",
741+
UUID: uuid.NewString(),
742+
Interfaces: []string{iface2.UUID},
743+
}
744+
br := &BridgeEntry{
745+
Name: "br-rail-1",
746+
UUID: uuid.NewString(),
747+
Ports: []string{port1.UUID, port2.UUID},
748+
DatapathType: "netdev",
749+
}
750+
ovsEntry := &OpenvSwitchEntry{
751+
UUID: uuid.NewString(),
752+
Bridges: []string{br.UUID},
753+
}
754+
initialDBContent := &testDBEntries{
755+
OpenVSwitch: []*OpenvSwitchEntry{ovsEntry},
756+
Bridge: []*BridgeEntry{br},
757+
Port: []*PortEntry{port1, port2},
758+
Interface: []*InterfaceEntry{iface1, iface2},
759+
}
760+
createInitialDBContent(ctx, ovsClient, initialDBContent)
761+
762+
// Config expects 4 uplinks but only 2 exist
763+
conf := map[string]*sriovnetworkv1.OVSConfigExt{
764+
"br-rail-1": {
765+
Name: "br-rail-1",
766+
Bridge: sriovnetworkv1.OVSBridgeConfig{
767+
DatapathType: "netdev",
768+
},
769+
Uplinks: []sriovnetworkv1.OVSUplinkConfigExt{
770+
{PciAddress: "0000:08:00.0", Name: "p0", Interface: sriovnetworkv1.OVSInterfaceConfig{Type: "dpdk", MTURequest: &mtu}},
771+
{PciAddress: "0000:08:00.1", Name: "p1", Interface: sriovnetworkv1.OVSInterfaceConfig{Type: "dpdk", MTURequest: &mtu}},
772+
{PciAddress: "0000:08:00.2", Name: "p2", Interface: sriovnetworkv1.OVSInterfaceConfig{Type: "dpdk", MTURequest: &mtu}},
773+
{PciAddress: "0000:08:00.3", Name: "p3", Interface: sriovnetworkv1.OVSInterfaceConfig{Type: "dpdk", MTURequest: &mtu}},
774+
},
775+
},
776+
}
777+
store.EXPECT().GetManagedOVSBridges().Return(conf, nil)
778+
ret, err := ovs.GetOVSBridges(ctx)
779+
Expect(err).NotTo(HaveOccurred())
780+
Expect(ret).To(HaveLen(1))
781+
// Should only return the 2 existing uplinks
782+
Expect(ret[0].Uplinks).To(HaveLen(2))
783+
uplinkNames := make([]string, 0, 2)
784+
for _, uplink := range ret[0].Uplinks {
785+
uplinkNames = append(uplinkNames, uplink.Name)
786+
}
787+
Expect(uplinkNames).To(ContainElements("p0", "p1"))
788+
})
596789
})
597790
Context("RemoveOVSBridge", func() {
598791
It("No config", func() {

0 commit comments

Comments
 (0)