fix: move link setup from CmdAdd to DetectIPConflictAndGatewayReachable

* move netlink link setup logic from command_add.go to ipam_detection.go
* set link up inside DetectIPConflictAndGatewayReachable before detection
* change early return to continue when IP version is nil during detection
* remove netlink import from command_add.go

Signed-off-by: Cyclinder Kuo <qifeng.guo@daocloud.io>

Author: cyclinder

cmd/spiderpool/cmd/command_add.go

@@ -16,7 +16,6 @@ import (
 	current "github.com/containernetworking/cni/pkg/types/100"
 	"github.com/containernetworking/plugins/pkg/ns"
 	"github.com/go-openapi/strfmt"
-	"github.com/vishvananda/netlink"
 	"go.uber.org/multierr"
 	"go.uber.org/zap"
 
@@ -66,25 +65,6 @@ func CmdAdd(args *skel.CmdArgs) (err error) {
 		return fmt.Errorf("failed to setup file logging: %w", err)
 	}
 
-	// When IPAM is invoked, the NIC is down and must be set it up in order to detect IP conflicts and
-	// gateway reachability.
-	err = netns.Do(func(netNS ns.NetNS) error {
-		l, err := netlink.LinkByName(args.IfName)
-		if err != nil {
-			return fmt.Errorf("failed to get link: %w", err)
-		}
-
-		if err = netlink.LinkSetUp(l); err != nil {
-			return fmt.Errorf("failed to set link up: %w", err)
-		}
-
-		logger.Sugar().Debugf("Set link %s to up for IP conflict and gateway detection", args.IfName)
-		return nil
-	})
-	if err != nil {
-		return fmt.Errorf("failed to set link up: %w", err)
-	}
-
 	hostNs, err := ns.GetCurrentNS()
 	if err != nil {
 		return fmt.Errorf("failed to get current netns: %w", err)

cmd/spiderpool/cmd/command_delete.go

@@ -89,7 +89,7 @@ func CmdDel(args *skel.CmdArgs) (err error) {
 		return err
 	}
 
-	ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second)
+	ctx, cancel := context.WithTimeout(context.Background(), 100*time.Second)
 	defer cancel()
 
 	params := daemonset.NewDeleteIpamIPParams().

images/spiderpool-plugins/version.sh

@@ -18,4 +18,4 @@ export IB_SRIOV_VERSION=${IB_SRIOV_VERSION:-"v1.3.0"}
 # https://github.com/Mellanox/ipoib-cni
 export IPOIB_VERSION=${IPOIB_VERSION:-"v1.2.2"}
 # https://github.com/spidernet-io/vlan-cni
-export VLAN_VERSION=${VLAN_VERSION:-"0.0.1"}
+export VLAN_VERSION=${VLAN_VERSION:-"v0.0.1"}

pkg/gcmanager/scanAll_IPPool.go

@@ -402,12 +402,13 @@ func (s *SpiderGC) executeScanAll(ctx context.Context) {
 						if endpoint != nil {
 							nodeName = endpoint.Status.Current.Node
 						}
-						if releaseErr := s.iaasClient.ReleaseIPs(ctx, &iaasclient.ReleaseIPsRequest{
+						if releaseErr := s.iaasClient.ReleaseIP(ctx, &iaasclient.ReleaseIPRequest{
 							PodName:      podName,
 							PodNamespace: podNS,
 							PodUID:       poolIPAllocation.PodUID,
 							NodeName:     nodeName,
-							IPAddresses:  []string{poolIP},
+							Subnet:       pool.Spec.Subnet,
+							IPAddress:    poolIP,
 						}); releaseErr != nil {
 							scanAllLogger.Sugar().Errorf("failed to release IaaS IP '%s', error: '%v'", poolIP, releaseErr)
 						} else {

pkg/gcmanager/tracePod_worker.go

@@ -6,6 +6,7 @@ package gcmanager
 import (
 	"context"
 	"fmt"
+	"net"
 	"sync"
 	"sync/atomic"
 	"time"
@@ -163,28 +164,30 @@ func (s *SpiderGC) releaseIPPoolIPExecutor(ctx context.Context, workerIndex int)
 
 				// Release IPs from IaaS provider after releasing from internal IPPools
 				if s.iaasClient != nil {
-					var ipAddresses []string
 					for _, detail := range endpoint.Status.Current.IPs {
 						if detail.IPv4 != nil {
-							ipAddresses = append(ipAddresses, *detail.IPv4)
+							ip, subnet, err := net.ParseCIDR(*detail.IPv4)
+							if err != nil {
+								log.Sugar().Errorf("failed to parse CIDR '%s', error: %v, skip releasing IaaS IP '%s'", *detail.IPv4, err, *detail.IPv4)
+								continue
+							}
+							req := &iaasclient.ReleaseIPRequest{
+								PodName:      podCache.PodName,
+								PodNamespace: podCache.Namespace,
+								PodUID:       podCache.UID,
+								NodeName:     endpoint.Status.Current.Node,
+								Subnet:       subnet.String(),
+								IPAddress:    ip.String(),
+							}
+							if err := s.iaasClient.ReleaseIP(ctx, req); err != nil {
+								log.Sugar().Errorf("failed to release IaaS IP '%s' for '%s/%s', error: %v",
+									ip.String(), podCache.Namespace, podCache.PodName, err)
+								return err
+							}
+							log.Sugar().Infof("successfully released IaaS IP '%s' for '%s/%s'",
+								ip.String(), podCache.Namespace, podCache.PodName)
 						}
 					}
-					if len(ipAddresses) > 0 {
-						req := &iaasclient.ReleaseIPsRequest{
-							PodName:      podCache.PodName,
-							PodNamespace: podCache.Namespace,
-							PodUID:       podCache.UID,
-							NodeName:     endpoint.Status.Current.Node,
-							IPAddresses:  ipAddresses,
-						}
-						if err := s.iaasClient.ReleaseIPs(ctx, req); err != nil {
-							log.Sugar().Errorf("failed to release IaaS IPs for '%s/%s', error: %v",
-								podCache.Namespace, podCache.PodName, err)
-							return err
-						}
-						log.Sugar().Infof("successfully released IaaS IPs %v for '%s/%s'",
-							ipAddresses, podCache.Namespace, podCache.PodName)
-					}
 				}
 
 				// delete StatefulSet/kubevirtVMI wep (other controller wep has OwnerReference, its lifecycle is same with pod)

pkg/iaas/client/client.go

@@ -10,7 +10,6 @@ import (
 	"encoding/json"
 	"fmt"
 	"io"
-	"net"
 	"net/http"
 	"net/url"
 	"sync"
@@ -26,25 +25,17 @@ const (
 	releaseAPIPath  = "/v1/apis/network.iaas.io/ipam/release-ip"
 )
 
-// ParentNicMacLookupFunc is a fallback function to look up parentNicMac
-// when the cache does not have the value. It receives the context and the IP CIDR string.
-type ParentNicMacLookupFunc func(ctx context.Context, ipCIDR string) (string, error)
-
 // Client is the interface for IaaS provider API client
 type Client interface {
 	// AllocateIPs calls the IaaS provider to allocate IPs
 	AllocateIPs(ctx context.Context, req *AllocateIPRequest) (*AllocateIPResponse, error)
 	// ReleaseIPs calls the IaaS provider to release IPs
-	ReleaseIPs(ctx context.Context, req *ReleaseIPsRequest) error
+	ReleaseIP(ctx context.Context, req *ReleaseIPRequest) error
 	// GetCachedParentNicMac returns the cached parent NIC MAC for the given key,
-	// or empty string if not cached. Key can be SpiderMultusConfig namespace/name
-	// or IP CIDR string.
+	// or empty string if not cached. Key is SpiderMultusConfig namespace/name.
 	GetCachedParentNicMac(key string) (string, bool)
 	// CacheParentNicMac stores a parent NIC MAC for the given key.
 	CacheParentNicMac(key string, mac string)
-	// SetParentNicMacLookupFunc sets a fallback lookup function for parentNicMac
-	// when cache misses (e.g., after agent restart).
-	SetParentNicMacLookupFunc(fn ParentNicMacLookupFunc)
 }
 
 // IaaSClient implements the Client interface
@@ -54,13 +45,8 @@ type IaaSClient struct {
 	logger     *zap.Logger
 
 	// parentNicMacCache caches key -> parent NIC MAC address.
-	// Keys include both SpiderMultusConfig namespace/name and IP CIDR strings,
-	// so that release path can look up parentNicMac by IP.
+	// Keys use SpiderMultusConfig namespace/name.
 	parentNicMacCache sync.Map
-
-	// parentNicMacLookupFunc is a fallback function to look up parentNicMac
-	// when the cache does not have the value (e.g., after agent restart).
-	parentNicMacLookupFunc ParentNicMacLookupFunc
 }
 
 // ValidateConfig validates the IaaS provider configuration.
@@ -177,62 +163,40 @@ func (c *IaaSClient) AllocateIPs(ctx context.Context, req *AllocateIPRequest) (*
 	return &allocateResp, nil
 }
 
-// ReleaseIPs calls the IaaS provider to release IPs.
-// The provider only supports releasing one IP per request, so this method
-// loops over each IP and calls the API individually.
-func (c *IaaSClient) ReleaseIPs(ctx context.Context, req *ReleaseIPsRequest) error {
+// ReleaseIP calls the IaaS provider to release an IP.
+func (c *IaaSClient) ReleaseIP(ctx context.Context, req *ReleaseIPRequest) error {
 	c.logger.Debug("Calling IaaS release API",
 		zap.String("url", c.baseURL),
 		zap.String("nodeName", req.NodeName),
-		zap.String("podName", req.PodName),
-		zap.String("podNamespace", req.PodNamespace),
-		zap.Strings("ipAddresses", req.IPAddresses),
+		zap.String("ipAddress", req.IPAddress),
+		zap.String("subnet", req.Subnet),
+		zap.String("parentNicMac", req.ParentNicMac),
 	)
 
 	reqURL, err := url.JoinPath(c.baseURL, releaseAPIPath)
 	if err != nil {
 		return fmt.Errorf("failed to construct release URL: %w", err)
 	}
 
-	for _, ip := range req.IPAddresses {
-		c.logger.Debug("Releasing single IP via IaaS", zap.String("ip", ip))
-
-		ipstr, ipnet, err := net.ParseCIDR(ip)
-		if err != nil {
-			c.logger.Error("Failed to parse IP for release", zap.String("ip", ip), zap.Error(err))
-			return fmt.Errorf("failed to parse IP %s: %w", ip, err)
-		}
-
-		// Look up parentNicMac via lookup function (queries SMC-keyed cache or resolves from SpiderMultusConfig)
-		var parentNicMac string
-		if c.parentNicMacLookupFunc != nil {
-			mac, lookupErr := c.parentNicMacLookupFunc(ctx, ip)
-			if lookupErr != nil {
-				c.logger.Warn("Failed to lookup parentNicMac, proceeding with empty value",
-					zap.String("ip", ip), zap.Error(lookupErr))
-			} else {
-				parentNicMac = mac
-			}
-		} else {
-			c.logger.Warn("No parentNicMac lookup function configured, proceeding with empty value",
-				zap.String("ip", ip))
-		}
-
-		singleReq := &ReleaseIPRequest{
-			NodeName:     req.NodeName,
-			IPAddress:    ipstr.String(),
-			Subnet:       ipnet.String(),
-			ParentNicMac: parentNicMac,
-		}
-
-		if err := c.releaseSingleIP(ctx, reqURL, singleReq); err != nil {
-			return fmt.Errorf("failed to release IP %s: %w", ip, err)
-		}
+	singleReq := &ReleaseIPRequest{
+		PodName:      req.PodName,
+		PodNamespace: req.PodNamespace,
+		PodUID:       req.PodUID,
+		NodeName:     req.NodeName,
+		IPAddress:    req.IPAddress,
+		Subnet:       req.Subnet,
+		ParentNicMac: req.ParentNicMac,
+	}
+
+	if err := c.releaseSingleIP(ctx, reqURL, singleReq); err != nil {
+		return fmt.Errorf("failed to release IP %s: %w", req.IPAddress, err)
 	}
 
 	c.logger.Info("IaaS release API succeeded",
 		zap.String("nodeName", req.NodeName),
-		zap.Strings("ipAddresses", req.IPAddresses),
+		zap.String("ipAddress", req.IPAddress),
+		zap.String("subnet", req.Subnet),
+		zap.String("parentNicMac", req.ParentNicMac),
 	)
 
 	return nil
@@ -293,12 +257,6 @@ func (c *IaaSClient) CacheParentNicMac(key string, mac string) {
 	c.parentNicMacCache.Store(key, mac)
 }
 
-// SetParentNicMacLookupFunc sets a fallback lookup function for parentNicMac
-// when cache misses (e.g., after agent restart).
-func (c *IaaSClient) SetParentNicMacLookupFunc(fn ParentNicMacLookupFunc) {
-	c.parentNicMacLookupFunc = fn
-}
-
 // Close closes the IaaS client
 func (c *IaaSClient) Close() error {
 	return nil

pkg/iaas/client/types.go

@@ -54,7 +54,7 @@ type IaaSIPAllocationResult struct {
 }
 
 // ReleaseIPRequest represents the request body for IaaS IP release API
-type ReleaseIPsRequest struct {
+type ReleaseIPRequest struct {
 	// PodName is optional
 	PodName string `json:"podName,omitempty"`
 	// PodNamespace is optional
@@ -63,15 +63,10 @@ type ReleaseIPsRequest struct {
 	PodUID string `json:"podUID,omitempty"`
 	// NodeName is required
 	NodeName string `json:"nodeName"`
-	// IPAddresses are the IPs being released
-	IPAddresses []string `json:"ipAddresses"`
-}
-
-type ReleaseIPRequest struct {
-	// NodeName is required
-	NodeName string `json:"nodeName"`
+	// ParentNicMac is optional
+	ParentNicMac string `json:"parentNicMac,omitempty"`
+	// Subnet is required
+	Subnet string `json:"subnet"`
 	// IPAddress is the IP being released
-	IPAddress    string `json:"ipAddress"`
-	Subnet       string `json:"subnet"`
-	ParentNicMac string `json:"parentNicMac"`
+	IPAddress string `json:"ipAddress"`
 }