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

Rdma subsytem mode #799

Merged
merged 2 commits into from
Nov 11, 2024
Merged

Rdma subsytem mode #799

merged 2 commits into from
Nov 11, 2024

Conversation

SchSeba
Copy link
Collaborator

@SchSeba SchSeba commented Oct 28, 2024

No description provided.

@github-actions GitHub Actions
Copy link

Thanks for your PR,
To run vendors CIs, Maintainers can use one of:

  • /test-all: To run all tests for all vendors.
  • /test-e2e-all: To run all E2E tests for all vendors.
  • /test-e2e-nvidia-all: To run all E2E tests for NVIDIA vendor.

To skip the vendors CIs, Maintainers can use one of:

  • /skip-all: To skip all tests for all vendors.
  • /skip-e2e-all: To skip all E2E tests for all vendors.
  • /skip-e2e-nvidia-all: To skip all E2E tests for NVIDIA vendor.
    Best regards.

@SchSeba SchSeba force-pushed the rdma-subsytem-mode branch 2 times, most recently from 511c308 to 4e22619 Compare October 29, 2024 09:49
@coveralls
Copy link

coveralls commented Oct 29, 2024 •
edited
Loading

Pull Request Test Coverage Report for Build 11723997336

Details

  • 158 of 293 (53.92%) changed or added relevant lines in 12 files are covered.
  • 3 unchanged lines in 2 files lost coverage.
  • Overall coverage increased (+0.2%) to 45.623%
Changes Missing Coverage Covered Lines Changed/Added Lines %
pkg/daemon/daemon.go 0 3 0.0%
pkg/host/internal/lib/netlink/netlink.go 0 3 0.0%
pkg/daemon/writer.go 0 7 0.0%
api/v1/zz_generated.deepcopy.go 2 12 16.67%
pkg/helper/mock/mock_helper.go 10 21 47.62%
controllers/sriovnetworknodepolicy_controller.go 0 12 0.0%
pkg/host/internal/network/network.go 19 31 61.29%
pkg/host/mock/mock_host.go 0 21 0.0%
controllers/helper.go 50 74 67.57%
pkg/plugins/generic/generic_plugin.go 65 97 67.01%
Files with Coverage Reduction New Missed Lines %
pkg/daemon/daemon.go 1 44.58%
pkg/plugins/generic/generic_plugin.go 2 64.86%
Totals Coverage Status
Change from base Build 11613650922: 0.2%
Covered Lines: 6880
Relevant Lines: 15080
💛 - Coveralls

@SchSeba SchSeba force-pushed the rdma-subsytem-mode branch from 4e22619 to a179fe4 Compare October 29, 2024 10:14
@SchSeba SchSeba closed this Oct 30, 2024
@SchSeba SchSeba force-pushed the rdma-subsytem-mode branch from a179fe4 to f7de046 Compare October 30, 2024 13:28
@SchSeba SchSeba reopened this Oct 30, 2024
@github-actions GitHub Actions
Copy link

Thanks for your PR,
To run vendors CIs, Maintainers can use one of:

  • /test-all: To run all tests for all vendors.
  • /test-e2e-all: To run all E2E tests for all vendors.
  • /test-e2e-nvidia-all: To run all E2E tests for NVIDIA vendor.

To skip the vendors CIs, Maintainers can use one of:

  • /skip-all: To skip all tests for all vendors.
  • /skip-e2e-all: To skip all E2E tests for all vendors.
  • /skip-e2e-nvidia-all: To skip all E2E tests for NVIDIA vendor.
    Best regards.

@SchSeba SchSeba force-pushed the rdma-subsytem-mode branch 4 times, most recently from fdaf09f to 0179a03 Compare October 30, 2024 19:42
e0ne
e0ne previously approved these changes Oct 30, 2024
View reviewed changes
Copy link
Collaborator

@e0ne e0ne left a comment

Choose a reason for hiding this comment

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

Tested with OCP 4.16 and Ubuntu 22.04

@SchSeba SchSeba force-pushed the rdma-subsytem-mode branch from 0179a03 to 88fc3f6 Compare October 30, 2024 21:02
zeeke
zeeke approved these changes Oct 31, 2024
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.

Did a first round of review. Overall approach looks good, though the GenericPlugin is starting to become a little complicated

pkg/plugins/generic/generic_plugin.go Outdated
@@ -264,84 +273,83 @@ func needDriverCheckVdpaType(state *sriovnetworkv1.SriovNetworkNodeState, driver
}

// setKernelArg Tries to add the kernel args via ostree or grubby.
func setKernelArg(karg string) (bool, error) {
func setKernelArg(helper helper.HostHelpersInterface, mode, karg string) error {
Copy link
Member

Choose a reason for hiding this comment

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

can you rename this to editKernelArg?

pkg/plugins/generic/generic_plugin.go Outdated
@@ -264,84 +273,83 @@ func needDriverCheckVdpaType(state *sriovnetworkv1.SriovNetworkNodeState, driver
}

// setKernelArg Tries to add the kernel args via ostree or grubby.
func setKernelArg(karg string) (bool, error) {
func setKernelArg(helper helper.HostHelpersInterface, mode, karg string) error {
log.Log.Info("generic plugin setKernelArg()")
Copy link
Member

Choose a reason for hiding this comment

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

not strictly related to this PR, but it would be useful to log the kernel argument we are trying to add:

log.Log.Info("generic plugin setKernelArg()", "karg", karg)

pkg/plugins/generic/generic_plugin.go Outdated

// getKernelArgs gets Kernel arguments on the running sy.
Copy link
Member

Choose a reason for hiding this comment

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

doc string is probably a leftover

test/conformance/tests/test_networkpool.go Outdated
WaitForSRIOVStable()
})

It("should run pod with RDMA cni and not expose nic metrics", func() {
Copy link
Member

Choose a reason for hiding this comment

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

isn't this supposed to be "without RDMA cni"?

test/scripts/rpm-ostree_mock Outdated
then
# Caller is trying to read kernel arguments.
cat /proc/cmdline
fi
Copy link
Member

Choose a reason for hiding this comment

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

nit: end file with an empty line

@SchSeba
Copy link
Collaborator Author

SchSeba commented Oct 31, 2024

about

Did a first round of review. Overall approach looks good, though the GenericPlugin is starting to become a little complicated

you are right I think we probably can add a kernel/host/driver plugin or something like that will only handle that part and leave the generic one with only creating VFs

I can try to work on that for a following PR

@SchSeba SchSeba mentioned this pull request Oct 31, 2024
Closed
@SchSeba SchSeba force-pushed the rdma-subsytem-mode branch 3 times, most recently from 0d4394a to 4be4279 Compare November 3, 2024 09:20
@SchSeba SchSeba force-pushed the rdma-subsytem-mode branch from 4be4279 to f2f7f33 Compare November 3, 2024 09:34
@e0ne e0ne dismissed their stale review November 5, 2024 10:27

need to review the latest changeset

e0ne
e0ne approved these changes Nov 5, 2024
View reviewed changes
Copy link
Collaborator

@e0ne e0ne left a comment

Choose a reason for hiding this comment

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

Thanks for adding tests!

@SchSeba SchSeba force-pushed the rdma-subsytem-mode branch from f2f7f33 to d672111 Compare November 7, 2024 13:15
@SchSeba SchSeba force-pushed the rdma-subsytem-mode branch from d672111 to 02c6b00 Compare November 7, 2024 13:29
zeeke
zeeke approved these changes Nov 7, 2024
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

Thanks for addressing my comments

@SchSeba SchSeba merged commit 92fee7b into k8snetworkplumbingwg:master Nov 11, 2024
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zeeke zeeke zeeke approved these changes

@e0ne e0ne e0ne 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.
4 participants
@SchSeba @coveralls @e0ne @zeeke