Hi Philipp,
On Mon, Aug 01, 2022 at 04:57:22PM +0200, Philipp Rudo wrote:
Hi Coiby,
On Tue, 21 Jun 2022 14:57:27 +0800
Coiby Xu <coxu(a)redhat.com> wrote:
> A NIC may get a different name in the kdump kernel from 1st kernel
> in cases like,
> - kernel assigned network interface names are not persistent e.g. [1]
> - there is an udev rule to rename the NIC in the 1st kernel but the
> kdump initrd may not have that rule e.g. [2]
>
> If NM tries to match a NIC with a connection profile based on NIC name
> i.e. connection.interface-name, it will fail the above bases. A simple
> solution is to ask NM to match a connection profile by MAC address.
> Note we don't need to do this for user-created NICs like vlan, bridge and
> bond.
>
> An remaining issue is passing the name of a NIC via the kdumpnic dracut
> command line parameter which requires passing ifname=<interface>:<MAC>
to
> have fixed NIC name. But we can simply drop this requirement. kdumpnic
> is needed because kdump needs to get the IP by NIC name and use the IP
> to created a dumping folder named "{IP}-{DATE}". We can simply pass the
> IP to the kdump kernel directly via a new dracut command line parameter
> kdumpip instead. In addition to the benefit of simplifying the code,
> there are other three benefits brought by this approach,
> - make use of whatever network to transfer the vmcore. Because as long
> as we have the network to we don't care which NIC is active.
> - if obtained IP in the kdump kernel is different from the one in the
> 1st kernel. "{IP}-{DATE}" would better tell where the dumped vmcore
> comes from.
> - without passing ifname=<interface>:<MAC> to to kdump initrd, the
s/to to/to/
Thanks for catching this typo.
> issue of there are two interfaces with the same MAC address for
> Azure Hyper-V NIC SR-IOV [3] is resolved automatically.
>
> [1]
https://bugzilla.redhat.com/show_bug.cgi?id=1121778
> [2]
https://bugzilla.redhat.com/show_bug.cgi?id=810107
> [3]
https://bugzilla.redhat.com/show_bug.cgi?id=1962421
>
> Signed-off-by: Coiby Xu <coxu(a)redhat.com>
> ---
> dracut-kdump.sh | 25 ++++++++--------
> dracut-module-setup.sh | 66 +++++++++++++++++-------------------------
> kdump-lib-initramfs.sh | 12 ++++++++
> 3 files changed, 51 insertions(+), 52 deletions(-)
>
> diff --git a/dracut-kdump.sh b/dracut-kdump.sh
> index f4456a1..21622f7 100755
> --- a/dracut-kdump.sh
> +++ b/dracut-kdump.sh
> @@ -479,22 +479,23 @@ save_vmcore_dmesg_ssh()
> get_host_ip()
> {
> if is_nfs_dump_target || is_ssh_dump_target; then
> - kdumpnic=$(getarg kdumpnic=)
> - if [ -z "$kdumpnic" ]; then
> - derror "failed to get kdumpnic!"
> - return 1
> - fi
> - if ! kdumphost=$(ip addr show dev "$kdumpnic" | grep '[
]*inet'); then
> - derror "wrong kdumpnic: $kdumpnic"
> + _kdump_remote_ip=$(getarg kdump_remote_ip=)
> +
> + if [ -z "$_kdump_remote_ip" ]; then
> + derror "failed to get remote IP address!"
> return 1
> fi
> - kdumphost=$(echo "$kdumphost" | head -n 1 | awk '{print $2}')
> - kdumphost="${kdumphost%%/*}"
> - if [ -z "$kdumphost" ]; then
> - derror "wrong kdumpnic: $kdumpnic"
> + _route=$(kdump_get_ip_route "$_kdump_remote_ip")
> + _netdev=$(kdump_get_ip_route_field "$_route" "dev")
> +
> + if ! _kdumpip=$(ip addr show dev "$_netdev" | grep '[ ]*inet');
then
> + derror "Failed to get IP of $_netdev"
> return 1
> fi
> - HOST_IP=$kdumphost
> +
> + _kdumpip=$(echo "$_kdumpip" | head -n 1 | awk '{print $2}')
> + _kdumpip="${_kdumpip%%/*}"
> + HOST_IP=$_kdumpip
> fi
> return 0
> }
As you are basically rewriting this function you could move everything
out of the if-block and simply have an
if ! is_nfs_dump_target && ! is_ssh_dump_target; then
return 0
fi
at the beginning of the function. With that you get rid of the extra
indentation for all the code.
Good suggestion!
Furthermore you could declare the variables as local.
dracut-kdump.sh is supposed to be POSIX-compatible so we shouldn't
decare the variables as local.
> diff --git a/dracut-module-setup.sh b/dracut-module-setup.sh
> index d5bec47..1a8f55f 100755
> --- a/dracut-module-setup.sh
> +++ b/dracut-module-setup.sh
> @@ -222,26 +222,6 @@ kdump_get_perm_addr() {
> fi
> }
>
> -# Prefix kernel assigned names with "kdump-". EX: eth0 -> kdump-eth0
> -# Because kernel assigned names are not persistent between 1st and 2nd
> -# kernel. We could probably end up with eth0 being eth1, eth0 being
> -# eth1, and naming conflict happens.
> -kdump_setup_ifname() {
> - local _ifname
> -
> - # If ifname already has 'kdump-' prefix, we must be switching from
> - # fadump to kdump. Skip prefixing 'kdump-' in this case as adding
> - # another prefix may truncate the ifname. Since an ifname with
> - # 'kdump-' is already persistent, this should be fine.
> - if [[ $1 =~ eth* ]] && [[ ! $1 =~ ^kdump-* ]]; then
> - _ifname="kdump-$1"
> - else
> - _ifname="$1"
> - fi
> -
> - echo "$_ifname"
> -}
> -
> _clone_nmconnection() {
> local _clone_output _name _unique_id
>
> @@ -255,10 +235,25 @@ _clone_nmconnection() {
> return 1
> }
>
> +_match_nmconnection_by_mac() {
> + local _unique_id _dev _mac
> +
> + _unique_id=$1
> + _dev=$2
> +
> + _mac=$(kdump_get_perm_addr "$_dev")
> + if [[ $_mac != 'not set' ]]; then
> + _mac_field=$(nmcli connection show "$_unique_id" | grep
'\.mac-address:' | cut -d: -f1)
not really sure if it's worth it but you should(tm) be able to drop the
two pipes and the regex by calling nmcli twice, i.e.
_type=$(nmcli --get-values connection.type connection show "$_unique_id")
_mac_field=$(nmcli --get-values ${_type}.mac_address connection show
"$_unique_id")
Actually I'm going to get the field name instead of value. But "nmcli
--get-values" is more robust then regex, so I will use
_mac_field=$(nmcli --get-values connection.type connection show
"$_unique_id").mac-address
Furthermore you should declare $_mac_field (and $_type if you use it)
local.
Sure, thanks for the reminder!
> + nmcli connection modify --temporary "$_unique_id"
"$_mac_field" "$_mac"
> + nmcli connection modify --temporary "$_unique_id"
"connection.interface-name" ""
> + fi
> +}
> +
> # Clone NM connection profiles
> #
> # This function makes use of "nmcli clone" to automatically convert
ifcfg-*
> -# files to Networkmanager .nmconnection connection profiles.
> +# files to Networkmanager .nmconnection connection profiles. It also uses
> +# "nmcli modify" to modify or remove properties
> clone_nmconnections() {
> local _dev _cloned_nmconnection_file_path _tmp_nmconnection_file_path _old_uuid
_uuid
>
> @@ -271,6 +266,10 @@ clone_nmconnections() {
> exit 1
> fi
>
> + # For physical NIC i.e. non-user created NIC, ask NM to match a
> + # connection profile based on MAC address
> + _match_nmconnection_by_mac "$_uuid" "$_dev"
> +
> _cloned_nmconnection_file_path=$(nmcli --get-values UUID,FILENAME connection
show | sed -n "s/^${_uuid}://p")
> _tmp_nmconnection_file_path=$_DRACUT_KDUMP_NM_TMP_DIR/$(basename
"$_nmconnection_file_path")
> cp "$_cloned_nmconnection_file_path"
"$_tmp_nmconnection_file_path"
> @@ -465,19 +464,6 @@ kdump_setup_znet() {
> echo "rd.znet=${NETTYPE},${SUBCHANNELS},${_options}
rd.znet_ifname=$_netdev:${SUBCHANNELS}" >
"${initdir}/etc/cmdline.d/30znet.conf"
> }
>
> -kdump_get_ip_route() {
> - local _route
> - if ! _route=$(/sbin/ip -o route get to "$1" 2>&1); then
> - derror "Bad kdump network destination: $1"
> - exit 1
> - fi
> - echo "$_route"
> -}
> -
> -kdump_get_ip_route_field() {
> - echo "$1" | sed -n -e
"s/^.*\<$2\>\s\+\(\S\+\).*$/\1/p"
> -}
> -
> kdump_get_remote_ip() {
> local _remote _remote_temp
> _remote=$(get_remote_host "$1")
> @@ -495,7 +481,7 @@ kdump_get_remote_ip() {
> # initramfs accessing giving destination
> # $1: destination host
> kdump_install_net() {
> - local _destaddr _route _netdev kdumpnic
> + local _destaddr _route _netdev
> local _znet_netdev _znet_conpath
> # each netowrk interface is managed by a NM connection profile
> declare -A nmconnection_map
> @@ -508,7 +494,6 @@ kdump_install_net() {
> _destaddr=$(kdump_get_remote_ip "$1")
> _route=$(kdump_get_ip_route "$_destaddr")
> _netdev=$(kdump_get_ip_route_field "$_route" "dev")
> - kdumpnic=$(kdump_setup_ifname "$_netdev")
>
> _znet_netdev=$(find_online_znet_device)
> if [[ -n $_znet_netdev ]]; then
> @@ -528,6 +513,7 @@ kdump_install_net() {
> elif kdump_is_vlan "$_netdev"; then
> kdump_setup_vlan "$_netdev"
> fi
> +
> kdump_copy_nmconnection_file "$_netdev"
>
> if [[ ! -f ${initdir}/etc/cmdline.d/50neednet.conf ]]; then
> @@ -547,8 +533,8 @@ kdump_install_net() {
> # the default gate way for network dump, eth1 in the fence kdump path will
> # call kdump_install_net again and we don't want eth1 to be the default
> # gateway.
> - if [[ ! -f ${initdir}/etc/cmdline.d/60kdumpnic.conf ]]; then
> - echo "kdumpnic=$kdumpnic" >
"${initdir}/etc/cmdline.d/60kdumpnic.conf"
> + if [[ ! -f ${initdir}/etc/cmdline.d/60kdumpip.conf ]]; then
> + echo "kdump_remote_ip=$_destaddr" >
"${initdir}/etc/cmdline.d/60kdumpip.conf"
> fi
>
> }
> @@ -1024,8 +1010,8 @@ install() {
>
> _netifs=$(cat "$_TMP_KDUMP_NETIFS")
> if [[ -n "$_netifs" ]]; then
> - kdump_install_nm_netif_allowlist "$_netifs"
> - kdump_install_nic_driver "$_netifs"
> + kdump_install_nm_netif_allowlist "$_netifs"
> + kdump_install_nic_driver "$_netifs"
This should go into the two patches that introduced the two lines.
Good catch!
Thanks
Philipp
--
Best regards,
Coiby