coordinator should only set rp_filter for pod but not the node #3906

cyclinder · 2024-08-20T06:37:18Z

Thanks for contributing!

What type of PR is this?

release/bug

What this PR does / why we need it:

keep hostRPFilter and add a new filed: podRPFilter, coordinater do not set rp_filter for each nodes, this should done by spiderpool-agent, see agent set rp_filter to 0 for the each node #3898

Which issue(s) this PR fixes:

Fixes #3905

Special notes for your reviewer:

codecov · 2024-08-20T06:42:37Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 81.28%. Comparing base (a5a29e1) to head (7e94f03).
Report is 10 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #3906   +/-   ##
=======================================
  Coverage   81.28%   81.28%           
=======================================
  Files          50       50           
  Lines        4408     4408           
=======================================
  Hits         3583     3583           
  Misses        669      669           
  Partials      156      156

Flag	Coverage Δ
unittests	`81.28% <ø> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

cyclinder · 2024-08-20T07:01:36Z

wait for #3908 and #3907.

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

weizhoublue · 2024-08-20T15:12:11Z

1 如果是bug 修复，项目承诺的是 0.8 0.9 是稳定版本，为什么只关注 0.9 的历史版本
2 这种 api break change 的修改代码，应该是升级有问题的，为什么 upgrade ci 没有测试出来 @ty-dc

cyclinder · 2024-08-21T02:08:37Z

因为 #3898 只 cherry-pick 到了 0.9 和 1.0, 这个 PR 依赖它。这个 PR 既有feature ，也有一个小的 bug fixes

ty-dc · 2024-08-21T03:34:32Z

1 如果是bug 修复，项目承诺的是 0.8 0.9 是稳定版本，为什么只关注 0.9 的历史版本 2 这种 api break change 的修改代码，应该是升级有问题的，为什么 upgrade ci 没有测试出来 @ty-dc

我稍后检查

weizhoublue · 2024-08-21T03:36:34Z

charts/spiderpool/crds/spiderpool.spidernet.io_spidercoordinators.yaml

@@ -48,9 +48,6 @@ spec:
                items:
                  type: string
                type: array
-              hostRPFilter:


这个需要保留，描述 deprecated ，后续处理

weizhoublue · 2024-08-21T09:29:52Z

charts/spiderpool/crds/spiderpool.spidernet.io_spidercoordinators.yaml

@@ -78,6 +81,9 @@ spec:
                type: string
              podMACPrefix:
                type: string
+              podRPFilter:


这个描述是不清晰的
什么值是不设置，保持默认，什么值是会生效，取值范围等

这整个 CRD 中的 field 都添加一些使用描述

如果只是 0 1 2 ，那用什么值表示不做任何动作，保留 pod 中的原始值

你说的对，目前代码 -1 表示不做任何操作

为什么只是 -1 ，不是负数。
如果 -2 -3 -4 会发生什么，有bug ？健壮性

weizhoublue · 2024-08-21T11:07:18Z

charts/spiderpool/crds/spiderpool.spidernet.io_spidercoordinators.yaml

                type: boolean
              hijackCIDR:
+                description: HijackCIDR specifies additional destination subnets that


additional destination subnets 这个需要明细下，否则看不懂

weizhoublue · 2024-08-21T11:07:55Z

charts/spiderpool/crds/spiderpool.spidernet.io_spidercoordinators.yaml

                type: integer
              hostRuleTable:
                default: 500
+                description: HostRuleTable specifies the routing table number of the


需要说明这个 table 是做什么用的

weizhoublue · 2024-08-21T11:10:26Z

charts/spiderpool/crds/spiderpool.spidernet.io_spidercoordinators.yaml

                type: boolean
              txQueueLen:
                default: 0
+                description: TxQueueLen to set the tx_queue_len of the pod.


需要说明下，0 代表什么，既然是 integer，负数代表什么

weizhoublue · 2024-08-21T11:12:37Z

charts/spiderpool/crds/spiderpool.spidernet.io_spidercoordinators.yaml

                type: string
              podMACPrefix:
+                description: PodMACPrefix the fixed MAC address prefix, e.g. 0a:1b


这需要体现：输入两个字节，并且，第一个字节的最低位必须是 0 ，它代表一个单播的 mac 。相关的 webhook 是否有做验证？

weizhoublue · 2024-08-21T11:13:19Z

charts/spiderpool/crds/spiderpool.spidernet.io_spidercoordinators.yaml

@@ -75,14 +89,26 @@ spec:
                - none
                type: string
              podDefaultRouteNIC:
+                description: PodDefaultRouteNIC PodDefaultRouteNIC is used to configure


缺省是什么
缺省是空串代表什么

weizhoublue · 2024-08-21T11:15:03Z

docs/concepts/coordinator-zh_CN.md

@@ -38,7 +38,8 @@ EOF
 | serviceCIDR        | 默认的集群 Service 子网， 会注入到 Pod 中。不需要配置，自动从 Spidercoordinator default 中获取                                                                                                                                                                                                    | []stirng | optional   | 默认从 Spidercoordinator default 中获取 |
 | hijackCIDR         | 额外的需要从主机转发的子网路由。比如nodelocaldns 的地址: 169.254.20.10/32                                                                                                                                                                                                                    | []stirng | optional   | 空                                 |
 | hostRuleTable      | 策略路由表号，同主机与 Pod 通信的路由将会存放于这个表号                                                                                                                                                                                                                                          | 整数型      | optional   | 500                               |
-| hostRPFilter       | 设置主机上的 sysctl 参数 rp_filter                                                                                                                                                                                                                                              | 整数型      | optional   | 0                                 |
+| podRPFilter       | 设置 Pod 的 sysctl 参数 rp_filter                                                                                                                                                                                                                                              | 整数型      | optional   | 0                                 |
+| hostRPFilter(遗弃)       | 设置节点 的 sysctl 参数 rp_filter                                                                                                                                                                                                                                              | 整数型      | optional   | 0                                 |


hostRPFilter | (遗弃) 设置节点的 sysctl 参数 rp_filter

weizhoublue · 2024-08-23T06:37:48Z

cmd/spiderpool-agent/cmd/coordinator.go

@@ -125,7 +125,7 @@ func (g *_unixGetCoordinatorConfig) Handle(params daemonset.GetCoordinatorConfig
 		TunePodRoutes:      coord.Spec.TunePodRoutes,
 		PodDefaultRouteNIC: nic,
 		HostRuleTable:      int64(*coord.Spec.HostRuleTable),
-		HostRPFilter:       int64(*coord.Spec.HostRPFilter),


这个结构体定义不应该删除，它只是不被处理，却不能缺少定义？

这里删除没有关系，这里是coordinator 请求 agent 获取 coordinatiorConfig,这个字段原来也没有用到

weizhoublue · 2024-08-23T06:39:42Z

cmd/coordinator/cmd/cni_types.go

@@ -55,7 +55,7 @@ type Config struct {
 	PodDefaultRouteNIC string         `json:"podDefaultRouteNic,omitempty"`
 	Mode               Mode           `json:"mode,omitempty"`
 	HostRuleTable      *int64         `json:"hostRuleTable,omitempty"`
-	RPFilter           int32          `json:"hostRPFilter,omitempty" `


这个结构体定义不应该删除，它只是不被处理，却不能缺少定义？

这里可以加回来

weizhoublue · 2024-08-23T06:42:24Z

docs/reference/crd-spidercoordinator.md

@@ -53,7 +54,8 @@ This is the Spidercoordinators spec for users to configure.
 | detectGateway      | enable detect gateway while launching pod, If the gateway is unreachable, pod will be failed to created; Note: We use ARP probes to detect if the gateway is reachable, and some gateway routers may warn about this                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                     | boolean | optional   | true,false                                    | false                        |                                          
 | detectIPConflict   | enable the pod's ip if is conflicting while launching pod. If an IP conflict of the pod is detected, pod will be failed to created                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                       | boolean | optional   | true,false                                    | false                        |                                          
 | podMACPrefix       | fix the pod's mac address with this prefix + 4 bytes IP                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                  | string  | optional   | a invalid mac address prefix                  | ""                           |                                          
-| hostRPFilter       | sysctls: rp_filter in host                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                               | int     | required   | 0,1,2;suggest to be 0                         | 0                            |
+| podRPFilter       | set rp_filter sysctl for the pod                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                               | int     | required   | 0,1,2;suggest to be 0                         | 0                            |
+| hostRPFilter(deprecated)       | set rp_filter sysctl for the node                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                               | int     | required   | 0,1,2;suggest to be 0                         | 0                            |


没有名字带deprecated，这属于描述

weizhoublue · 2024-08-23T06:44:52Z

cmd/coordinator/cmd/cni_types.go

@@ -55,7 +55,7 @@ type Config struct {
 	PodDefaultRouteNIC string         `json:"podDefaultRouteNic,omitempty"`
 	Mode               Mode           `json:"mode,omitempty"`
 	HostRuleTable      *int64         `json:"hostRuleTable,omitempty"`
-	RPFilter           int32          `json:"hostRPFilter,omitempty" `
+	RPFilter           *int32         `json:"podRPFilter,omitempty" `


Podrpfilter 准确，和原来代码区分

weizhoublue · 2024-08-24T07:34:53Z

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

                    type: string
                  podMACPrefix:
+                    description: 'PodMACPrefix the fixed MAC address prefix, the length
+                      is two bytes. the lowest bit of the first byte must be 0, which
+                      indicates the \ unicast MAC address. example: 0a:1b'


这个单播mac是否有检验

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

cyclinder · 2024-08-26T03:05:32Z

https://github.com/spidernet-io/spiderpool/actions/runs/10552124403/job/29230614968?pr=3906

ALL_IMAGES: ghcr.io/k8snetworkplumbingwg/multus-cni:v3.9.3 ghcr.io/mellanox/k8s-rdma-shared-dev-plugin:v1.5.1 ghcr.io/spidernet-io/spiderpool/spiderpool-plugins:82659d90cae0d6a5169eac2869e47c989932d775 ghcr.io/spidernet-io/sriov-network-operator:v1.3.0 spiderpool-agent-race:7e94f03c4bc7d4edf4df3a005a80df10c92b1d58 spiderpool-controller-race:7e94f03c4bc7d4edf4df3a005a80df10c92b1d58  
==> ghcr.io/k8snetworkplumbingwg/multus-cni:v3.9.3 no found, pulling....
Error response from daemon: Get "https://ghcr.io/v2/": net/http: request canceled while waiting for connection (Client.Timeout exceeded while awaiting headers)

coordinator should only set rp_filter for pod but not the node Signed-off-by: robot <tao.yang@daocloud.io>

cyclinder added release/bug cherrypick-release-v0.9 cherrypick-release-v1.0 Cherry-pick the PR to branch release-v1.0. labels Aug 20, 2024

cyclinder requested review from weizhoublue and windsonsea as code owners August 20, 2024 06:37

cyclinder force-pushed the spidermultusconfig/hostRPFilter branch from ffdab9b to cba03ad Compare August 20, 2024 06:38

cyclinder force-pushed the spidermultusconfig/hostRPFilter branch from cba03ad to 34d9912 Compare August 20, 2024 06:46

cyclinder mentioned this pull request Aug 20, 2024

coordinator: fix wrong policy route when there is more than 1 secondary nics #3873

Merged

cyclinder force-pushed the spidermultusconfig/hostRPFilter branch from 34d9912 to 0d1592e Compare August 20, 2024 08:10

weizhoublue reviewed Aug 20, 2024

View reviewed changes

charts/spiderpool/crds/spiderpool.spidernet.io_spidermultusconfigs.yaml Show resolved Hide resolved

weizhoublue reviewed Aug 21, 2024

View reviewed changes

cyclinder force-pushed the spidermultusconfig/hostRPFilter branch from 0d1592e to 166f344 Compare August 21, 2024 06:56

cyclinder added the cherrypick-release-v0.8 Cherry-pick the PR to branch release-v0.8. label Aug 21, 2024

cyclinder force-pushed the spidermultusconfig/hostRPFilter branch 3 times, most recently from 47d5cec to d7df0d4 Compare August 21, 2024 07:42

cyclinder changed the title ~~coordinator should only set rp_filter for pod rather than the node~~ coordinator should only set rp_filter for pod but not the node Aug 21, 2024

weizhoublue reviewed Aug 21, 2024

View reviewed changes

cyclinder force-pushed the spidermultusconfig/hostRPFilter branch 3 times, most recently from 31d7d19 to e900656 Compare August 21, 2024 10:11

weizhoublue reviewed Aug 21, 2024

View reviewed changes

cyclinder force-pushed the spidermultusconfig/hostRPFilter branch 3 times, most recently from ef2a004 to 0fac10c Compare August 23, 2024 02:59

weizhoublue reviewed Aug 23, 2024

View reviewed changes

cyclinder force-pushed the spidermultusconfig/hostRPFilter branch 3 times, most recently from 90371ea to d4258d1 Compare August 24, 2024 02:56

weizhoublue reviewed Aug 24, 2024

View reviewed changes

cyclinder force-pushed the spidermultusconfig/hostRPFilter branch 2 times, most recently from f250e64 to 8b2837a Compare August 26, 2024 01:37

cyclinder requested review from ty-dc and bzsuni as code owners August 26, 2024 01:37

coordinator should only set rp_filter for pod not the node

7e94f03

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

cyclinder force-pushed the spidermultusconfig/hostRPFilter branch from 8b2837a to 7e94f03 Compare August 26, 2024 01:50

weizhoublue approved these changes Aug 26, 2024

View reviewed changes

ty-dc approved these changes Aug 26, 2024

View reviewed changes

cyclinder merged commit 53dcfeb into spidernet-io:main Aug 26, 2024
59 of 60 checks passed

This was referenced Aug 26, 2024

failed to cherry pick PR 3906 from cyclinder, to branch release-v0.8 #3945

Closed

failed to cherry pick PR 3906 from cyclinder, to branch release-v0.9 #3946

Closed

github-actions bot pushed a commit that referenced this pull request Aug 26, 2024

Merge pull request #3906 from cyclinder/spidermultusconfig/hostRPFilter

8861102

coordinator should only set rp_filter for pod but not the node Signed-off-by: robot <tao.yang@daocloud.io>

weizhoublue mentioned this pull request Aug 26, 2024

coordinator should only set rp_filter for pod but not the node #3947

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

coordinator should only set rp_filter for pod but not the node #3906

coordinator should only set rp_filter for pod but not the node #3906

cyclinder commented Aug 20, 2024 •

edited

Loading

codecov bot commented Aug 20, 2024 •

edited

Loading

cyclinder commented Aug 20, 2024

weizhoublue commented Aug 20, 2024 •

edited

Loading

cyclinder commented Aug 21, 2024

ty-dc commented Aug 21, 2024

weizhoublue Aug 21, 2024

cyclinder Aug 21, 2024

weizhoublue Aug 21, 2024 •

edited

Loading

cyclinder Aug 21, 2024

weizhoublue Aug 21, 2024

cyclinder Aug 22, 2024

weizhoublue Aug 23, 2024 •

edited

Loading

cyclinder Aug 24, 2024

weizhoublue Aug 21, 2024

weizhoublue Aug 21, 2024

weizhoublue Aug 21, 2024

cyclinder Aug 23, 2024

cyclinder Aug 23, 2024

weizhoublue Aug 21, 2024

weizhoublue Aug 21, 2024 •

edited

Loading

weizhoublue Aug 21, 2024

weizhoublue Aug 23, 2024

cyclinder Aug 23, 2024

weizhoublue Aug 23, 2024

cyclinder Aug 23, 2024

weizhoublue Aug 23, 2024

cyclinder Aug 23, 2024

weizhoublue Aug 23, 2024

cyclinder Aug 23, 2024

weizhoublue Aug 24, 2024

cyclinder Aug 25, 2024

cyclinder commented Aug 26, 2024 •

edited

Loading

coordinator should only set rp_filter for pod but not the node #3906

coordinator should only set rp_filter for pod but not the node #3906

Conversation

cyclinder commented Aug 20, 2024 • edited Loading

Thanks for contributing!

What type of PR is this?

codecov bot commented Aug 20, 2024 • edited Loading

Codecov Report

cyclinder commented Aug 20, 2024

weizhoublue commented Aug 20, 2024 • edited Loading

cyclinder commented Aug 21, 2024

ty-dc commented Aug 21, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

weizhoublue Aug 21, 2024 • 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

weizhoublue Aug 23, 2024 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

weizhoublue Aug 21, 2024 • 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

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

cyclinder commented Aug 26, 2024 • edited Loading

cyclinder commented Aug 20, 2024 •

edited

Loading

codecov bot commented Aug 20, 2024 •

edited

Loading

weizhoublue commented Aug 20, 2024 •

edited

Loading

weizhoublue Aug 21, 2024 •

edited

Loading

weizhoublue Aug 23, 2024 •

edited

Loading

weizhoublue Aug 21, 2024 •

edited

Loading

cyclinder commented Aug 26, 2024 •

edited

Loading