[rhcos-4.17] mantle/kola/tests: adapt mpath verification #4353
Conversation
The test needs to be adapted so it doesn't verify that we're on multipath based on whether the device is `/dev/mapper/mpath` or not, but instead based on udevadm output. This verification works whether or not the `user_friendly_names` option is used while configuring multipath.
In order to have a more generic test, let's not use user-friendly names while configuring multipath and specify instead a World Wide ID to the stub device. We also adapt the systemd units accordingly.
|
Hi @openshift-cherrypick-robot. Thanks for your PR. I'm waiting for a github.com member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adapts the multipath tests to support configurations where user-friendly multipath device names are disabled. The changes correctly update the Butane configuration to use deterministic device paths based on WWID and modify the test verification logic accordingly. My review focuses on the correctness and efficiency of the updated test logic. I've identified a bug in handling command output and an opportunity to improve efficiency in the verifyMultipath function, providing a single suggestion that addresses both issues.
| srcdev := string(c.MustSSHf(m, "findmnt -nvr %s -o SOURCE", path)) | ||
| if !strings.HasPrefix(srcdev, "/dev/mapper/mpath") { | ||
| udevinfo := string(c.MustSSHf(m, "udevadm info %s", srcdev)) | ||
| if !strings.Contains(udevinfo, "/dev/disk/by-id/dm-uuid-mpath-") && !strings.Contains(udevinfo, "DM_MPATH") { | ||
| c.Fatalf("mount %s has non-multipath source %s", path, srcdev) | ||
| } |
There was a problem hiding this comment.
There are a couple of issues in this function:
- Bug: The output from
c.MustSSHfcan include a trailing newline. Whensrcdevis passed to theudevadm infocommand, this newline is included, which will likely cause the command to fail because the path is incorrect (e.g.,/dev/dm-0\n). - Efficiency: The function makes two separate SSH calls. These can be combined into a single call to improve test efficiency by reducing network round-trips.
The suggested change below addresses both points by combining the commands into a single shell command and parsing the output in Go. The $(...) command substitution in the shell correctly handles stripping the newline from findmnt's output.
| srcdev := string(c.MustSSHf(m, "findmnt -nvr %s -o SOURCE", path)) | |
| if !strings.HasPrefix(srcdev, "/dev/mapper/mpath") { | |
| udevinfo := string(c.MustSSHf(m, "udevadm info %s", srcdev)) | |
| if !strings.Contains(udevinfo, "/dev/disk/by-id/dm-uuid-mpath-") && !strings.Contains(udevinfo, "DM_MPATH") { | |
| c.Fatalf("mount %s has non-multipath source %s", path, srcdev) | |
| } | |
| out := string(c.MustSSHf(m, "srcdev=$(findmnt -nvr %s -o SOURCE) && echo \"$srcdev\" && udevadm info \"$srcdev\"", path)) | |
| parts := strings.SplitN(out, "\n", 2) | |
| if len(parts) < 2 { | |
| c.Fatalf("could not get device and udev info for mount %s: %s", path, out) | |
| } | |
| srcdev, udevinfo := parts[0], parts[1] | |
| if !strings.Contains(udevinfo, "/dev/disk/by-id/dm-uuid-mpath-") && !strings.Contains(udevinfo, "DM_MPATH") { | |
| c.Fatalf("mount %s has non-multipath source %s", path, srcdev) | |
| } |
|
/ok-to-test |
3 participants
This is an automated cherry-pick of #4113
/assign joelcapitao