diff --git a/lib/instances/network_test.go b/lib/instances/network_test.go index 2611cb2b..0c25455f 100644 --- a/lib/instances/network_test.go +++ b/lib/instances/network_test.go @@ -65,6 +65,7 @@ func TestCreateInstanceWithNetwork(t *testing.T) { t.Log("Initializing network...") err = manager.networkManager.Initialize(ctx, nil) require.NoError(t, err) + require.NoError(t, manager.networkManager.SetupHTB(ctx, 100*1024*1024)) t.Log("Network initialized") // Create instance with nginx:alpine and default network @@ -126,6 +127,18 @@ func TestCreateInstanceWithNetwork(t *testing.T) { require.NoError(t, err) _, isBridge := master.(*netlink.Bridge) assert.True(t, isBridge, "TAP should be attached to a bridge") + bridgeName := master.Attrs().Name + + t.Log("Verifying orphaned bridge tc cleanup preserves live TAP state...") + liveFlowID := createBridgeTCForTest(t, bridgeName, tap.Attrs().Index) + require.True(t, bridgeClassExists(t, bridgeName, liveFlowID), "live bridge tc class should exist before cleanup") + staleFlowID := createBridgeTCForTest(t, bridgeName, 1) + deletedTC := manager.networkManager.CleanupOrphanedClasses(ctx) + require.GreaterOrEqual(t, deletedTC, 2, "expected stale filter and class to be deleted") + assert.True(t, bridgeFilterExistsForFlowID(t, bridgeName, liveFlowID), "live bridge tc filter should remain") + assert.True(t, bridgeClassExists(t, bridgeName, liveFlowID), "live bridge tc class should remain") + assert.False(t, bridgeFilterExistsForFlowID(t, bridgeName, staleFlowID), "stale bridge tc filter should be deleted") + assert.False(t, bridgeClassExists(t, bridgeName, staleFlowID), "stale bridge tc class should be deleted") // Wait for nginx to start t.Log("Waiting for nginx to start...") @@ -301,6 +314,123 @@ func TestCreateInstanceWithNetwork(t *testing.T) { t.Log("Network integration test complete!") } +func tcForTest(t *testing.T) string { + t.Helper() + if path, err := exec.LookPath("tc"); err == nil { + return path + } + return "/usr/sbin/tc" +} + +func runTCForTest(t *testing.T, args ...string) string { + t.Helper() + output, err := exec.Command(tcForTest(t), args...).CombinedOutput() + require.NoError(t, err, "tc %s output: %s", strings.Join(args, " "), string(output)) + return string(output) +} + +func bridgeClassesForTest(t *testing.T, bridgeName string) []string { + t.Helper() + output := runTCForTest(t, "class", "show", "dev", bridgeName) + var classes []string + for _, line := range strings.Split(output, "\n") { + if !strings.Contains(line, "class htb 1:") { + continue + } + fields := strings.Fields(line) + if len(fields) >= 3 { + classes = append(classes, fields[2]) + } + } + return classes +} + +func bridgeClassExists(t *testing.T, bridgeName, classID string) bool { + t.Helper() + for _, class := range bridgeClassesForTest(t, bridgeName) { + if class == classID { + return true + } + } + return false +} + +func bridgeFilterExistsForFlowID(t *testing.T, bridgeName, flowID string) bool { + t.Helper() + return len(bridgeFilterHandlesForFlowID(t, bridgeName, flowID)) > 0 +} + +func bridgeFilterHandlesForFlowID(t *testing.T, bridgeName, flowID string) []string { + t.Helper() + output := runTCForTest(t, "filter", "show", "dev", bridgeName, "parent", "1:") + var handles []string + for _, line := range strings.Split(output, "\n") { + if !strings.HasPrefix(line, "filter ") { + continue + } + fields := strings.Fields(line) + handle, gotFlowID := "", "" + for i, field := range fields { + if i+1 >= len(fields) { + break + } + switch field { + case "handle": + handle = fields[i+1] + case "flowid": + gotFlowID = fields[i+1] + } + } + if handle != "" && gotFlowID == flowID { + handles = append(handles, handle) + } + } + return handles +} + +func createBridgeTCForTest(t *testing.T, bridgeName string, rtIif int) string { + t.Helper() + used := make(map[string]bool) + for _, classID := range bridgeClassesForTest(t, bridgeName) { + used[classID] = true + } + + flowID := "" + for id := 0xff00; id <= 0xffff; id++ { + candidate := fmt.Sprintf("1:%04x", id) + if !used[candidate] { + flowID = candidate + break + } + } + require.NotEmpty(t, flowID, "expected an unused test class id") + + t.Cleanup(func() { + bestEffortDeleteBridgeFiltersForFlowID(t, bridgeName, flowID) + _ = exec.Command(tcForTest(t), "qdisc", "del", "dev", bridgeName, "parent", flowID).Run() + _ = exec.Command(tcForTest(t), "class", "del", "dev", bridgeName, "classid", flowID).Run() + }) + + runTCForTest(t, "class", "add", "dev", bridgeName, "parent", "1:1", + "classid", flowID, "htb", "rate", "1mbit", "ceil", "1mbit") + runTCForTest(t, "qdisc", "add", "dev", bridgeName, "parent", flowID, "fq_codel") + runTCForTest(t, "filter", "add", "dev", bridgeName, "parent", "1:", + "protocol", "all", "prio", "1", "basic", + "match", fmt.Sprintf("meta(rt_iif eq %d)", rtIif), "flowid", flowID) + + require.True(t, bridgeClassExists(t, bridgeName, flowID), "staged bridge tc class should exist") + require.True(t, bridgeFilterExistsForFlowID(t, bridgeName, flowID), "staged bridge tc filter should exist") + return flowID +} + +func bestEffortDeleteBridgeFiltersForFlowID(t *testing.T, bridgeName, flowID string) { + t.Helper() + for _, handle := range bridgeFilterHandlesForFlowID(t, bridgeName, flowID) { + _ = exec.Command(tcForTest(t), "filter", "del", "dev", bridgeName, "parent", "1:", + "protocol", "all", "prio", "1", "handle", handle, "basic").Run() + } +} + func startRestartPolicyControllerForTest(t *testing.T, ctx context.Context, manager *manager) { t.Helper() diff --git a/lib/instances/tap_gc.go b/lib/instances/tap_gc.go index ed9ae213..aea67e27 100644 --- a/lib/instances/tap_gc.go +++ b/lib/instances/tap_gc.go @@ -49,24 +49,30 @@ func (m *manager) reconcileTAPs(ctx context.Context) { // Initializing, and Unknown. "Better to leave a stale TAP than crash a // running VM." insts, err := m.ListInstances(ctx, nil) + var preserve []string if err != nil { - log.WarnContext(ctx, "TAP GC: failed to list instances, skipping pass", "error", err) - return - } - preserve := make([]string, 0, len(insts)) - for _, inst := range insts { - if inst.State == StateRunning || inst.State == StateInitializing || inst.State == StateUnknown { - preserve = append(preserve, inst.Id) + log.WarnContext(ctx, "TAP GC: failed to list instances, skipping TAP pass", "error", err) + } else { + preserve = make([]string, 0, len(insts)) + for _, inst := range insts { + if inst.State == StateRunning || inst.State == StateInitializing || inst.State == StateUnknown { + preserve = append(preserve, inst.Id) + } } } - if len(preserve) == 0 { - // CleanupOrphanedTAPs short-circuits on empty preserve set to avoid - // clobbering TAPs from concurrent hypeman processes/tests. Mirror that here - // rather than calling it with an empty slice. - log.DebugContext(ctx, "TAP GC: no preserve candidates, skipping pass") - return + + if err == nil { + if len(preserve) == 0 { + // CleanupOrphanedTAPs short-circuits on empty preserve set to avoid + // clobbering TAPs from concurrent hypeman processes/tests. Mirror that here + // rather than calling it with an empty slice. + log.DebugContext(ctx, "TAP GC: no preserve candidates, skipping TAP pass") + } else if deleted := m.networkManager.CleanupOrphanedTAPs(ctx, preserve, tapGCMinAge); deleted > 0 { + log.InfoContext(ctx, "TAP GC: cleaned up orphaned TAP devices", "count", deleted) + } } - if deleted := m.networkManager.CleanupOrphanedTAPs(ctx, preserve, tapGCMinAge); deleted > 0 { - log.InfoContext(ctx, "TAP GC: cleaned up orphaned TAP devices", "count", deleted) + + if deleted := m.networkManager.CleanupOrphanedClasses(ctx); deleted > 0 { + log.InfoContext(ctx, "TAP GC: cleaned up orphaned tc filters/classes", "count", deleted) } } diff --git a/lib/network/allocate.go b/lib/network/allocate.go index 565a28ca..112ee151 100644 --- a/lib/network/allocate.go +++ b/lib/network/allocate.go @@ -62,7 +62,7 @@ func (m *manager) CreateAllocation(ctx context.Context, req AllocateRequest) (*N err = m.createTAPDevice(tapCtx, netConfig.TAPDevice, network.Bridge, network.Isolated) tapSpanEnd(err) if err != nil { - cleanupErr := m.deleteTAPDeviceSerialized(netConfig.TAPDevice, "") + cleanupErr := m.deleteTAPDeviceForInstanceSerialized(ctx, req.InstanceID, netConfig.TAPDevice) if cleanupErr != nil { log.WarnContext(ctx, "failed to clean up TAP after create failure", "tap", netConfig.TAPDevice, "error", cleanupErr) } @@ -146,7 +146,7 @@ func (m *manager) RecreateAllocation(ctx context.Context, instanceID string, dow err = m.createTAPDevice(tapCtx, alloc.TAPDevice, network.Bridge, network.Isolated) tapSpanEnd(err) if err != nil { - _ = m.deleteTAPDeviceSerialized(alloc.TAPDevice, alloc.ClassID) + _ = m.deleteTAPDeviceForInstanceSerialized(ctx, instanceID, alloc.TAPDevice) return fmt.Errorf("create TAP device: %w", err) } m.recordTAPOperation(ctx, "create") @@ -386,10 +386,10 @@ func (m *manager) ReleaseAllocation(ctx context.Context, alloc *Allocation) erro } m.forgetPendingAllocation(alloc.InstanceID) - // 1. Delete TAP device (best effort), using stored class ID for correct HTB cleanup. + // 1. Delete TAP device (best effort), using the TAP's live tc filter for HTB cleanup. // Serialize with async tc setup so queued rate-limit work cannot race with // class removal and leave stale bridge state behind. - if err := m.deleteTAPDeviceForInstanceSerialized(alloc.InstanceID, alloc.TAPDevice, alloc.ClassID); err != nil { + if err := m.deleteTAPDeviceForInstanceSerialized(ctx, alloc.InstanceID, alloc.TAPDevice); err != nil { log.WarnContext(ctx, "failed to delete TAP device", "tap", alloc.TAPDevice, "error", err) } else { m.recordTAPOperation(ctx, "delete") @@ -403,26 +403,20 @@ func (m *manager) ReleaseAllocation(ctx context.Context, alloc *Allocation) erro return nil } -func (m *manager) deleteTAPDeviceSerialized(tapName, classID string) error { +func (m *manager) deleteTAPDeviceSerialized(ctx context.Context, tapName string) error { m.tcMu.Lock() defer m.tcMu.Unlock() - return m.deleteTAPDevice(tapName, classID) + return m.deleteTAPDevice(ctx, tapName) } -func (m *manager) deleteTAPDeviceForInstanceSerialized(instanceID, tapName, classID string) error { +func (m *manager) deleteTAPDeviceForInstanceSerialized(ctx context.Context, instanceID, tapName string) error { m.tcMu.Lock() defer m.tcMu.Unlock() - return m.deleteTAPDevice(tapName, m.classIDForDelete(instanceID, classID)) -} - -func (m *manager) classIDForDelete(instanceID, fallback string) string { - if instanceID == "" { - return fallback - } - if classID := m.loadClassID(instanceID); classID != "" { - return classID + if err := m.deleteTAPDevice(ctx, tapName); err != nil { + return err } - return fallback + m.clearClassID(instanceID) + return nil } // getOrInitDefaultNetwork resolves the default network and self-heals by running diff --git a/lib/network/bridge_darwin.go b/lib/network/bridge_darwin.go index b44ea6f9..c6e09cc3 100644 --- a/lib/network/bridge_darwin.go +++ b/lib/network/bridge_darwin.go @@ -50,7 +50,7 @@ func (m *manager) addVMClass(ctx context.Context, bridgeName, tapName string, ra } // deleteTAPDevice is a no-op on macOS as we use NAT networking. -func (m *manager) deleteTAPDevice(tapName, classID string) error { +func (m *manager) deleteTAPDevice(ctx context.Context, tapName string) error { return nil } @@ -79,3 +79,7 @@ func (m *manager) CleanupOrphanedTAPs(ctx context.Context, preserveInstanceIDs [ func (m *manager) CleanupOrphanedClasses(ctx context.Context) int { return 0 } + +func (m *manager) bridgeHTBClassCount(ctx context.Context) (int64, error) { + return 0, nil +} diff --git a/lib/network/bridge_linux.go b/lib/network/bridge_linux.go index 1c064cc9..31297826 100644 --- a/lib/network/bridge_linux.go +++ b/lib/network/bridge_linux.go @@ -10,6 +10,7 @@ import ( "net" "os" "os/exec" + "strconv" "strings" "syscall" "time" @@ -510,7 +511,7 @@ func (m *manager) createTAPDevice(ctx context.Context, tapName, bridgeName strin attribute.String("operation", "delete_existing"), attribute.String("tap", tapName), ) - err := m.deleteTAPDeviceSerialized(tapName, "") + err := m.deleteTAPDeviceSerialized(ctx, tapName) deleteEnd(err) if err != nil { return fmt.Errorf("delete existing TAP: %w", err) @@ -794,57 +795,230 @@ func (m *manager) addVMClass(ctx context.Context, bridgeName, tapName string, ra return "", fmt.Errorf("tc class add failed after %d attempts: %w", maxAttempts, lastErr) } -// removeVMClass removes the HTB class for a VM from the bridge. -// If classID is non-empty, it is used directly; otherwise falls back to deriveClassID. -func (m *manager) removeVMClass(bridgeName, tapName, classID string) error { - if classID == "" { - classID = deriveClassID(tapName) - } - fullClassID := fmt.Sprintf("1:%s", classID) +type bridgeFilter struct { + handle string + flowID string + rtIif int +} - // Delete filter first (by matching flowid) - // List filters and delete matching ones - listCmd := exec.Command("tc", "filter", "show", "dev", bridgeName, "parent", htbRootHandle) - listCmd.SysProcAttr = &syscall.SysProcAttr{ - AmbientCaps: []uintptr{unix.CAP_NET_ADMIN}, +func parseBridgeFilters(output string) []bridgeFilter { + var filters []bridgeFilter + var current bridgeFilter + haveCurrent := false + + flush := func() { + if haveCurrent && current.handle != "" && current.flowID != "" { + filters = append(filters, current) + } } - output, _ := listCmd.Output() - // Parse filter output to find handle for this flowid - lines := strings.Split(string(output), "\n") - for _, line := range lines { - if strings.Contains(line, fullClassID) && strings.Contains(line, "filter") { - // Extract filter handle (e.g., "filter parent 1: protocol all pref 1 basic chain 0 handle 0x1") + for _, line := range strings.Split(output, "\n") { + if strings.HasPrefix(line, "filter ") { + flush() + current = bridgeFilter{rtIif: -1} + haveCurrent = true + fields := strings.Fields(line) - for i, f := range fields { - if f == "handle" && i+1 < len(fields) { - handle := fields[i+1] - delCmd := exec.Command("tc", "filter", "del", "dev", bridgeName, "parent", htbRootHandle, - "protocol", "all", "prio", "1", "handle", handle, "basic") - delCmd.SysProcAttr = &syscall.SysProcAttr{ - AmbientCaps: []uintptr{unix.CAP_NET_ADMIN}, - } - delCmd.Run() // Best effort + for i, field := range fields { + if i+1 >= len(fields) { break } + switch field { + case "handle": + current.handle = fields[i+1] + case "flowid": + current.flowID = fields[i+1] + } + } + continue + } + + if !haveCurrent { + continue + } + if idx := strings.Index(line, "rt_iif eq "); idx >= 0 { + rest := line[idx+len("rt_iif eq "):] + if end := strings.IndexFunc(rest, func(r rune) bool { return r < '0' || r > '9' }); end >= 0 { + rest = rest[:end] + } + if rtIif, err := strconv.Atoi(rest); err == nil { + current.rtIif = rtIif } } } + flush() + + return filters +} - // Delete child qdisc (fq_codel) before deleting the class +func listBridgeFilters(bridgeName string) ([]bridgeFilter, error) { + cmd := exec.Command("tc", "filter", "show", "dev", bridgeName, "parent", htbRootHandle) + cmd.SysProcAttr = &syscall.SysProcAttr{ + AmbientCaps: []uintptr{unix.CAP_NET_ADMIN}, + } + output, err := cmd.Output() + if err != nil { + return nil, fmt.Errorf("tc filter show: %w", err) + } + return parseBridgeFilters(string(output)), nil +} + +func deleteBridgeFilter(bridgeName, handle string) error { + cmd := exec.Command("tc", "filter", "del", "dev", bridgeName, "parent", htbRootHandle, + "protocol", "all", "prio", "1", "handle", handle, "basic") + cmd.SysProcAttr = &syscall.SysProcAttr{ + AmbientCaps: []uintptr{unix.CAP_NET_ADMIN}, + } + if output, err := cmd.CombinedOutput(); err != nil { + return fmt.Errorf("tc filter del: %w (output: %s)", err, strings.TrimSpace(string(output))) + } + return nil +} + +func deleteBridgeClass(bridgeName, fullClassID string) error { qdiscCmd := exec.Command("tc", "qdisc", "del", "dev", bridgeName, "parent", fullClassID) qdiscCmd.SysProcAttr = &syscall.SysProcAttr{ AmbientCaps: []uintptr{unix.CAP_NET_ADMIN}, } qdiscCmd.Run() // Best effort - may not exist - // Delete the class cmd := exec.Command("tc", "class", "del", "dev", bridgeName, "classid", fullClassID) cmd.SysProcAttr = &syscall.SysProcAttr{ AmbientCaps: []uintptr{unix.CAP_NET_ADMIN}, } - // Ignore errors - class may not exist - cmd.Run() + if output, err := cmd.CombinedOutput(); err != nil { + return fmt.Errorf("tc class del: %w (output: %s)", err, strings.TrimSpace(string(output))) + } + return nil +} + +func minorClassID(fullClassID string) (string, bool) { + major, minor, ok := strings.Cut(fullClassID, ":") + if !ok || major != "1" || minor == "" { + return "", false + } + return minor, true +} + +func parseBridgeClasses(output string) []string { + var classes []string + for _, line := range strings.Split(output, "\n") { + if !strings.Contains(line, "class htb 1:") { + continue + } + fields := strings.Fields(line) + if len(fields) >= 3 { + classes = append(classes, fields[2]) + } + } + return classes +} + +func countBridgeHTBClasses(classes []string) int64 { + var count int64 + for _, class := range classes { + if class == htbRootClassID { + continue + } + count++ + } + return count +} + +func (m *manager) bridgeHTBClassCount(ctx context.Context) (int64, error) { + bridgeName := m.config.Network.BridgeName + cmd := exec.CommandContext(ctx, "tc", "class", "show", "dev", bridgeName) + cmd.SysProcAttr = &syscall.SysProcAttr{ + AmbientCaps: []uintptr{unix.CAP_NET_ADMIN}, + } + output, err := cmd.Output() + if err != nil { + return 0, fmt.Errorf("tc class show: %w", err) + } + return countBridgeHTBClasses(parseBridgeClasses(string(output))), nil +} + +func planOrphanedBridgeTC(liveTapIndexes map[int]bool, filters []bridgeFilter, classes []string) ([]bridgeFilter, []string, bool) { + candidates, parsed := 0, 0 + for _, filter := range filters { + if filter.handle == "" || filter.flowID == "" { + continue + } + candidates++ + if filter.rtIif >= 0 { + parsed++ + } + } + if candidates > 0 && parsed == 0 { + return nil, nil, false + } + + protectedClassIDs := make(map[string]bool) + staleFilters := make([]bridgeFilter, 0) + for _, filter := range filters { + if filter.handle == "" || filter.flowID == "" { + continue + } + if filter.rtIif < 0 { + if classID, ok := minorClassID(filter.flowID); ok { + protectedClassIDs[classID] = true + } + continue + } + if liveTapIndexes[filter.rtIif] { + if classID, ok := minorClassID(filter.flowID); ok { + protectedClassIDs[classID] = true + } + continue + } + staleFilters = append(staleFilters, filter) + } + + staleClasses := make([]string, 0) + for _, fullClassID := range classes { + if fullClassID == htbRootClassID { + continue + } + classID, ok := minorClassID(fullClassID) + if !ok { + continue + } + if !protectedClassIDs[classID] { + staleClasses = append(staleClasses, fullClassID) + } + } + return staleFilters, staleClasses, true +} + +// removeVMClass removes bridge tc state proven to belong to a TAP. +func (m *manager) removeVMClass(ctx context.Context, bridgeName string, tapIndex int) error { + if tapIndex <= 0 { + return nil + } + + filters, err := listBridgeFilters(bridgeName) + if err != nil { + logger.FromContext(ctx).ErrorContext(ctx, + "failed to list bridge filters for TAP tc cleanup", + "bridge", bridgeName, + "tap_ifindex", tapIndex, + "error", err, + ) + return nil + } + + deletedClasses := make(map[string]struct{}) + for _, filter := range filters { + if filter.rtIif != tapIndex { + continue + } + _ = deleteBridgeFilter(bridgeName, filter.handle) + if _, ok := deletedClasses[filter.flowID]; ok { + continue + } + deletedClasses[filter.flowID] = struct{}{} + _ = deleteBridgeClass(bridgeName, filter.flowID) + } return nil } @@ -868,17 +1042,16 @@ func deriveClassID(tapName string) string { } // deleteTAPDevice removes TAP device and its associated HTB class on the bridge. -// If classID is non-empty, it is used for removeVMClass; otherwise falls back to deriveClassID. -func (m *manager) deleteTAPDevice(tapName, classID string) error { - // Remove HTB class from bridge before deleting TAP - m.removeVMClass(m.config.Network.BridgeName, tapName, classID) - +func (m *manager) deleteTAPDevice(ctx context.Context, tapName string) error { link, err := netlink.LinkByName(tapName) if err != nil { // TAP doesn't exist, nothing to do return nil } + // Remove HTB class from bridge before deleting TAP + m.removeVMClass(ctx, m.config.Network.BridgeName, link.Attrs().Index) + if err := netlink.LinkDel(link); err != nil { return fmt.Errorf("delete TAP device: %w", err) } @@ -991,8 +1164,8 @@ func (m *manager) CleanupOrphanedTAPs(ctx context.Context, preserveInstanceIDs [ } } - // Orphaned TAP - delete it (no stored classID available, falls back to deriveClassID) - if err := m.deleteTAPDeviceSerialized(name, ""); err != nil { + // Orphaned TAP - delete it by tc filters anchored to the TAP ifindex. + if err := m.deleteTAPDeviceSerialized(ctx, name); err != nil { log.WarnContext(ctx, "failed to delete orphaned TAP", "tap", name, "error", err) continue } @@ -1021,126 +1194,69 @@ func tapDeviceAge(name string, now time.Time) (time.Duration, error) { return now.Sub(ctime), nil } -// CleanupOrphanedClasses removes HTB classes on the bridge that don't have matching TAP devices. -// This handles the case where a TAP was deleted externally (manual deletion, reboot, etc.) -// but the HTB class persists on the bridge. -// Returns the number of classes deleted. +// CleanupOrphanedClasses removes bridge filters and HTB classes that are no +// longer referenced by a live TAP's filter. Returns the number of tc objects +// deleted. func (m *manager) CleanupOrphanedClasses(ctx context.Context) int { log := logger.FromContext(ctx) bridgeName := m.config.Network.BridgeName - // List all HTB classes on the bridge + m.tcMu.Lock() + defer m.tcMu.Unlock() + cmd := exec.Command("tc", "class", "show", "dev", bridgeName) output, err := cmd.Output() if err != nil { log.DebugContext(ctx, "no HTB classes to clean up", "bridge", bridgeName) return 0 } + classes := parseBridgeClasses(string(output)) + if len(classes) == 0 { + return 0 + } - // Build set of class IDs that belong to existing TAP devices. - // Include both derived class IDs and stored class IDs from allocations - // (which may differ if a collision was resolved by probing). - validClassIDs := make(map[string]bool) + liveTapIndexes := make(map[int]bool) links, err := netlink.LinkList() - if err == nil { - for _, link := range links { - name := link.Attrs().Name - if strings.HasPrefix(name, TAPPrefix) { - validClassIDs[deriveClassID(name)] = true - } - } - } - // Also include stored class IDs from allocations (handles probed IDs). - // If ListAllocations fails, bail out to avoid deleting valid probed classes. - allocs, allocErr := m.ListAllocations(ctx) - if allocErr != nil { - log.WarnContext(ctx, "skipping orphaned class cleanup: failed to list allocations", "error", allocErr) + if err != nil { + log.WarnContext(ctx, "skipping orphaned tc cleanup: failed to list links", "error", err) return 0 } - for _, alloc := range allocs { - if alloc.ClassID != "" { - validClassIDs[alloc.ClassID] = true + for _, link := range links { + name := link.Attrs().Name + if strings.HasPrefix(name, TAPPrefix) { + liveTapIndexes[link.Attrs().Index] = true } } - // Parse class output and find orphaned classes - // Format: "class htb 1:xxxx parent 1:1 ..." - deleted := 0 - lines := strings.Split(string(output), "\n") - for _, line := range lines { - if !strings.Contains(line, "class htb 1:") { - continue - } - - // Extract class ID (e.g., "1:a3f2") - fields := strings.Fields(line) - if len(fields) < 3 { - continue - } - fullClassID := fields[2] // "1:xxxx" - - // Skip root class - if fullClassID == htbRootClassID { - continue - } + filters, err := listBridgeFilters(bridgeName) + if err != nil { + log.WarnContext(ctx, "skipping orphaned tc cleanup: failed to list filters", "error", err) + return 0 + } - // Extract just the minor part (after "1:") - parts := strings.Split(fullClassID, ":") - if len(parts) != 2 { - continue - } - classID := parts[1] + staleFilters, staleClasses, safe := planOrphanedBridgeTC(liveTapIndexes, filters, classes) + if !safe { + log.WarnContext(ctx, "skipping orphaned tc cleanup: no rt_iif matches parsed from tc filter output", + "bridge", bridgeName, "filters", len(filters)) + return 0 + } - // Check if this class belongs to an existing TAP - if validClassIDs[classID] { + deleted := 0 + for _, filter := range staleFilters { + log.WarnContext(ctx, "cleaning up orphaned tc filter", + "handle", filter.handle, "flowid", filter.flowID, "rt_iif", filter.rtIif, "bridge", bridgeName) + if err := deleteBridgeFilter(bridgeName, filter.handle); err != nil { + log.WarnContext(ctx, "failed to delete orphaned tc filter", + "handle", filter.handle, "error", err) continue } + deleted++ + } - // Orphaned class - delete it with warning + for _, fullClassID := range staleClasses { log.WarnContext(ctx, "cleaning up orphaned HTB class", "class", fullClassID, "bridge", bridgeName) - - // Delete filter first (find and delete by flowid) - // Filters are created with 'basic' classifier, format: "handle 0xN flowid 1:xxxx" - filterCmd := exec.Command("tc", "filter", "show", "dev", bridgeName) - filterCmd.SysProcAttr = &syscall.SysProcAttr{ - AmbientCaps: []uintptr{unix.CAP_NET_ADMIN}, - } - if filterOutput, err := filterCmd.Output(); err == nil { - filterLines := strings.Split(string(filterOutput), "\n") - for _, fline := range filterLines { - if strings.Contains(fline, fullClassID) { - // Extract filter handle (format: "handle 0x2 flowid 1:ffd") - ffields := strings.Fields(fline) - for i, f := range ffields { - if f == "handle" && i+1 < len(ffields) { - handle := ffields[i+1] - // Use 'basic' classifier (not u32) to match how filters were created - delCmd := exec.Command("tc", "filter", "del", "dev", bridgeName, "parent", "1:", "handle", handle, "prio", "1", "basic") - delCmd.SysProcAttr = &syscall.SysProcAttr{ - AmbientCaps: []uintptr{unix.CAP_NET_ADMIN}, - } - delCmd.Run() // Best effort - break - } - } - } - } - } - - // Delete child qdisc (fq_codel) before deleting the class - delQdiscCmd := exec.Command("tc", "qdisc", "del", "dev", bridgeName, "parent", fullClassID) - delQdiscCmd.SysProcAttr = &syscall.SysProcAttr{ - AmbientCaps: []uintptr{unix.CAP_NET_ADMIN}, - } - delQdiscCmd.Run() // Best effort - may not exist - - // Delete the class - delClassCmd := exec.Command("tc", "class", "del", "dev", bridgeName, "classid", fullClassID) - delClassCmd.SysProcAttr = &syscall.SysProcAttr{ - AmbientCaps: []uintptr{unix.CAP_NET_ADMIN}, - } - if output, err := delClassCmd.CombinedOutput(); err != nil { - log.WarnContext(ctx, "failed to delete orphaned class", "class", fullClassID, "error", err, "output", string(output)) + if err := deleteBridgeClass(bridgeName, fullClassID); err != nil { + log.WarnContext(ctx, "failed to delete orphaned class", "class", fullClassID, "error", err) continue } deleted++ diff --git a/lib/network/bridge_linux_test.go b/lib/network/bridge_linux_test.go new file mode 100644 index 00000000..3c86fd1b --- /dev/null +++ b/lib/network/bridge_linux_test.go @@ -0,0 +1,68 @@ +//go:build linux + +package network + +import ( + "testing" + + "github.com/stretchr/testify/assert" +) + +func TestParseBridgeFilters(t *testing.T) { + output := `filter parent 1: protocol all pref 1 basic chain 0 +filter parent 1: protocol all pref 1 basic chain 0 handle 0x1 flowid 1:a3f2 + meta(rt_iif eq 42) +filter parent 1: protocol all pref 1 basic chain 0 handle 0x2 flowid 1:b001 + meta(rt_iif eq 57) +filter parent 1: protocol all pref 1 basic chain 0 handle 0x3 flowid 1:000c +` + + assert.Equal(t, []bridgeFilter{ + {handle: "0x1", flowID: "1:a3f2", rtIif: 42}, + {handle: "0x2", flowID: "1:b001", rtIif: 57}, + {handle: "0x3", flowID: "1:000c", rtIif: -1}, + }, parseBridgeFilters(output)) +} + +func TestParseBridgeFiltersEmpty(t *testing.T) { + assert.Empty(t, parseBridgeFilters("")) +} + +func TestCountBridgeHTBClassesExcludesRoot(t *testing.T) { + output := `class htb 1:1 root rate 100Mbit ceil 100Mbit burst 1600b cburst 1600b +class htb 1:a3f2 parent 1:1 leaf e001: prio 1 rate 1Mbit ceil 1Mbit burst 1600b cburst 1600b +class htb 1:b001 parent 1:1 leaf e002: prio 1 rate 1Mbit ceil 1Mbit burst 1600b cburst 1600b +` + + assert.Equal(t, int64(2), countBridgeHTBClasses(parseBridgeClasses(output))) +} + +func TestPlanOrphanedBridgeTC(t *testing.T) { + staleFilters, staleClasses, safe := planOrphanedBridgeTC( + map[int]bool{42: true}, + []bridgeFilter{ + {handle: "0x1", flowID: "1:a3f2", rtIif: 42}, + {handle: "0x2", flowID: "1:b001", rtIif: 57}, + {handle: "0x3", flowID: "1:000c", rtIif: -1}, + }, + []string{"1:1", "1:a3f2", "1:b001", "1:000c", "1:9999"}, + ) + + assert.True(t, safe) + assert.Equal(t, []bridgeFilter{ + {handle: "0x2", flowID: "1:b001", rtIif: 57}, + }, staleFilters) + assert.Equal(t, []string{"1:b001", "1:9999"}, staleClasses) +} + +func TestPlanOrphanedBridgeTCBailsWhenNoRTIIFParses(t *testing.T) { + staleFilters, staleClasses, safe := planOrphanedBridgeTC( + map[int]bool{42: true}, + []bridgeFilter{{handle: "0x1", flowID: "1:a3f2", rtIif: -1}}, + []string{"1:a3f2"}, + ) + + assert.False(t, safe) + assert.Nil(t, staleFilters) + assert.Nil(t, staleClasses) +} diff --git a/lib/network/manager.go b/lib/network/manager.go index 607a38f6..bc657117 100644 --- a/lib/network/manager.go +++ b/lib/network/manager.go @@ -40,6 +40,10 @@ type Manager interface { // against in-flight CreateAllocation calls whose metadata hasn't been persisted. CleanupOrphanedTAPs(ctx context.Context, preserveInstanceIDs []string, minAge time.Duration) int + // CleanupOrphanedClasses removes bridge tc filters/classes not referenced by + // a live TAP's filter. + CleanupOrphanedClasses(ctx context.Context) int + // GetUploadBurstMultiplier returns the configured multiplier for upload burst ceiling. GetUploadBurstMultiplier() int diff --git a/lib/network/manager_test.go b/lib/network/manager_test.go index 51fdc144..b9e0699c 100644 --- a/lib/network/manager_test.go +++ b/lib/network/manager_test.go @@ -127,21 +127,6 @@ func TestPendingAllocationLoadsPersistedClassID(t *testing.T) { assert.Equal(t, "00ab", alloc.ClassID) } -func TestClassIDForDeletePrefersPersistedClassID(t *testing.T) { - m := &manager{ - paths: paths.New(t.TempDir()), - config: &config.Config{}, - } - - const instanceID = "inst-delete" - require.NoError(t, os.MkdirAll(m.paths.InstanceDir(instanceID), 0755)) - require.NoError(t, m.saveClassID(instanceID, "00ab")) - - assert.Equal(t, "00ab", m.classIDForDelete(instanceID, "0010")) - assert.Equal(t, "0010", m.classIDForDelete("missing", "0010")) - assert.Equal(t, "0010", m.classIDForDelete("", "0010")) -} - func TestDefaultNetworkCacheReturnsCopy(t *testing.T) { m := &manager{} m.setDefaultNetwork(&Network{ diff --git a/lib/network/metrics.go b/lib/network/metrics.go index a76b8d82..ee4cd7ff 100644 --- a/lib/network/metrics.go +++ b/lib/network/metrics.go @@ -32,16 +32,29 @@ func newNetworkMetrics(meter metric.Meter, m *manager) (*Metrics, error) { return nil, err } + bridgeHTBClassCount, err := meter.Int64ObservableGauge( + "hypeman_network_bridge_htb_class_count", + metric.WithDescription("Current number of non-root HTB classes on the network bridge"), + ) + if err != nil { + return nil, err + } + _, err = meter.RegisterCallback( func(ctx context.Context, o metric.Observer) error { allocs, err := m.ListAllocations(ctx) - if err != nil { - return nil + if err == nil { + o.ObserveInt64(allocationsTotal, int64(len(allocs))) + } + + classCount, err := m.bridgeHTBClassCount(ctx) + if err == nil { + o.ObserveInt64(bridgeHTBClassCount, classCount) } - o.ObserveInt64(allocationsTotal, int64(len(allocs))) return nil }, allocationsTotal, + bridgeHTBClassCount, ) if err != nil { return nil, err