Skip to content

Commit 0d6e7cd

Browse files
corherecorhere
committed
libnetwork/osl: stop tracking neighbor entries
The Namespace keeps some state for each inserted neighbor-table entry which is used to delete the entry (and any related entries) given only the IP and MAC address of the entry to delete. This state is not strictly required as the retained data is a pure function of the parameters passed to AddNeighbor(), and the kernel can inform us whether an attempt to add a neighbor entry would conflict with an existing entry. Get rid of the neighbor state in Namespace. It's just one more piece of state that can cause lots of grief if it falls out of sync with ground truth. Require callers to call DeleteNeighbor() with the same aguments as they had passed to AddNeighbor(). Push the responsibility for detecting attempts to insert conflicting entries into the neighbor table onto the kernel by using (*netlink.Handle).NeighAdd() instead of NeighSet(). Modernize the error messages and logging in DeleteNeighbor() and AddNeighbor(). Signed-off-by: Cory Snider <[email protected]>
1 parent 9866738 commit 0d6e7cd

3 files changed

Lines changed: 99 additions & 99 deletions

File tree

‎libnetwork/drivers/overlay/peerdb.go‎

Lines changed: 10 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -375,8 +375,16 @@ func (d *driver) peerDeleteOp(nid, eid string, peerIP net.IP, peerIPMask net.IPM
375375

376376
// Local peers do not have any local configuration to delete
377377
if !localPeer {
378+
IP := &net.IPNet{
379+
IP: peerIP,
380+
Mask: peerIPMask,
381+
}
382+
s := n.getSubnetforIP(IP)
383+
if s == nil {
384+
return fmt.Errorf("could not find the subnet %q in network %q", IP.String(), n.id)
385+
}
378386
// Remove fdb entry to the bridge for the peer mac
379-
if err := sbox.DeleteNeighbor(vtep, peerMac); err != nil {
387+
if err := sbox.DeleteNeighbor(vtep, peerMac, osl.WithLinkName(s.vxlanName)); err != nil {
380388
if _, ok := err.(osl.NeighborSearchError); ok && dbEntries > 0 {
381389
// We fall in here if there is a transient state and if the neighbor that is being deleted
382390
// was never been configured into the kernel (we allow only 1 configuration at the time per <ip,mac> mapping)
@@ -386,7 +394,7 @@ func (d *driver) peerDeleteOp(nid, eid string, peerIP net.IP, peerIPMask net.IPM
386394
}
387395

388396
// Delete neighbor entry for the peer IP
389-
if err := sbox.DeleteNeighbor(peerIP, peerMac); err != nil {
397+
if err := sbox.DeleteNeighbor(peerIP, peerMac, osl.WithLinkName(s.vxlanName), osl.WithFamily(syscall.AF_BRIDGE)); err != nil {
390398
return fmt.Errorf("could not delete neighbor entry for nid:%s eid:%s into the sandbox:%v", nid, eid, err)
391399
}
392400
}

‎libnetwork/osl/namespace_linux.go‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -232,7 +232,6 @@ type Namespace struct {
232232
defRoute4SrcName string
233233
defRoute6SrcName string
234234
staticRoutes []*types.StaticRoute
235-
neighbors []*neigh
236235
isDefault bool // isDefault is true when Namespace represents the host network namespace. It is safe to access it concurrently.
237236
ipv6LoEnabledOnce sync.Once
238237
ipv6LoEnabledCached bool

‎libnetwork/osl/neigh_linux.go‎

Lines changed: 89 additions & 96 deletions
Original file line numberDiff line numberDiff line change
@@ -1,123 +1,126 @@
11
package osl
22

33
import (
4-
"bytes"
54
"context"
65
"errors"
76
"fmt"
87
"net"
98
"os"
9+
"strings"
1010

1111
"github.com/containerd/log"
1212
"github.com/vishvananda/netlink"
1313
)
1414

1515
// NeighborSearchError indicates that the neighbor is already present
1616
type NeighborSearchError struct {
17-
ip net.IP
18-
mac net.HardwareAddr
19-
present bool
20-
}
21-
22-
func (n NeighborSearchError) Error() string {
23-
return fmt.Sprintf("Search neighbor failed for IP %v, mac %v, present in db:%t", n.ip, n.mac, n.present)
24-
}
25-
26-
type neigh struct {
27-
dstIP net.IP
28-
dstMac net.HardwareAddr
17+
ip net.IP
18+
mac net.HardwareAddr
2919
linkName string
30-
linkDst string
31-
family int
20+
present bool
3221
}
3322

34-
func (n *Namespace) findNeighbor(dstIP net.IP, dstMac net.HardwareAddr) *neigh {
35-
n.mu.Lock()
36-
defer n.mu.Unlock()
37-
38-
for _, nh := range n.neighbors {
39-
if nh.dstIP.Equal(dstIP) && bytes.Equal(nh.dstMac, dstMac) {
40-
return nh
41-
}
23+
func (n NeighborSearchError) Error() string {
24+
var b strings.Builder
25+
b.WriteString("neighbor entry ")
26+
if n.present {
27+
b.WriteString("already exists ")
28+
} else {
29+
b.WriteString("not found ")
4230
}
43-
44-
return nil
31+
b.WriteString("for IP ")
32+
b.WriteString(n.ip.String())
33+
b.WriteString(", mac ")
34+
b.WriteString(n.mac.String())
35+
if n.linkName != "" {
36+
b.WriteString(", link ")
37+
b.WriteString(n.linkName)
38+
}
39+
return b.String()
4540
}
4641

47-
// DeleteNeighbor deletes neighbor entry from the sandbox.
48-
func (n *Namespace) DeleteNeighbor(dstIP net.IP, dstMac net.HardwareAddr) error {
49-
nh := n.findNeighbor(dstIP, dstMac)
50-
if nh == nil {
51-
return NeighborSearchError{dstIP, dstMac, false}
42+
// DeleteNeighbor deletes a neighbor entry from the sandbox.
43+
//
44+
// To delete an entry inserted by [AddNeighbor] the caller must provide the same
45+
// parameters used to add it.
46+
func (n *Namespace) DeleteNeighbor(dstIP net.IP, dstMac net.HardwareAddr, options ...NeighOption) error {
47+
nlnh, linkName, err := n.nlNeigh(dstIP, dstMac, options...)
48+
if err != nil {
49+
return err
5250
}
5351

54-
var linkIndex int
55-
if nh.linkDst != "" {
56-
iface, err := n.nlHandle.LinkByName(nh.linkDst)
57-
if err != nil {
58-
return fmt.Errorf("could not find interface with destination name %s: %v", nh.linkDst, err)
52+
if err := n.nlHandle.NeighDel(nlnh); err != nil {
53+
log.G(context.TODO()).WithFields(log.Fields{
54+
"ip": dstIP,
55+
"mac": dstMac,
56+
"ifc": linkName,
57+
"error": err,
58+
}).Warn("error deleting neighbor entry")
59+
if errors.Is(err, os.ErrNotExist) {
60+
return NeighborSearchError{dstIP, dstMac, linkName, false}
5961
}
60-
linkIndex = iface.Attrs().Index
61-
}
62-
63-
nlnh := &netlink.Neigh{
64-
LinkIndex: linkIndex,
65-
IP: dstIP,
66-
State: netlink.NUD_PERMANENT,
67-
Family: nh.family,
68-
}
69-
70-
if nh.family > 0 {
71-
nlnh.HardwareAddr = dstMac
72-
nlnh.Flags = netlink.NTF_SELF
73-
}
74-
75-
// If the kernel deletion fails for the neighbor entry still remove it
76-
// from the namespace cache, otherwise kernel update can fail if the
77-
// neighbor moves back to the same host again.
78-
if err := n.nlHandle.NeighDel(nlnh); err != nil && !errors.Is(err, os.ErrNotExist) {
79-
log.G(context.TODO()).Warnf("Deleting neighbor IP %s, mac %s failed, %v", dstIP, dstMac, err)
62+
return fmt.Errorf("could not delete neighbor %+v: %w", nlnh, err)
8063
}
8164

8265
// Delete the dynamic entry in the bridge
83-
if nh.family > 0 {
84-
if err := n.nlHandle.NeighDel(&netlink.Neigh{
85-
LinkIndex: linkIndex,
86-
IP: dstIP,
87-
Family: nh.family,
88-
HardwareAddr: dstMac,
89-
Flags: netlink.NTF_MASTER,
90-
}); err != nil && !errors.Is(err, os.ErrNotExist) {
91-
log.G(context.TODO()).WithError(err).Warn("error while deleting neighbor entry")
66+
if nlnh.Family > 0 {
67+
nlnh.Flags = netlink.NTF_MASTER
68+
if err := n.nlHandle.NeighDel(nlnh); err != nil && !errors.Is(err, os.ErrNotExist) {
69+
log.G(context.TODO()).WithFields(log.Fields{
70+
"ip": dstIP,
71+
"mac": dstMac,
72+
"ifc": linkName,
73+
"error": err,
74+
}).Warn("error deleting dynamic neighbor entry")
9275
}
9376
}
9477

95-
n.mu.Lock()
96-
for i, neighbor := range n.neighbors {
97-
if neighbor.dstIP.Equal(dstIP) && bytes.Equal(neighbor.dstMac, dstMac) {
98-
n.neighbors = append(n.neighbors[:i], n.neighbors[i+1:]...)
99-
break
100-
}
101-
}
102-
n.mu.Unlock()
103-
log.G(context.TODO()).Debugf("Neighbor entry deleted for IP %v, mac %v", dstIP, dstMac)
78+
log.G(context.TODO()).WithFields(log.Fields{
79+
"ip": dstIP,
80+
"mac": dstMac,
81+
"ifc": linkName,
82+
}).Debug("Neighbor entry deleted")
10483

10584
return nil
10685
}
10786

10887
// AddNeighbor adds a neighbor entry into the sandbox.
10988
func (n *Namespace) AddNeighbor(dstIP net.IP, dstMac net.HardwareAddr, options ...NeighOption) error {
110-
nh := n.findNeighbor(dstIP, dstMac)
111-
if nh != nil {
112-
log.G(context.TODO()).Warnf("Neighbor entry already present for IP %v, mac %v neighbor:%+v", dstIP, dstMac, nh)
113-
return NeighborSearchError{dstIP, dstMac, true}
89+
nlnh, linkName, err := n.nlNeigh(dstIP, dstMac, options...)
90+
if err != nil {
91+
return err
11492
}
11593

116-
nh = &neigh{
117-
dstIP: dstIP,
118-
dstMac: dstMac,
94+
if err := n.nlHandle.NeighAdd(nlnh); err != nil {
95+
if errors.Is(err, os.ErrExist) {
96+
log.G(context.TODO()).WithFields(log.Fields{
97+
"ip": dstIP,
98+
"mac": dstMac,
99+
"ifc": linkName,
100+
"neigh": fmt.Sprintf("%+v", nlnh),
101+
}).Warn("Neighbor entry already present")
102+
return NeighborSearchError{dstIP, dstMac, linkName, true}
103+
} else {
104+
return fmt.Errorf("could not add neighbor entry %+v: %w", nlnh, err)
105+
}
119106
}
120107

108+
log.G(context.TODO()).WithFields(log.Fields{
109+
"ip": dstIP,
110+
"mac": dstMac,
111+
"ifc": linkName,
112+
}).Debug("Neighbor entry added")
113+
114+
return nil
115+
}
116+
117+
type neigh struct {
118+
linkName string
119+
family int
120+
}
121+
122+
func (n *Namespace) nlNeigh(dstIP net.IP, dstMac net.HardwareAddr, options ...NeighOption) (*netlink.Neigh, string, error) {
123+
var nh neigh
121124
nh.processNeighOptions(options...)
122125

123126
nlnh := &netlink.Neigh{
@@ -132,26 +135,16 @@ func (n *Namespace) AddNeighbor(dstIP net.IP, dstMac net.HardwareAddr, options .
132135
}
133136

134137
if nh.linkName != "" {
135-
nh.linkDst = n.findDst(nh.linkName, false)
136-
if nh.linkDst == "" {
137-
return fmt.Errorf("could not find the interface with name %s", nh.linkName)
138+
linkDst := n.findDst(nh.linkName, false)
139+
if linkDst == "" {
140+
return nil, nh.linkName, fmt.Errorf("could not find the interface with name %s", nh.linkName)
138141
}
139-
140-
iface, err := n.nlHandle.LinkByName(nh.linkDst)
142+
iface, err := n.nlHandle.LinkByName(linkDst)
141143
if err != nil {
142-
return fmt.Errorf("could not find interface with destination name %s: %v", nh.linkDst, err)
144+
return nil, nh.linkName, fmt.Errorf("could not find interface with destination name %s: %w", linkDst, err)
143145
}
144146
nlnh.LinkIndex = iface.Attrs().Index
145147
}
146148

147-
if err := n.nlHandle.NeighSet(nlnh); err != nil {
148-
return fmt.Errorf("could not add neighbor entry:%+v error:%v", nlnh, err)
149-
}
150-
151-
n.mu.Lock()
152-
n.neighbors = append(n.neighbors, nh)
153-
n.mu.Unlock()
154-
log.G(context.TODO()).Debugf("Neighbor entry added for IP:%v, mac:%v on ifc:%s", dstIP, dstMac, nh.linkName)
155-
156-
return nil
149+
return nlnh, nh.linkName, nil
157150
}

0 commit comments

Comments
 (0)