Hi Philipp,
On 10/24/23 17:02, Philipp Rudo wrote:
besides a few nits in this patch the series looks good to me.
On Mon, 16 Oct 2023 19:57:35 +0200
Steffen Maier <maier(a)linux.ibm.com> wrote:
> As of dracut commit ("feat(znet): use zdev for consolidated device
^^^^^^^
id? repo?
In general you are citing quite a few commits. Could you please
always also give the repo they are from. Otherwise it's a total pain to
find them. Especially once the patch is merged without the cover
letter.
I thought "dracut commit" would be enough to indicate the repo. ;-)
Anyway, above is just a forward reference to a commit in an unmerged pull
request so there is no (stable) commit ID yet.
That's why I also did not provide a full github URL to the repo.
I agree about info only in the cover letter and tried to pull a cover summary
into the description of this commit here for patch version 2 replacing this old
first paragraph here:
<quote from v2>
This is a preparation for consolidating s390 network device config with
https://github.com/dracutdevs/dracut/pull/2534
("feat(znet): use zdev for consolidated device configuration")
https://github.com/steffen-maier/s390utils/pull/1/commits
("znet: migrate to consolidated persistent device config with zdev
(#1937046,#1937048))"
("znet: clean up old deprecated persistent device config
(#1937046,#1937048)").
With above consolidation, s390-specific low-level configuration information
will no longer be in NetworkManager connections (nor ifcfg files), but in
the persistent configuration database of chzdev from s390-tools.
</quote from v2>
All other commits are local to the kexec-tools repo and with commit ID. I made
the effort of checking git history regarding znet handling in kexec-tools in
order to learn from the history and to avoid repeating mistakes. I reference
the commits here as a reminder for myself and others coming across this later
on so they don't have to repeat finding the commits in the git history.
> configuration"), s390-specific low-level configuration information is no
> longer in NetworkManager connections (nor ifcfg files), but in the
> persistent configuration database of chzdev from s390-tools.
>
> Since the kdump dracut module here depends on the "znet" dracut module [1]
> and "znet" copies all persistent configuration into initrd as of above
> commit, all s390-specific information is already in the kdump initrd.
> [1] 08de71252814 ("Move some dracut module dependencies checks to
> module-setup.sh"), 7148c0a30dfc ("add s390x netdev setup")
>
> Because it is more appropriate and also removes the copy dependency from
> "znet", introduce the consolidated zdev mechanism to import just the
> required network device config from the current active system
> configuration. It does not depend on any existing persistent configuration
> and can replace the old function code. This is similar to dracut block
> device dependency handling in s390-tools zdev/dracut/95zdev-kdump.
>
> The old code only seems to work if there is exactly one s390-specific
> nmconnection (or ifcfg file). Related commits:
> b5577c163aff ("Simplify setup_znet by copying connection profile to
initrd"),
> 7d472515688f ("Iterate /sys/bus/ccwgroup/devices to tell if we should set up
rd.znet"),
> 8b08b4f17ba0 ("Set up s390 znet cmdline by "nmcli
--get-values""),
> ce0305d4f95c ("Add a new option 'rd.znet_ifname' in order to use it in
udev rules"),
> 7148c0a30dfc ("add s390x netdev setup").
>
> A bonding or teaming setup would have multiple following network
> interfaces, each of which would need a low-level config if they're s390
> channel-attached network devices. The new code should be able to handle
> that by iterating the involved network interfaces. Chzdev only exports
> something if it's a device type it deems itself responsible for.
>
> Additional debugging output can be generated with e.g. dracut option
> "--stdlog 5" (or short -L5). It shows the chzdev export result, the output
> of chzdev import, and an overview of the resulting persistent config within
> the initrd. On systems, which default to using dracut option "--quiet",
you
> might need an additional "--verbose" to counter "--quiet" so -L5
has
> effect. Typically combined with "--debug" to get a shell trace from
> building an initrd (Note: --debug does not increase the log levels).
>
> Signed-off-by: Steffen Maier <maier(a)linux.ibm.com>
> ---
> dracut-module-setup.sh | 19 ++++++++++++++++++-
> 1 file changed, 18 insertions(+), 1 deletion(-)
>
> diff --git a/dracut-module-setup.sh b/dracut-module-setup.sh
> index ff53d084a9f0..64ce253936dd 100755
> --- a/dracut-module-setup.sh
> +++ b/dracut-module-setup.sh
> @@ -26,6 +26,9 @@ check() {
> if [[ -z $IN_KDUMP ]] || [[ ! -f /etc/kdump.conf ]]; then
> return 1
> fi
> + if [[ "$(uname -m)" == "s390x" ]]; then
> + require_binaries chzdev || return 1
> + fi
> return 0
> }
>
> @@ -474,6 +477,8 @@ _find_znet_nmconnection() {
> #
> # Note part of code is extracted from ccw_init provided by s390utils
> kdump_setup_znet() {
> + local _netif
> + local _tempfile=$(mktemp --tmpdir kdump-dracut-zdev.XXXXXX)
Please move the tempfile under _DRACUT_KDUMP_NM_TMP_DIR. Then the file
also gets cleaned up in case something goes wrong. Plus there is no
temptation to add an additional trap handler that overwrites the one
set by dracut.
Good idea, wasn't aware of it, done for v2.
> local _config_file _unique_name _NM_conf_dir
> local
__sed_discard_ignored_files='/\(~\|\.bak\|\.old\|\.orig\|\.rpmnew\|\.rpmorig\|\.rpmsave\)$/d'
>
> @@ -481,6 +486,18 @@ kdump_setup_znet() {
> return
> fi
>
> + for _netif in $1; do
> + chzdev --export "$_tempfile" --active --by-interface "$_netif"
\
> + 2>&1 | ddebug
> + sed -i -e 's/^\[active /\[persistent /' "$_tempfile"
> + ddebug < "$_tempfile"
> + chzdev --import "$_tempfile" --persistent --base
"/etc=$initdir/etc" \
> + --yes --no-root-update --force 2>&1 | ddebug
> + lszdev --configured --persistent --info --by-interface "$_netif" \
> + --base "/etc=$initdir/etc" 2>&1 | ddebug
Just for my personal understanding. Is the lszdev needed? Or is it only
used for debugging in case something goes wrong?
Only the two chzdev calls (export && import) are functionally required.
Dumping with 'ddebug < "$_tempfile"' and the lszdev call are only for
debugging
purposes.
They refer to the following parts in the commit description:
"It shows the chzdev export result" and
"overview of the resulting persistent config within the initrd".
The former is a format internal to chzdev and subject to change without further
notice; currently it is an ini-style text file and thus also human readable.
The lszdev output provides (a subset of the same) information in a form humans
using the zdev framework are used to and can easily understand.
Thanks
Philipp
> + done
> + rm -f "$_tempfile"
> +
> _NM_conf_dir="/etc/NetworkManager/system-connections"
>
> _config_file=$(_find_znet_nmconnection "$initdir/$_NM_conf_dir"
"$__sed_discard_ignored_files")
> @@ -572,7 +589,7 @@ kdump_install_net() {
> if [[ -n "$_netifs" ]]; then
> kdump_install_nmconnections
> apply_nm_initrd_generator_timeouts
> - kdump_setup_znet
> + kdump_setup_znet "$_netifs"
> kdump_install_nm_netif_allowlist "$_netifs"
> kdump_install_nic_driver "$_netifs"
> fi
--
Mit freundlichen Gruessen / Kind regards
Steffen Maier
Linux on IBM Z and LinuxONE
https://www.ibm.com/privacy/us/en/
IBM Deutschland Research & Development GmbH
Vorsitzender des Aufsichtsrats: Gregor Pillen
Geschaeftsfuehrung: David Faller
Sitz der Gesellschaft: Boeblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294