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

supplement spidermultusconfig default value #2622

Merged
merged 1 commit into from
Nov 22, 2023
Merged

supplement spidermultusconfig default value #2622

merged 1 commit into from
Nov 22, 2023

Conversation

Icarus9913
Copy link
Collaborator

  1. CRD optional property should be with omitempty tag
  2. If no vlan or bond specified, we should not create ifacer plugin call in SpiderMultusConfig

enhancement for #2603

Signed-off-by: Icarus9913 icaruswu66@qq.com

@Icarus9913 Icarus9913 added pr/ready-review This pull is ready for review enhancement source codes enhancement release/none no release note labels Nov 20, 2023
@codecov Codecov
Copy link

codecov bot commented Nov 20, 2023 •
edited
Loading

Codecov Report

Merging #2622 (8c4d334) into main (d97cc55) will decrease coverage by 0.06%.
Report is 8 commits behind head on main.
The diff coverage is n/a.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #2622      +/-   ##
==========================================
- Coverage   80.87%   80.82%   -0.06%     
==========================================
  Files          49       49              
  Lines        5303     5303              
==========================================
- Hits         4289     4286       -3     
- Misses        857      859       +2     
- Partials      157      158       +1
Flag Coverage Δ
unittests 80.82% <ø> (-0.06%) ⬇️

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

see 1 file with indirect coverage changes

Icarus9913
Icarus9913 commented Nov 20, 2023
View reviewed changes
pkg/multuscniconfig/multusconfig_informer.go Outdated
Comment on lines 388 to 392
plugins = append([]interface{}{macvlanCNIConf}, plugins...)
if len(multusConfSpec.MacvlanConfig.Master) > 0 || (multusConfSpec.MacvlanConfig.VlanID != nil && *multusConfSpec.MacvlanConfig.VlanID != 0) {
if multusConfSpec.MacvlanConfig.VlanID != nil && *multusConfSpec.MacvlanConfig.VlanID != 0 {
// we need to set Subvlan as first at the CNI plugin chain
subVlanCNIConf := generateIfacer(multusConfSpec.MacvlanConfig.Master,
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

需要ifacer的情况：

  1. 指定了单卡 + vlan
  2. 指定了多网卡组bond + vlan (ifacer代码中bond需要结合vlan一起使用)

Copy link
Collaborator

Choose a reason for hiding this comment

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

指定了多网卡组bond + vlan (ifacer代码中bond需要结合vlan一起使用)

多卡 或 vlanId

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

spiderpool/cmd/ifacer/cmd/cmd_add.go

Lines 44 to 56 in d97cc55

default:
if conf.Bond == nil {
return types.PrintResult(result, conf.CNIVersion)
}
bond, err := createBondDevice(conf)
if err != nil {
return fmt.Errorf("failed to createBondDevice: %v", err)
}
if conf.VlanID == 0 {
return types.PrintResult(result, conf.CNIVersion)
}

多卡下vlan为0，直接return了。因此此处只需要判断vlan是否被设置才调用ifacer

Copy link
Collaborator

Choose a reason for hiding this comment

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

多网卡时需要 ifacer 创建bond

Copy link
Collaborator Author

@Icarus9913 Icarus9913 Nov 20, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

所以此处54-56行有bug？

设置了多网卡，但是又没有设置vlan。 然后ifacer单纯的就创建了一个bond网卡(没有slave指定网卡)，就因为vlan==0就return了？

Copy link
Collaborator

Choose a reason for hiding this comment

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

逻辑没问题，没vlan 就建bond就行了

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

cc @weizhoublue

Copy link
Collaborator

Choose a reason for hiding this comment

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

len(multusConfSpec.MacvlanConfig.Master) > 0 这个逻辑 应该在的吧 ？

Copy link
Collaborator Author

@Icarus9913 Icarus9913 Nov 22, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

多网卡组bond可以使用vlan0，因此此处判断为len(multusConfSpec.MacvlanConfig.Master) >= 2

此处ifacer有bug，已关联issue：#2638

Icarus9913
Icarus9913 commented Nov 20, 2023
View reviewed changes
pkg/multuscniconfig/multusconfig_mutate.go
Comment on lines 60 to 61
func setBondDefaultConfig(bond *spiderpoolv2beta1.BondConfig) *spiderpoolv2beta1.BondConfig {
if bond == nil {
return &spiderpoolv2beta1.BondConfig{
Name: "",
Mode: 0,
Options: pointer.String(""),
}
}

if bond.Options == nil {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

无需给bond赋空值。
webhook validate已做校验，若指定了多网卡但未设置bond会报错。

此处，单网卡下就无需展示bond

cyclinder
cyclinder reviewed Nov 20, 2023
View reviewed changes
pkg/multuscniconfig/multusconfig_informer.go Outdated
Comment on lines 388 to 392
plugins = append([]interface{}{macvlanCNIConf}, plugins...)
if len(multusConfSpec.MacvlanConfig.Master) > 0 || (multusConfSpec.MacvlanConfig.VlanID != nil && *multusConfSpec.MacvlanConfig.VlanID != 0) {
if multusConfSpec.MacvlanConfig.VlanID != nil && *multusConfSpec.MacvlanConfig.VlanID != 0 {
// we need to set Subvlan as first at the CNI plugin chain
subVlanCNIConf := generateIfacer(multusConfSpec.MacvlanConfig.Master,
Copy link
Collaborator

Choose a reason for hiding this comment

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

指定了多网卡组bond + vlan (ifacer代码中bond需要结合vlan一起使用)

多卡 或 vlanId

pkg/multuscniconfig/multusconfig_mutate.go Outdated Show resolved Hide resolved
@cyclinder cyclinder added the cherrypick-release-v0.8 Cherry-pick the PR to branch release-v0.8. label Nov 20, 2023
@Icarus9913
supplement spidermultusconfig default value
8c4d334 
Signed-off-by: Icarus9913 <icaruswu66@qq.com>
@weizhoublue weizhoublue merged commit d8e42e9 into spidernet-io:main Nov 22, 2023
38 checks passed
github-actions bot pushed a commit that referenced this pull request Nov 22, 2023
@weizhoublue
Merge pull request #2622 from Icarus9913/fix/wk/crd
88cac5a 
supplement spidermultusconfig default value
@github-actions github-actions bot mentioned this pull request Nov 22, 2023
Merged
@Icarus9913 Icarus9913 deleted the fix/wk/crd branch December 14, 2023 02:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cyclinder cyclinder cyclinder left review comments

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

Assignees
No one assigned
Labels
cherrypick-release-v0.8 Cherry-pick the PR to branch release-v0.8. enhancement source codes enhancement pr/ready-review This pull is ready for review release/none no release note
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Icarus9913 @weizhoublue @cyclinder