-
Notifications
You must be signed in to change notification settings - Fork 114
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
Remove logic that installs rdma-core package #746
Remove logic that installs rdma-core package #746
Conversation
Thanks for your PR,
To skip the vendors CIs, Maintainers can use one of:
|
Pull Request Test Coverage Report for Build 10194187385Details
💛 - Coveralls |
we should not unload mlx5_core driver from kernel, thats dangerous as primary NIC may become unavailable. +1 on just logging if something is wrong like we do in coreos. perhaps with a meaningfull msg like |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am soooo happy to see this code going away from the operator!
never like it :)
Can you please just add in the read me information that the user must install the rdma package on the system?
also you can add the in RedHat CoreOS that is not needed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
The logic removed to align the code for all platforms. Before this change the state was following: * Ubuntu - the logic is completely broken and newer executed * RHCOS - validation only * RHEL - package installation (partially broken in RHEL 8 and RHEL 9) The commit changes logic for all OS to do a validation only. Signed-off-by: Yury Kulazhenkov <ykulazhenkov@nvidia.com>
7c5b97c
to
f8cc336
Compare
@SchSeba I updated the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Remove logic that installs
rdma-core
package to align the code for all platforms.Before this commit the state is following:
Reason: buggy IsUbuntuCheck()
We use grep with
--quiet
flag andIsUbuntuCheck()
returns true only ifstdout
is not empty. Because of the--quiet
flag, stdout will be always empty. Broken for a long time, nobody reported an issue about this.The operator currently uses quite dangerous logic to trigger loading of RDMA modules
sriov-network-operator/pkg/host/internal/kernel/kernel.go
Line 525 in ee40683
I think we don't want the operator to install the package and completely unload the driver, this can break multiple use-cases (NVIDIA driver in container, Externally managed PF and potentially more)
The commit changes logic for all OS to do only a validation of RDMA modules and print warning if they are not found.
@adrianchiris @SchSeba