-
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
[switchdev 9/9] Enable new switchdev implementation #643
[switchdev 9/9] Enable new switchdev implementation #643
Conversation
Thanks for your PR,
To skip the vendors CIs use one of:
|
92b5cea
to
72cbc06
Compare
Thanks for your PR,
To skip the vendors CIs use one of:
|
Pull Request Test Coverage Report for Build 8373549591Details
💛 - Coveralls |
72cbc06
to
ca23af7
Compare
Thanks for your PR,
To skip the vendors CIs use one of:
|
ca23af7
to
168086e
Compare
Thanks for your PR,
To skip the vendors CIs use one of:
|
168086e
to
6f63b38
Compare
Thanks for your PR,
To skip the vendors CIs use one of:
|
6f63b38
to
4157e8f
Compare
Thanks for your PR,
To skip the vendors CIs use one of:
|
4157e8f
to
a271d5a
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:
|
a271d5a
to
f5678a8
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.
partial review.
nice work one comment that is critical is the removed of the vfs to skip I didn't see any replacement for that functionallity
# Restart system services | ||
chroot $chroot_path /bin/bash -c systemctl restart NetworkManager.service >/dev/null 2>&1 || true | ||
chroot $chroot_path /bin/bash -c systemctl restart ovs-vswitchd.service >/dev/null 2>&1 || true | ||
fi |
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 do not release ovs and network manager anymore on cleanup?
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 don't patch NetworkManager anymore so there is no need to reload it. Restart of the ovs was always useless, because it doesn't not change anything (hw-offloading option is stored in ovsdb and will be preserved after restart)
log.Log.Error(err, "PrepareVFRepUdevRule(): failed to write representor name UDEV script") | ||
return err | ||
} | ||
if err := os.Chmod(targetPath, 0755); err != nil { |
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 we write the file with that permission should we also change the permissions?
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.
Permissions from WriteFile function are used for new files only. We need to hanlde scenario with exisiting file as well.
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 file may be written with permissions other than 755? or you want to handle a case where the file's permissions somehow changed ?
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 file may be written with permissions other than 755? or you want to handle a case where the file's permissions somehow changed ?
Yes, to handle permission change. I'm fine to remove this call if you both think that this 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.
im fine with either :)
} | ||
|
||
// skipConfigVf Use systemd service to configure switchdev mode or BF-2 NICs in OpenShift | ||
func skipConfigVf(ifSpec sriovnetworkv1.Interface, ifStatus sriovnetworkv1.InterfaceExt, mlxHelper mlx.MellanoxInterface) (bool, 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.
how we handle the BF cards if we remove this one?
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 assume there is no need for special handling of BF cards anymore. Creation of VFs should work in both BF modes(DPU and NIC) when the operator is in any configuration mode(in daemon or in systemd mode). If you need to create VFs on boot, e.g. for hw-offloading use-case, you need to switch the operator to systemd mode.
Did I miss something?
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 are running some validations internally base on OCP deployment and BF2 just to be sure everything continue to work as expected
cc @zeeke
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.
Great work @ykulazhenkov went over the PR added some nits.
Overall LGTM from my side.
any idea why k8s CI is failing ?
Hi @ykulazhenkov will you be able to rebase this PR please :) |
Signed-off-by: Yury Kulazhenkov <[email protected]>
Additional checks to perform: - Eswitch mode - VdpaType for VFs Signed-off-by: Yury Kulazhenkov <[email protected]>
PrepareVFRepUdevRule() function creates helper scrip for udev rules that rename VF representors in switchdev mode. Signed-off-by: Yury Kulazhenkov <[email protected]>
This commit removes services and scripts related to old switchdev implementations Signed-off-by: Yury Kulazhenkov <[email protected]>
f5678a8
to
ca9cc92
Compare
Thanks for your PR,
To skip the vendors CIs use one of:
|
@SchSeba |
Thanks for your PR,
To skip the vendors CIs use one of:
|
Signed-off-by: Yury Kulazhenkov <[email protected]>
Fix the function to correctly handle devices which doesn't support software_steering Signed-off-by: Yury Kulazhenkov <[email protected]>
e0bfa73
to
b7fb8ce
Compare
Thanks for your PR,
To skip the vendors CIs use one of:
|
b7fb8ce
to
9cdd37c
Compare
Thanks for your PR,
To skip the vendors CIs use one of:
|
9cdd37c
to
c5a086f
Compare
Thanks for your PR,
To skip the vendors CIs use one of:
|
c5a086f
to
b40c9ba
Compare
Thanks for your PR,
To skip the vendors CIs use one of:
|
The operator will create two rules specific for PFs in swithcdev mode: - rule to persist PF name after switching to switchdev mode - rule to rename VF representors First rule can be applied before PF is switched to switchdev mode. Second rule can be applied only when the PF is already in switchdev mode(phys_port_name and phys_switch_id) may not be available when PF is in legacy mode. reload of UDEV rules is required to apply VF representor rule for already created VFs. Signed-off-by: Yury Kulazhenkov <[email protected]>
b40c9ba
to
0bdf464
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.
Glad to see bash scripts removal!
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.
No regression has been showed against OpenShift/OVNk Hardware Offloading scenario
LGTM
This PR contains multiple commits to disable and remove current switchdev implementation and to enable new one.
Depends on #628 and #642
This PR should be the last one in the series
cc @adrianchiris @zeeke @SchSeba
last 5 commits are relevant for the PR