[rhcos-4.18] mantle/kola/tests: adapt mpath verification #4354
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. |
|
/ok-to-test |
![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 handle UUID-based device names, which is a good improvement for test reliability. The changes to use mpathconf --user_friendly_names n, set a wwn for the disk, and update systemd units are all consistent and logical. The updated verification logic in verifyMultipath is also more robust. I have one suggestion to make the verification even more reliable by handling potential whitespace in command 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)) |
There was a problem hiding this comment.
The output of c.MustSSHf includes the raw standard output of the command, which might include trailing whitespace or newlines. This could cause the subsequent udevadm info command to fail or behave unexpectedly. It's safer to trim the whitespace from the srcdev string before using it in the next command.
| 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)) | |
| srcdev := strings.TrimSpace(string(c.MustSSHf(m, "findmnt -nvr %s -o SOURCE", path))) | |
| udevinfo := string(c.MustSSHf(m, "udevadm info %s", srcdev)) |
There was a problem hiding this comment.
Code Review
This pull request adapts multipath tests to work with non-user-friendly device names. This involves updating systemd unit files to use stable, WWID-based device paths, configuring the test to create a disk with a specific WWN, and making the verification logic more robust by checking udev properties instead of relying on device name prefixes. The changes are consistent and correct. I have one suggestion to improve security and robustness in the verification function by properly quoting a variable used in a shell command.
| func verifyMultipath(c cluster.TestCluster, m platform.Machine, path string) { | ||
| 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)) |
There was a problem hiding this comment.
The srcdev variable is obtained from the output of a command and then used to construct another shell command. This could be a potential command injection vulnerability if srcdev contains shell metacharacters. It's safer to quote it. Using the %q format verb in fmt.Sprintf will properly quote the string, preventing misinterpretation by the shell.
| udevinfo := string(c.MustSSHf(m, "udevadm info %s", srcdev)) | |
| udevinfo := string(c.MustSSHf(m, "udevadm info %q", srcdev)) |
|
/override continuous-integration/jenkins/pr-merge For some reason the worker pod is terminated consistently when cheking format with gofmt |
|
@joelcapitao: Overrode contexts on behalf of joelcapitao: continuous-integration/jenkins/pr-merge In response to this:
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. |
3 participants
This is an automated cherry-pick of #4113
/assign joelcapitao