Node IP handler: Fix (include) keepalived VIPs by martinkennelly · Pull Request #4168 · ovn-kubernetes/ovn-kubernetes

martinkennelly · 2024-02-20T09:43:55Z

#4040 introduced a constraint to filter VIPs. Remove that.
Also remove the external call to get these addresses.
Previously, we needed to use an external pkg func call because we wanted to filter EIPs but that is no longer needed so get the IP addrs locally and filter the addresses locally, and dont do it in two places which is confusing.

coveralls · 2024-02-20T10:08:29Z

coverage: 52.017% (+0.03%) from 51.984%
when pulling 327f353 on martinkennelly:fix-keepalived-vips
into 23c8e0c on ovn-org:master.

tssurya

thanks for the commit splits! made review easier.
reviewed all commits except 6th, will come back to that with fresh mind tomorrow

tssurya · 2024-02-26T12:14:20Z

+	// address but only later update kubelet configuration and restart kubelet (which in turn will update the reported
+	// IP address inside the node's status field).
+	nodeInformer := c.watchFactory.NodeInformer()
+	_, err := nodeInformer.AddEventHandler(cache.ResourceEventHandlerFuncs{


nit: c.watchFactory.NodeInformer().AddEventHandler(cache.ResourceEventHandlerFuncs{... ? I find it weird we are initializing a new variable with the informer cache? Is it intentional because we don't want to change the watchFactory node informer?

I find it weird we are initializing a new variable with the informer cache?

Not sure I am following you. var nodeInformer is just retrieving the existing node informer that was init by factory.

func (wf *WatchFactory) NodeInformer() cache.SharedIndexInformer { return wf.informers[NodeType].inf }

yeah no nothing major my comment was simply: c.watchFactory.NodeInformer().AddEventHandler( why don't we just do this in one go like that without assigning it to a variable first and if we had a reason to have a variable in the first place.. nw, its a nit..

ya - its a bit needless to do that. I just copy, pasted.

tssurya · 2024-02-26T12:17:38Z

+	}()
 }

 // runInternal can be used by testcases to provide a fake subscription function


its not just used by testcases right? we probably wanna update the func desc to be more informative

tssurya · 2024-02-26T12:43:37Z

+	})
+
+	Context("valid addresses", func() {
+		It("allows keepalived VIP", func() {


can you make sure we add a unit test for:

link local addresses, OVN reserved IPs, and addresses marked as secondary or deprecated.

and double check to see these are not filtered at this stage?
if I am asking for too much and perhaps most of these are in the same line as what you have added, push back please but since you already have all the plumbing in place I was hoping this will be easier

Link local addresses would be filtered - but uniq link local addresses are not and I added a test for that.

ack thanks! and perhaps also for masquerade IPs (with combo of V6 addresses as well for robustness)

tssurya · 2024-02-26T12:46:06Z

ack based on your commit message, if you feel we need to have a cleanup done or consolidate things please open an upstream issue (perhaps we can try to mark that as "good-first-issue" or something)

tssurya · 2024-02-26T12:52:47Z

+	fakeBridgeConfiguration := &bridgeConfiguration{bridgeName: "breth0"}
+
+	k := &kube.Kube{KClient: tc.fakeClient}
+	if createNs {


so I would just make this another util:
configureKubeOVNContextWithNs which calls configureKubeOVNContext and then adds the extra plumbing you need like replace the ipManager or whatever..
then use configureKubeOVNContext from the old test and configureKubeOVNContextWithNs from your new test in next commit

Ya, thats cleaner. Thanks!

tssurya · 2024-02-26T12:53:25Z

+			Expect(setupPrimaryInfFn()).ShouldNot(HaveOccurred())
+		}
+	}
+	//FIXME (mk): newAddressManagerInternal calls a sync - when this is removed, remove the need to call that func within NS


I don't understand this comment very well :( I'll come back to this tomorrow. (too much "this" and "that" for my empty stomach)

I explained on slack but for anyone else - a call to newAddressManagerInternal performs the following commands:

// reproducibility of unit tests. func newAddressManagerInternal(nodeName string, k kube.Interface, config *managementPortConfig, watchFactory factory.NodeWatchFactory, gwBridge *bridgeConfiguration, useNetlink bool) *addressManager { mgr := &addressManager{ nodeName: nodeName, watchFactory: watchFactory, cidrs: sets.New[string](), mgmtPortConfig: config, gatewayBridge: gwBridge, OnChanged: func() {}, useNetlink: useNetlink, syncPeriod: 30 * time.Second, } mgr.nodeAnnotator = kube.NewNodeAnnotator(k, nodeName) mgr.sync() <------Notice return mgr }

Notice the sync() call. Therefore, to ensure sync() exec in the correct netns, we must call newAddressManagerInternal in the correct netns.

thanks Mairt! makes sense,

tssurya

reviewed all commits, logic lgtm
so once you push with:
#4168 (comment)
#4168 (comment)
#4168 (comment)
rebased on master
we can merge this

thank you Martin! ++ cookies for the tests

tssurya · 2024-02-28T10:09:11Z

 	addrSubscribeOptions := netlink.AddrSubscribeOptions{
 		ErrorCallback: func(err error) {
 			klog.Errorf("Failed during AddrSubscribe callback: %v", err)
 			// Note: Not calling sync() from here: it is redudant and unsafe when stopChan is closed.


Suggested change

// Note: Not calling sync() from here: it is redudant and unsafe when stopChan is closed.

// Note: Not calling sync() from here: it is redundant and unsafe when stopChan is closed.

tssurya · 2024-02-28T10:15:04Z

 	doneWg.Add(1)
 	go func() {
-		c.runInternal(stopChan, subscribe)
+		c.runInternal(stopChan, c.getNetlinkAddrSubFunc(stopChan))


self-note: the same channel is passed into runInternal and into addrSubscribeAt
the former simply returns if stop is called and latter has a goroutine which listens to that channel and then calls socket close..

Previous changes in PR ovn-kubernetes#4040 added a new constraint to not include keepalived vips in an external func even though we filtered node IPs within node IP handler. Remove the external func which filters addresses and keep the filtering logic within node ip handler code. Signed-off-by: Martin Kennelly <mkennell@redhat.com>

…ternal Do this in-order to allow follow on work to allow func 'runInternal' to be exec directly with 'go runInternal()' Signed-off-by: Martin Kennelly <mkennell@redhat.com>

Because it is harder to test this func when we spawn the go routine within our main logic func for node IP handler. No logic change within runInternal. Signed-off-by: Martin Kennelly <mkennell@redhat.com>

No func change. Signed-off-by: Martin Kennelly <mkennell@redhat.com>

..to allow new tests to consume Signed-off-by: Martin Kennelly <mkennell@redhat.com>

For testing, we do not want to call run directly because it will spawn a goroutine which can escape any network namespace in which it is called. Seperate it into its own func so it can be called during testing and pinned to a net namespace. Signed-off-by: Martin Kennelly <mkennell@redhat.com>

This test case was difficult to add to existing test cases because they are focused on netlink addr update events and ignored the sync func. This commit creates a new test cases that test events and sync func. Decided to use a new netns instead of mocking as it seems simpler and easier to extend however there maybe overlap with existing test cases. Signed-off-by: Martin Kennelly <mkennell@redhat.com>

…N reserved IPs Signed-off-by: Martin Kennelly <mkennell@redhat.com>

Otherwise, we get exception printed out in logs. Signed-off-by: Martin Kennelly <mkennell@redhat.com>

tssurya

the tests soo nice! thanks Martin
change lgtm (checked only the new changes/bits in 3rd iteration).
will merge once CI is green.

tssurya · 2024-02-28T11:27:20Z

 		tc.doneWg.Wait()
 		tc.watchFactory.Shutdown()
 		close(tc.addrChan)
+		util.ResetRunner()


oh nice, learnt something new today.

tssurya · 2024-02-28T11:28:48Z

+	useNetlink := true
+	var tc *testCtx
+	testNs.Do(func(netNS ns.NetNS) error {
+		tc = configureKubeOVNContext(nodeName, useNetlink)


martinkennelly · 2024-02-28T11:37:32Z

TC allows unique local address passed locally for me.. checking why it may fail.
Edit: ipv6 is disabled so in sync() it wipes out the added address... Seems like a problem that addr "event" handler doesnt respect the enabled IP family.

Some of the tcs require v6. Unable to switch this support on per tc, because we currently start the node ip handler before running each test case and it inspects this IP support before a tc is ever executed. Enable for all tcs. Signed-off-by: Martin Kennelly <mkennell@redhat.com>

tssurya

/lgtm

martinkennelly requested review from dcbw, girishmg, jcaamano, trozet and tssurya as code owners February 20, 2024 09:43

tssurya self-assigned this Feb 20, 2024

tssurya reviewed Feb 26, 2024

View reviewed changes

andfasano mentioned this pull request Feb 27, 2024

AGENT-853: Add ClusterInfo asset openshift/installer#7997

Merged

tssurya mentioned this pull request Feb 28, 2024

addressManager and linkManager sync functions are not having retry mechanisms for transient errors #4189

Closed

tssurya reviewed Feb 28, 2024

View reviewed changes

martinkennelly added 9 commits February 28, 2024 11:20

Node IP handler: extract handling node primary addr change from runIn…

9e5b429

…ternal Do this in-order to allow follow on work to allow func 'runInternal' to be exec directly with 'go runInternal()' Signed-off-by: Martin Kennelly <mkennell@redhat.com>

Node IP handler: extract spawning go routine from within internal func

b9bc73d

Because it is harder to test this func when we spawn the go routine within our main logic func for node IP handler. No logic change within runInternal. Signed-off-by: Martin Kennelly <mkennell@redhat.com>

Node IP handler: allow sync period to be customizable for testing

ebb2665

No func change. Signed-off-by: Martin Kennelly <mkennell@redhat.com>

Node IP handler: setup test env in a func

ee35bba

..to allow new tests to consume Signed-off-by: Martin Kennelly <mkennell@redhat.com>

Node IP handler: add test cases for uniq link local, secondary and OV…

49be374

…N reserved IPs Signed-off-by: Martin Kennelly <mkennell@redhat.com>

Node IP handler: fix fake ovs ctl calls

6402f56

Otherwise, we get exception printed out in logs. Signed-off-by: Martin Kennelly <mkennell@redhat.com>

martinkennelly force-pushed the fix-keepalived-vips branch from 327f353 to 6402f56 Compare February 28, 2024 11:20

tssurya previously approved these changes Feb 28, 2024

View reviewed changes

martinkennelly dismissed tssurya’s stale review via f34c07f February 28, 2024 11:51

martinkennelly force-pushed the fix-keepalived-vips branch from f34c07f to bd82594 Compare February 28, 2024 11:52

martinkennelly force-pushed the fix-keepalived-vips branch from bd82594 to 7a9819b Compare February 28, 2024 14:42

tssurya approved these changes Feb 28, 2024

View reviewed changes

patrickdillon mentioned this pull request Feb 28, 2024

SPLAT-1460: Make vSphere default ResourcePool formatting not contain double slash. openshift/installer#8044

Merged

tssurya merged commit db63899 into ovn-kubernetes:master Feb 28, 2024

andfasano mentioned this pull request Feb 29, 2024

OCPBUGS-29350,SDN-4542,OCPBUGS-29389: Downstream Merge 29th Feb 2024 openshift/ovn-kubernetes#2089

Merged

rwsu mentioned this pull request Mar 4, 2024

AGENT-724: Deploy vSphere clusters with credentials openshift-metal3/dev-scripts#1599

Merged

tssurya added the kind/bug All issues that are bugs and PRs opened to fix bugs label Mar 12, 2024

tssurya added this to the v1.0.0 milestone Mar 12, 2024

	// Note: Not calling sync() from here: it is redudant and unsafe when stopChan is closed.
	// Note: Not calling sync() from here: it is redundant and unsafe when stopChan is closed.

Conversation

martinkennelly commented Feb 20, 2024

Uh oh!

coveralls commented Feb 20, 2024

Uh oh!

tssurya left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tssurya Feb 28, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tssurya Feb 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tssurya left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tssurya left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

martinkennelly commented Feb 28, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tssurya left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

tssurya Feb 28, 2024 •

edited

Loading

tssurya Feb 26, 2024 •

edited

Loading

tssurya left a comment •

edited

Loading

martinkennelly commented Feb 28, 2024 •

edited

Loading