Rename vf representor with udev rules #29
Conversation
|
LGTM |
|
Overall looks OK, could you add a couple of sentences in the commit message describing why the change is needed and what is done in this commit |
Done |
| path: "/etc/udev/switchdev-vf-link-name.sh" | ||
| contents: | ||
| inline: | | ||
| #!/bin/bash |
There was a problem hiding this comment.
the switchdev-vf-link-name.sh is doing lot of logic which can case races. We saw it in openstack. It is better to do it like it done in os-net-config [1]. The script is just running simple echo command. We also mark the VF rep to be unmanaged by the NM with 'ENV{NM_UNMANAGED}="1"
[1] https://github.com/openstack/os-net-config/blob/3144a8fc4fae95e3cfdc680ccdcbd5144a9a17a8/os_net_config/sriov_config.py#L344-L373
[2] - https://github.com/openstack/os-net-config/blob/3144a8fc4fae95e3cfdc680ccdcbd5144a9a17a8/os_net_config/sriov_config.py#L52-L58
There was a problem hiding this comment.
the switchdev-vf-link-name.sh is doing lot of logic which can case races. We saw it in openstack. It is better to do it like it done in os-net-config [1]. The script is just running simple echo command. We also mark the VF rep to be unmanaged by the NM with 'ENV{NM_UNMANAGED}="1"
[1] https://github.com/openstack/os-net-config/blob/3144a8fc4fae95e3cfdc680ccdcbd5144a9a17a8/os_net_config/sriov_config.py#L344-L373
[2] - https://github.com/openstack/os-net-config/blob/3144a8fc4fae95e3cfdc680ccdcbd5144a9a17a8/os_net_config/sriov_config.py#L52-L58
But Moshe in os-net-config, we know the name of the sriov_pf and the switch_id and we add the udev rule according to that, Do we have such info here ? @moshe010
There was a problem hiding this comment.
the switchdev-vf-link-name.sh is doing lot of logic which can case races. We saw it in openstack. It is better to do it like it done in os-net-config [1]. The script is just running simple echo command. We also mark the VF rep to be unmanaged by the NM with 'ENV{NM_UNMANAGED}="1"
[1] https://github.com/openstack/os-net-config/blob/3144a8fc4fae95e3cfdc680ccdcbd5144a9a17a8/os_net_config/sriov_config.py#L344-L373
[2] - https://github.com/openstack/os-net-config/blob/3144a8fc4fae95e3cfdc680ccdcbd5144a9a17a8/os_net_config/sriov_config.py#L52-L58But Moshe in os-net-config, we know the name of the sriov_pf and the switch_id and we add the udev rule according to that, Do we have such info here ? @moshe010
I think it's better to add a udev rule per pf here [1], you can see this one [2]
[1] https://github.com/k8snetworkplumbingwg/sriov-network-operator/blob/master/bindata/manifests/machine-config/files/configure-switchdev.sh.yaml
[2] https://github.com/Mellanox/switchdev-utils/blob/master/switchdev_utils/switchdev.sh#L12-L74
There was a problem hiding this comment.
switch_id is only available when switchdev mode is configured. If we were to add switch_id in udev rules, we also need to add logic in config daemon to reload udev rules which I intended to avoid. because appliance of udev rules will be done automatically along with rebooting (which is required to configure switchdev device) and config daemon doesn't have to track whether node is rebooted or not (to apply the udev rules). The current change looks simple than updating config daemon code since we handle all the switchdev config via plugins.
There was a problem hiding this comment.
Removed the long bash script for renaming vf rep
There was a problem hiding this comment.
can you please check again? maybe NM starting them up?
There was a problem hiding this comment.
If it's NM, then I probably need to set the UM udev rules before any switchdev device is configured, otherwise if NM udev rules is set after rebooting, then it takes another reboot to take effect.
I'll split the rules to separate functions and call them differently.
There was a problem hiding this comment.
I'm still testing it, what I observed was:
On first reboot, VF rep was not renamed correctly, still has the name ethx
On subsequential reboot, VF rep was renamed correctly.
There was a problem hiding this comment.
ok, the issue that vf was not renamed on first reboot is because udev rules are added after reboot so they are not re-loaded automatically. Fixed it by explicitly reload the udev rules before triggering.
zshi-redhat
force-pushed
the
rename-vf-representor
branch
from
14c7630 to
5c2c0a7
Compare
| return err | ||
| } | ||
| new_content = new_content + fmt.Sprintf("SUBSYSTEM==\"net\", ACTION==\"add|move\", ATTRS{phys_switch_id}==\"%s\", ATTR{phys_port_name}==\"pf%svf*\", IMPORT{program}=\"/etc/udev/switchdev-vf-link-name.sh $attr{phys_port_name}\", NAME=\"%s_$env{NUMBER}\"\n", switchID, strings.TrimPrefix(portName, "p"), ifaceStatus.Name) | ||
| } |
There was a problem hiding this comment.
in os-net-config we have udev rule unmanage vf_representors by NM. I think you should added it as well.
pkg/daemon/daemon.go
Outdated
|
|
||
| // if the file does not exist or if old_content != new_content | ||
| // write to file and create it if it doesn't exist | ||
| err = ioutil.WriteFile(filePath, []byte(new_content), 0666) |
There was a problem hiding this comment.
why rw to other? isn't 0664 is better?
|
|
||
| old_content, err := ioutil.ReadFile(filePath) | ||
| // if old_content = new_content, don't do anything | ||
| if err == nil && new_content == string(old_content) { |
There was a problem hiding this comment.
you can string(old_content) once. currently you doing in L861 and L866
There was a problem hiding this comment.
I don't see how I can do it once w/o adding a new string var, any idea?
zshi-redhat
force-pushed
the
rename-vf-representor
branch
from
5c2c0a7 to
4305a77
Compare
zshi-redhat
force-pushed
the
rename-vf-representor
branch
from
4305a77 to
aedaf1d
Compare
Signed-off-by: Zenghui Shi <[email protected]>
zshi-redhat
force-pushed
the
rename-vf-representor
branch
from
aedaf1d to
88b18b2
Compare
|
@moshe010 any other suggestions on the renaming PR? |
|
@zshi-redhat I think we good to go with this one. Can I merge it? |
Yes, please. I just don't want to merge my own PR. |
VF representor is named in the format of ethX, where X is an incremental
number, it doesn't tell which PF it belongs to and which VF is the peer device.
By renaming it with udev rules, VF rep name will be in the format of PF_[vf-index].
This commit add udev rules file and relevant script for machine config plugin,
which work only with openshift clusterType.
Signed-off-by: Zenghui Shi [email protected]