Remove unnecessary extra ip route calls when checking for routes to add and delete in network module.

Currently, the `ip route` check happens twice -- once when checking if there are missing routes and extra routes, and another time when we actually add missing routes/remove extra routes. This optimization makes it so that we store the extra/missing routes upon the first check, and use those for the actual route setup instead of checking again.

PiperOrigin-RevId: 874856285

Author: drewhli

cmd/core_plugin/network/manager_linux.go

@@ -61,8 +61,12 @@ func managerSetup(ctx context.Context, nics []*nic.Configuration, networkChanged
 	}
 
 	// Attempt to setup the routes.
-	if err := routeSetup(ctx, opts); err != nil {
-		return fmt.Errorf("failed to setup routes: %w", err)
+	if networkChanged.routes {
+		if err := routeSetup(ctx, &networkChanged.routeSetupOptions); err != nil {
+			return fmt.Errorf("failed to setup routes: %w", err)
+		}
+	} else {
+		galog.Debugf("No routes to setup.")
 	}
 
 	galog.Infof("Finished linux network management module setup.")

cmd/core_plugin/network/manager_linux_test.go

@@ -24,6 +24,7 @@ import (
 	"github.com/GoogleCloudPlatform/google-guest-agent/internal/cfg"
 	"github.com/GoogleCloudPlatform/google-guest-agent/internal/network/ethernet"
 	"github.com/GoogleCloudPlatform/google-guest-agent/internal/network/nic"
+	"github.com/GoogleCloudPlatform/google-guest-agent/internal/network/route"
 	"github.com/GoogleCloudPlatform/google-guest-agent/internal/network/service"
 	"github.com/google/go-cmp/cmp"
 )
@@ -467,15 +468,15 @@ func TestNetworkInterfacesSetupDisabled(t *testing.T) {
 	t.Cleanup(func() {
 		routeSetup = oldRouteSetup
 	})
-	routeSetup = func(ctx context.Context, opts *service.Options) error {
+	routeSetup = func(ctx context.Context, opts *route.SetupOptions) error {
 		routeSetupCalled = true
 		return nil
 	}
 
 	testOpts := service.NewOptions(nil, []*nic.Configuration{
 		{Index: 1},
 	})
-	if err := managerSetup(ctx, testOpts.NICConfigs(), networkChanged{true, false}); err != nil {
+	if err := managerSetup(ctx, testOpts.NICConfigs(), networkChanged{true, true, route.SetupOptions{}}); err != nil {
 		t.Errorf("managerSetup(ctx, %+v) = %v, want nil", testOpts, err)
 	}
 
@@ -579,7 +580,7 @@ func TestManagerSetup(t *testing.T) {
 				defaultLinuxManagers = oldManagers
 			})
 
-			err := managerSetup(ctx, tc.opts.NICConfigs(), networkChanged{true, false})
+			err := managerSetup(ctx, tc.opts.NICConfigs(), networkChanged{true, false, route.SetupOptions{ServiceOptions: tc.opts}})
 			if (err == nil) == tc.wantError {
 				t.Errorf("runManagerSetup(ctx, %+v) = %v, want error", tc.opts, err)
 			}

cmd/core_plugin/network/network.go

@@ -155,15 +155,15 @@ func (mod *module) networkSetup(ctx context.Context, config *cfg.Sections, mds *
 	// If the metadata has not changed then we return early to avoid unnecessary
 	// work.
 	metadataChanged := mod.networkMetadataChanged(mds, config)
-	routeChanged := mod.routeChanged(ctx, nicConfigs)
+	routeChanged, setupOptions := mod.routeChanged(ctx, nicConfigs)
 	if !metadataChanged && !routeChanged && !mod.failedConfiguration {
 		return true, nil
 	}
 
 	galog.V(1).Debugf("Network metadata has changed or failed configuration, setting up network interfaces.")
 
 	// Forward the network configuration to the platform's network manager.
-	if err := managerSetup(ctx, nicConfigs, networkChanged{networkInterfaces: metadataChanged, routes: routeChanged}); err != nil {
+	if err := managerSetup(ctx, nicConfigs, networkChanged{networkInterfaces: metadataChanged, routes: routeChanged, routeSetupOptions: setupOptions}); err != nil {
 		failedSetup = true
 		return false, fmt.Errorf("failed to setup network interfaces: %v", err)
 	}
@@ -178,6 +178,8 @@ type networkChanged struct {
 	networkInterfaces bool
 	// routes indicates if the routes have changed.
 	routes bool
+	// routeSetupOptions is the route setup options.
+	routeSetupOptions route.SetupOptions
 }
 
 // networkMetadataChanged returns true if the metadata has changed or if it's being
@@ -213,26 +215,36 @@ func (mod *module) networkMetadataChanged(mds *metadata.Descriptor, config *cfg.
 // routeChanged returns true if the route metadata has changed, or if the routes
 // present on the system have changed from what is expected based on the network
 // interfaces configuration.
-func (mod *module) routeChanged(ctx context.Context, nicConfigs []*nic.Configuration) bool {
+func (mod *module) routeChanged(ctx context.Context, nicConfigs []*nic.Configuration) (bool, route.SetupOptions) {
+	missingRoutes := make(map[string][]route.Handle)
+	extraRoutes := make(map[string][]route.Handle)
+
+	var foundChanged bool
 	for _, nic := range nicConfigs {
 		if nic.Invalid || nic.Interface == nil {
 			continue
 		}
 		wantedRoutes := nic.ExtraAddresses.MergedMap()
 
-		if missing, err := route.MissingRoutes(ctx, nic.Interface.Name(), wantedRoutes); err != nil {
+		missing, err := route.MissingRoutes(ctx, nic.Interface.Name(), wantedRoutes)
+		if err != nil {
 			galog.V(2).Debugf("Failed to get missing routes for interface %q: %v", nic.Interface.Name(), err)
-			continue
 		} else if len(missing) > 0 {
-			return true
+			foundChanged = true
+			missingRoutes[nic.Interface.Name()] = missing
 		}
 
-		if extra, err := route.ExtraRoutes(ctx, nic.Interface.Name(), wantedRoutes); err != nil {
+		extra, err := route.ExtraRoutes(ctx, nic.Interface.Name(), wantedRoutes)
+		if err != nil {
 			galog.V(2).Debugf("Failed to get extra routes for interface %q: %v", nic.Interface.Name(), err)
-			continue
 		} else if len(extra) > 0 {
-			return true
+			foundChanged = true
+			extraRoutes[nic.Interface.Name()] = extra
 		}
 	}
-	return false
+	return foundChanged, route.SetupOptions{
+		AlreadyChecked: true,
+		MissingRoutes:  missingRoutes,
+		ExtraRoutes:    extraRoutes,
+	}
 }

cmd/core_plugin/network/network_linux_test.go

@@ -63,10 +63,12 @@ func TestRouteChanged(t *testing.T) {
 	}
 
 	tests := []struct {
-		name           string
-		nicConfigs     []*nic.Configuration
-		hasExtraRoutes bool
-		want           bool
+		name             string
+		nicConfigs       []*nic.Configuration
+		hasExtraRoutes   bool
+		want             bool
+		numExtraRoutes   int
+		numMissingRoutes int
 	}{
 		{
 			name: "no-change-empty-extra-addresses",
@@ -125,7 +127,8 @@ func TestRouteChanged(t *testing.T) {
 					ExtraAddresses: address.NewExtraAddresses(desc.Instance().NetworkInterfaces()[0], cfg.Retrieve(), nil),
 				},
 			},
-			want: true,
+			want:             true,
+			numMissingRoutes: 2,
 		},
 		{
 			name: "change-extra-route",
@@ -139,6 +142,7 @@ func TestRouteChanged(t *testing.T) {
 			},
 			hasExtraRoutes: true,
 			want:           true,
+			numExtraRoutes: 2,
 		},
 	}
 
@@ -151,10 +155,18 @@ func TestRouteChanged(t *testing.T) {
 			})
 
 			mod := &module{}
-			got := mod.routeChanged(context.Background(), test.nicConfigs)
+			got, opts := mod.routeChanged(context.Background(), test.nicConfigs)
 			if got != test.want {
 				t.Errorf("routeChanged(%v) = %t, want %t", test.nicConfigs, got, test.want)
 			}
+
+			if len(opts.ExtraRoutes["eth0"]) != test.numExtraRoutes {
+				t.Errorf("routeChanged(%v) = %v, want %d extra routes", test.nicConfigs, opts.ExtraRoutes, test.numExtraRoutes)
+			}
+
+			if len(opts.MissingRoutes["eth0"]) != test.numMissingRoutes {
+				t.Errorf("routeChanged(%v) = %v, want %d missing routes", test.nicConfigs, opts.MissingRoutes, test.numMissingRoutes)
+			}
 		})
 	}
 }

cmd/core_plugin/network/network_windows_test.go

@@ -42,7 +42,7 @@ func TestRouteChanged(t *testing.T) {
 	}
 
 	mod := &module{}
-	got := mod.routeChanged(context.Background(), nicConfigs)
+	got, _ := mod.routeChanged(context.Background(), nicConfigs)
 	if got {
 		t.Errorf("routeChanged(%v) = %t, want false", nicConfigs, got)
 	}

cmd/ggactl/commands/routes/routes.go

@@ -71,7 +71,7 @@ func setupRoutes(cmd *cobra.Command, args []string) error {
 	opts := service.NewOptions(nil, nicConfigs)
 
 	// Add the routes, if any.
-	if err := route.Setup(ctx, opts); err != nil {
+	if err := route.Setup(ctx, &route.SetupOptions{ServiceOptions: opts}); err != nil {
 		return fmt.Errorf("failed to setup routes: %v", err)
 	}
 	return nil

internal/network/route/route.go

@@ -83,6 +83,21 @@ type Options struct {
 	Device string
 }
 
+// SetupOptions wraps route setup options.
+type SetupOptions struct {
+	// ServiceOptions contains the NIC configurations for route setup.
+	ServiceOptions *service.Options
+	// AlreadyChecked indicates whether missing and extra routes have already
+	// been checked.
+	AlreadyChecked bool
+	// MissingRoutes contains the routes that are missing from the system. This
+	// is only relevant if AlreadyChecked is true.
+	MissingRoutes map[string][]Handle
+	// ExtraRoutes contains the routes that are extra to the system. This is only
+	// relevant if AlreadyChecked is true.
+	ExtraRoutes map[string][]Handle
+}
+
 // routeOperations is the interface for a route backend.
 type routeOperations interface {
 	// Add adds a route to the system.
@@ -103,7 +118,7 @@ type routeOperations interface {
 	// interface.
 	RemoveRoutes(ctx context.Context, iface string) error
 	// Setup sets up the routes for the network interfaces.
-	Setup(ctx context.Context, opts *service.Options) error
+	Setup(ctx context.Context, opts *SetupOptions) error
 }
 
 // Add adds a route to the system.
@@ -133,7 +148,7 @@ func MissingRoutes(ctx context.Context, iface string, wantedRoutes address.IPAdd
 }
 
 // Setup sets up the routes for the network interfaces.
-func Setup(ctx context.Context, opts *service.Options) error {
+func Setup(ctx context.Context, opts *SetupOptions) error {
 	return client.Setup(ctx, opts)
 }
 

internal/network/route/route_linux.go

@@ -25,7 +25,6 @@ import (
 	"github.com/GoogleCloudPlatform/galog"
 	"github.com/GoogleCloudPlatform/google-guest-agent/internal/cfg"
 	"github.com/GoogleCloudPlatform/google-guest-agent/internal/network/address"
-	"github.com/GoogleCloudPlatform/google-guest-agent/internal/network/service"
 	"github.com/GoogleCloudPlatform/google-guest-agent/internal/run"
 	"golang.org/x/exp/maps"
 )
@@ -398,16 +397,40 @@ func (lc *linuxClient) RemoveRoutes(ctx context.Context, iface string) error {
 
 // SetupRoutes sets up the routes for the network interfaces. This uses ip route
 // commands to delete extra routes and add missing routes.
-func (lc *linuxClient) Setup(ctx context.Context, opts *service.Options) error {
-	nicConfigs := opts.FilteredNICConfigs()
+func (lc *linuxClient) Setup(ctx context.Context, opts *SetupOptions) error {
+	if opts == nil {
+		galog.Debugf("No setup options provided.")
+		return nil
+	}
+	galog.Debugf("Running ip route setup.")
+
+	if opts.AlreadyChecked {
+		if len(opts.MissingRoutes) == 0 && len(opts.ExtraRoutes) == 0 {
+			galog.Debugf("No routes to setup.")
+			return nil
+		}
+
+		for iface, routes := range opts.ExtraRoutes {
+			lc.deleteExtraRoutes(ctx, iface, routes)
+		}
+		for iface, routes := range opts.MissingRoutes {
+			lc.addMissingRoutes(ctx, iface, routes)
+		}
+		return nil
+	}
+
+	if opts.ServiceOptions == nil {
+		galog.Debugf("No service options provided.")
+		return nil
+	}
+	nicConfigs := opts.ServiceOptions.FilteredNICConfigs()
 	if len(nicConfigs) == 0 {
 		galog.Debugf("No NICs to setup routes for.")
 		return nil
 	}
-	galog.Debugf("Running ip route setup.")
 
 	// Fallback route setup uses ip commands.
-	for _, nic := range opts.FilteredNICConfigs() {
+	for _, nic := range nicConfigs {
 		if nic.Interface == nil {
 			galog.Debugf("Skipping route setup for interface index %d: interface is nil", nic.Index)
 			continue
@@ -418,44 +441,60 @@ func (lc *linuxClient) Setup(ctx context.Context, opts *service.Options) error {
 		}
 
 		// Find extra routes to delete.
-		extraAddrs := nic.ExtraAddresses.MergedMap()
-		extraRoutes, err := lc.ExtraRoutes(ctx, nic.Interface.Name(), extraAddrs)
-		if err != nil {
-			return fmt.Errorf("failed to get extra routes for interface %q: %w", nic.Interface.Name(), err)
-		}
-
-		if len(extraRoutes) == 0 {
-			galog.Debugf("No extra routes to delete for interface %q", nic.Interface.Name())
+		var extraAddrs address.IPAddressMap
+		var extraRoutes []Handle
+		var err error
+		if opts.AlreadyChecked {
+			extraRoutes = opts.ExtraRoutes[nic.Interface.Name()]
 		} else {
-			galog.Infof("Deleting extra routes %v for interface %q", extraRoutes, nic.Interface.Name())
-			for _, r := range extraRoutes {
-				if err = lc.Delete(ctx, r); err != nil {
-					// Continue to delete the rest of the routes, and only log the error.
-					galog.Errorf("Failed to delete route %q for interface %q: %v", r.Destination.String(), nic.Interface.Name(), err)
-				}
+			extraAddrs = nic.ExtraAddresses.MergedMap()
+			extraRoutes, err = lc.ExtraRoutes(ctx, nic.Interface.Name(), extraAddrs)
+			if err != nil {
+				return fmt.Errorf("failed to get extra routes for interface %q: %w", nic.Interface.Name(), err)
 			}
 		}
+		lc.deleteExtraRoutes(ctx, nic.Interface.Name(), extraRoutes)
 
 		// Find missing routes for the given interface.
-		missingRoutes, err := lc.MissingRoutes(ctx, nic.Interface.Name(), extraAddrs)
-		if err != nil {
-			return fmt.Errorf("failed to get missing routes for interface %q: %w", nic.Interface.Name(), err)
+		var missingRoutes []Handle
+		if opts.AlreadyChecked {
+			missingRoutes = opts.MissingRoutes[nic.Interface.Name()]
+		} else {
+			missingRoutes, err = lc.MissingRoutes(ctx, nic.Interface.Name(), extraAddrs)
+			if err != nil {
+				return fmt.Errorf("failed to get missing routes for interface %q: %w", nic.Interface.Name(), err)
+			}
 		}
+		lc.addMissingRoutes(ctx, nic.Interface.Name(), missingRoutes)
+	}
+	galog.Debugf("Finished ip route setup.")
+	return nil
+}
 
-		if len(missingRoutes) == 0 {
-			galog.Debugf("No missing routes to add for interface %q", nic.Interface.Name())
-			continue
+func (lc *linuxClient) deleteExtraRoutes(ctx context.Context, iface string, extraRoutes []Handle) {
+	if len(extraRoutes) == 0 {
+		galog.Debugf("No extra routes to delete for interface %q", iface)
+		return
+	}
+	galog.Infof("Deleting extra routes %v for interface %q", extraRoutes, iface)
+	for _, r := range extraRoutes {
+		if err := lc.Delete(ctx, r); err != nil {
+			// Continue to delete the rest of the routes, and only log the error.
+			galog.Errorf("Failed to delete route %q for interface %q: %v", r.Destination.String(), iface, err)
 		}
-		galog.Infof("Adding routes %v for interface %q", missingRoutes, nic.Interface.Name())
+	}
+}
 
-		// Add the missing routes.
-		for _, r := range missingRoutes {
-			if err = lc.Add(ctx, r); err != nil {
-				// Continue to add the rest of the routes, and only log the error.
-				galog.Errorf("Failed to add route %q for interface %q: %v", r.Destination.String(), nic.Interface.Name(), err)
-			}
+func (lc *linuxClient) addMissingRoutes(ctx context.Context, iface string, missingRoutes []Handle) {
+	if len(missingRoutes) == 0 {
+		galog.Debugf("No missing routes to add for interface %q", iface)
+		return
+	}
+	galog.Infof("Adding routes %v for interface %q", missingRoutes, iface)
+	for _, r := range missingRoutes {
+		if err := lc.Add(ctx, r); err != nil {
+			// Continue to add the rest of the routes, and only log the error.
+			galog.Errorf("Failed to add route %q for interface %q: %v", r.Destination.String(), iface, err)
 		}
 	}
-	galog.Debugf("Finished ip route setup.")
-	return nil
 }