Skip to content

Conversation

@aaroncirillo-cision
Copy link

@aaroncirillo-cision aaroncirillo-cision commented Oct 24, 2024

Description

This PR adds the ability to add a path suffix to the chroot command issued when trying to invoke multipath commands through an environment variable called MP_CHROOT_SUFFIX. This was necessary on the environment I work in because our multipath daemon and associated binaries are located in /usr/local. When the chroot command would run it would try to chroot to /noderoot, but in my case it needed to chroot to /noderoot/usr/local. The additional env var allows this to happen without impacting current functionality. If there is another way you would like this implemented I'm open to suggestions.

The reason my multipath daemon is installed in /usr/local is because we are running a distribution called Talos (https://www.talos.dev/). This distribution does not come with multipath, and also has an immutable root filesystem, but they do have a system for adding extensions into /usr/local. So I have created a multipath extension for this distro that installs multipath into /usr/local.

GitHub Issues

N/A

GitHub Issue #

Checklist:

  • I have performed a self-review of my own code to ensure there are no formatting, vetting, linting, or security issues
  • I have verified that new and existing unit tests pass locally with my changes
  • I have not allowed coverage numbers to degenerate
  • I have maintained at least 90% code coverage
  • I have commented on my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works
  • Backward compatibility is not broken

How Has This Been Tested?

This has been tested by running local unit tests as well as running in my environment with the Dell Unity CSI Driver (https://github.com/dell/csi-unity). I validated that in our environment the multipath commands are now being executed correctly because of the updated chroot path.

If this PR is accepted there should also be an update made to the values file for the csi-unity helm chart that documents that this variable is available and the impact it can have.

@karthikk92
Copy link

Please run sanity tests and attach the results

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants