openebs / lvm-localpv

Dynamically provision Stateful Persistent Node-Local Volumes & Filesystems for Kubernetes that is integrated with a backend LVM2 data storage stack.
Apache License 2.0
235 stars 92 forks source link

fix(lvm): change calling operation to avoid garbage in json object #250

Closed kro-cat closed 10 months ago

kro-cat commented 11 months ago

Why is this PR required? What issue does it fix?: 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.

What this PR does?: Drop the STDERR stream contents from output to 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. Printing vgs's STDERR to the log should provide enough diagnostic information for the administrator to perform any additional steps, if needed.

Checklist:

kro-cat commented 11 months ago

First PR. Please let me know if there's anything I should change or if I need to do anything else to help.

Abhinandan-Purkait commented 11 months ago

Hi @kro-cat Can you please go through and resolve the [shellcheck] lint errors in files section. Thanks

dnugmanov commented 10 months ago

It will be great to fix also pvs, lvs - ListLVMPhysicalVolume(), ListLVMLogicalVolume()

kro-cat commented 10 months ago

@dnugmanov Good catch, I'll refactor those as well

Abhinandan-Purkait commented 10 months ago

@dnugmanov Good catch, I'll refactor those as well

@kro-cat Let's take that refactor on a different PR.

kro-cat commented 10 months ago

@Abhinandan-Purkait got it. Should I open a new issue for it, or is that not required for a refactor?

abhilashshetty04 commented 10 months ago

@kro-cat , New issue might not be required if you explain all changes in PR

Abhinandan-Purkait commented 10 months ago

@kro-cat We hope all your commits are in, so are we good to merge now?

kro-cat commented 10 months ago

@Abhinandan-Purkait All commits are in, we're good to merge. :)