-
Notifications
You must be signed in to change notification settings - Fork 111
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
Implement RDMA subsystem mode change #666
base: master
Are you sure you want to change the base?
Conversation
Thanks for your PR,
To skip the vendors CIs use one of:
|
c986251
to
d3641ab
Compare
Thanks for your PR,
To skip the vendors CIs use one of:
|
d3641ab
to
a0af988
Compare
Thanks for your PR,
To skip the vendors CIs use one of:
|
a0af988
to
a3ce3a3
Compare
Thanks for your PR,
To skip the vendors CIs use one of:
|
a3ce3a3
to
3952e04
Compare
Thanks for your PR,
To skip the vendors CIs use one of:
|
3952e04
to
bcb0804
Compare
Thanks for your PR,
To skip the vendors CIs use one of:
|
Pull Request Test Coverage Report for Build 10772338186Details
💛 - Coveralls |
pkg/systemd/systemd.go
Outdated
newFile := false | ||
// remove the device plugin revision as we don't need it here | ||
newState.Spec.DpConfigVersion = "" | ||
|
||
// shared mode is a default on OS | ||
rdmaMode := consts.RdmaSubsystemModeShared |
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 think we should try to query/change mode only in case if rdmaMode parameter is explicitly set in the poolConfig, to provide a safer behavior for ENVs which doesn't use RDMA.
@@ -152,6 +152,17 @@ func phasePre(setupLog logr.Logger, conf *systemd.SriovConfig, hostHelpers helpe | |||
hostHelpers.TryEnableTun() | |||
hostHelpers.TryEnableVhostNet() | |||
|
|||
rdmaSubsystem, err := hostHelpers.GetRDMASubsystem() |
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 think we should execute this logic only if mode configuration is explicitly requested by a user.
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.
done
|
||
// +kubebuilder:validation:Enum=shared;exclusive | ||
// RDMA subsystem. Allowed value "shared", "exclusive". | ||
RdmaMode string `json:"rdmaMode,omitempty"` |
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.
This option is only valid for systemd mode?
Do we want to document this somehow?
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.
done as log message in a SriovNetworkPoolConfig controller
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.
@e0ne cant we set this using module parameter ?
bcb0804
to
79013d7
Compare
Thanks for your PR,
To skip the vendors CIs use one of:
|
79013d7
to
f60fdf7
Compare
Thanks for your PR,
To skip the vendors CIs use one of:
|
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 added few additional comments
if conf.RdmaMode != "" { | ||
rdmaSubsystem, err := hostHelpers.GetRDMASubsystem() | ||
if err != nil { | ||
setupLog.Error(err, "failed to get RDMA subsystem mode") |
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.
If conf.RdmaMode is not empty string, then the user explicitly requested RDMA mode configuration. I think we can return error in this case. WDYT?
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.
done
if rdmaSubsystem != conf.RdmaMode { | ||
err = hostHelpers.SetRDMASubsystem(conf.RdmaMode) | ||
if err != nil { | ||
setupLog.Error(err, "failed to set RDMA subsystem mode") |
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.
Do we want to return error here?
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.
we want, thanks!
pkg/host/internal/kernel/kernel.go
Outdated
@@ -522,6 +522,34 @@ func (k *kernel) InstallRDMA(packageManager string) error { | |||
return nil | |||
} | |||
|
|||
func (k *kernel) GetRDMASubsystem() (string, error) { | |||
log.Log.Info("GetRDMASubsystem(): retrieving RDMA subsystem mode") | |||
chrootDefinition := utils.GetChrootExtension() |
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.
We have helper to enter chroot (part of utilsHelper). Do we want to use it here?
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.
We'd got the same implementation in all `kernel' methods. Let's do it in a scope of a separate PR
f60fdf7
to
a5f0d3b
Compare
Thanks for your PR,
To skip the vendors CIs use one of:
|
Thanks for your PR,
To skip the vendors CIs use one of:
|
feb4bd0
to
ab92392
Compare
Thanks for your PR,
To skip the vendors CIs use one of:
|
ab92392
to
f37c6d1
Compare
Thanks for your PR,
To skip the vendors CIs use one of:
|
@SchSeba could you please review this PR? |
f37c6d1
to
3d19033
Compare
Thanks for your PR,
To skip the vendors CIs use one of:
|
Thanks for your PR,
To skip the vendors CIs use one of:
|
Thanks for your PR,
To skip the vendors CIs use one of:
|
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.
nice work getting close to be ready :)
@@ -23,9 +23,14 @@ import ( | |||
// EDIT THIS FILE! THIS IS SCAFFOLDING FOR YOU TO OWN! | |||
// NOTE: json tags are required. Any new fields you add must have json tags for the fields to be serialized. | |||
|
|||
type System struct { |
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.
nit: please move this one below the SriovNetworkNodeStateSpec
struct
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.
done
@@ -152,6 +153,21 @@ func phasePre(setupLog logr.Logger, conf *systemd.SriovConfig, hostHelpers helpe | |||
hostHelpers.TryEnableTun() | |||
hostHelpers.TryEnableVhostNet() | |||
|
|||
if conf.Spec.System.RdmaMode != "" { | |||
rdmaSubsystem, err := netlink.RdmaSystemGetNetnsMode() |
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.
please use the hostHelper interface don't call the netlink lib directly so it will be easier to add unit-tests
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.
done
return fmt.Errorf("failed to get RDMA subsystem mode: %v", err) | ||
} | ||
if rdmaSubsystem != conf.Spec.System.RdmaMode { | ||
err = netlink.RdmaSystemSetNetnsMode(conf.Spec.System.RdmaMode) |
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.
please use the hostHelper interface don't call the netlink lib directly so it will be easier to add unit-tests
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.
done
@@ -58,3 +58,6 @@ rules: | |||
- apiGroups: [ "config.openshift.io" ] | |||
resources: [ "infrastructures" ] | |||
verbs: [ "get", "list", "watch" ] | |||
- apiGroups: [ "sriovnetwork.openshift.io" ] |
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 don't think we need this one as the daemon will continue to only see the nodeState
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.
done
pkg/daemon/writer.go
Outdated
@@ -127,7 +128,13 @@ func (w *NodeStateStatusWriter) pollNicStatus() error { | |||
if err != nil { | |||
return err | |||
} | |||
rdmaMode, err = w.hostHelper.GetRDMASubsystem() |
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.
please lets have a nice function in hostHelper like DiscoverSriovDevices
to DiscoverSystemConfig()
or something like that it will return System struct
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.
renamed to DiscoverRDMASubsystem
. It will always return a string so there is no need to introduce a new type
Thanks for your PR,
To skip the vendors CIs use one of:
|
Thanks for your PR,
To skip the vendors CIs use one of:
|
@e0ne can you rebase the pr ? |
Thanks for your PR,
To skip the vendors CIs use one of:
|
Thanks for your PR,
To skip the vendors CIs use one of:
|
pkg/utils/cluster.go
Outdated
@@ -161,3 +171,94 @@ func AnnotateNode(ctx context.Context, nodeName string, key, value string, c cli | |||
|
|||
return AnnotateObject(ctx, node, key, value, c) | |||
} | |||
|
|||
func FindNodePoolConfig(ctx context.Context, node *corev1.Node, c client.Client) (*sriovnetworkv1.SriovNetworkPoolConfig, []corev1.Node, error) { |
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.
nit: add docstring
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.
also im thinking we should have two functions:
- find node pool for node
- find nodes for node pool (with special handling for case where default node pool was provided)
WDYT ?
Also please add UT for whatever we end up with
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.
+1
pkg/utils/cluster.go
Outdated
@@ -26,6 +28,14 @@ const ( | |||
controlPlaneNodeLabelKey = "node-role.kubernetes.io/control-plane" | |||
) | |||
|
|||
var ( | |||
oneNode = intstr.FromInt32(1) | |||
defaultNpcl = &sriovnetworkv1.SriovNetworkPoolConfig{Spec: sriovnetworkv1.SriovNetworkPoolConfigSpec{ |
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.
nit: can we use full name here ? e.g defaultPoolConfig ?
also the 'l' at the end is not related
pkg/utils/cluster.go
Outdated
return nil, nil, err | ||
} | ||
|
||
// list all the nodes that are also part of this pool and return them |
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.
for those nodes why arent we validating they match exactly one ncp ? like in L223
pkg/daemon/daemon.go
Outdated
@@ -420,6 +421,13 @@ func (dn *Daemon) nodeStateSyncHandler() error { | |||
// When using systemd configuration we write the file | |||
if vars.UsingSystemdMode { | |||
log.Log.V(0).Info("nodeStateSyncHandler(): writing systemd config file to host") | |||
// get node object | |||
node := &corev1.Node{} |
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 dont see node is being used in this scope.
@@ -92,6 +92,23 @@ spec: | |||
mountPath: /host/etc/os-release | |||
readOnly: true | |||
{{- end }} | |||
{{- if .RDMACNIImage }} |
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.
Hi please rebase this PR now that we merged the rdma-cni deployment
@@ -152,6 +152,21 @@ func phasePre(setupLog logr.Logger, conf *systemd.SriovConfig, hostHelpers helpe | |||
hostHelpers.TryEnableTun() | |||
hostHelpers.TryEnableVhostNet() | |||
|
|||
if conf.Spec.System.RdmaMode != "" { |
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 think we can remove this one as we do the configure via the modeprobe file
@@ -114,10 +115,15 @@ type OVSUplinkConfigExt struct { | |||
Interface OVSInterfaceConfig `json:"interface,omitempty"` | |||
} | |||
|
|||
type System struct { | |||
RdmaMode string `json:"rdmaMode,omitempty"` |
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 think you can add here also
// +kubebuilder:validation:Enum=shared;exclusive
// RDMA subsystem. Allowed value "shared", "exclusive".
@@ -269,6 +270,13 @@ func (r *SriovNetworkNodePolicyReconciler) syncAllSriovNetworkNodeStates(ctx con | |||
ns.Name = node.Name | |||
ns.Namespace = vars.Namespace | |||
j, _ := json.Marshal(ns) | |||
netPoolConfig, _, err := utils.FindNodePoolConfig(context.Background(), &node, r.Client) |
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.
please use the context from the function don't create a new one
@@ -73,6 +73,19 @@ func (r *SriovNetworkPoolConfigReconciler) Reconcile(ctx context.Context, req ct | |||
return reconcile.Result{}, err | |||
} | |||
|
|||
// RdmaMode could be set in systemd mode only | |||
if instance.Spec.RdmaMode != "" { |
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.
please remove this one as we support this on both modes
pkg/host/internal/kernel/kernel.go
Outdated
@@ -522,6 +523,29 @@ func (k *kernel) InstallRDMA(packageManager string) error { | |||
return nil | |||
} | |||
|
|||
func (k *kernel) DiscoverRDMASubsystem() (string, error) { |
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 think we can move this function to the network or sriov package
pkg/host/internal/kernel/kernel.go
Outdated
@@ -522,6 +523,29 @@ func (k *kernel) InstallRDMA(packageManager string) error { | |||
return nil | |||
} | |||
|
|||
func (k *kernel) DiscoverRDMASubsystem() (string, error) { | |||
log.Log.Info("DiscoverRDMASubsystem(): retrieving RDMA subsystem mode") | |||
subsystem, err := netlink.RdmaSystemGetNetnsMode() |
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.
please use the netlink interface in the project so we can have a mock for it on unit tests
pkg/host/internal/kernel/kernel.go
Outdated
return subsystem, nil | ||
} | ||
|
||
func (k *kernel) SetRDMASubsystem(mode string) error { |
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 think this function is no needed now that we use the modprobe file
pkg/utils/cluster.go
Outdated
@@ -161,3 +171,94 @@ func AnnotateNode(ctx context.Context, nodeName string, key, value string, c cli | |||
|
|||
return AnnotateObject(ctx, node, key, value, c) | |||
} | |||
|
|||
func FindNodePoolConfig(ctx context.Context, node *corev1.Node, c client.Client) (*sriovnetworkv1.SriovNetworkPoolConfig, []corev1.Node, error) { |
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.
+1
e0ae318
to
8ce9b72
Compare
Hi @e0ne can you please rebase the PR? |
done |
e294415
to
288e028
Compare
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.
nice work!
I left some small comments
controllers/drain_controller.go
Outdated
} | ||
return defaultNpcl, defaultNodeLists, nil | ||
} | ||
return utils.FindNodePoolConfig(ctx, node, dr.Client) |
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.
should we put this in the helper of the controllers?
I don't want to utils to start growing again
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.
it makes sense. done
@@ -272,6 +273,13 @@ func (r *SriovNetworkNodePolicyReconciler) syncAllSriovNetworkNodeStates(ctx con | |||
ns.Name = node.Name | |||
ns.Namespace = vars.Namespace | |||
j, _ := json.Marshal(ns) | |||
netPoolConfig, _, err := utils.FindNodePoolConfig(ctx, &node, r.Client) |
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.
just a general todo here we should have in memory map I think for this
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.
could you please elaborate on this?
pkg/host/internal/network/network.go
Outdated
@@ -23,6 +24,8 @@ import ( | |||
"github.com/k8snetworkplumbingwg/sriov-network-operator/pkg/vars" | |||
) | |||
|
|||
var ManifestsPath = "./bindata/manifests" |
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.
lets put this in consts
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.
It's not needed anymore, so I deleted it
if mode == "exclusive" { | ||
modeValue = 0 | ||
} | ||
config := fmt.Sprintf("options ib_core netns_mode=%d\n", modeValue) |
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 think if you use this we can remove the other file in the manifests no?
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.
it's deleted now
pkg/host/internal/network/network.go
Outdated
modeValue = 0 | ||
} | ||
config := fmt.Sprintf("options ib_core netns_mode=%d\n", modeValue) | ||
err := os.WriteFile("/etc/modprobe.d/ib_core.conf", []byte(config), 0644) |
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.
please use the getExtention here so we know if we are not inside a chroot
pkg/host/internal/network/network.go
Outdated
return fmt.Errorf("failed to write ib_core config: %v", err) | ||
} | ||
|
||
err = os.WriteFile(path.Join(consts.Chroot, "/etc/modprobe.d/ib_core.conf"), []byte(config), 0644) |
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.
this looks like a duplicate?
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.
rebase issue, it's deleted now
pkg/render/render.go
Outdated
@@ -77,6 +77,14 @@ func RenderDir(manifestDir string, d *RenderData) ([]*unstructured.Unstructured, | |||
return out, nil | |||
} | |||
|
|||
func RenderToString(path string, d *RenderData) (string, error) { |
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 don't think we need this function where we use it?
pkg/utils/cluster.go
Outdated
@@ -161,3 +171,94 @@ func AnnotateNode(ctx context.Context, nodeName string, key, value string, c cli | |||
|
|||
return AnnotateObject(ctx, node, key, value, c) | |||
} | |||
|
|||
func FindNodePoolConfig(ctx context.Context, node *corev1.Node, c client.Client) (*sriovnetworkv1.SriovNetworkPoolConfig, []corev1.Node, error) { |
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.
please move this function to the helpers in controllers better then adding more stuff to utils
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.
deleted
288e028
to
596e73d
Compare
Now it's possible to configure RDMA subsystem mode using SR-IOV Network Operator in systemd mode.
We can't configure RDMA subsystem in a daemon mode because it should be done on host before any network namespace is created.