diff --git a/go-controller/pkg/clustermanager/secondary_network_cluster_manager.go b/go-controller/pkg/clustermanager/secondary_network_cluster_manager.go index 0986c59713d..03715a164cf 100644 --- a/go-controller/pkg/clustermanager/secondary_network_cluster_manager.go +++ b/go-controller/pkg/clustermanager/secondary_network_cluster_manager.go @@ -26,7 +26,7 @@ const ( // by NetAttachDefinitionController to add and delete NADs. type secondaryNetworkClusterManager struct { // net-attach-def controller handle net-attach-def and create/delete network controllers - nadController *nad.NetAttachDefinitionController + nadController *nad.NADController ovnClient *util.OVNClusterManagerClientset watchFactory *factory.WatchFactory // networkIDAllocator is used to allocate a unique ID for each secondary layer3 network @@ -54,7 +54,7 @@ func newSecondaryNetworkClusterManager(ovnClient *util.OVNClusterManagerClientse recorder: recorder, } - sncm.nadController, err = nad.NewNetAttachDefinitionController("cluster-manager", sncm, wf, recorder) + sncm.nadController, err = nad.NewClusterNADController("cluster-manager", sncm, wf, recorder) if err != nil { return nil, err } @@ -104,6 +104,7 @@ func (sncm *secondaryNetworkClusterManager) Stop() { } func (cm *secondaryNetworkClusterManager) GetDefaultNetworkController() nad.ReconcilableNetworkController { + // TODO implement return nil } diff --git a/go-controller/pkg/network-attach-def-controller/network_attach_def_controller.go b/go-controller/pkg/network-attach-def-controller/network_attach_def_controller.go index d0aa6ed5b85..005382f2709 100644 --- a/go-controller/pkg/network-attach-def-controller/network_attach_def_controller.go +++ b/go-controller/pkg/network-attach-def-controller/network_attach_def_controller.go @@ -11,6 +11,7 @@ import ( apierrors "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/labels" "k8s.io/apimachinery/pkg/util/sets" + coreinformers "k8s.io/client-go/informers/core/v1" "k8s.io/client-go/tools/cache" "k8s.io/client-go/tools/record" "k8s.io/client-go/util/workqueue" @@ -20,6 +21,8 @@ import ( nadinformers "github.com/k8snetworkplumbingwg/network-attachment-definition-client/pkg/client/informers/externalversions/k8s.cni.cncf.io/v1" nadlisters "github.com/k8snetworkplumbingwg/network-attachment-definition-client/pkg/client/listers/k8s.cni.cncf.io/v1" "github.com/ovn-org/ovn-kubernetes/go-controller/pkg/controller" + rainformers "github.com/ovn-org/ovn-kubernetes/go-controller/pkg/crd/routeadvertisements/v1/apis/informers/externalversions/routeadvertisements/v1" + "github.com/ovn-org/ovn-kubernetes/go-controller/pkg/types" "github.com/ovn-org/ovn-kubernetes/go-controller/pkg/util" ) @@ -52,15 +55,17 @@ type NetworkControllerManager interface { type watchFactory interface { NADInformer() nadinformers.NetworkAttachmentDefinitionInformer + RouteAdvertisementsInformer() rainformers.RouteAdvertisementsInformer + NodeCoreInformer() coreinformers.NodeInformer } -// NetAttachDefinitionController handles namespaced scoped NAD events and +// NADController handles namespaced scoped NAD events and // manages cluster scoped networks defined in those NADs. NADs are mostly // referred from pods to give them access to the network. Different NADs can // define the same network as long as those definitions are actually equal. // Unexpected situations are handled on best effort basis but improper NAD // adminstration can lead to undefined behavior in referred from running pods. -type NetAttachDefinitionController struct { +type NADController struct { name string netAttachDefLister nadlisters.NetworkAttachmentDefinitionLister controller controller.Controller @@ -74,71 +79,122 @@ type NetAttachDefinitionController struct { nads map[string]string } -func NewNetAttachDefinitionController( +// NewClusterNADController builds a NAD controller for cluster manager +func NewClusterNADController( name string, ncm NetworkControllerManager, wf watchFactory, recorder record.EventRecorder, -) (*NetAttachDefinitionController, error) { - nadInformer := wf.NADInformer() - nadController := &NetAttachDefinitionController{ +) (*NADController, error) { + return newNADController( + name, + "", + "", + ncm, + wf, + recorder, + ) +} + +// NewZoneNADController builds a NAD controller for OVN network manager +func NewZoneNADController( + name string, + zone string, + ncm NetworkControllerManager, + wf watchFactory, +) (*NADController, error) { + return newNADController( + name, + zone, + "", + ncm, + wf, + nil, + ) +} + +// NewNodeNADController builds a NAD controller for node network manager +func NewNodeNADController( + name string, + node string, + ncm NetworkControllerManager, + wf watchFactory, +) (*NADController, error) { + return newNADController( + name, + "", + node, + ncm, + wf, + nil, + ) +} + +func newNADController( + name string, + zone string, + node string, + ncm NetworkControllerManager, + wf watchFactory, + recorder record.EventRecorder, +) (*NADController, error) { + c := &NADController{ name: fmt.Sprintf("[%s NAD controller]", name), recorder: recorder, - netAttachDefLister: nadInformer.Lister(), - networkManager: newNetworkManager(name, ncm), + netAttachDefLister: wf.NADInformer().Lister(), + networkManager: newNetworkManager(name, zone, node, ncm, wf), networks: map[string]util.NetInfo{}, nads: map[string]string{}, } config := &controller.ControllerConfig[nettypes.NetworkAttachmentDefinition]{ RateLimiter: workqueue.DefaultControllerRateLimiter(), - Informer: nadInformer.Informer(), - Lister: nadController.netAttachDefLister.List, - Reconcile: nadController.sync, + Informer: wf.NADInformer().Informer(), + Lister: c.netAttachDefLister.List, + Reconcile: c.sync, ObjNeedsUpdate: nadNeedsUpdate, // this controller is not thread safe Threadiness: 1, } - - nadController.controller = controller.NewController( - nadController.name, + c.controller = controller.NewController( + c.name, config, ) - return nadController, nil + return c, nil } -func (nadController *NetAttachDefinitionController) Start() error { +func (c *NADController) Start() error { // initial sync here will ensure networks in network manager // network manager will use this initial set of ensured networks to consider // any other network stale on its own sync err := controller.StartWithInitialSync( - nadController.syncAll, - nadController.controller, + c.syncAll, + c.controller, ) if err != nil { return err } - err = nadController.networkManager.Start() + err = c.networkManager.Start() if err != nil { return err } - klog.Infof("%s: started", nadController.name) + klog.Infof("%s: started", c.name) return nil } -func (nadController *NetAttachDefinitionController) Stop() { - klog.Infof("%s: shutting down", nadController.name) - controller.Stop(nadController.controller) - nadController.networkManager.Stop() +func (c *NADController) Stop() { + klog.Infof("%s: shutting down", c.name) + controller.Stop(c.controller) + c.networkManager.Stop() } -func (nadController *NetAttachDefinitionController) syncAll() (err error) { - existingNADs, err := nadController.netAttachDefLister.List(labels.Everything()) +func (c *NADController) syncAll() (err error) { + existingNADs, err := c.netAttachDefLister.List(labels.Everything()) if err != nil { - return fmt.Errorf("%s: failed to list NADs: %w", nadController.name, err) + return fmt.Errorf("%s: failed to list NADs: %w", c.name, err) } // create all networks with their updated list of NADs and only then start @@ -148,40 +204,40 @@ func (nadController *NetAttachDefinitionController) syncAll() (err error) { for _, nad := range existingNADs { key, err := cache.MetaNamespaceKeyFunc(nad) if err != nil { - klog.Errorf("%s: failed to sync %v: %v", nadController.name, nad, err) + klog.Errorf("%s: failed to sync %v: %v", c.name, nad, err) continue } - err = nadController.syncNAD(key, nad) + err = c.syncNAD(key, nad) if err != nil { - return fmt.Errorf("%s: failed to sync %s: %v", nadController.name, key, err) + return fmt.Errorf("%s: failed to sync %s: %v", c.name, key, err) } } return nil } -func (nadController *NetAttachDefinitionController) sync(key string) error { +func (c *NADController) sync(key string) error { startTime := time.Now() - klog.V(5).Infof("%s: sync NAD %s", nadController.name, key) + klog.V(5).Infof("%s: sync NAD %s", c.name, key) defer func() { - klog.V(4).Infof("%s: finished syncing NAD %s, took %v", nadController.name, key, time.Since(startTime)) + klog.V(4).Infof("%s: finished syncing NAD %s, took %v", c.name, key, time.Since(startTime)) }() namespace, name, err := cache.SplitMetaNamespaceKey(key) if err != nil { - klog.Errorf("%s: failed splitting key %s: %v", nadController.name, key, err) + klog.Errorf("%s: failed splitting key %s: %v", c.name, key, err) return nil } - nad, err := nadController.netAttachDefLister.NetworkAttachmentDefinitions(namespace).Get(name) + nad, err := c.netAttachDefLister.NetworkAttachmentDefinitions(namespace).Get(name) if err != nil && !apierrors.IsNotFound(err) { return err } - return nadController.syncNAD(key, nad) + return c.syncNAD(key, nad) } -func (nadController *NetAttachDefinitionController) syncNAD(key string, nad *nettypes.NetworkAttachmentDefinition) error { +func (c *NADController) syncNAD(key string, nad *nettypes.NetworkAttachmentDefinition) error { var nadNetworkName string var nadNetwork, oldNetwork, ensureNetwork util.NetInfo var err error @@ -189,11 +245,11 @@ func (nadController *NetAttachDefinitionController) syncNAD(key string, nad *net if nad != nil { nadNetwork, err = util.ParseNADInfo(nad) if err != nil { - if nadController.recorder != nil { - nadController.recorder.Eventf(&corev1.ObjectReference{Kind: nad.Kind, Namespace: nad.Namespace, Name: nad.Name}, corev1.EventTypeWarning, + if c.recorder != nil { + c.recorder.Eventf(&corev1.ObjectReference{Kind: nad.Kind, Namespace: nad.Namespace, Name: nad.Name}, corev1.EventTypeWarning, "InvalidConfig", "Failed to parse network config: %v", err.Error()) } - klog.Errorf("%s: failed parsing NAD %s: %v", nadController.name, key, err) + klog.Errorf("%s: failed parsing NAD %s: %v", c.name, key, err) return nil } nadNetworkName = nadNetwork.GetNetworkName() @@ -207,17 +263,17 @@ func (nadController *NetAttachDefinitionController) syncNAD(key string, nad *net // - Return an error AND clean up NAD from the old network // the NAD refers to a different network than before - if nadNetworkName != nadController.nads[key] { - oldNetwork = nadController.networks[nadController.nads[key]] + if nadNetworkName != c.nads[key] { + oldNetwork = c.networks[c.nads[key]] } - currentNetwork := nadController.networks[nadNetworkName] + currentNetwork := c.networks[nadNetworkName] switch { case currentNetwork == nil: // the NAD refers to a new network, ensure it ensureNetwork = nadNetwork - nadController.networks[nadNetworkName] = ensureNetwork + c.networks[nadNetworkName] = ensureNetwork case currentNetwork.Equals(nadNetwork): // the NAD refers to an existing compatible network, ensure that // existing network holds a reference to this NAD @@ -236,7 +292,7 @@ func (nadController *NetAttachDefinitionController) syncNAD(key string, nad *net oldNetwork = currentNetwork fallthrough default: - err = fmt.Errorf("%s: NAD %s CNI config does not match that of network %s", nadController.name, key, nadNetworkName) + err = fmt.Errorf("%s: NAD %s CNI config does not match that of network %s", c.name, key, nadNetworkName) } // remove the NAD reference from the old network and delete the network if @@ -245,28 +301,24 @@ func (nadController *NetAttachDefinitionController) syncNAD(key string, nad *net oldNetworkName := oldNetwork.GetNetworkName() oldNetwork.DeleteNADs(key) if len(oldNetwork.GetNADs()) == 0 { - nadController.networkManager.DeleteNetwork(oldNetworkName) - delete(nadController.networks, oldNetworkName) + c.networkManager.DeleteNetwork(oldNetworkName) + delete(c.networks, oldNetworkName) } else { - nadController.networkManager.EnsureNetwork(oldNetwork) + c.networkManager.EnsureNetwork(oldNetwork) } } // this was a nad delete if ensureNetwork == nil { - delete(nadController.nads, key) + delete(c.nads, key) return err } - if ensureNetwork.IsDefault() { - klog.V(5).Infof("%s: NAD add for default network (key %s), skip it", nadController.name, key) - return nil - } - - // ensure the network associated with the NAD + // ensure the network associated with the NAD we also reconcile the network + // in case route advertisements changed ensureNetwork.AddNADs(key) - nadController.nads[key] = ensureNetwork.GetNetworkName() - nadController.networkManager.EnsureNetwork(ensureNetwork) + c.nads[key] = ensureNetwork.GetNetworkName() + c.networkManager.EnsureNetwork(ensureNetwork) return err } @@ -281,5 +333,7 @@ func nadNeedsUpdate(oldNAD, newNAD *nettypes.NetworkAttachmentDefinition) bool { return false } - return !reflect.DeepEqual(oldNAD.Spec, newNAD.Spec) + // also reconcile the network in case its route advertisements changed + return !reflect.DeepEqual(oldNAD.Spec, newNAD.Spec) || + oldNAD.Annotations[types.OvnRouteAdvertisementsKey] != newNAD.Annotations[types.OvnRouteAdvertisementsKey] } diff --git a/go-controller/pkg/network-attach-def-controller/network_attach_def_controller_test.go b/go-controller/pkg/network-attach-def-controller/network_attach_def_controller_test.go index 279d9c1758d..344749d079b 100644 --- a/go-controller/pkg/network-attach-def-controller/network_attach_def_controller_test.go +++ b/go-controller/pkg/network-attach-def-controller/network_attach_def_controller_test.go @@ -127,9 +127,8 @@ func TestNetAttachDefinitionController(t *testing.T) { } network_Default := &ovncnitypes.NetConf{ - Topology: types.Layer3Topology, NetConf: cnitypes.NetConf{ - Name: "default", + Name: types.DefaultNetworkName, Type: "ovn-k8s-cni-overlay", }, MTU: 1400, @@ -150,14 +149,19 @@ func TestNetAttachDefinitionController(t *testing.T) { expected []expected }{ { - name: "NAD on default network should be skipped", + name: "NAD on default network is tracked with default controller", args: []args{ { nad: "test/nad_1", network: network_Default, }, }, - expected: []expected{}, + expected: []expected{ + { + network: network_Default, + nads: []string{"test/nad_1"}, + }, + }, }, { name: "NAD added", @@ -382,14 +386,19 @@ func TestNetAttachDefinitionController(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { g := gomega.NewWithT(t) + tncm := &testNetworkControllerManager{ controllers: map[string]NetworkController{}, + defaultNetwork: &testNetworkController{ + NetInfo: &util.DefaultNetInfo{}, + }, } - nadController := &NetAttachDefinitionController{ + nadController := &NADController{ networks: map[string]util.NetInfo{}, nads: map[string]string{}, - networkManager: newNetworkManager("", tncm), + networkManager: newNetworkManager("", "", "", tncm, nil), } + nm := nadController.networkManager.(*networkManagerImpl) g.Expect(nadController.networkManager.Start()).To(gomega.Succeed()) defer nadController.networkManager.Stop() @@ -414,24 +423,36 @@ func TestNetAttachDefinitionController(t *testing.T) { } meetsExpectations := func(g gomega.Gomega) { + nm.Lock() + defer nm.Unlock() tncm.Lock() defer tncm.Unlock() var expectRunning []string + g.Expect(nm.networks).To(gomega.HaveLen(len(tt.expected))) for _, expected := range tt.expected { netInfo, err := util.NewNetInfo(expected.network) g.Expect(err).ToNot(gomega.HaveOccurred()) + // test that the desired networks have the expected config and NADs name := netInfo.GetNetworkName() - testNetworkKey := testNetworkKey(netInfo) - - // test that the controller have the expected config and NADs - g.Expect(tncm.controllers).To(gomega.HaveKey(testNetworkKey)) - g.Expect(tncm.controllers[testNetworkKey].Equals(netInfo)).To(gomega.BeTrue(), + g.Expect(nm.networks).To(gomega.HaveKey(name)) + g.Expect(nm.networks[name].Equals(netInfo)).To(gomega.BeTrue(), fmt.Sprintf("matching network config for network %s", name)) - g.Expect(tncm.controllers[testNetworkKey].GetNADs()).To(gomega.ConsistOf(expected.nads), + g.Expect(nm.networks[name].GetNADs()).To(gomega.ConsistOf(expected.nads), fmt.Sprintf("matching NADs for network %s", name)) - expectRunning = append(expectRunning, testNetworkKey) + + // test that the actual secondary controllers have the + // expected config and NADs + if name != types.DefaultNetworkName { + testNetworkKey := testNetworkKey(netInfo) + g.Expect(tncm.controllers).To(gomega.HaveKey(testNetworkKey)) + g.Expect(tncm.controllers[testNetworkKey].Equals(netInfo)).To(gomega.BeTrue(), + fmt.Sprintf("matching network config for network controller %s", name)) + g.Expect(tncm.controllers[testNetworkKey].GetNADs()).To(gomega.ConsistOf(expected.nads), + fmt.Sprintf("matching NADs for network controller %s", name)) + expectRunning = append(expectRunning, testNetworkKey) + } } expectStopped := sets.New(tncm.started...).Difference(sets.New(expectRunning...)).UnsortedList() @@ -502,7 +523,7 @@ func TestSyncAll(t *testing.T) { tncm := &testNetworkControllerManager{ controllers: map[string]NetworkController{}, } - nadController, err := NewNetAttachDefinitionController( + nadController, err := NewClusterNADController( "SUT", tncm, wf, diff --git a/go-controller/pkg/network-attach-def-controller/network_manager.go b/go-controller/pkg/network-attach-def-controller/network_manager.go index 9a2a67cab2f..9c3e3ee281c 100644 --- a/go-controller/pkg/network-attach-def-controller/network_manager.go +++ b/go-controller/pkg/network-attach-def-controller/network_manager.go @@ -2,14 +2,26 @@ package networkAttachDefController import ( "context" + "encoding/json" "fmt" + "reflect" "sync" "time" + nadlisters "github.com/k8snetworkplumbingwg/network-attachment-definition-client/pkg/client/listers/k8s.cni.cncf.io/v1" + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/util/sets" + corelisters "k8s.io/client-go/listers/core/v1" + "k8s.io/client-go/tools/cache" "k8s.io/client-go/util/workqueue" "k8s.io/klog/v2" + "github.com/ovn-org/ovn-kubernetes/go-controller/pkg/config" "github.com/ovn-org/ovn-kubernetes/go-controller/pkg/controller" + ratypes "github.com/ovn-org/ovn-kubernetes/go-controller/pkg/crd/routeadvertisements/v1" + ralisters "github.com/ovn-org/ovn-kubernetes/go-controller/pkg/crd/routeadvertisements/v1/apis/listers/routeadvertisements/v1" + "github.com/ovn-org/ovn-kubernetes/go-controller/pkg/types" "github.com/ovn-org/ovn-kubernetes/go-controller/pkg/util" ) @@ -34,24 +46,63 @@ type networkManager interface { Stop() } -func newNetworkManager(name string, ncm NetworkControllerManager) networkManager { +func newNetworkManager(name, zone, node string, ncm NetworkControllerManager, wf watchFactory) networkManager { nc := &networkManagerImpl{ name: fmt.Sprintf("[%s network manager]", name), + node: node, + zone: zone, ncm: ncm, networks: map[string]util.NetInfo{}, networkControllers: map[string]*networkControllerState{}, } + // this controller does not feed from an informer, networks are manually // added to the queue for processing - config := &controller.ReconcilerConfig{ + networkConfig := &controller.ReconcilerConfig{ RateLimiter: workqueue.DefaultControllerRateLimiter(), - Reconcile: nc.syncLocked, + Reconcile: nc.syncNetworkLocked, Threadiness: 1, } - nc.controller = controller.NewReconciler( + nc.networkReconciler = controller.NewReconciler( nc.name, - config, + networkConfig, ) + + // we don't care about route advertisements in cluster manager + if nc.hasRouteAdvertisements() { + nc.nadLister = wf.NADInformer().Lister() + nc.raLister = wf.RouteAdvertisementsInformer().Lister() + nc.nodeLister = wf.NodeCoreInformer().Lister() + + // ra controller + raConfig := &controller.ControllerConfig[ratypes.RouteAdvertisements]{ + RateLimiter: workqueue.DefaultControllerRateLimiter(), + Informer: wf.RouteAdvertisementsInformer().Informer(), + Lister: nc.raLister.List, + Reconcile: func(string) error { return nc.syncRunningNetworks() }, + ObjNeedsUpdate: raNeedsUpdate, + Threadiness: 1, + } + nc.raController = controller.NewController( + nc.name, + raConfig, + ) + + // node controller + nodeConfig := &controller.ControllerConfig[corev1.Node]{ + RateLimiter: workqueue.DefaultControllerRateLimiter(), + Informer: wf.NodeCoreInformer().Informer(), + Lister: nc.nodeLister.List, + Reconcile: func(string) error { return nc.syncRunningNetworks() }, + ObjNeedsUpdate: nodeNeedsUpdate, + Threadiness: 1, + } + nc.nodeController = controller.NewController( + nc.name, + nodeConfig, + ) + } + return nc } @@ -62,8 +113,19 @@ type networkControllerState struct { type networkManagerImpl struct { sync.Mutex - name string - controller controller.Reconciler + + name string + zone string + node string + + nadLister nadlisters.NetworkAttachmentDefinitionLister + raLister ralisters.RouteAdvertisementsLister + nodeLister corelisters.NodeLister + + networkReconciler controller.Reconciler + raController controller.Controller + nodeController controller.Controller + ncm NetworkControllerManager networks map[string]util.NetInfo networkControllers map[string]*networkControllerState @@ -72,11 +134,28 @@ type networkManagerImpl struct { // Start will cleanup stale networks that have not been ensured via // EnsuredNetwork before this call func (nm *networkManagerImpl) Start() error { - return controller.StartWithInitialSync(nm.syncAll, nm.controller) + controllers := []controller.Reconciler{nm.networkReconciler} + if nm.raController != nil { + controllers = append(controllers, nm.raController) + } + if nm.nodeController != nil { + controllers = append(controllers, nm.nodeController) + } + return controller.StartWithInitialSync( + nm.syncAllNetworks, + controllers..., + ) } func (nm *networkManagerImpl) Stop() { - controller.Stop(nm.controller) + controllers := []controller.Reconciler{nm.networkReconciler} + if nm.raController != nil { + controllers = append(controllers, nm.raController) + } + if nm.nodeController != nil { + controllers = append(controllers, nm.nodeController) + } + controller.Stop(controllers...) for _, networkControllerState := range nm.networkControllers { networkControllerState.controller.Stop() @@ -87,24 +166,72 @@ func (nm *networkManagerImpl) EnsureNetwork(network util.NetInfo) { nm.Lock() defer nm.Unlock() nm.networks[network.GetNetworkName()] = network - nm.controller.Reconcile(network.GetNetworkName()) + nm.networkReconciler.Reconcile(network.GetNetworkName()) } func (nm *networkManagerImpl) DeleteNetwork(network string) { nm.Lock() defer nm.Unlock() delete(nm.networks, network) - nm.controller.Reconcile(network) + if network == types.DefaultNetworkName { + // for the default network however ensure it runs with the default + // config + nm.networks[types.DefaultNetworkName] = &util.DefaultNetInfo{} + } + nm.networkReconciler.Reconcile(network) } -func (nm *networkManagerImpl) syncLocked(network string) error { +func (nm *networkManagerImpl) syncAllNetworks() error { nm.Lock() defer nm.Unlock() - return nm.sync(network) + + // as we sync upon start, consider networks that have not been ensured as + // stale and clean them up + validNetworks := make([]util.BasicNetInfo, 0, len(nm.networks)) + networkNames := make([]string, 0, len(nm.networks)) + for name, network := range nm.networks { + validNetworks = append(validNetworks, network) + networkNames = append(networkNames, name) + } + if err := nm.ncm.CleanupDeletedNetworks(validNetworks...); err != nil { + return err + } + + // sync all known networks. There is no informer for networks. Keys are added by NAD controller. + // Certain downstream controllers that handle configuration for multiple networks depend on being + // aware of all the existing networks on initialization. To achieve that, we need to start existing + // networks synchronously. Otherwise, these controllers might incorrectly assess valid configuration + // as stale. + start := time.Now() + klog.Infof("%s: syncing all networks", nm.name) + for _, networkName := range networkNames { + if err := nm.syncNetwork(networkName); err != nil { + return fmt.Errorf("failed to sync network %s: %w", networkName, err) + } + } + klog.Infof("%s: finished syncing all networks. Time taken: %s", nm.name, time.Since(start)) + return nil +} + +func (nm *networkManagerImpl) syncRunningNetworks() error { + nm.Lock() + defer nm.Unlock() + + for network := range nm.networkControllers { + nm.networkReconciler.Reconcile(network) + } + + return nil +} + +func (nm *networkManagerImpl) syncNetworkLocked(network string) error { + nm.Lock() + defer nm.Unlock() + return nm.syncNetwork(network) } -// sync must be called with nm mutex locked -func (nm *networkManagerImpl) sync(network string) error { +// syncNetwork must be called with nm mutex locked +func (nm *networkManagerImpl) syncNetwork(network string) error { startTime := time.Now() klog.V(5).Infof("%s: sync network %s", nm.name, network) defer func() { @@ -115,19 +242,13 @@ func (nm *networkManagerImpl) sync(network string) error { have := nm.networkControllers[network] // we will dispose of the old network if deletion is in progress or if - // configuration changed + // non-reconcilable configuration changed dispose := have != nil && (have.stoppedAndDeleting || !have.controller.Equals(want)) - if dispose { - if !have.stoppedAndDeleting { - have.controller.Stop() - } - have.stoppedAndDeleting = true - err := have.controller.Cleanup() + err := nm.deleteNetwork(network) if err != nil { - return fmt.Errorf("%s: failed to cleanup network %s: %w", nm.name, network, err) + return err } - delete(nm.networkControllers, network) } // no network needed so nothing to do @@ -135,54 +256,178 @@ func (nm *networkManagerImpl) sync(network string) error { return nil } - // this might just be an update of the network NADs - if have != nil && !dispose { - have.controller.SetNADs(want.GetNADs()...) + // ensure the network + err := nm.ensureNetwork(want) + if err != nil { + return fmt.Errorf("%s: failed to ensure network %s: %w", nm.name, network, err) + } + + return nil +} + +func (nm *networkManagerImpl) ensureNetwork(network util.NetInfo) error { + networkName := network.GetNetworkName() + + err := nm.setVRFs(network) + if err != nil { + return fmt.Errorf("failed to set VRFs for network %s: %w", networkName, err) + } + + var reconcilable ReconcilableNetworkController + switch network.IsDefault() { + case true: + reconcilable = nm.ncm.GetDefaultNetworkController() + if reconcilable == nil { + // no default network controller to act on + return nil + } + default: + state := nm.networkControllers[network.GetNetworkName()] + if state != nil { + reconcilable = state.controller + } + } + + // this might just be an update of reconcilable network configuration + if reconcilable != nil { + err := reconcilable.Reconcile(network) + if err != nil { + return fmt.Errorf("failed to reconcile network %s: %w", networkName, err) + } return nil } - // setup & start the new network controller - nc, err := nm.ncm.NewNetworkController(util.CopyNetInfo(want)) + // otherwise setup & start the new network controller + nc, err := nm.ncm.NewNetworkController(util.CopyNetInfo(network)) if err != nil { - return fmt.Errorf("%s: failed to create network %s: %w", nm.name, network, err) + return fmt.Errorf("failed to create network %s: %w", networkName, err) } err = nc.Start(context.Background()) if err != nil { - return fmt.Errorf("%s: failed to start network %s: %w", nm.name, network, err) + return fmt.Errorf("failed to start network %s: %w", networkName, err) } - nm.networkControllers[network] = &networkControllerState{controller: nc} + nm.networkControllers[network.GetNetworkName()] = &networkControllerState{controller: nc} return nil } -func (nm *networkManagerImpl) syncAll() error { - nm.Lock() - defer nm.Unlock() - // as we sync upon start, consider networks that have not been ensured as - // stale and clean them up - validNetworks := make([]util.BasicNetInfo, 0, len(nm.networks)) - networkNames := make([]string, 0, len(nm.networks)) - for name, network := range nm.networks { - validNetworks = append(validNetworks, network) - networkNames = append(networkNames, name) +func (nm *networkManagerImpl) deleteNetwork(network string) error { + have := nm.networkControllers[network] + if have == nil { + return nil } - if err := nm.ncm.CleanupDeletedNetworks(validNetworks...); err != nil { - return err + + if !have.stoppedAndDeleting { + have.controller.Stop() } + have.stoppedAndDeleting = true - // sync all known networks. There is no informer for networks. Keys are added by NAD controller. - // Certain downstream controllers that handle configuration for multiple networks depend on being - // aware of all the existing networks on initialization. To achieve that, we need to start existing - // networks synchronously. Otherwise, these controllers might incorrectly assess valid configuration - // as stale. - start := time.Now() - klog.Infof("%s: syncing all networks", nm.name) - for _, networkName := range networkNames { - if err := nm.sync(networkName); err != nil { - return fmt.Errorf("failed to sync network %s: %w", networkName, err) + err := have.controller.Cleanup() + if err != nil { + return fmt.Errorf("%s: failed to cleanup network %s: %w", nm.name, network, err) + } + + delete(nm.networkControllers, network) + return nil +} + +func (nm *networkManagerImpl) setVRFs(network util.NetInfo) error { + if !network.IsDefault() && !network.IsPrimaryNetwork() { + return nil + } + if !nm.hasRouteAdvertisements() { + // we won't look after VRFs in cluster manager + return nil + } + + raNames := sets.New[string]() + for _, nadNamespacedName := range network.GetNADs() { + namespace, name, err := cache.SplitMetaNamespaceKey(nadNamespacedName) + if err != nil { + return err } + + nad, err := nm.nadLister.NetworkAttachmentDefinitions(namespace).Get(name) + if err != nil { + return err + } + + var nadRANames []string + if nad.Annotations[types.OvnRouteAdvertisementsKey] != "" { + err = json.Unmarshal([]byte(nad.Annotations[types.OvnRouteAdvertisementsKey]), &nadRANames) + if err != nil { + return err + } + } + + raNames.Insert(nadRANames...) } - klog.Infof("%s: finished syncing all networks. Time taken: %s", nm.name, time.Since(start)) + + vrfs := map[string][]string{} + for raName := range raNames { + ra, err := nm.raLister.Get(raName) + if err != nil { + return err + } + + if !ra.Spec.Advertisements.PodNetwork { + continue + } + + // TODO check RA status + + nodeSelector, err := metav1.LabelSelectorAsSelector(&ra.Spec.NodeSelector) + if err != nil { + return err + } + + nodes, err := nm.nodeLister.List(nodeSelector) + if err != nil { + return err + } + + for _, node := range nodes { + if node.Name == nm.node || util.GetNodeZone(node) == nm.zone { + vrfs[node.Name] = append(vrfs[node.Name], ra.Spec.TargetVRF) + } + } + } + + network.SetVRFs(vrfs) return nil } + +func (nm *networkManagerImpl) hasRouteAdvertisements() bool { + isClusterManager := nm.zone == "" && nm.node == "" + return config.OVNKubernetesFeature.EnableRouteAdvertisements && !isClusterManager +} + +func raNeedsUpdate(oldRA, newRA *ratypes.RouteAdvertisements) bool { + if oldRA == nil || newRA == nil { + // handle RA add/delete through the NAD annotation update + return false + } + + // don't process resync or objects that are marked for deletion + if oldRA.ResourceVersion == newRA.ResourceVersion || + !newRA.GetDeletionTimestamp().IsZero() { + return false + } + + return oldRA.Spec.TargetVRF != newRA.Spec.TargetVRF || !reflect.DeepEqual(oldRA.Spec.NodeSelector, newRA.Spec.NodeSelector) +} + +func nodeNeedsUpdate(oldNode, newNode *corev1.Node) bool { + if oldNode == nil || newNode == nil { + return true + } + + // don't process resync or objects that are marked for deletion + if oldNode.ResourceVersion == newNode.ResourceVersion || + !newNode.GetDeletionTimestamp().IsZero() { + return false + } + + return !reflect.DeepEqual(oldNode.Labels, newNode.Labels) || oldNode.Annotations[util.OvnNodeZoneName] != newNode.Annotations[util.OvnNodeZoneName] +} diff --git a/go-controller/pkg/network-attach-def-controller/network_manager_test.go b/go-controller/pkg/network-attach-def-controller/network_manager_test.go new file mode 100644 index 00000000000..acf5271bbc9 --- /dev/null +++ b/go-controller/pkg/network-attach-def-controller/network_manager_test.go @@ -0,0 +1,182 @@ +package networkAttachDefController + +import ( + "context" + "testing" + + "github.com/onsi/gomega" + + cnitypes "github.com/containernetworking/cni/pkg/types" + corev1 "k8s.io/api/core/v1" + v1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/client-go/tools/cache" + + ovncnitypes "github.com/ovn-org/ovn-kubernetes/go-controller/pkg/cni/types" + "github.com/ovn-org/ovn-kubernetes/go-controller/pkg/config" + ratypes "github.com/ovn-org/ovn-kubernetes/go-controller/pkg/crd/routeadvertisements/v1" + "github.com/ovn-org/ovn-kubernetes/go-controller/pkg/factory" + "github.com/ovn-org/ovn-kubernetes/go-controller/pkg/types" + "github.com/ovn-org/ovn-kubernetes/go-controller/pkg/util" +) + +func TestSetVRFs(t *testing.T) { + testZoneName := "testZone" + testNodeName := "testNode" + testNodeOnZoneName := "testNodeOnZone" + testNADName := "test/NAD" + testRAName := "testRA" + testVRFName := "testVRF" + + defaultNetwork := &ovncnitypes.NetConf{ + NetConf: cnitypes.NetConf{ + Name: types.DefaultNetworkName, + Type: "ovn-k8s-cni-overlay", + }, + MTU: 1400, + } + + podNetworkRA := &ratypes.RouteAdvertisements{ + ObjectMeta: v1.ObjectMeta{ + Name: testRAName, + }, + Spec: ratypes.RouteAdvertisementsSpec{ + TargetVRF: testVRFName, + Advertisements: ratypes.Advertisements{ + PodNetwork: true, + }, + }, + } + nonPodNetworkRA := &ratypes.RouteAdvertisements{ + ObjectMeta: v1.ObjectMeta{ + Name: testRAName, + }, + Spec: ratypes.RouteAdvertisementsSpec{ + TargetVRF: testVRFName, + }, + } + + testNode := corev1.Node{ + ObjectMeta: v1.ObjectMeta{ + Name: testNodeName, + }, + } + testNodeOnZone := corev1.Node{ + ObjectMeta: v1.ObjectMeta{ + Name: testNodeOnZoneName, + Annotations: map[string]string{ + util.OvnNodeZoneName: testZoneName, + }, + }, + } + otherNode := corev1.Node{ + ObjectMeta: v1.ObjectMeta{ + Name: "otherNode", + }, + } + + tests := []struct { + name string + network *ovncnitypes.NetConf + ra *ratypes.RouteAdvertisements + node corev1.Node + expected map[string][]string + }{ + { + name: "reconciles VRF for selected node of default node network controller", + network: defaultNetwork, + ra: podNetworkRA, + node: testNode, + expected: map[string][]string{ + testNodeName: {testVRFName}, + }, + }, + { + name: "reconciles VRF for selected node in same zone as default OVN network controller", + network: defaultNetwork, + ra: podNetworkRA, + node: testNodeOnZone, + expected: map[string][]string{ + testNodeOnZoneName: {testVRFName}, + }, + }, + { + name: "ignores a route advertisement that is not for the pod network", + network: defaultNetwork, + ra: nonPodNetworkRA, + node: testNode, + }, + { + name: "ignores a route advertisement that is not for applicable node", + network: defaultNetwork, + ra: podNetworkRA, + node: otherNode, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + g := gomega.NewWithT(t) + + config.OVNKubernetesFeature.EnableMultiNetwork = true + config.OVNKubernetesFeature.EnableRouteAdvertisements = true + fakeClient := util.GetOVNClientset().GetOVNKubeControllerClientset() + wf, err := factory.NewOVNKubeControllerWatchFactory(fakeClient) + g.Expect(err).ToNot(gomega.HaveOccurred()) + + tncm := &testNetworkControllerManager{ + controllers: map[string]NetworkController{}, + defaultNetwork: &testNetworkController{ + NetInfo: &util.DefaultNetInfo{}, + }, + } + nm := newNetworkManager("", testZoneName, testNodeName, tncm, wf) + + namespace, name, err := cache.SplitMetaNamespaceKey(testNADName) + g.Expect(err).ToNot(gomega.HaveOccurred()) + nad, err := buildNAD(name, namespace, tt.network) + g.Expect(err).ToNot(gomega.HaveOccurred()) + nad.Annotations = map[string]string{ + types.OvnRouteAdvertisementsKey: "[\"" + tt.ra.Name + "\"]", + } + + _, err = fakeClient.KubeClient.CoreV1().Nodes().Create(context.Background(), &tt.node, v1.CreateOptions{}) + g.Expect(err).ToNot(gomega.HaveOccurred()) + _, err = fakeClient.RouteAdvertisementsClient.K8sV1().RouteAdvertisements().Create(context.Background(), tt.ra, v1.CreateOptions{}) + g.Expect(err).ToNot(gomega.HaveOccurred()) + _, err = fakeClient.NetworkAttchDefClient.K8sCniCncfIoV1().NetworkAttachmentDefinitions(namespace).Create(context.Background(), nad, v1.CreateOptions{}) + g.Expect(err).ToNot(gomega.HaveOccurred()) + + err = wf.Start() + g.Expect(err).ToNot(gomega.HaveOccurred()) + defer wf.Shutdown() + g.Expect(nm.Start()).To(gomega.Succeed()) + defer nm.Stop() + + netInfo, err := util.NewNetInfo(tt.network) + g.Expect(err).ToNot(gomega.HaveOccurred()) + netInfo.AddNADs(testNADName) + + nm.EnsureNetwork(netInfo) + + meetsExpectations := func(g gomega.Gomega) { + tncm.Lock() + defer tncm.Unlock() + var reconcilable util.ReconcilableNetInfo + switch tt.network.Name { + case types.DefaultNetworkName: + reconcilable = tncm.GetDefaultNetworkController().(util.ReconcilableNetInfo) + default: + reconcilable = tncm.controllers[testNetworkKey(netInfo)] + } + + g.Expect(reconcilable).ToNot(gomega.BeNil()) + if tt.expected == nil { + tt.expected = map[string][]string{} + } + g.Expect(reconcilable.GetVRFs()).To(gomega.Equal(tt.expected)) + } + + g.Eventually(meetsExpectations).Should(gomega.Succeed()) + g.Consistently(meetsExpectations).Should(gomega.Succeed()) + }) + } +} diff --git a/go-controller/pkg/network-controller-manager/network_controller_manager.go b/go-controller/pkg/network-controller-manager/network_controller_manager.go index 800f81d56e8..e29244868ef 100644 --- a/go-controller/pkg/network-controller-manager/network_controller_manager.go +++ b/go-controller/pkg/network-controller-manager/network_controller_manager.go @@ -53,7 +53,7 @@ type NetworkControllerManager struct { defaultNetworkController nad.BaseNetworkController // net-attach-def controller handle net-attach-def and create/delete network controllers - nadController *nad.NetAttachDefinitionController + nadController *nad.NADController } func (cm *NetworkControllerManager) NewNetworkController(nInfo util.NetInfo) (nad.NetworkController, error) { @@ -209,12 +209,13 @@ func NewNetworkControllerManager(ovnClient *util.OVNClientset, wf *factory.Watch } var err error - if config.OVNKubernetesFeature.EnableMultiNetwork { - cm.nadController, err = nad.NewNetAttachDefinitionController("network-controller-manager", cm, wf, nil) + if config.OVNKubernetesFeature.EnableMultiNetwork || config.OVNKubernetesFeature.EnableRouteAdvertisements { + cm.nadController, err = nad.NewZoneNADController("network-controller-manager", config.Default.Zone, cm, wf) if err != nil { return nil, err } } + return cm, nil } @@ -406,14 +407,18 @@ func (cm *NetworkControllerManager) Start(ctx context.Context) error { if err != nil { return fmt.Errorf("failed to init default network controller: %v", err) } - err = cm.defaultNetworkController.Start(ctx) - if err != nil { - return fmt.Errorf("failed to start default network controller: %v", err) - } // nadController is nil if multi-network is disabled if cm.nadController != nil { - return cm.nadController.Start() + err = cm.nadController.Start() + if err != nil { + return fmt.Errorf("failed to start default network NAD controller: %v", err) + } + } + + err = cm.defaultNetworkController.Start(ctx) + if err != nil { + return fmt.Errorf("failed to start default network controller: %v", err) } return nil diff --git a/go-controller/pkg/network-controller-manager/node_network_controller_manager.go b/go-controller/pkg/network-controller-manager/node_network_controller_manager.go index 1ca52705e60..65bcfe78694 100644 --- a/go-controller/pkg/network-controller-manager/node_network_controller_manager.go +++ b/go-controller/pkg/network-controller-manager/node_network_controller_manager.go @@ -40,7 +40,7 @@ type nodeNetworkControllerManager struct { // net-attach-def controller handle net-attach-def and create/delete secondary controllers // nil in dpu-host mode - nadController *nad.NetAttachDefinitionController + nadController *nad.NADController // vrf manager that creates and manages vrfs for all UDNs vrfManager *vrfmanager.Controller // route manager that creates and manages routes @@ -110,6 +110,7 @@ func (ncm *nodeNetworkControllerManager) newCommonNetworkControllerInfo() *node. // (2) primary user defined networks is enabled (all modes) func isNodeNADControllerRequired() bool { return ((config.OVNKubernetesFeature.EnableMultiNetwork && config.OvnKubeNode.Mode == ovntypes.NodeModeDPU) || + (config.OVNKubernetesFeature.EnableRouteAdvertisements && config.OvnKubeNode.Mode == ovntypes.NodeModeFull) || util.IsNetworkSegmentationSupportEnabled()) } @@ -131,7 +132,7 @@ func NewNodeNetworkControllerManager(ovnClient *util.OVNClientset, wf factory.No // need to start NAD controller on node side for programming gateway pieces for UDNs var err error if isNodeNADControllerRequired() { - ncm.nadController, err = nad.NewNetAttachDefinitionController("node-network-controller-manager", ncm, wf, nil) + ncm.nadController, err = nad.NewNodeNADController("node-network-controller-manager", name, ncm, wf) } if util.IsNetworkSegmentationSupportEnabled() { ncm.vrfManager = vrfmanager.NewController(ncm.routeManager) @@ -140,6 +141,7 @@ func NewNodeNetworkControllerManager(ovnClient *util.OVNClientset, wf factory.No if err != nil { return nil, err } + return ncm, nil } @@ -198,18 +200,20 @@ func (ncm *nodeNetworkControllerManager) Start(ctx context.Context) (err error) if err != nil { return fmt.Errorf("failed to init default node network controller: %v", err) } - err = ncm.defaultNodeNetworkController.Start(ctx) - if err != nil { - return fmt.Errorf("failed to start default node network controller: %v", err) - } // nadController is nil if multi-network is disabled if ncm.nadController != nil { err = ncm.nadController.Start() if err != nil { - return fmt.Errorf("failed to start NAD controller: %w", err) + return fmt.Errorf("failed to start default node network NAD controller: %v", err) } } + + err = ncm.defaultNodeNetworkController.Start(ctx) + if err != nil { + return fmt.Errorf("failed to start default node network controller: %v", err) + } + if ncm.vrfManager != nil { // Let's create VRF manager that will manage VRFs for all UDNs err = ncm.vrfManager.Run(ncm.stopChan, ncm.wg)