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 annotation cni.spidernet.io/network-resource-inject #4421

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

yanhb-nil
Copy link

@yanhb-nil yanhb-nil commented Dec 19, 2024

Thanks for contributing!

Notice:

What issue(s) does this PR fix:

Fixes #4358

Special notes for your reviewer:
Maybe could rename some funcs

Copy link
Collaborator

@cyclinder cyclinder left a comment

Choose a reason for hiding this comment

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

也需要添加下 docs 和 e2e

if !ok {
needInject := false
annos := []string{constant.AnnoPodResourceInject, constant.AnnoNetworkResourceInject}
for _, anno := range annos {
Copy link
Collaborator

Choose a reason for hiding this comment

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

可以直接这样:

for _, anno := range ]string{constant.AnnoPodResourceInject, constant.AnnoNetworkResourceInject} {
    ...
}

Copy link
Author

Choose a reason for hiding this comment

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

好的

Copy link
Collaborator

Choose a reason for hiding this comment

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

似乎修改还没推送上来?

Key: constant.AnnoPodResourceInject,
Operator: metav1.LabelSelectorOpIn,
Values: []string{multusLabelValue},
for _, anno := range annos {
Copy link
Collaborator

Choose a reason for hiding this comment

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

我在想两个 annotations 应该是 2 选一 ,所以当检查到 pod 有其中一个 anno 并处理完后,就可以跳出这个 loop

Copy link
Author

Choose a reason for hiding this comment

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

ok

return nil
annos := []struct {
key string
checkCNI bool
Copy link
Collaborator

Choose a reason for hiding this comment

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

这个 checkCNI 似乎是多余的,AnnoNetworkResourceInject 是一个更大的范围,包括 RDMA 以及 Underlay 场景

Copy link
Author

Choose a reason for hiding this comment

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

ok

Copy link

codecov bot commented Dec 23, 2024

Codecov Report

Attention: Patch coverage is 0% with 34 lines in your changes missing coverage. Please review.

Project coverage is 79.15%. Comparing base (7fd26d8) to head (057d432).

Files with missing lines Patch % Lines
pkg/podmanager/utils.go 0.00% 27 Missing ⚠️
pkg/podmanager/pod_webhook.go 0.00% 7 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #4421      +/-   ##
==========================================
- Coverage   79.26%   79.15%   -0.12%     
==========================================
  Files          54       54              
  Lines        6283     6288       +5     
==========================================
- Hits         4980     4977       -3     
- Misses       1108     1115       +7     
- Partials      195      196       +1     
Flag Coverage Δ
unittests 79.15% <0.00%> (-0.12%) ⬇️

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

Files with missing lines Coverage Δ
pkg/podmanager/pod_webhook.go 0.00% <0.00%> (ø)
pkg/podmanager/utils.go 52.29% <0.00%> (-0.49%) ⬇️

... and 1 file with indirect coverage changes

@yanhb-nil
Copy link
Author

add e2e test case and docs


[**English**](./multi-underlay-nic.md) | **简体中文**

## 基于 Webhook 自动注入 Multi Underlay 网络资源
Copy link
Collaborator

Choose a reason for hiding this comment

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

把这个作为标题:

基于 Webhook 自动为 Pod 附加多张 Underlay 网卡

@@ -135,6 +135,7 @@ EOF
Note:

> subnet 应该与节点网卡 ens192 的子网保持一致,并且不与现有任何 IP 冲突。
> 如果需要为Pod添加多张Underlay网卡的可以参考[**Multi-Underlay-NIC**](./multi-underlay-nic-zh_CN.md)。
Copy link
Collaborator

Choose a reason for hiding this comment

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

英文单词中加个空格

apiVersion: spiderpool.spidernet.io/v2beta1
kind: SpiderMultusConfig
metadata:
name: macvlan1
Copy link
Collaborator

Choose a reason for hiding this comment

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

name 可改为 macvlan-ens192

apiVersion: spiderpool.spidernet.io/v2beta1
kind: SpiderMultusConfig
metadata:
name: macvlan2
Copy link
Collaborator

Choose a reason for hiding this comment

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

name 可改为 macvlan-ens193

@cyclinder
Copy link
Collaborator

  • DCO : git commit --amend -s
  • CI 失败看下:
• [FAILED] [1.047 seconds]
test spidermultus [SpiderMultusConfig]
/home/runner/work/spiderpool/spiderpool/test/e2e/spidermultus/spidermultus_test.go:31
  [It] return an err if resoucename is set without ippools config when spidermutlus with annotation: cni.spidernet.io/network-resource-inject [M00032]
  /home/runner/work/spiderpool/spiderpool/test/e2e/spidermultus/spidermultus_test.go:915

  Timeline >>
  > Enter [BeforeEach] test spidermultus - /home/runner/work/spiderpool/spiderpool/test/e2e/spidermultus/spidermultus_test.go:34 @ 12/27/24 02:01:45.392
  < Exit [BeforeEach] test spidermultus - /home/runner/work/spiderpool/spiderpool/test/e2e/spidermultus/spidermultus_test.go:34 @ 12/27/24 02:01:46.406 (1.014s)
  > Enter [It] return an err if resoucename is set without ippools config when spidermutlus with annotation: cni.spidernet.io/network-resource-inject - /home/runner/work/spiderpool/spiderpool/test/e2e/spidermultus/spidermultus_test.go:915 @ 12/27/24 02:01:46.406
  spidermultus cr: &{TypeMeta:{Kind: APIVersion:} ObjectMeta:{Name:ann-network-resourcea8fe628118 GenerateName: Namespace:ns-a3fc97f592 SelfLink: UID: ResourceVersion: Generation:0 CreationTimestamp:0001-01-01 00:00:00 +0000 UTC DeletionTimestamp:<nil> DeletionGracePeriodSeconds:<nil> Labels:map[] Annotations:map[cni.spidernet.io/network-resource-inject:test] OwnerReferences:[] Finalizers:[] ManagedFields:[]} Spec:{CniType:0xc000235ce0 MacvlanConfig:0xc000473860 IPVlanConfig:<nil> SriovConfig:<nil> OvsConfig:<nil> IbSriovConfig:<nil> IpoibConfig:<nil> EnableCoordinator:0xc00072bd0f DisableIPAM:<nil> CoordinatorConfig:0xc000482f80 ChainCNIJsonData:[] CustomCNIConfig:<nil>}} 
  [FAILED] Expected an error to have occurred.  Got:
      <nil>: nil
  In [It] at: /home/runner/work/spiderpool/spiderpool/test/e2e/spidermultus/spidermultus_test.go:940 @ 12/27/24 02:01:46.436
  < Exit [It] return an err if resoucename is set without ippools config when spidermutlus with annotation: cni.spidernet.io/network-resource-inject - /home/runner/work/spiderpool/spiderpool/test/e2e/spidermultus/spidermultus_test.go:915 @ 12/27/24 02:01:46.436 (30ms)
  > Enter [DeferCleanup (Each)] test spidermultus - /home/runner/work/spiderpool/spiderpool/test/e2e/spidermultus/spidermultus_test.go:40 @ 12/27/24 02:01:46.438
  If the use case fails, the cleanup step will be skipped
  < Exit [DeferCleanup (Each)] test spidermultus - 

@cyclinder cyclinder added release/feature-new release note for new feature pr/ready-review This pull is ready for review labels Dec 27, 2024
@@ -422,7 +422,7 @@

## 基于 Webhook 自动注入 RDMA 网络资源

在上述步骤中,我们展示了如何使用 SR-IOV 技术在 RoCE 和 Infiniband 网络环境中为容器提供 RDMA 通信能力。然而,当配置多网卡的 AI 应用时,过程会变得复杂。为简化这个过程,Spiderpool 通过 annotations(`cni.spidernet.io/rdma-resource-inject`) 支持对一组网卡配置进行分类。用户只需要为应用添加与网卡配置相同的注解,Spiderpool 就会通过 webhook 自动为应用注入所有具有相同注解的对应网卡和网络资源。
在上述步骤中,我们展示了如何使用 SR-IOV 技术在 RoCE 和 Infiniband 网络环境中为容器提供 RDMA 通信能力。然而,当配置多网卡的 AI 应用时,过程会变得复杂。为简化这个过程,Spiderpool 通过 annotations(`cni.spidernet.io/rdma-resource-inject` 或 `cni.spidernet.io/network-resource-inject`) 支持对一组网卡配置进行分类。用户只需要为应用添加与网卡配置相同的注解,Spiderpool 就会通过 webhook 自动为应用注入所有具有相同注解的对应网卡和网络资源。
Copy link
Collaborator

@weizhoublue weizhoublue Dec 31, 2024

Choose a reason for hiding this comment

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

(1)pls tell the differences between the two annotation in the document
(2) cni.spidernet.io/network-resource-inject seems not to apply to the RDMA document

Copy link
Collaborator

Choose a reason for hiding this comment

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

  1. 在 AI docs 中 说明下 使用 这两种 anno 的差异:

cni.spidernet.io/rdma-resource-inject 只适用于 AI 场景,自动注入 RDMA 网卡及 RDMA Resources。 cni.spidernet.io/network-resource-inject 不但可以用于 AI 场景,也支持 Underlay 场景

  1. 在未来我们希望都统一使用 cni.spidernet.io/network-resource-inject 统一支持 这两种场景。

Copy link
Collaborator

Choose a reason for hiding this comment

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

Copy link
Author

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
pr/ready-review This pull is ready for review release/feature-new release note for new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Multi values for the RDMA resource injection annotation of the spidermultusconfig
4 participants