fix(lvm): change calling operation to avoid garbage in json object #250
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
Drop the STDERR stream contents from `output` to avoid JSON mangling. The `vgs` command may print non-critical warnings to STDERR. Warnings may not necessarily result in a failure return code, which allows the program to continue with marshalling the JSON-formatted output. Combining this stream with STDIN will cause the next step at `decodeVgsJSON()` to fail due to garbage mixed in the JSON. Fixes #247 Signed-off-by: Kara <[email protected]>
Add PR changelog Signed-off-by: Kara <[email protected]>
Move exec code into separate function which returns STDOUT and STDERR streams separately. This is to facilitate future occurences of that same pattern within lvm_util.go . Additionally, add log messages to assist with debugging lvm and json. Signed-off-by: Kara <[email protected]>
This test aims to determine whether lvm-driver is capable of operating with a volume group whose systemid is foreign to the kubernetes host system. The test ensures the program in pkg/lvm/lvm_utils.go is capable of operating in a foreign lvm environment. This is important when the kubernetes host system has a different lvm configuration than the lvm-driver container. Differences in configuration may cause unforseen consequences when the host machine provisions a volume group which is to be consumed by the lvm-driver container. Additionally, this feature includes teardown code in the form of the cleanup functions which aims to aid with cleaning up ci resources in a local development environment. To make the cleanup actions compatible with the original operation of the ci-test.sh script, the feature is only enabled through the use of environment variables. While this commit fixes issue #249, it's intent was as an integration test for issue #247. Fixes #249 Signed-off-by: Kara <[email protected]>
|
First PR. Please let me know if there's anything I should change or if I need to do anything else to help. |
To avoid volume group caching, explicitly remove volume groups (and their pvs) before removing the disk and loopback device. Signed-off-by: Kara <[email protected]>
|
Hi @kro-cat Can you please go through and resolve the [shellcheck] lint errors in files section. Thanks |
Fix linter errors: use -n instead of ! -z in test statements. Last statement in file is used as the return code of the file, so it must return true; change `[ ! -z ... ] && ...` into `[ -z ... ] || ...` to achieve this. Signed-off-by: kro <[email protected]>
|
It will be great to fix also |
|
@dnugmanov Good catch, I'll refactor those as well |
@kro-cat Let's take that refactor on a different PR. |
Abhinandan-Purkait
requested review from
Abhinandan-Purkait,
abhilashshetty04 and
niladrih
|
@Abhinandan-Purkait got it. Should I open a new issue for it, or is that not required for a refactor? |
|
@kro-cat , New issue might not be required if you explain all changes in PR |
abhilashshetty04
approved these changes
Aug 30, 2023
|
@kro-cat We hope all your commits are in, so are we good to merge now? |
|
@Abhinandan-Purkait All commits are in, we're good to merge. :) |
kro-cat
added a commit
to kro-cat/lvm-localpv
that referenced
this pull request
Oct 8, 2023
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 <[email protected]>
7 tasks
Abhinandan-Purkait
pushed a commit
that referenced
this pull request
Oct 18, 2023
…#261) * refactor(ci): rename env variables and update vgcreate command 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 <[email protected]> * refactor(lvm): update all lvm commands to use split output 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 #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 #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 <[email protected]> --------- Signed-off-by: kro <[email protected]>
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.
Why is this PR required? What issue does it fix?:
The
vgscommand may print non-critical warnings to STDERR. Warnings may not necessarily result in a failure return code, which allows the program to continue with marshalling the JSON-formatted output. Combining this stream with STDIN will cause the next step atdecodeVgsJSON()to fail due to garbage mixed in the JSON.What this PR does?:
Drop the STDERR stream contents from
outputto avoid JSON mangling.Does this PR require any upgrade changes?:
If the changes in this PR are manually verified, list down the scenarios covered::
Any additional information for your reviewer? :
Volume groups created on the host may not necessarily be compatible with the lvm-driver container. This PR simply introduces a fix to isolate any warnings generated by
vgs, but this won't fix any issues with incompatible volume groups: there may be additional steps required of the system's administrator in order to ensure his or her configuration will work with lvm-localpv. Printingvgs's STDERR to the log should provide enough diagnostic information for the administrator to perform any additional steps, if needed.Checklist:
<type>(<scope>): <subject>