refactor(lvm): continue refactoring of lvm command calling convention #261
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Env vars LVM_SYSTEMID and LVM_CONFIG are used to describe a potentially foreign lvm system on the kubernetes host. The lvm system is only meant to be foreign to the lvm-localpv containers. Due to the way the ci tests are written, the kubernetes host and the lvm-localpv conatiners must have identical lvm configurations or ci tests might fail. The variables LVM_SYSTEMID and LVM_CONFIG have been renamed to FOREIGN_LVM_SYSTEMID and FOREIGN_LVM_CONFIG, respectively. This is helpful when determining their roles at a glance. A secondary change was made to the foreign pv creation: the lvm option `--config` has been used in place of `--systemid` to hopefully minimize unintended side effects of using the same lvm config (with the exception of the system id) during volume group creation. Signed-off-by: kro <33235324+kro-cat@users.noreply.github.com>
The LVM system may sometimes produce non-critical warnings which are written to STDERR without formatting. Combining STDERR with STDIN may cause failures when the output is being formatted or otherwise interpreted. Following pull openebs#250, it's been requested that all lvm commands be refactored to use a split output in order to resolve this issue under non-tested scenarios. See issue openebs#247. The definition for RunCommandSplit has been moved above all uses of the function, and any Command using CombinedOutput and an lvm command (s.a.: lvs, vgs, lvcreate, &c.) has been refactored to instead use RunCommandSplit to obtain the command's output. If anything is written to STDERR by the lvm commands, RunCommandSplit prints the message to the log as a warning. Signed-off-by: kro <33235324+kro-cat@users.noreply.github.com>
abhilashshetty04
approved these changes
Oct 11, 2023
Abhinandan-Purkait
approved these changes
Oct 18, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The number of this branch refers to pull request #250.
Why is this PR required? What issue does it fix?:
The calling convention of lvm commands in pkg/lvm/lvm_util.go should all follow the same format, following issue #247. This reduces the impact of unforseen (non-critical) warnings being printed to the STDERR output stream, which CombinedOutput mixes with STDOUT.
What this PR does?:
Refactor all occurrences of Commands using CombinedOutput with an lvm command to use RunCommandSplit, a function which splits the STDERR and STDOUT streams into separate return values. Additionally, RunCommandSplit prints any STDERR messages to the log as a warning so that these messages aren't lost.
Additionally, this PR refactors some changes in ci/ci-test.sh that were added in pull #250. During ci tests of these changes, I found that the tests were failing for an unknown reason. A small patch has been applied to ci/ci-test.sh which may help resolve that issue on some machines.
Does this PR require any upgrade changes?:
Most likely not.
If the changes in this PR are manually verified, list down the scenarios covered::
No manual verification. The refactors in this PR are a follow-up to issue #247.
Any additional information for your reviewer? :
This PR is a continuation of pull request #250.
Checklist:
<type>(<scope>): <subject>