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

Add support for allmulticast flag #268

Merged
merged 1 commit into from
Jun 20, 2023
Merged

Add support for allmulticast flag #268

merged 1 commit into from
Jun 20, 2023

Conversation

mlguerrero12
Copy link
Contributor

This allows users to set the allmulticast mode for a VF.

@coveralls
Copy link

coveralls commented May 30, 2023 •
edited
Loading

Pull Request Test Coverage Report for Build 5323513802

  • 41 of 61 (67.21%) changed or added relevant lines in 4 files are covered.
  • 1 unchanged line in 1 file lost coverage.
  • Overall coverage increased (+1.4%) to 39.848%
Changes Missing Coverage Covered Lines Changed/Added Lines %
pkg/config/config.go 9 11 81.82%
pkg/utils/mocks/netlink_manager_mock.go 20 24 83.33%
pkg/utils/netlink_manager.go 0 6 0.0%
pkg/sriov/sriov.go 12 20 60.0%
Files with Coverage Reduction New Missed Lines %
pkg/utils/mocks/netlink_manager_mock.go 1 80.12%
Totals Coverage Status
Change from base Build 5210643953: 1.4%
Covered Lines: 473
Relevant Lines: 1187
💛 - Coveralls

@mlguerrero12
Copy link
Contributor Author

@SchSeba, @adrianchiris, could you please take a look a this?

cgoncalves
cgoncalves suggested changes May 31, 2023
View reviewed changes
Copy link
Contributor

@cgoncalves cgoncalves left a comment

Choose a reason for hiding this comment

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

Add new parameter to docs/configuration-reference.md

pkg/types/types.go Show resolved Hide resolved
@cgoncalves
Copy link
Contributor

Add new parameter to docs/configuration-reference.md

Also to README.md

@mlguerrero12
Copy link
Contributor Author

@cgoncalves, done

cgoncalves
cgoncalves approved these changes May 31, 2023
View reviewed changes
Copy link
Contributor

@cgoncalves cgoncalves left a comment

Choose a reason for hiding this comment

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

LGTM

zeeke
zeeke approved these changes May 31, 2023
View reviewed changes
Copy link
Member

@zeeke zeeke left a comment

Choose a reason for hiding this comment

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

LGTM

SchSeba
SchSeba reviewed May 31, 2023
View reviewed changes
pkg/config/config.go Outdated Show resolved Hide resolved
pkg/types/types.go Show resolved Hide resolved
SchSeba
SchSeba reviewed Jun 12, 2023
View reviewed changes
Copy link
Collaborator

@SchSeba SchSeba left a comment

Choose a reason for hiding this comment

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

LGTM just a small nic in the documentation and we should be good to go.

nice work!

docs/configuration-reference.md Outdated
@@ -13,6 +13,7 @@ The SR-IOV CNI configures networks through a CNI spec configuration object. In a
* `mac` (string, optional): MAC address to assign for the VF
* `spoofchk` (string, optional): turn packet spoof checking on or off for the VF
* `trust` (string, optional): turn trust setting on or off for the VF
* `all_multicast` (string, optional): turn allmulticast setting on or off for the VF. Trust setting must be enabled when this setting is on.
Copy link
Collaborator

Choose a reason for hiding this comment

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

also comment here that this will work only for kernel drivers and not userspace ones

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

pkg/types/types.go Show resolved Hide resolved
@mlguerrero12
Copy link
Contributor Author

Thank you @SchSeba, I addressed your comment.

SchSeba
SchSeba approved these changes Jun 13, 2023
View reviewed changes
Copy link
Collaborator

@SchSeba SchSeba left a comment

Choose a reason for hiding this comment

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

LGTM

@SchSeba
Copy link
Collaborator

SchSeba commented Jun 14, 2023

Hi @mlguerrero12 can you please fix the test?

@mlguerrero12
Copy link
Contributor Author

Hi @Eoghan1232, could you please review this?

adrianchiris
adrianchiris reviewed Jun 20, 2023
View reviewed changes
pkg/sriov/sriov.go Outdated Show resolved Hide resolved
@mlguerrero12
Add support for allmulticast flag
50701a0 
This allows users to set the allmulticast mode for a VF.

Signed-off-by: Marcelo Guerrero <marguerr@redhat.com>
@mlguerrero12
Copy link
Contributor Author

@adrianchiris, I addressed your comments. Thank you.

@adrianchiris
Copy link
Contributor

@adrianchiris, I addressed your comments. Thank you.

thanks for addressing !

@adrianchiris adrianchiris merged commit 30ed341 into k8snetworkplumbingwg:master Jun 20, 2023
@adrianchiris
Copy link
Contributor

@mlguerrero12 @SchSeba i see that tuning CNI supports setting all multicast.

https://github.com/containernetworking/cni.dev/blob/main/content/plugins/current/meta/tuning.md

started to think if we really need it part of sriov-cni ? as all multicast is general for netdev no nessecarily for VF and tuning CNI already supports it.

@adrianchiris adrianchiris mentioned this pull request Jun 21, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@adrianchiris adrianchiris adrianchiris approved these changes

@SchSeba SchSeba SchSeba approved these changes

@zeeke zeeke zeeke approved these changes

@cgoncalves cgoncalves cgoncalves approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@mlguerrero12 @coveralls @cgoncalves @SchSeba @adrianchiris @zeeke