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

api: update enableRdma/rdmaIsolation field for spiderMultusConfig #4301

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

cyclinder
Copy link
Collaborator

@cyclinder cyclinder commented Nov 20, 2024

Thanks for contributing!

Notice:

What issue(s) does this PR fix:

Fixes #4267

Special notes for your reviewer:

  1. Remove enableRdma field for macvlan and ipvlan CNI

    Since Macvlan/IPVlan CNI works with the shared RDMA subsystem mode, which doesn't need the RDMA CNI
    plugin ensures isolation of RDMA traffic from other workloads in the system by moving
    the associated RDMA interfaces of the provided network interface to the container's
    network namespace path. this field is not currently referenced anywhere. we can remove it safely.

  2. Make the enableRdma field DEPRECATED for sriov-cni, and use rdmaIsolation instead. which keep consistant with ibsriov-cni.

  3. Update the comments of the spiderMultusConfig fields and docs.

@cyclinder cyclinder added the release/feature-new release note for new feature label Nov 20, 2024
@cyclinder cyclinder force-pushed the spidermultusconfig/api_update branch 3 times, most recently from 1e0f7cd to 4dee59d Compare November 20, 2024 08:19
Copy link

codecov bot commented Nov 20, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 79.52%. Comparing base (335952e) to head (fd7b567).

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #4301   +/-   ##
=======================================
  Coverage   79.51%   79.52%           
=======================================
  Files          53       53           
  Lines        6252     6255    +3     
=======================================
+ Hits         4971     4974    +3     
  Misses       1088     1088           
  Partials      193      193           
Flag Coverage Δ
unittests 79.52% <100.00%> (+<0.01%) ⬆️

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

Files with missing lines Coverage Δ
pkg/podmanager/utils.go 74.40% <100.00%> (+0.36%) ⬆️
---- 🚨 Try these New Features:

Signed-off-by: cyclinder <qifeng.guo@daocloud.io>
@@ -229,10 +246,6 @@ spec:
- mode
- name
type: object
enableRdma:
Copy link
Collaborator

Choose a reason for hiding this comment

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

这个 是 破坏性 的 版本 变更,只能说 deprecated ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

我也是考虑了一下,目前没有任何地方在引用这个代码,所以感觉可以直接移除?

Copy link
Collaborator

@weizhoublue weizhoublue Nov 25, 2024

Choose a reason for hiding this comment

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

不是代码引用的问题。
升级 流程上 是否能 会问题,存量实例 的转换 怎么办

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

升级指的是老 CRD 用新镜像,因为新镜像不会用到 enableRdma 这个字段,所以不会影响升级。手动测试过都是正常的,我也是考虑到没地方引用这个字段,所以想一步到位

Copy link
Collaborator

@weizhoublue weizhoublue Nov 26, 2024

Choose a reason for hiding this comment

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

那不现实的,只换镜像,那不叫升级,不处理 CRD 的升级 和转换。那未来 任何新功能,crd 新的定义,存量环境都享受不到,结果它们升级的意义 只是为了 修复 bug

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

我的意思是即使未升级 CRD只升级镜像测试都没问题,正常升级流程CRD和镜像都要升级,所以更没问题, crd 升级后,crs 中该字段自动移除

Copy link
Collaborator Author

@cyclinder cyclinder Nov 26, 2024

Choose a reason for hiding this comment

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

是否要一步到位, 你决定就好,我都OK @weizhoublue

Copy link
Collaborator

Choose a reason for hiding this comment

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

我的意思是即使未升级 CRD只升级镜像测试都没问题--- 不是一个升级服务的流程,不考虑

正常升级流程CRD和镜像都要升级 --- crd 升级后 删除字段,相关实例 是否能正常,是否有验证

@@ -280,10 +303,6 @@ spec:
- mode
- name
type: object
enableRdma:
Copy link
Collaborator

Choose a reason for hiding this comment

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

同理

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release/feature-new release note for new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Optimize spidermultusconfig enableRDMA
2 participants