9801202042

Committed 04 Jul 2024 08:45AM UTC coverage: 52.658% (+0.04%) from 52.62%

Build # 9801202042

Build Type

push

github

Committed by

jcaamano

Commit Message

Refactor NAD controller: Do not attach to default network

Default network does add NADs.
This is a follow up from PR https://github.com/ovn-org/ovn-kubernetes/pull/4461.

This change addresses the following panic:

Image built from ovn-kubernetes ref: refs/heads/master  commit: 0a6853062

I0703 14:17:52.721946      52 shared_informer.go:311] Waiting for caches to sync for [cluster-manager NAD controller]
I0703 14:17:52.721970      52 shared_informer.go:318] Caches are synced for [cluster-manager NAD controller]
panic: unexpected call for default network

goroutine 89 [running]:
github.com/ovn-org/ovn-kubernetes/go-controller/pkg/util.(*DefaultNetInfo).AddNADs(0x205a120?, {0xc0005d6d50?, 0x23f9ce2?, 0x7?})
    /builds/nithyar/ovn_upstream_validation/ovn-kubernetes/go-controller/pkg/util/multi_network.go:148 +0x25

github.com/ovn-org/ovn-kubernetes/go-controller/pkg/network-attach-def-controller.(*NetAttachDefinitionController).syncNAD(0xc0004c1630, {0xc000e3ae40, 0x13}, 0x43e02e?)
    /builds/nithyar/ovn_upstream_validation/ovn-kubernetes/go-controller/pkg/network-attach-def-controller/network_attach_def_controller.go:243 +0x67b

github.com/ovn-org/ovn-kubernetes/go-controller/pkg/network-attach-def-controller.(*NetAttachDefinitionController).syncAll(0xc0004c1630)
    /builds/nithyar/ovn_upstream_validation/ovn-kubernetes/go-controller/pkg/network-attach-def-controller/network_attach_def_controller.go:139 +0x2ba

github.com/ovn-org/ovn-kubernetes/go-controller/pkg/controller.StartWithInitialSync(0xc000e03e08, {0xc00050bdf8, 0x1, 0x0?})
    /builds/nithyar/ovn_upstream_validation/ovn-kubernetes/go-controller/pkg/controller/controller.go:293 +0x18e

github.com/ovn-org/ovn-kubernetes/go-controller/pkg/network-attach-def-controller.(*NetAttachDefinitionController).Start(0xc0004c1630)
    /builds/nithyar/ovn_upstream_validation/ovn-kubernetes/go-controller/pkg/network-attach-def-controller/network_attach_def_controller.go:100... (continued)

Run Details

1 of 4 new or added lines in 1 file covered. (25.0%)

11 existing lines in 3 files now uncovered.

28953 of 54983 relevant lines covered (52.66%)

99.63 hits per line

Source File
Press 'n' to go to next uncovered line, 'b' for previous

39.58

/go-controller/pkg/network-attach-def-controller/network_attach_def_controller.go

package networkAttachDefController

import (
        "context"
        "errors"
        "fmt"
        "reflect"
        "time"

        apierrors "k8s.io/apimachinery/pkg/api/errors"
        "k8s.io/apimachinery/pkg/labels"
        "k8s.io/apimachinery/pkg/util/sets"
        "k8s.io/client-go/tools/cache"
        "k8s.io/client-go/util/workqueue"
        "k8s.io/klog/v2"

        nettypes "github.com/k8snetworkplumbingwg/network-attachment-definition-client/pkg/apis/k8s.cni.cncf.io/v1"
        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"
        "github.com/ovn-org/ovn-kubernetes/go-controller/pkg/util"
)

var ErrNetworkControllerTopologyNotManaged = errors.New("no cluster network controller to manage topology")

type BaseNetworkController interface {
        Start(ctx context.Context) error
        Stop()
}

type NetworkController interface {
        BaseNetworkController
        util.NetInfo
        // Cleanup cleans up the NetworkController-owned resources, it could be called to clean up network controllers that are deleted when
        // ovn-k8s is down; so it's receiver could be a dummy network controller, it just needs to know its network name.
        Cleanup() error
}

// NetworkControllerManager manages all network controllers
type NetworkControllerManager interface {
        NewNetworkController(netInfo util.NetInfo) (NetworkController, error)
        CleanupDeletedNetworks(allControllers []NetworkController) error
}

type watchFactory interface {
        NADInformer() nadinformers.NetworkAttachmentDefinitionInformer
}

// NetAttachDefinitionController 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 {
        name               string
        netAttachDefLister nadlisters.NetworkAttachmentDefinitionLister
        controller         controller.Controller

        // networkManager is used to manage the network controllers
        networkManager networkManager

        networks map[string]util.NetInfo

        // nads to network mapping
        nads map[string]string
}

func NewNetAttachDefinitionController(
        name string,
        ncm NetworkControllerManager,
        wf watchFactory,
) (*NetAttachDefinitionController, error) {
        nadInformer := wf.NADInformer()
        nadController := &NetAttachDefinitionController{
                name:               fmt.Sprintf("[%s NAD controller]", name),
                netAttachDefLister: nadInformer.Lister(),
                networkManager:     newNetworkManager(name, ncm),
                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,
                ObjNeedsUpdate: nadNeedsUpdate,
                // this controller is not thread safe
                Threadiness: 1,
        }
        nadController.controller = controller.NewController(
                nadController.name,
                config,
        )

        return nadController, nil
}

func (nadController *NetAttachDefinitionController) Start() error {
        err := controller.StartWithInitialSync(
                nadController.syncAll,
                nadController.controller,
        )
        if err != nil {
                return err
        }

        err = nadController.networkManager.Start()
        if err != nil {
                return err
        }

        klog.Infof("%s: started", nadController.name)
        return nil
}

func (nadController *NetAttachDefinitionController) Stop() {
        klog.Infof("%s: shutting down", nadController.name)
        controller.Stop(nadController.controller)
        nadController.networkManager.Stop()
}

func (nadController *NetAttachDefinitionController) syncAll() (err error) {
        existingNADs, err := nadController.netAttachDefLister.List(labels.Everything())
        if err != nil {
                return fmt.Errorf("%s: failed to list NADs: %w", nadController.name, err)
        }

        // create all networks with their updated list of NADs and only then start
        // the corresponding controllers so as to avoid to the extent possible the
        // errors and retries that would result if the controller attempted to
        // process pods attached with NADs we wouldn't otherwise know about yet
        for _, nad := range existingNADs {
                key, err := cache.MetaNamespaceKeyFunc(nad)
                if err != nil {
                        klog.Errorf("%s: failed to sync %v: %v", nadController.name, nad, err)
                        continue
                }
                err = nadController.syncNAD(key, nad)
                if err != nil {
                        return fmt.Errorf("%s: failed to sync %s: %v", nadController.name, key, err)
                }
        }

        return nil
}

func (nadController *NetAttachDefinitionController) sync(key string) error {
        startTime := time.Now()
        klog.V(5).Infof("%s: sync NAD %s", nadController.name, key)
        defer func() {
                klog.V(4).Infof("%s: finished syncing NAD %s, took %v", nadController.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)
                return nil
        }

        nad, err := nadController.netAttachDefLister.NetworkAttachmentDefinitions(namespace).Get(name)
        if err != nil && !apierrors.IsNotFound(err) {
                return err
        }

        return nadController.syncNAD(key, nad)
}

func (nadController *NetAttachDefinitionController) syncNAD(key string, nad *nettypes.NetworkAttachmentDefinition) error {
        var nadNetworkName string
        var nadNetwork, oldNetwork, ensureNetwork util.NetInfo
        var err error

        if nad != nil {
                nadNetwork, err = util.ParseNADInfo(nad)
                if err != nil {
                        klog.Errorf("%s: failed parsing NAD %s: %v", nadController.name, key, err)
                        return nil
                }
                nadNetworkName = nadNetwork.GetNetworkName()
        }

        // As multiple NADs may define networks with the same name, these networks
        // should also have the same config to be considered compatible. If an
        // incompatible network change happens on NAD update, we can:
        // - Re-create network with the same name but different config, if it is not
        //   referred to by any other NAD
        // - 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]]
        }

        currentNetwork := nadController.networks[nadNetworkName]

        switch {
        case currentNetwork == nil:
                // the NAD refers to a new network, ensure it
                ensureNetwork = nadNetwork
                nadController.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
                ensureNetwork = currentNetwork
        case sets.New(key).HasAll(currentNetwork.GetNADs()...):
                // the NAD is the only NAD referring to an existing incompatible
                // network, remove the reference from the old network and ensure that
                // existing network holds a reference to this NAD
                oldNetwork = currentNetwork
                ensureNetwork = nadNetwork
        // the NAD refers to an existing incompatible network referred by other
        // NADs, return error
        case oldNetwork == nil:
                // the NAD refers to the same network as before but with different
                // incompatible configuration, remove the NAD reference from the network
                oldNetwork = currentNetwork
                fallthrough
        default:
                err = fmt.Errorf("%s: NAD %s CNI config does not match that of network %s", nadController.name, key, nadNetworkName)
        }

        // remove the NAD reference from the old network and delete the network if
        // it is no longer referenced
        if oldNetwork != nil {
                oldNetworkName := oldNetwork.GetNetworkName()
                oldNetwork.DeleteNADs(key)
                if len(oldNetwork.GetNADs()) == 0 {
                        nadController.networkManager.DeleteNetwork(oldNetworkName)
                        delete(nadController.networks, oldNetworkName)
                } else {
                        nadController.networkManager.EnsureNetwork(oldNetwork)
                }
        }

        // this was a nad delete
        if ensureNetwork == nil {
                delete(nadController.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
        ensureNetwork.AddNADs(key)
        nadController.nads[key] = ensureNetwork.GetNetworkName()
        nadController.networkManager.EnsureNetwork(ensureNetwork)
        return err
}

func nadNeedsUpdate(oldNAD, newNAD *nettypes.NetworkAttachmentDefinition) bool {
        if oldNAD == nil || newNAD == nil {
                return true
        }

        // don't process resync or objects that are marked for deletion
        if oldNAD.ResourceVersion == newNAD.ResourceVersion ||
                !newNAD.GetDeletionTimestamp().IsZero() {
                return false
        }

        return !reflect.DeepEqual(oldNAD.Spec, newNAD.Spec)
}

1	package networkAttachDefController
2
3	import (
4	"context"
5	"errors"
6	"fmt"
7	"reflect"
8	"time"
9
10	apierrors "k8s.io/apimachinery/pkg/api/errors"
11	"k8s.io/apimachinery/pkg/labels"
12	"k8s.io/apimachinery/pkg/util/sets"
13	"k8s.io/client-go/tools/cache"
14	"k8s.io/client-go/util/workqueue"
15	"k8s.io/klog/v2"
16
17	nettypes "github.com/k8snetworkplumbingwg/network-attachment-definition-client/pkg/apis/k8s.cni.cncf.io/v1"
18	nadinformers "github.com/k8snetworkplumbingwg/network-attachment-definition-client/pkg/client/informers/externalversions/k8s.cni.cncf.io/v1"
19	nadlisters "github.com/k8snetworkplumbingwg/network-attachment-definition-client/pkg/client/listers/k8s.cni.cncf.io/v1"
20	"github.com/ovn-org/ovn-kubernetes/go-controller/pkg/controller"
21	"github.com/ovn-org/ovn-kubernetes/go-controller/pkg/util"
22	)
23
24	var ErrNetworkControllerTopologyNotManaged = errors.New("no cluster network controller to manage topology")
25
26	type BaseNetworkController interface {
27	Start(ctx context.Context) error
28	Stop()
29	}
30
31	type NetworkController interface {
32	BaseNetworkController
33	util.NetInfo
34	// Cleanup cleans up the NetworkController-owned resources, it could be called to clean up network controllers that are deleted when
35	// ovn-k8s is down; so it's receiver could be a dummy network controller, it just needs to know its network name.
36	Cleanup() error
37	}
38
39	// NetworkControllerManager manages all network controllers
40	type NetworkControllerManager interface {
41	NewNetworkController(netInfo util.NetInfo) (NetworkController, error)
42	CleanupDeletedNetworks(allControllers []NetworkController) error
43	}
44
45	type watchFactory interface {
46	NADInformer() nadinformers.NetworkAttachmentDefinitionInformer
47	}
48
49	// NetAttachDefinitionController handles namespaced scoped NAD events and
50	// manages cluster scoped networks defined in those NADs. NADs are mostly
51	// referred from pods to give them access to the network. Different NADs can
52	// define the same network as long as those definitions are actually equal.
53	// Unexpected situations are handled on best effort basis but improper NAD
54	// adminstration can lead to undefined behavior in referred from running pods.
55	type NetAttachDefinitionController struct {
56	name string
57	netAttachDefLister nadlisters.NetworkAttachmentDefinitionLister
58	controller controller.Controller
59
60	// networkManager is used to manage the network controllers
61	networkManager networkManager
62
63	networks map[string]util.NetInfo
64
65	// nads to network mapping
66	nads map[string]string
67	}
68
69	func NewNetAttachDefinitionController(
70	name string,
71	ncm NetworkControllerManager,
72	wf watchFactory,
73	) (*NetAttachDefinitionController, error) {	×
74	nadInformer := wf.NADInformer()	×
75	nadController := &NetAttachDefinitionController{	×
76	name: fmt.Sprintf("[%s NAD controller]", name),	×
77	netAttachDefLister: nadInformer.Lister(),	×
78	networkManager: newNetworkManager(name, ncm),	×
79	networks: map[string]util.NetInfo{},	×
80	nads: map[string]string{},	×
81	}	×
82	config := &controller.ControllerConfig[nettypes.NetworkAttachmentDefinition]{	×
83	RateLimiter: workqueue.DefaultControllerRateLimiter(),	×
84	Informer: nadInformer.Informer(),	×
85	Lister: nadController.netAttachDefLister.List,	×
86	Reconcile: nadController.sync,	×
87	ObjNeedsUpdate: nadNeedsUpdate,	×
88	// this controller is not thread safe	×
89	Threadiness: 1,	×
90	}	×
91	nadController.controller = controller.NewController(	×
92	nadController.name,	×
93	config,	×
94	)	×
95		×
96	return nadController, nil	×
97	}	×
98
99	func (nadController *NetAttachDefinitionController) Start() error {	×
100	err := controller.StartWithInitialSync(	×
101	nadController.syncAll,	×
102	nadController.controller,	×
103	)	×
104	if err != nil {	×
105	return err	×
106	}	×
107
108	err = nadController.networkManager.Start()	×
109	if err != nil {	×
110	return err	×
111	}	×
112
113	klog.Infof("%s: started", nadController.name)	×
114	return nil	×
115	}
116
117	func (nadController *NetAttachDefinitionController) Stop() {	×
118	klog.Infof("%s: shutting down", nadController.name)	×
119	controller.Stop(nadController.controller)	×
120	nadController.networkManager.Stop()	×
121	}	×
122
123	func (nadController *NetAttachDefinitionController) syncAll() (err error) {	×
124	existingNADs, err := nadController.netAttachDefLister.List(labels.Everything())	×
125	if err != nil {	×
126	return fmt.Errorf("%s: failed to list NADs: %w", nadController.name, err)	×
127	}	×
128
129	// create all networks with their updated list of NADs and only then start
130	// the corresponding controllers so as to avoid to the extent possible the
131	// errors and retries that would result if the controller attempted to
132	// process pods attached with NADs we wouldn't otherwise know about yet
133	for _, nad := range existingNADs {	×
134	key, err := cache.MetaNamespaceKeyFunc(nad)	×
135	if err != nil {	×
136	klog.Errorf("%s: failed to sync %v: %v", nadController.name, nad, err)	×
137	continue	×
138	}
139	err = nadController.syncNAD(key, nad)	×
140	if err != nil {	×
141	return fmt.Errorf("%s: failed to sync %s: %v", nadController.name, key, err)	×
142	}	×
143	}
144
145	return nil	×
146	}
147
148	func (nadController *NetAttachDefinitionController) sync(key string) error {	×
149	startTime := time.Now()	×
150	klog.V(5).Infof("%s: sync NAD %s", nadController.name, key)	×
151	defer func() {	×
152	klog.V(4).Infof("%s: finished syncing NAD %s, took %v", nadController.name, key, time.Since(startTime))	×
153	}()	×
154
155	namespace, name, err := cache.SplitMetaNamespaceKey(key)	×
156	if err != nil {	×
157	klog.Errorf("%s: failed splitting key %s: %v", nadController.name, key, err)	×
158	return nil	×
159	}	×
160
161	nad, err := nadController.netAttachDefLister.NetworkAttachmentDefinitions(namespace).Get(name)	×
162	if err != nil && !apierrors.IsNotFound(err) {	×
163	return err	×
164	}	×
165
166	return nadController.syncNAD(key, nad)	×
167	}
168
169	func (nadController NetAttachDefinitionController) syncNAD(key string, nad nettypes.NetworkAttachmentDefinition) error {	28✔
170	var nadNetworkName string	28✔
171	var nadNetwork, oldNetwork, ensureNetwork util.NetInfo	28✔
172	var err error	28✔
173		28✔
174	if nad != nil {	52✔
175	nadNetwork, err = util.ParseNADInfo(nad)	24✔
176	if err != nil {	25✔
177	klog.Errorf("%s: failed parsing NAD %s: %v", nadController.name, key, err)	1✔
178	return nil	1✔
179	}	1✔
180	nadNetworkName = nadNetwork.GetNetworkName()	23✔
181	}
182
183	// As multiple NADs may define networks with the same name, these networks
184	// should also have the same config to be considered compatible. If an
185	// incompatible network change happens on NAD update, we can:
186	// - Re-create network with the same name but different config, if it is not
187	// referred to by any other NAD
188	// - Return an error AND clean up NAD from the old network
189
190	// the NAD refers to a different network than before
191	if nadNetworkName != nadController.nads[key] {	52✔
192	oldNetwork = nadController.networks[nadController.nads[key]]	25✔
193	}	25✔
194
195	currentNetwork := nadController.networks[nadNetworkName]	27✔
196		27✔
197	switch {	27✔
198	case currentNetwork == nil:	18✔
199	// the NAD refers to a new network, ensure it	18✔
200	ensureNetwork = nadNetwork	18✔
201	nadController.networks[nadNetworkName] = ensureNetwork	18✔
202	case currentNetwork.Equals(nadNetwork):	6✔
203	// the NAD refers to an existing compatible network, ensure that	6✔
204	// existing network holds a reference to this NAD	6✔
205	ensureNetwork = currentNetwork	6✔
206	case sets.New(key).HasAll(currentNetwork.GetNADs()...):	1✔
207	// the NAD is the only NAD referring to an existing incompatible	1✔
208	// network, remove the reference from the old network and ensure that	1✔
209	// existing network holds a reference to this NAD	1✔
210	oldNetwork = currentNetwork	1✔
211	ensureNetwork = nadNetwork	1✔
212	// the NAD refers to an existing incompatible network referred by other
213	// NADs, return error
214	case oldNetwork == nil:	2✔
215	// the NAD refers to the same network as before but with different	2✔
216	// incompatible configuration, remove the NAD reference from the network	2✔
217	oldNetwork = currentNetwork	2✔
218	fallthrough	2✔
219	default:	2✔
220	err = fmt.Errorf("%s: NAD %s CNI config does not match that of network %s", nadController.name, key, nadNetworkName)	2✔
221	}
222
223	// remove the NAD reference from the old network and delete the network if
224	// it is no longer referenced
225	if oldNetwork != nil {	37✔
226	oldNetworkName := oldNetwork.GetNetworkName()	10✔
227	oldNetwork.DeleteNADs(key)	10✔
228	if len(oldNetwork.GetNADs()) == 0 {	15✔
229	nadController.networkManager.DeleteNetwork(oldNetworkName)	5✔
230	delete(nadController.networks, oldNetworkName)	5✔
231	} else {	10✔
232	nadController.networkManager.EnsureNetwork(oldNetwork)	5✔
233	}	5✔
234	}
235
236	// this was a nad delete
237	if ensureNetwork == nil {	33✔
238	delete(nadController.nads, key)	6✔
239	return err	6✔
240	}	6✔
241
242	if ensureNetwork.IsDefault() {	21✔
NEW 243	klog.V(5).Infof("%s: NAD add for default network (key %s), skip it", nadController.name, key)	×
NEW 244	return nil	×
NEW 245	}	×
246
247	// ensure the network associated with the NAD
248	ensureNetwork.AddNADs(key)	21✔
249	nadController.nads[key] = ensureNetwork.GetNetworkName()	21✔
250	nadController.networkManager.EnsureNetwork(ensureNetwork)	21✔
251	return err	21✔
252	}
253
254	func nadNeedsUpdate(oldNAD, newNAD *nettypes.NetworkAttachmentDefinition) bool {	×
255	if oldNAD == nil \|\| newNAD == nil {	×
256	return true	×
257	}	×
258
259	// don't process resync or objects that are marked for deletion
260	if oldNAD.ResourceVersion == newNAD.ResourceVersion \|\|	×
261	!newNAD.GetDeletionTimestamp().IsZero() {	×
262	return false	×
263	}	×
264
265	return !reflect.DeepEqual(oldNAD.Spec, newNAD.Spec)	×
266	}

ovn-org / ovn-kubernetes / 9801202042

Source File Press 'n' to go to next uncovered line, 'b' for previous

Source File
Press 'n' to go to next uncovered line, 'b' for previous