DHCP lease maintenance should terminate when interface no longer exists. by dougbtv · Pull Request #1143 · containernetworking/plugins

dougbtv · 2025-01-21T16:11:15Z

Due to oberservations that threads can grow and the dhcp daemon uses an increasing amount of memory.

This situation can happen organically when using say, bridge CNI, and the bridge has been removed outside of the bridge CNI lifecycle, and an interface no longer exists on a pod.

Also, for reproduction steps (admittedly, openshift biased), see my steps in https://gist.github.com/dougbtv/803316016c1e96a529725013402d5f0d

plugins/ipam/dhcp/lease.go

dougbtv · 2025-01-21T18:12:30Z

plugins/ipam/dhcp/lease.go

+
+    ctx, cancel := context.WithTimeout(ctx, linkCheckTotalTimeout)
+    defer cancel()
+    _, err := backoffRetry(ctx, linkCheckRetryMax, checkFunc)


the only downside here is that backoffRetry() sets a "base delay" using the resendDelay0 const which was from the DHCP RFC stuff, so, it's 4 seconds the first time. This could probably be quicker? Will happily rejig backoffRetry to take a base delay parameter.

oops missed a major part of this... fix incoming.

dougbtv · 2025-01-21T20:32:21Z

Soooo.... yeah what I have currently pushed has some things I totally whiffed on and I'm working on correcting it, but, in an ideal world, the netlink library release v1.3.0 doesn't include the commit to detect the ErrDumpInterrupted

I'm looking into working around it until that can get released, which I also requested in vishvananda/netlink#1019

dougbtv · 2025-01-21T22:07:20Z

plugins/ipam/dhcp/lease.go

+	return exists, err
+}
+
+func backoffRetryLinkExists(ctx context.Context, maxDelay time.Duration, f func() (bool, error)) (bool, error) {


I wound up creating another backoff method because I needed to have the retry method f func() (bool, error) return a bool and an error, so, I additionally added linkCheckDelay0

dougbtv · 2025-01-21T22:08:24Z

plugins/ipam/dhcp/lease.go

+		if err != nil {
+			log.Printf("!bang: Error checking network link '%s': %v, type: %v", l.link.Attrs().Name, err, reflect.TypeOf(err))
+			var linkNotFoundErr *netlink.LinkNotFoundError
+			if errors.As(err, &linkNotFoundErr) {


This wasn't evaluating to true even though I was seeing logs as:

2025/01/21 21:36:46 Error checking network link 'net1': Link not found, type: netlink.LinkNotFoundError

I'll remove the reflect package and also the lines marked with !bang once I get it sorted.

dougbtv · 2025-01-22T00:21:20Z

I'm going to change to checking for the string Link Not Found which I'm not particularly stoked on, but, happy to take any input on why this didn't work:

var linkNotFoundErr *netlink.LinkNotFoundError
if errors.As(err, &linkNotFoundErr) {
	log.Printf("!bang Link not found error detected! this is what I want to see.")
	return false, nil
}

That would never evaluate as true, even when I could find the error as netlink.LinkNotFoundError when using a reflect output like this: log.Printf("!bang: Error checking network link '%s': %v, type: %v", l.link.Attrs().Name, err, reflect.TypeOf(err))

zeeke · 2025-01-22T11:00:14Z

I'm going to change to checking for the string Link Not Found which I'm not particularly stoked on, but, happy to take any input on why this didn't work:
var linkNotFoundErr *netlink.LinkNotFoundError
if errors.As(err, &linkNotFoundErr) {
	log.Printf("!bang Link not found error detected! this is what I want to see.")
	return false, nil
}
That would never evaluate as true, even when I could find the error as netlink.LinkNotFoundError when using a reflect output like this: log.Printf("!bang: Error checking network link '%s': %v, type: %v", l.link.Attrs().Name, err, reflect.TypeOf(err))

You're passing the address of an empty pointer (pointer of pointer) to errors.As(...).

This should do the job

var linkNotFoundErr *netlink.LinkNotFoundError = &netlink.LinkNotFoundError{}
if errors.As(err, linkNotFoundErr) {
	log.Printf("!bang Link not found error detected! this is what I want to see.")
	return false, nil
}

Try it in https://go.dev/play/p/GZJO1RdVR8g

dougbtv · 2025-01-22T14:59:12Z

plugins/ipam/dhcp/lease.go

+		_, err := netlink.LinkByName(l.link.Attrs().Name)
+		if err != nil {
+			var linkNotFoundErr *netlink.LinkNotFoundError = &netlink.LinkNotFoundError{}
+			if errors.As(err, linkNotFoundErr) {


Owe you one @zeeke !!! Thanks. Appreciate that, it works a charm and much cleaner than the string check, thanks.

s1061123 · 2025-01-24T08:13:18Z

plugins/ipam/dhcp/lease.go

+	ctx, cancel := context.WithTimeout(ctx, linkCheckTotalTimeout)
+	defer cancel()
+
+	exists, err := backoffRetryLinkExists(ctx, linkCheckRetryMax, checkFunc)


linkCheckRetryMax is constant variable, defined as global scope in this file. So we may remove it from function argument. Otherwise, remove constant variable, linkCheckRetryMax and define it in function variable.

thanks, simplified so that it isn't passed through the method param and just used directly there, good call

s1061123 · 2025-01-24T08:25:48Z

plugins/ipam/dhcp/lease.go

+func backoffRetryLinkExists(ctx context.Context, maxDelay time.Duration, f func() (bool, error)) (bool, error) {
+	baseDelay := linkCheckDelay0
+	for {
+		exists, err := f()


Currently, this function is pretty for backoffRetryLinkExists, not for common retry, hence we may not required to have f func() (bool, error)) as argument, I guess. Instead of that, having linkName in function argument makes it simple, I feel.

func backoffRetryLinkExists(ctx context.Context, maxDelay time.Duration, linkName string) (bool, error) { checkFunc := func() (bool, error) { _, err := netlink.LinkByName(linkName) if err != nil { var linkNotFoundErr *netlink.LinkNotFoundError = &netlink.LinkNotFoundError{} if errors.As(err, linkNotFoundErr) { return false, nil } return false, err } return true, nil }

Thanks Tomo, nice simplification. I did wind up breaking out a method checkLinkByName to use there, and then just check it in a for loop in a condensed version of the checkLinkExistsWithBackoff method.

s1061123 · 2025-01-24T08:29:33Z

plugins/ipam/dhcp/lease.go


+func (l *DHCPLease) checkLinkExistsWithBackoff(ctx context.Context) (bool, error) {
+	checkFunc := func() (bool, error) {
+		_, err := netlink.LinkByName(l.link.Attrs().Name)


If we assume that this link is already removed, then l.link.Attrs() may be invalid. We should store interface name ahead.

interesting, I didn't really account for this, even though it seems it was pulling up the correct name. However, just in case this does happen in other situations, I instead opted to store linkName as a property of the DHCPLease object. Thanks!

Due to oberservations that threads can grow and the dhcp daemon uses an increasing amount of memory. This situation can happen organically when using say, bridge CNI, and the bridge has been removed outside of the bridge CNI lifecycle, and an interface no longer exists on a pod. Does so on a retry loop using the `backoffRetry()` method. Signed-off-by: dougbtv <dosmith@redhat.com>

SchSeba

LGTM!

s1061123

LGTM!

SchSeba · 2025-02-16T13:29:47Z

Hi @s1061123 @squeed,
can we merge this one?

s1061123 · 2025-02-17T00:05:43Z

I could not secure the time to review this week.
Could you ask someone else?

s1061123 · 2025-02-19T12:17:03Z

@squeed Could you check merge criteria and merge that if it is okey? This PR already has two maingainer approval.

openshift-merge-robot · 2025-04-14T22:34:20Z

Fix included in accepted release 4.19.0-0.nightly-2025-04-02-065200

openshift-merge-robot · 2025-04-16T13:35:09Z

Fix included in accepted release 4.19.0-0.nightly-2025-04-02-170034

openshift-merge-robot · 2025-04-16T22:26:24Z

Fix included in accepted release 4.19.0-0.nightly-2025-04-04-023411

openshift-merge-robot · 2025-04-18T15:01:20Z

Fix included in accepted release 4.19.0-0.nightly-2025-04-04-170728

dougbtv force-pushed the dhcp-daemon-terminate-iface-nonexist branch 6 times, most recently from 3cb9c6b to 20e6ba9 Compare January 21, 2025 16:20

dougbtv mentioned this pull request Jan 21, 2025

[DNM] Adds DHCP daemon interface removal fix attempt on top of #169 openshift/containernetworking-plugins#172

Closed

dougbtv commented Jan 21, 2025

View reviewed changes

plugins/ipam/dhcp/lease.go Show resolved Hide resolved

dougbtv force-pushed the dhcp-daemon-terminate-iface-nonexist branch from 20e6ba9 to 0834450 Compare January 21, 2025 18:10

dougbtv commented Jan 21, 2025

View reviewed changes

dougbtv force-pushed the dhcp-daemon-terminate-iface-nonexist branch from 0834450 to 0249707 Compare January 21, 2025 22:06

dougbtv commented Jan 21, 2025

View reviewed changes

dougbtv force-pushed the dhcp-daemon-terminate-iface-nonexist branch 2 times, most recently from ea7c4f0 to 55262dc Compare January 21, 2025 22:09

dougbtv force-pushed the dhcp-daemon-terminate-iface-nonexist branch 4 times, most recently from c0f73b9 to 78d100b Compare January 22, 2025 00:42

dougbtv force-pushed the dhcp-daemon-terminate-iface-nonexist branch from 78d100b to 7758510 Compare January 22, 2025 14:58

dougbtv commented Jan 22, 2025

View reviewed changes

s1061123 requested changes Jan 24, 2025

View reviewed changes

dougbtv force-pushed the dhcp-daemon-terminate-iface-nonexist branch from 7758510 to ce0c680 Compare January 27, 2025 13:58

squeed approved these changes Jan 27, 2025

View reviewed changes

zeeke approved these changes Feb 3, 2025

View reviewed changes

Merge branch 'main' into dhcp-daemon-terminate-iface-nonexist

5805f53

SchSeba approved these changes Feb 12, 2025

View reviewed changes

s1061123 approved these changes Feb 12, 2025

View reviewed changes

squeed approved these changes Mar 3, 2025

View reviewed changes

squeed merged commit e20492f into containernetworking:main Mar 3, 2025
6 checks passed

Conversation

dougbtv commented Jan 21, 2025

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dougbtv commented Jan 21, 2025

Uh oh!

dougbtv Jan 21, 2025 • 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!

dougbtv commented Jan 22, 2025

Uh oh!

zeeke commented Jan 22, 2025

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SchSeba left a comment

Choose a reason for hiding this comment

Uh oh!

s1061123 left a comment

Choose a reason for hiding this comment

Uh oh!

SchSeba commented Feb 16, 2025

Uh oh!

s1061123 commented Feb 17, 2025

Uh oh!

s1061123 commented Feb 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

openshift-merge-robot commented Apr 14, 2025

Uh oh!

openshift-merge-robot commented Apr 16, 2025

Uh oh!

openshift-merge-robot commented Apr 16, 2025

Uh oh!

openshift-merge-robot commented Apr 18, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

dougbtv Jan 21, 2025 •

edited

Loading

s1061123 commented Feb 19, 2025 •

edited

Loading