search

projectcalico / bird

Calico's fork of the BIRD protocol stack
90 stars 86 forks source link

Remove interfaces that are flagged as shutdown. #111

Closed axel7born closed 11 months ago

axel7born commented 11 months ago

Description

Proposal for an additional fix for https://github.com/projectcalico/bird/issues/95. PR https://github.com/projectcalico/bird/pull/104 already improved the situation a lot, as most deleted interfaces are removed from the list. However, over time the list is still slowly growing in clusters with a high amount of pod creations and deletions. This PR deletes all interfaces that disappeared, instead of just flagging as SHUTDOWN.

CLAassistant commented 11 months ago

CLA assistant check
All committers have signed the CLA.

axel7born commented 11 months ago

@nelljerram Thank's a lot for looking at this PR!

Before https://github.com/projectcalico/bird/pull/104, interfaces were not removed from the list at all. With that PR, interfaces are now deleted via an asynchronous update when we receive a message from the kernel about an interface change.

However, the synchronous case is not yet handled. In this scenario, a request is sent to the kernel to dump all network interfaces. If an interface is not present in the list we receive from the kernel (i.e., IF_UPDATED is not set in if_end_update), it means the interface has been removed.

I suspect that during periods of high load, messages from the kernel might get dropped, causing us to miss some interface changes. These missed changes should be cleaned up by the synchronous scan.

nelljerram commented 11 months ago

/sem-approve

nelljerram commented 11 months ago

Once this has merged, we'll need a change at https://github.com/projectcalico/calico/blob/master/metadata.mk#L27 to pull it into Calico. Would you like to propose that change as well, or prefer us to handle that?

axel7born commented 11 months ago

Thanks for reviewing!

Once this has merged, we'll need a change at https://github.com/projectcalico/calico/blob/master/metadata.mk#L27 to pull it into Calico. Would you like to propose that change as well, or prefer us to handle that?

I would prefer if you could handle that.

nelljerram commented 11 months ago

Sure, FYI https://github.com/projectcalico/calico/pull/8064