Detect IPConflicting and gatewayReachable in ipam rather than coordinator #4560

cyclinder · 2025-01-15T07:54:08Z

Thanks for contributing!

Notice:

unite test or E2E test
do not forget essential code comment and log
document for the PR
release note label
"release/none"
"release/bug"
"release/feature"
read about Contribution notice: https://spidernet-io.github.io/spiderpool/latest/develop/contributing/

What issue(s) does this PR fix:

Fixes #4582

Special notes for your reviewer:

作为 #4474 的后续修复：

确保 IP 冲突检测先于网关检测，这防止当 IP 冲突检测失败且网关检测发送 ARP 探测报时意外更新了错误的 ARP 缓存表，导致通信失败。
移动 IP 冲突检测和网关检测的逻辑到 IPAM 中完成，在之前是由 Coordinator 完成。之前检测到 IP 冲突的时候，冲突的 IP 已经配置到了网卡上，可能会导致 IP 偶现不可通信的情况。
在 IPAM 中完成 IP 冲突检测和网关检测，这可能会增加 IPAM 调用的时间，在同时检测 IPv4 和 IPv6 的情况下，大概增加 1 s，具体时间视网络而定。特别对于 IPv6，开启 Duplicate Address Detection（DAD）的情况下，内核会检查本地链路地址是否冲突，这可能会花费一段时间。

As a follow-up fix to spidernet-io/spiderpool#4474:

Ensure that IP conflict detection precedes gateway detection. This prevents the ARP cache table from being incorrectly updated when IP conflict detection fails and gateway detection sends ARP probe packets, which could lead to communication failures.
Move the logic for IP conflict detection and gateway detection into IPAM, which was previously handled by the Coordinator. Previously, when an IP conflict was detected, the conflicting IP was already configured on the network interface card, potentially causing occasional IP communication failures.
Performing IP conflict detection and gateway detection within IPAM may increase the time for IPAM calls. When detecting both IPv4 and IPv6, the time may increase by approximately 1 second, depending on the network. Particularly for IPv6, when Duplicate Address Detection (DAD) is enabled, the kernel will check for conflicts with local link addresses, which may take some time.

weizhoublue · 2025-01-15T09:46:51Z

api/v1/agent/openapi.yaml

@@ -110,6 +110,29 @@ paths:
          description: Forbid to release IPs for stateful workload
          schema:
            $ref: "#/definitions/Error"
+  "/ipam/ip-detection-configs":


我建议不新加接口，而是在原来分配 ip 的接口中增加两个字段，表示是否要进行 2 个 ip 的检测

减少 api 调用，耦合配置在一起

未来如果要基于 ippool crd 中配置子网级别的检测开关，新增这个api，会导致获取该 ip或者所属子网的检测开关非常难实现，因为它拆开了 ip 和该ip 是否要检测的 2个强关联信息

我当时也纠结一阵，本意不想耦合 ipam response 的配置，已更新

weizhoublue · 2025-01-15T12:32:11Z

应该还有一些变更
1 chart
2 crd 定义删减

cyclinder · 2025-01-16T12:13:40Z

应该还有一些变更
1 chart
2 crd 定义删减

chart 已改， CRD 我想在另一个 PR 中统一变更

weizhoublue · 2025-01-17T12:24:18Z

应该还有一些变更
1 chart
2 crd 定义删减

chart 已改， CRD 我想在另一个 PR 中统一变更

如果和这个强相关的修改没有一起，可能会导致 v0.9 和 v1 很难 cherry pick
我想要 CRD 在另一个统一改后，那个 pr 是不能 cherry pick 到 v0.9 的

weizhoublue · 2025-01-17T12:30:24Z

这个应该有相关的 doc 的修改

weizhoublue · 2025-01-23T09:57:36Z

docs/concepts/coordinator.md

@@ -32,15 +32,14 @@ Let's delve into how coordinator implements these features.
 | tunePodRoutes      | Tune the pod's routing tables while a pod is in multi-NIC mode                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                          | bool     | optional   | true        |
 | podDefaultRouteNic | Configure the default routed NIC for the pod while a pod is in multi-NIC mode, The default value is 0, indicate that the first network interface of the pod has the default route.                                                                                                                                                                                                                                                                                                                                                                                                      | string   | optional   | ""          |
 | podDefaultCniNic   | The name of the pod's first NIC defaults to eth0 in kubernetes                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                          | bool     | optional   | eth0        |
-| detectGateway      | Enable gateway detection while creating pods, which prevent pod creation if the gateway is unreachable                                                                                                                                                                                                                                                                                                                                                                                                                                                                                  | bool     | optional   | false       |
-| detectIPConflict   | Enable IP conflicting checking for pods, which prevent pod creation if the pod's ip is conflicting                                                                                                                                                                                                                                                                                                                                                                                                                                                                                      | bool     | optional   | false       |
+| detectGateway      | DEPRECATED: Enable gateway detection while creating pods, which prevent pod creation if the gateway is unreachable                                                                                                                                                                                                                                                                                                                                                                                                                                                                                  | bool     | optional   | false       |


这个 DEPRECATED 描述，更重要的是，在 CRD 定义的 yaml 中的 description，应该也提到

CRD 描述是有的，这个是 coordinator 的配置文件，标记为 DEPRECATED 应该没问题，实际上字段已经被移除

weizhoublue · 2025-01-24T03:00:47Z

charts/spiderpool/crds/spiderpool.spidernet.io_spidermultusconfigs.yaml

@@ -59,12 +59,13 @@ spec:
                properties:
                  detectGateway:
                    default: false
-                    description: DetectGateway to detect the gateway for the pod
+                    description: DetectGateway to detect the gateway for the pod Deprecated,


vlan: default: 0 description: 'DEPRECATED: Vlan is deprecated.'

这是另一个字段的描述，请对齐所有的格式，不要出现好几种风格

weizhoublue · 2025-01-24T03:28:32Z

cmd/spiderpool/cmd/ipam_detection.go

+					return fmt.Errorf("%w: pod's interface %s with an conflicting ip %s, %s is located at %s",
+						constant.ErrIPConflict, d.iface, d.ip4.String(), d.ip4.String(), senderMAC.String())
+				}
+				continue


这里如果没有 IP 冲突，continue ？再尝试发送 arp ？

weizhoublue · 2025-01-24T03:39:05Z

cmd/spiderpool/cmd/ipam_detection.go

+
+	// When DAD(Duplicate Address Detection) is enanled, the kernel will check if this local link address is in conflict,
+	// this may take a while, set the maximum timeout to 10s
+	ndpReady := false


listen 之前，先判断先看 d.enableIPv6ConflictDetection ，是否要花这个时间

weizhoublue · 2025-01-24T03:44:03Z

cmd/spiderpool/cmd/command_add.go

@@ -125,6 +128,35 @@ func CmdAdd(args *skel.CmdArgs) (err error) {
 		return err
 	}

+	// do ip conflict and gateway detection


后面这么多逻辑的前提，是要先判断是否开启了冲突检测和网关检测

在这个函数里面做了判断，也做到尽量提前返回

我看那个函数里还有做一大堆的遍历后再判断，这里依据简单的 if ，是否就先省了一些流程

必须做遍历分配的 IPConfigs 才能知道是否做检测，这一点我之前就考虑到了，已经做到尽量提前判断了

weizhoublue · 2025-01-24T03:46:43Z

cmd/spiderpool/cmd/command_add.go

@@ -50,8 +53,8 @@ func CmdAdd(args *skel.CmdArgs) (err error) {
 		return fmt.Errorf("failed to load CNI network configuration: %v", err)
 	}

-	logger, err = setupFileLogging(conf)
-	if nil != err {
+	logger, err = SetupFileLogging(conf)


如果开启了冲突检测，第一步就先把网卡 up

开启冲突检测一定要等 IP 分配完之后才知道

那可以无论是否要开启，都 up 网卡，应该没毛病
这样可能能节省一半的时间

weizhoublue · 2025-01-24T04:05:09Z

cmd/spiderpool/cmd/ipam_detection.go

+	// CNI will set disable_ipv6 to 0 AFTER ipam is invoked
+	// in order to detect IPv6 address conflicts and gateway reachability
+	// we should ensure disable_ipv6 is 0
+	if err := sysctl.EnableIPv6ForInterfaces([]string{"lo", d.iface}); err != nil {


这个时间可能是不需要花费的，并且意义不大
如果系统本身不能 ipv6 接口工作，有很多参数影响，不是简单这一个参数，主机上的网卡就不能 ipv6 工作
另外，如果有问题，后续 cni 也无法把 ip 配置上去

这其实不是为 ipv6, 我是参考 sriov 的实践，它发送免费 arp/ndp 前会等待网卡就绪

https://github.com/k8snetworkplumbingwg/sriov-cni/blob/d1f41e4c018c456661aa2f27a3cdf737e46e9bcd/cmd/sriov/main.go#L212 如果意义不大，可以考虑移除这段逻辑

Signed-off-by: Cyclinder Kuo <[email protected]>

…spidernet-io#4560) Signed-off-by: Cyclinder Kuo <[email protected]>

…#4560) (#4587) Signed-off-by: Cyclinder Kuo <[email protected]>

cyclinder added the release/feature-new release note for new feature label Jan 15, 2025

cyclinder requested review from weizhoublue and lou-lan as code owners January 15, 2025 07:54

cyclinder marked this pull request as draft January 15, 2025 07:54

cyclinder force-pushed the ipam/dectetion branch from a98228c to b296786 Compare January 15, 2025 08:10

weizhoublue reviewed Jan 15, 2025

View reviewed changes

cyclinder force-pushed the ipam/dectetion branch 2 times, most recently from 392a3f7 to 4f90cd4 Compare January 16, 2025 12:02

cyclinder marked this pull request as ready for review January 16, 2025 12:03

cyclinder force-pushed the ipam/dectetion branch from 4f90cd4 to f32dbd3 Compare January 16, 2025 12:08

cyclinder force-pushed the ipam/dectetion branch 4 times, most recently from 4ee161a to 7230ca0 Compare January 16, 2025 15:40

weizhoublue changed the title ~~Detect IPConflicting and gatewayReachable in ipam without coordinator~~ Detect IPConflicting and gatewayReachable in ipam rather than coordinator Jan 17, 2025

cyclinder force-pushed the ipam/dectetion branch 3 times, most recently from 52520d8 to 8bbd659 Compare January 21, 2025 11:17

weizhoublue added release/bug and removed release/feature-new release note for new feature labels Jan 22, 2025

cyclinder force-pushed the ipam/dectetion branch 6 times, most recently from 4fb933f to bd2a188 Compare January 22, 2025 08:48

weizhoublue reviewed Jan 23, 2025

View reviewed changes

cyclinder force-pushed the ipam/dectetion branch 3 times, most recently from cc1b766 to be0d2e2 Compare January 23, 2025 13:23

weizhoublue reviewed Jan 24, 2025

View reviewed changes

cyclinder force-pushed the ipam/dectetion branch from be0d2e2 to 801c17d Compare January 24, 2025 03:33

weizhoublue reviewed Jan 24, 2025

View reviewed changes

cyclinder force-pushed the ipam/dectetion branch from 801c17d to 3cc98bf Compare January 24, 2025 03:39

weizhoublue reviewed Jan 24, 2025

View reviewed changes

cyclinder force-pushed the ipam/dectetion branch from 3cc98bf to e33a180 Compare January 24, 2025 03:44

weizhoublue reviewed Jan 24, 2025

View reviewed changes

cyclinder force-pushed the ipam/dectetion branch 8 times, most recently from 1ed0bd2 to 189192d Compare January 25, 2025 00:48

cyclinder mentioned this pull request Jan 25, 2025

Remove deprecated fields for v2beta1 APIs #4549

Merged

5 tasks

Detect IPConflicting and gatewayReachable in ipam without coordinator

748e8af

Signed-off-by: Cyclinder Kuo <[email protected]>

cyclinder force-pushed the ipam/dectetion branch from 189192d to 748e8af Compare January 26, 2025 04:09

weizhoublue approved these changes Jan 26, 2025

View reviewed changes

cyclinder merged commit 655c389 into spidernet-io:main Jan 26, 2025
58 of 59 checks passed

cyclinder deleted the ipam/dectetion branch January 26, 2025 06:57

cyclinder added a commit to cyclinder/spiderpool that referenced this pull request Jan 26, 2025

Detect IPConflicting and gatewayReachable in ipam without coordinator (…

4132390

…spidernet-io#4560) Signed-off-by: Cyclinder Kuo <[email protected]>

cyclinder added a commit to cyclinder/spiderpool that referenced this pull request Jan 26, 2025

Detect IPConflicting and gatewayReachable in ipam without coordinator (…

c619841

…spidernet-io#4560) Signed-off-by: Cyclinder Kuo <[email protected]>

cyclinder added a commit that referenced this pull request Jan 26, 2025

Detect IPConflicting and gatewayReachable in ipam without coordinator (…

5a29bf4

…#4560) (#4587) Signed-off-by: Cyclinder Kuo <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Detect IPConflicting and gatewayReachable in ipam rather than coordinator #4560

Detect IPConflicting and gatewayReachable in ipam rather than coordinator #4560

cyclinder commented Jan 15, 2025 •

edited

Loading

weizhoublue Jan 15, 2025 •

edited

Loading

cyclinder Jan 16, 2025

weizhoublue commented Jan 15, 2025

cyclinder commented Jan 16, 2025

weizhoublue commented Jan 17, 2025

weizhoublue commented Jan 17, 2025

weizhoublue Jan 23, 2025

cyclinder Jan 23, 2025

weizhoublue Jan 24, 2025

cyclinder Jan 24, 2025

weizhoublue Jan 24, 2025

weizhoublue Jan 24, 2025

weizhoublue Jan 24, 2025

cyclinder Jan 24, 2025

weizhoublue Jan 24, 2025

cyclinder Jan 24, 2025

weizhoublue Jan 24, 2025

cyclinder Jan 24, 2025

weizhoublue Jan 24, 2025 •

edited

Loading

weizhoublue Jan 24, 2025

cyclinder Jan 24, 2025

cyclinder Jan 24, 2025

cyclinder Jan 24, 2025

Detect IPConflicting and gatewayReachable in ipam rather than coordinator #4560

Detect IPConflicting and gatewayReachable in ipam rather than coordinator #4560

Conversation

cyclinder commented Jan 15, 2025 • edited Loading

Thanks for contributing!

weizhoublue Jan 15, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

weizhoublue commented Jan 15, 2025

cyclinder commented Jan 16, 2025

weizhoublue commented Jan 17, 2025

weizhoublue commented Jan 17, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

weizhoublue Jan 24, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cyclinder commented Jan 15, 2025 •

edited

Loading

weizhoublue Jan 15, 2025 •

edited

Loading

weizhoublue Jan 24, 2025 •

edited

Loading