Fix compatibility with bootc systems #28

licliu · 2024-07-23T03:11:47Z

No description provided.

kdump-lib.sh

liutgnu · 2024-07-23T05:12:20Z

For the v2, ack

prudo1 · 2024-07-23T09:43:34Z

kdump-lib-initramfs.sh

 get_mntpoint_from_target()
 {
-	# --source is applied to ensure non-bind mount is returned
-	get_mount_info TARGET source "$1" -f
+	local SOURCE TARGET
+	findmnt -k --pairs -o SOURCE,TARGET "$1" | while read line; do
+		eval "$line"
+		# omit sources that are bind mounts i.e. they contain a [/path/to/subpath].
+		if [[ ! "$SOURCE" =~ \[ ]]; then
+			echo $TARGET
+			break
+		fi
+	done


I really like to avoid 'eval' here. Especially as it's not necessary.
For me the following worked

while read _source _target; do ... done < <(findmnt -k -n -o SOURCE,TARGET "$1")

Other than that the series looks fine to me.

That would fail if the source or target contained whitespace, which is pretty unlikely of course...but, still.

(OTOH, the real fix is to not write any nontrivial shell script at all...)

Hi @prudo1 , I think we can merge this first since it works well now. And we can make changes later to avoid code conflicts if we touch this code in the future.

licliu · 2024-08-15T03:59:51Z

Hi @prudo1, I changed the code to avoid 'eval'. And shellcheck doesn't like done < <(program) so I still use pipe.

travier · 2024-08-21T16:57:38Z

dracut-module-setup.sh

+    # Disable ostree as we only need the physical root
+    systemctl -q --root "$initdir" mask ostree-prepare-root.service


If you do this, this means that the crash dump file will be written in the "real root" partition (/sysroot) and not in the actual location set in the configuration file.

I did test this. I think the reason it works is because the earlier code is resolving through the bind mount to the real sysroot target.

👍🏻 Looks good then

prudo1 · 2024-08-22T11:49:21Z

kdump-lib-initramfs.sh

+	local _source _target
+	# omit sources that are bind mounts i.e. they contain a [/path/to/subpath].
+	findmnt -k --pairs -o SOURCE,TARGET "$1" | while read -r line; do
+		_source="${line#SOURCE=\"}"
+		_source="${_source%%\" TARGET=*}"
+
+		_target="${line#*TARGET=\"}"
+		_target="${_target%%\"}"
+


Hi Lichen,
a few nits here.

shellcheck complains about the use of local as it's not POSIX compliant

you can read multiple variables with read, i.e. you can drop line

when you drop --pairs findmnt should omit the SOURCE= and TARGET= prefix as well as the quotation (please double check)

putting it all together this should simplify the code to a single line

findmnt -k -o SOURCE,TARGET "$1" | while read -r _source _target; do

Other than that the series looks fine

Oups, missed the comment from @cgwalters above. He's right, that 3. would break if either _source or _target contains a space. So probably better to keep option --pairs.

Hi @prudo1 ,thanks for reviewing. I removed the local _source _target and use read -r _source _target now.
I keep --pairs becasue in my test, I need to handle cases when _source or _target contains a space.

There's comment here that `--source` somehow avoids bind mounts, but that appears not to be the case in my testing. I think we just happened to be lucky before now with the `--first` picking the value we wanted. Instead of using `--first` and hoping for the best, parse the mounts and skip ones which are bind mounts explicitly. Signed-off-by: Colin Walters <[email protected]> Reviewed-by: Lichen Liu <[email protected]>

In some images such as the recent fedora/rhel bootc base image, the ostree dracut module is statically enabled: https://gitlab.com/fedora/bootc/base-images/-/blob/40df0eb3827763039e4d17264968e37f850b70e5/tier-0/initramfs.yaml#L9 And also recently, we changed the ostree systemd unit to enter emergency.target if it fails in: ostreedev/ostree@05b3b66 These two things combine mean we'll fail before kdump gets a chance to run. For our use case we don't need ostree in the initrd. I tried to override this with `--omit=ostree` in our dracut invocation, but that causes an error (dracut doesn't let the cmdline override static config). For now, let's just mask the service in our initrd. Signed-off-by: Colin Walters <[email protected]> Reviewed-by: Lichen Liu <[email protected]>

coiby · 2024-09-02T09:52:19Z

kdump-lib-initramfs.sh

Unfortunately, this patch has several issues,

it will cause the same regression as the previous RHEL-only commit will expr "X$_source" : 'X.*\[' will also match an IPv6 target specified in kdump.conf e.g. nfs [2203...]:/nfs_server

The regex also matches brtfs subvol so it will return empty value for btrfs target

[root@localhost ~]# findmnt -k --pairs -o SOURCE,TARGET /dev/vda4 SOURCE="/dev/vda4[/root]" TARGET="/" SOURCE="/dev/vda4[/home]" TARGET="/home" SOURCE="/dev/vda4[/var]" TARGET="/var" SOURCE="/dev/vda4[/var]" TARGET="/tt2"

I think a better way to to fix get_bind_mount_source is to check /proc/self/mountinfo,

[root@localhost ~]# mkdir /tt2 [root@localhost ~]# mount --bind /var /tt2 [root@localhost ~]# mkdir "/tt2 space" [root@localhost ~]# mount --bind /var "/tt2 space" [root@localhost ~]# grep tt2 /proc/self/mountinfo 192 77 0:39 /var /tt2 rw,relatime shared:118 - btrfs /dev/vda4 rw,seclabel,compress=zstd:1,discard=async,space_cache=v2,subvolid=258,subvol=/var 488 77 0:39 /var /tt2\040space rw,relatime shared:118 - btrfs /dev/vda4 rw,seclabel,compress=zstd:1,discard=async,space_cache=v2,subvolid=258,subvol=/var

Another reason is changing get_mntpoint_from_target could bring regression as it's used on other places. Of course get_mntpoint_from_target is flawed to call findmnt with -f and this flaw can also been on non-composefs. Suppose we want to dump vmcore to /tt2/crash (/tt2 is bind mounted to /var), it returns / in this case,

[root@localhost ~]# findmnt -n -o source /tt2 /dev/vda4[/var] [root@localhost ~]# findmnt -k --pairs -o SOURCE,TARGET /dev/vda4 SOURCE="/dev/vda4" TARGET="/" SOURCE="/dev/vda4[/var]" TARGET="/tt2"

Btw, The get_mntpoint_from_target names suggests it returns a mount point based on a target. But what it does is return a mount point based on a source. It should be renamed in order to not confuse readers.

To fix the regression first, I've created a centos-stream kdump-utils MR. But we need to fix the btrfs subvol issue.

Btw, there may be another problem with this PR. In this PR get_mntpoint_from_target no longer call get_mount_info which will fallback to find mount info in /etc/fstab and only call findmnt with -k argument. So this PR's get_mntpoint_from_target will not be able to make use of /etc/fstab and may cause problem for specifying a not-mounted target.

Hi @coiby , maybe we can use FSTYPE to handle this case.
For btrfs, it is in the Unsupported Dump targets and we have a fedora bug.
Maybe we can take this opportunity to refactor our code to handle different file systems.

liutgnu reviewed Jul 23, 2024

View reviewed changes

kdump-lib.sh Outdated Show resolved Hide resolved

licliu force-pushed the bootc-bind-mount-issue branch from fec2434 to 71e8c95 Compare July 23, 2024 05:05

prudo1 requested changes Jul 23, 2024

View reviewed changes

licliu force-pushed the bootc-bind-mount-issue branch 2 times, most recently from d74a97d to 5527112 Compare August 15, 2024 03:55

licliu requested a review from prudo1 August 15, 2024 04:00

jbtrystram mentioned this pull request Aug 21, 2024

Complete composefs integration in Fedora CoreOS coreos/fedora-coreos-tracker#1718

Closed

travier reviewed Aug 21, 2024

View reviewed changes

prudo1 reviewed Aug 22, 2024

View reviewed changes

cgwalters added 2 commits August 23, 2024 16:04

licliu force-pushed the bootc-bind-mount-issue branch from 5527112 to 474d837 Compare August 23, 2024 08:06

coiby requested changes Sep 2, 2024

View reviewed changes

jbtrystram mentioned this pull request Sep 4, 2024

kdump fails with composeFS enabled coreos/fedora-coreos-tracker#1791

Closed

licliu mentioned this pull request Sep 29, 2024

Bind mount and btrfs dump target enhancement #37

Draft

coiby mentioned this pull request Oct 8, 2024

kdump-lib-initramfs: Improve mount point retrieval logic #41

Merged

licliu mentioned this pull request Oct 11, 2024

dracut: Disable ostree-prepare-root #44

Merged

licliu closed this Oct 11, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix compatibility with bootc systems #28

Fix compatibility with bootc systems #28

licliu commented Jul 23, 2024

liutgnu commented Jul 23, 2024

prudo1 Jul 23, 2024

cgwalters Jul 23, 2024

licliu Aug 2, 2024

licliu commented Aug 15, 2024

travier Aug 21, 2024

cgwalters Aug 21, 2024

travier Aug 21, 2024

prudo1 Aug 22, 2024

prudo1 Aug 22, 2024

licliu Aug 23, 2024

coiby Sep 2, 2024 •

edited

Loading

coiby Sep 9, 2024

licliu Sep 11, 2024

		# Disable ostree as we only need the physical root
		systemctl -q --root "$initdir" mask ostree-prepare-root.service

Fix compatibility with bootc systems #28

Fix compatibility with bootc systems #28

Conversation

licliu commented Jul 23, 2024

liutgnu commented Jul 23, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

licliu commented Aug 15, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coiby Sep 2, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coiby Sep 2, 2024 •

edited

Loading