rootmap: use full path for 'root=' karg when rootfs is directly on multipath

[nikita-dubrovskii]

Issue: coreos/fedora-coreos-tracker#1980

This was first observed on s390x builders under high system load, where ext.config.multipath.resilient would intermittently fail during subsequent boot:

[    2.781559] multipathd[321]: sdd [8:48]: path added to devmap 0xcadf6fadb3ee446d
[    2.853163] multipathd[321]: sdb [8:16]: path added to devmap 0x000000000000000b
[    3.012431] systemd[1]: Reached target coreos-multipath-wait.target - CoreOS Wait For Multipathed Boot.
[    3.139605] systemd[1]: Mounting sysroot.mount - /sysroot...
[    3.450666] mount[806]: mount: /sysroot: fsconfig system call failed: /dev/sdd4: Can't open blockdev.

It looks like a race condition between multipathd taking ownership of the root device and systemd trying to mount /sysroot. This might happen because the udev database isn't ready in time.

[gemini-code-assist]

Summary of Changes

Hello @nikita-dubrovskii, I'm Gemini Code Assist¹! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request enhances the robustness of the boot process for systems utilizing multipath for their root filesystem. By modifying how the root= kernel argument is constructed, it mitigates a race condition between multipathd and systemd that could lead to boot failures, particularly under high system load.

Highlights

• Root filesystem mounting reliability: I've implemented a change to address a race condition observed during boot on s390x systems, specifically when the root filesystem resides directly on a multipath device. This race could cause intermittent failures during /sysroot mounting.
• Kernel argument generation: The root= kernel argument generation logic has been updated in rootmap.rs. It now dynamically determines if multipath is in use and, if so, constructs the root= argument using a full device-mapper path (/dev/disk/by-uuid/dm-mpath-<UUID>) instead of just the filesystem UUID.
• Multipath detection: A new helper function, is_multipath(), was added to check for the presence of rd.multipath=default in the kernel command line, which indicates that multipath is active for the root device.

Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.

Feature	Command	Description
Code Review	/gemini review	Performs a code review for the current pull request in its current state.
Pull Request Summary	/gemini summary	Provides a summary of the current pull request in its current state.
Comment	@gemini-code-assist	Responds in comments when explicitly tagged, both in issue comments and review comments.
Help	/gemini help	Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist is currently in preview and may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments to provide feedback.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

Footnotes

1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩

[gemini-code-assist]

Code Review

The pull request introduces a change to the rootmap utility to use the full path for the 'root=' kernel argument when the root filesystem is directly on multipath. This is done to prevent a race condition between multipathd taking ownership of the root device and systemd trying to mount /sysroot. The changes include adding a function to check if multipath is enabled and modifying the rootmap function to use the full path when multipath is enabled.

[gemini-code-assist]

This function reads the entire contents of /proc/cmdline into a string. While this file is generally small, it's better to avoid reading the entire file into memory if possible. Consider using std::io::BufReader and reading line by line, or using a more efficient method to check for the presence of rd.multipath=default. Also, the function lacks error handling if the file does not exist or cannot be read. This could lead to unexpected behavior. Ensure that the function handles the case where the file is not found or is unreadable, and returns an appropriate error message.

use std::io::{BufReader, Read};
use std::fs::File;

fn is_multipath() -> Result<bool> {
    let file = File::open("/proc/cmdline").context("opening kernel command line")?;
    let mut reader = BufReader::new(file);
    let mut line = String::new();
    reader.read_to_string(&mut line).context("reading kernel command line")?;
    Ok(line.contains("rd.multipath=default"))
}

[gemini-code-assist]

The format string "root=/dev/disk/by-uuid/dm-mpath-{}" is hardcoded. It would be beneficial to define this as a constant to improve maintainability and readability. If the format of the root path changes, it only needs to be updated in one place. Also, consider adding a comment explaining why this specific path is being used, referencing the related udev rules file.

const DM_MPATH_ROOT_FORMAT: &str = "root=/dev/disk/by-uuid/dm-mpath-{}";

    let root = if is_multipath()? {
        // https://github.com/coreos/fedora-coreos-config/blob/testing-devel/overlay.d/05core/usr/lib/udev/rules.d/90-coreos-device-mapper.rules#L25
        // Use the full path for root when on multipath to avoid race conditions.
        format!(
            DM_MPATH_ROOT_FORMAT,
            physical_mount.get_filesystem_uuid()?
        )

[gemini-code-assist]

The code uses an if/else block to determine the root parameter. This can be simplified by using a single format! call with a conditional format string. This reduces code duplication and improves readability.

let root = format!(
        "root={}",
        if is_multipath()? {
            format!("/dev/disk/by-uuid/dm-mpath-{}", physical_mount.get_filesystem_uuid()?) // Multipath root path
        } else {
            format!("UUID={}", physical_mount.get_filesystem_uuid()?) // UUID root path
        }
    );

[jlebon]

Thanks for picking this up! Did you confirm this fixes coreos/fedora-coreos-tracker#1980 ?

[nikita-dubrovskii]

Thanks for picking this up! Did you confirm this fixes coreos/fedora-coreos-tracker#1980 ?

Yes, all multipath tests work fine.

[jlebon]

Thanks for working on this! This is actually part of a larger story around being more resilient to multiple root labels (openshift/os#1787).

[nikita-dubrovskii]

Do not merge, need to re-check secex case

[nikita-dubrovskii]

Do not merge, need to re-check secex case

Ok to merge