Skip to content

networkmanager: repair pooled TAP drift instead of failing #32

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
tianyuzhou95 merged 7 commits into from
Sep 3, 2026
Merged

networkmanager: repair pooled TAP drift instead of failing #32

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
fee39db
networkmanager: repair pooled TAP drift instead of failing
hky1999 Aug 25, 2026
f7eae7f
networkmanager: persist the refreshed lease key on TAP recovery
hky1999 Aug 26, 2026
f8f789f
networkmanager: resolve refreshed leases by immutable identity
hky1999 Aug 26, 2026
f1c3f7c
networkmanager: reject externally replaced active TAPs at recovery
hky1999 Aug 27, 2026
6a1ef4e
fix(networkmanager): keep lease lookups generation-safe
tianyuzhou95 Aug 27, 2026
def1947
Merge branch 'main' into fix/tap-pool-drift-repair
hky1999 Sep 3, 2026
daa6f1f
networkmanager: persist pooled leases before Allocate returns
hky1999 Sep 3, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
47 changes: 35 additions & 12 deletions pkg/networkmanager/tap.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import (
"net"

"github.com/inclusionAI/sandboxd/internal/util"
"github.com/sirupsen/logrus"
"github.com/vishvananda/netlink"
"golang.org/x/sys/unix"
)
Expand Down Expand Up @@ -190,16 +191,36 @@ func (m *InterfaceManager) setTapState(resource *NetResource, up bool) error {
if link.Type() != "tuntap" {
return fmt.Errorf("pooled endpoint %s has type %q, want tuntap", expectedName, link.Type())
}
if m.bridgeLink == nil || link.Attrs().MasterIndex != m.bridgeLink.Attrs().Index {
return fmt.Errorf("pooled TAP %s is not attached to %s", expectedName, BridgeName)
// Host-side drift (stale bridge attach, host MAC randomised by an
// unclean predecessor or foreign tooling) is repaired in place instead of
// failing the allocation or daemon recovery: the endpoint identity is
// fully determined by its name, so re-stamping the deterministic
// attributes can only converge it back to the expected state. Identity
// violations (wrong type above, wrong name earlier) stay hard errors.
if m.bridgeLink == nil {
return fmt.Errorf("pooled TAP %s has no %s to attach to", expectedName, BridgeName)
}
if link.Attrs().MasterIndex != m.bridgeLink.Attrs().Index {
if err := netlink.LinkSetMaster(link, m.bridgeLink); err != nil {
return fmt.Errorf(
"reattach pooled TAP %s to %s: %w", expectedName, BridgeName, err,
)
}
logrus.Warnf(
"networkmanager: repaired pooled TAP %s bridge attach (was not on %s)",
expectedName, BridgeName,
)
}
expectedHostMAC, _ := tapHostMAC(ip4)
if !bytes.Equal(link.Attrs().HardwareAddr, expectedHostMAC) {
return fmt.Errorf(
"pooled TAP %s host MAC is %s, want %s",
expectedName,
link.Attrs().HardwareAddr,
expectedHostMAC,
if err := netlink.LinkSetHardwareAddr(link, expectedHostMAC); err != nil {
return fmt.Errorf(
"restore pooled TAP %s host MAC to %s: %w", expectedName, expectedHostMAC, err,
)
}

@hky1999 hky1999 Aug 26, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

False positive: net.HardwareAddr implements Stringer; fmt %s invokes it (verified output is colon-separated), and the pre-PR code used the same pattern.

logrus.Warnf(
"networkmanager: repaired pooled TAP %s host MAC %s -> %s",
expectedName, link.Attrs().HardwareAddr, expectedHostMAC,
)
}
expectedGuestMAC, _ := tapGuestMAC(ip4)
Expand All @@ -212,12 +233,14 @@ func (m *InterfaceManager) setTapState(resource *NetResource, up bool) error {
)
}
if resource.Interface.Index != 0 && resource.Interface.Index != link.Attrs().Index {
return fmt.Errorf(
"pooled TAP %s index is %d, lease records %d",
expectedName,
link.Attrs().Index,
resource.Interface.Index,
// The kernel ifindex is bookkeeping, not identity (it changes whenever
// the device is recreated); refresh the lease record instead of
// failing over a value nobody consumes.
logrus.Warnf(
"networkmanager: pooled TAP %s index drifted (%d, lease records %d); refreshing lease",
expectedName, link.Attrs().Index, resource.Interface.Index,
)
resource.Interface.Index = link.Attrs().Index
Comment on lines +243 to +247

@hky1999 hky1999 Aug 26, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in f7eae7f (durable key swap on recovery)

@hky1999 hky1999 Aug 27, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Superseded by f1c3f7c: active-lease ifindex drift is now rejected outright at recovery (see tianyuzhou95's thread), so recovery never rewrites the durable key anymore. The remaining refresh paths (markUsing re-handout, Recycle) persist the refreshed serialization through the returned/queued string, which is what the durable key and the handed-out annotation both become.

}
if up {
if err := netlink.LinkSetUp(link); err != nil {
Expand Down
9 changes: 9 additions & 0 deletions pkg/networkmanager/tap_recovery.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -170,6 +170,15 @@ func (m *InterfaceManager) load(ips sets.Set[string]) error {
if stateErr := m.setTapState(stored, true); stateErr != nil {
return fmt.Errorf("recover active pooled TAP %s: %w", dev.Name, stateErr)
}
// setTapState may repair lease bookkeeping in memory (e.g. an
// ifindex refresh after the device was recreated). Swap the
// durable key so the repaired lease is what gets stored and
// handed back on recycle, instead of re-warning every restart.
if refreshed := stored.ToString(); refreshed != activeID {

@tianyuzhou95 tianyuzhou95 Aug 26, 2026 •
edited
Loading

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do not repair active TAP replacement by rewriting the lease key

An active TAP should not be recreated by sandboxd: createTapDevice refuses to replace an existing device, MAC and bridge repairs do not change the ifindex, and a TAP left in the LinkAdd crash window has not yet become a durable active lease. An ifindex mismatch on an active lease therefore means that the kernel device was replaced externally, which is an identity violation rather than ordinary bookkeeping drift. Please keep this case as a hard error instead of accepting the replacement.

Rewriting the key here is also not sufficient to migrate ownership. The original serialized NetResource remains in the sandbox OCI annotations and is later passed to Deactivate and Release. Those operations look up the old string, do not find it after this swap, and return success without deactivating or recycling the TAP, leaving the refreshed lease active after sandbox deletion.

The MAC and bridge self-repair can remain independent of this check. A regression test should verify that active ifindex replacement is rejected while idle orphan MAC and bridge drift are still repaired.

@hky1999 hky1999 Aug 27, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed in f1c3f7c — agreed on all three points.

  • Active-lease ifindex mismatch is a hard error again, checked in load() before any repair: active pooled TAP … was replaced externally (ifindex N, durable lease records M). The durable lease is left untouched for the operator and the replacement is never adopted (no LinkSetUp on it).
  • The durable-key swap from f7eae7f is reverted: with active-lease replacement rejected, recovery no longer mutates the stored serialization, so no rename can ever diverge from the sandbox OCI annotations — your second concern is gone at the root rather than patched over. resolveLeaseKey (f8f789f) stays as defense in depth for release-path lookups.
  • MAC re-stamp / bridge re-attach repairs are unchanged, and the ifindex refresh now only runs on consumer-less paths (markUsing re-handout, Recycle), where the refreshed serialization is exactly the string the caller persists and later releases.
  • Regression tests added per your ask: TestRecoveryRejectsExternallyReplacedActiveTap (replacement rejected) and TestRecoveryRepairsIdleOrphanMacAndBridgeDrift (idle orphan with randomized MAC + detached bridge still adopted with both repairs). Full gates green (gofmt/go vet/go test ./...).

m.usingInterfaces.Pop(activeID)
m.usingInterfaces.Set(refreshed, struct{}{})
m.storeMark.Store(true)
}
} else {
if stateErr := m.setTapState(current, false); stateErr != nil {
return fmt.Errorf("recover idle pooled TAP %s: %w", dev.Name, stateErr)
Expand Down