DHCP lease maintenance should terminate when interface no longer exists.

[dougbtv]

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

[dougbtv]

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.

[dougbtv]

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

[dougbtv]

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]

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]

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]

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]

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]

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

[s1061123]

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.

[dougbtv]

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

[s1061123]

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
    }

[dougbtv]

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]

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

[dougbtv]

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!