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.

Sign up for GitHub

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

Jump to bottom

update spiderpool apis version from v2beta1 to v2 #4301

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

update spiderpool apis version from v2beta1 to v2 #4301

wants to merge 1 commit into from

Conversation

cyclinder
Copy link
Collaborator

@cyclinder cyclinder commented Nov 20, 2024 •
edited
Loading

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
@codecov Codecov
Copy link

codecov bot commented Nov 20, 2024 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 79.22%. Comparing base (7fd26d8) to head (f390bd7).

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #4301      +/-   ##
==========================================
- Coverage   79.26%   79.22%   -0.04%     
==========================================
  Files          54       54              
  Lines        6283     6286       +3     
==========================================
  Hits         4980     4980              
- Misses       1108     1110       +2     
- Partials      195      196       +1
Flag Coverage Δ
unittests 79.22% <100.00%> (-0.04%) ⬇️

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

Files with missing lines Coverage Δ
pkg/podmanager/utils.go 54.05% <100.00%> (+1.27%) ⬆️

... and 1 file with indirect coverage changes

@cyclinder cyclinder force-pushed the spidermultusconfig/api_update branch from 4dee59d to fd7b567 Compare November 20, 2024 11:18
weizhoublue
weizhoublue reviewed Nov 25, 2024
View reviewed changes
charts/spiderpool/crds/spiderpool.spidernet.io_spidermultusconfigs.yaml
@@ -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 •
edited
Loading

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 •
edited
Loading

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 •
edited
Loading

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 升级后 删除字段，相关实例 是否能正常，是否有验证

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

这个验证过，没问题

weizhoublue
weizhoublue reviewed Nov 25, 2024
View reviewed changes
charts/spiderpool/crds/spiderpool.spidernet.io_spidermultusconfigs.yaml
@@ -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.

同理

@weizhoublue weizhoublue added the pr/not-ready not ready for merging label Nov 27, 2024
@cyclinder cyclinder force-pushed the spidermultusconfig/api_update branch from fd7b567 to 6ea762a Compare November 27, 2024 07:11
@cyclinder cyclinder marked this pull request as draft November 28, 2024 10:08
@cyclinder cyclinder force-pushed the spidermultusconfig/api_update branch from 6ea762a to 8b09d25 Compare December 3, 2024 09:58
@cyclinder cyclinder changed the title api: update enableRdma/rdmaIsolation field for spiderMultusConfig update spiderpool apis version from v2beta1 to v1 Dec 3, 2024
@cyclinder cyclinder marked this pull request as ready for review December 4, 2024 02:33
@cyclinder cyclinder requested a review from lou-lan as a code owner December 4, 2024 02:33
@cyclinder cyclinder changed the title update spiderpool apis version from v2beta1 to v1 update spiderpool apis version from v2beta1 to v2 Dec 9, 2024
@cyclinder
Copy link
Collaborator Author

@yanhb-nil

@cyclinder cyclinder force-pushed the spidermultusconfig/api_update branch from 8b09d25 to aba40d6 Compare December 26, 2024 03:00
@cyclinder cyclinder removed the pr/not-ready not ready for merging label Dec 26, 2024
@cyclinder cyclinder force-pushed the spidermultusconfig/api_update branch from aba40d6 to abe14a1 Compare December 26, 2024 06:32
lou-lan
lou-lan previously approved these changes Dec 26, 2024
View reviewed changes
@cyclinder cyclinder force-pushed the spidermultusconfig/api_update branch from abe14a1 to 6dd282b Compare December 27, 2024 02:32
@cyclinder cyclinder force-pushed the spidermultusconfig/api_update branch from 6dd282b to f390bd7 Compare December 27, 2024 02:49
@cyclinder cyclinder added the pr/ready-review This pull is ready for review label Dec 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@weizhoublue weizhoublue weizhoublue left review comments

@lou-lan lou-lan lou-lan left review comments

@ty-dc ty-dc Awaiting requested review from ty-dc

@bzsuni bzsuni Awaiting requested review from bzsuni

@windsonsea windsonsea Awaiting requested review from windsonsea windsonsea is a code owner

At least 2 approving reviews are required to merge this pull request.

Assignees
No one assigned
Labels
pr/ready-review This pull is ready for review release/feature-new release note for new feature
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Optimize spidermultusconfig enableRDMA
3 participants
@cyclinder @lou-lan @weizhoublue