Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add chainCNI support for spidermultusconfig #3918

Merged
merged 1 commit into from
Aug 26, 2024

Conversation

cyclinder
Copy link
Collaborator

@cyclinder cyclinder commented Aug 21, 2024

Thanks for contributing!

What type of PR is this?

  • release/feature

What this PR does / why we need it:

Add chainCNI support for spidermultusconfig, In this way, we use a plugin like tuning to help configure the kernel parameters of the pod.

Which issue(s) this PR fixes:

Fixes #3919

Special notes for your reviewer:

root@worker-node-1:/home/cyclinder/spiderpool/charts# cat test-chain-cni.yaml
apiVersion: spiderpool.spidernet.io/v2beta1
kind: SpiderMultusConfig
metadata:
  name: test-chaincni
  namespace: kube-system
spec:
  cniType: macvlan
  coordinator:
    detectGateway: false
    detectIPConflict: false
    hostRPFilter: 1
    hostRuleTable: 500
    mode: auto
    podDefaultRouteNIC: net1
    podMACPrefix: ""
    tunePodRoutes: true
    txQueueLen: 0
  disableIPAM: false
  enableCoordinator: true
  chainCNIJsonData: 
  - | 
    {
      "type": "tuning",
      "sysctl": {
          "net.core.conn.max": "4096"
      }
    }
  macvlan:
    enableRdma: false
    ippools:
      ipv4:
      - default-v4-ippool
      ipv6:
      - default-v6-ippool
    master:
    - eth0
    rdmaResourceName: ""
    vlanID: 0

nad:

root@worker-node-1:/home/cyclinder/spiderpool/charts# kubectl get network-attachment-definitions.k8s.cni.cncf.io -n kube-system test-chaincni -o json | jq -r '.spec.config' | jq
{
  "cniVersion": "0.3.1",
  "name": "test-chaincni",
  "plugins": [
    {
      "type": "macvlan",
      "master": "eth0",
      "mode": "bridge",
      "ipam": {
        "type": "spiderpool",
        "default_ipv4_ippool": [
          "default-v4-ippool"
        ],
        "default_ipv6_ippool": [
          "default-v6-ippool"
        ]
      }
    },
    {
      "txQueueLen": 0,
      "detectIPConflict": false,
      "detectGateway": false,
      "tunePodRoutes": true,
      "mode": "auto",
      "type": "coordinator",
      "podDefaultRouteNic": "net1"
    },
    {
      "sysctl": {
        "net.core.conn.max": "4096"
      },
      "type": "tuning"
    }
  ]
}

Copy link

codecov bot commented Aug 21, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 80.83%. Comparing base (802f450) to head (074381e).
Report is 14 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #3918      +/-   ##
==========================================
- Coverage   80.88%   80.83%   -0.05%     
==========================================
  Files          51       51              
  Lines        4514     4514              
==========================================
- Hits         3651     3649       -2     
- Misses        698      699       +1     
- Partials      165      166       +1     
Flag Coverage Δ
unittests 80.83% <ø> (-0.05%) ⬇️

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

see 1 file with indirect coverage changes

@cyclinder cyclinder force-pushed the spidermultusconfig/chain_cni branch from 5d6c872 to 57bc92f Compare August 21, 2024 10:34
"sysctl": {
"net.core.somaxconn": "4096"
}
}
Copy link
Collaborator

@weizhoublue weizhoublue Aug 21, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

这个 字典格式,不符合我们讨论的预期
我们设计的是 数组描述,否则 格式过于 宽泛,

chianCNIJsonData: -|
[{
    chain 1
},{
    chain 2
}]

这样能插入任意多个

webhook 对 数组格式进行校验

另外,这个新字段 等于 划开一个口子,用户能够随意输入东西,例如缺少一些 field 等, webhook 是否 能够调用 cni 相关接口,验证 最终生成的 cni 配置 是否符合 cni 规范的

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

这是字符串数组 :) 每个元素对应一个 chainCNI 的配置。

这个新字段 等于 划开一个口子,用户能够随意输入东西,例如缺少一些 field 等, webhook 是否 能够调用 cni 相关接口,验证 最终生成的 cni 配置 是否符合 cni 规范的

这个可以

Copy link
Collaborator Author

@cyclinder cyclinder Aug 22, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

chianCNIJsonData: -|
[{
    chain 1
},{
    chain 2
}]

这种方式还不能实现,因为这里面是多个不同 chainCNI 的配置,没办法用一个统一得结构去 unmarshal ,所以没法转化为 plugins
可以用 []interface{} 实现,但会反复marshal 和 unmarshal

只能或者还是按照以下得方式:

chianCNIJsonData: 
- chain1
- chain2

每个元素是一个chainCNI 的配置,可以校验,不需要反复的 marshal 和 unmarshal 就可以方便append 到已有的 plugins 中,哪种方式更好?

@weizhoublue

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

也不错

chianCNIJsonData: 
- chain1 
- chain2

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

chianCNIJsonData: 
- chain1 
- chain2

// The automatically generated multus configuration should be associated with spidermultus
if config.ObjectMeta.OwnerReferences[0].Kind != constant.KindSpiderMultusConfig {
return false
}
Copy link
Collaborator

@weizhoublue weizhoublue Aug 21, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

这个最好 拉起个 pod 能够 running,完成最终的闭环 ,避免想当然了

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@cyclinder cyclinder force-pushed the spidermultusconfig/chain_cni branch 9 times, most recently from db48a81 to d4ba810 Compare August 23, 2024 07:17
It("create a spidermultusconfig to verify chainCNI json config", Label("M00021"), func() {
var smcName string = "chain-cni-multus" + common.GenerateString(10, true)

invalidJson := `{ "invalid" }`
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

这里只验证了 非 json 格式, 如果如下呢,是否有能力,是否要补 CI

  1. json格式, 但是不是字典
  2. json格式, 但 缺少 type 什么的,基本的 cni 配置都不满足

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@cyclinder cyclinder force-pushed the spidermultusconfig/chain_cni branch 2 times, most recently from a20a4ed to bd3188b Compare August 24, 2024 03:14
weizhoublue
weizhoublue previously approved these changes Aug 24, 2024
@cyclinder cyclinder merged commit dd8560b into spidernet-io:main Aug 26, 2024
59 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add chainCNI support for the spiderMultusConfig
3 participants