refactor(lvm): continue refactoring of lvm command calling convention (#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]>

Author: kro-cat

ci/ci-test.sh

@@ -29,9 +29,9 @@ then
   export KUBECONFIG="${HOME}/.kube/config"
 fi
 
-# systemid for the testing environment. The kubernetes host machine will serve as the foreign lvm system.
-LVM_SYSTEMID="openebs-ci-test-system"
-LVM_CONFIG="global{system_id_source=lvmlocal}local{system_id=${LVM_SYSTEMID}}"
+# foreign systemid for the testing environment.
+FOREIGN_LVM_SYSTEMID="openebs-ci-test-system"
+FOREIGN_LVM_CONFIG="global{system_id_source=lvmlocal}local{system_id=${FOREIGN_LVM_SYSTEMID}}"
 
 # Clean up generated resources for successive tests.
 cleanup_loopdev() {
@@ -54,7 +54,7 @@ cleanup_lvmvg() {
 cleanup_foreign_lvmvg() {
   if [ -f /tmp/openebs_ci_foreign_disk.img ]
   then
-    sudo vgremove foreign_lvmvg --config="${LVM_CONFIG}" -y || true
+    sudo vgremove foreign_lvmvg --config="${FOREIGN_LVM_CONFIG}" -y || true
     rm /tmp/openebs_ci_foreign_disk.img
   fi
   cleanup_loopdev
@@ -91,7 +91,7 @@ cleanup_foreign_lvmvg
 truncate -s 1024G /tmp/openebs_ci_foreign_disk.img
 foreign_disk="$(sudo losetup -f /tmp/openebs_ci_foreign_disk.img --show)"
 sudo pvcreate "${foreign_disk}"
-sudo vgcreate foreign_lvmvg "${foreign_disk}" --systemid="${LVM_SYSTEMID}"
+sudo vgcreate foreign_lvmvg "${foreign_disk}" --config="${FOREIGN_LVM_CONFIG}"
 
 # install snapshot and thin volume module for lvm
 sudo modprobe dm-snapshot

pkg/lvm/lvm_util.go

@@ -261,6 +261,27 @@ func buildLVMDestroyArgs(vol *apis.LVMVolume) []string {
 	return LVMVolArg
 }
 
+// RunCommandSplit is a wrapper function to run a command and receive its
+// STDERR and STDOUT streams in separate []byte vars.
+func RunCommandSplit(command string, args ...string) ([]byte, []byte, error) {
+	var cmdStdout bytes.Buffer
+	var cmdStderr bytes.Buffer
+
+	cmd := exec.Command(command, args...)
+	cmd.Stdout = &cmdStdout
+	cmd.Stderr = &cmdStderr
+	err := cmd.Run()
+
+	output := cmdStdout.Bytes()
+	error_output := cmdStderr.Bytes()
+
+	if len(error_output) > 0 {
+		klog.Warningf("lvm: said into stderr: %s", error_output)
+	}
+
+	return output, error_output, err
+}
+
 // CreateVolume creates the lvm volume
 func CreateVolume(vol *apis.LVMVolume) error {
 	volume := vol.Spec.VolGroup + "/" + vol.Name
@@ -275,8 +296,7 @@ func CreateVolume(vol *apis.LVMVolume) error {
 	}
 
 	args := buildLVMCreateArgs(vol)
-	cmd := exec.Command(LVCreate, args...)
-	out, err := cmd.CombinedOutput()
+	out, _, err := RunCommandSplit(LVCreate, args...)
 
 	if err != nil {
 		err = newExecError(out, err)
@@ -314,8 +334,7 @@ func DestroyVolume(vol *apis.LVMVolume) error {
 	}
 
 	args := buildLVMDestroyArgs(vol)
-	cmd := exec.Command(LVRemove, args...)
-	out, err := cmd.CombinedOutput()
+	out, _, err := RunCommandSplit(LVRemove, args...)
 
 	if err != nil {
 		klog.Errorf(
@@ -406,8 +425,7 @@ func ResizeLVMVolume(vol *apis.LVMVolume, resizefs bool) error {
 	volume := vol.Spec.VolGroup + "/" + vol.Name
 
 	args := buildVolumeResizeArgs(vol, resizefs)
-	cmd := exec.Command(LVExtend, args...)
-	out, err := cmd.CombinedOutput()
+	out, _, err := RunCommandSplit(LVExtend, args...)
 
 	if err != nil {
 		klog.Errorf(
@@ -430,8 +448,7 @@ func getLVSize(vol *apis.LVMVolume) (uint64, error) {
 		"--nosuffix",
 	}
 
-	cmd := exec.Command(LVList, args...)
-	raw, err := cmd.CombinedOutput()
+	raw, _, err := RunCommandSplit(LVList, args...)
 	if err != nil {
 		return 0, errors.Wrapf(
 			err,
@@ -495,8 +512,7 @@ func CreateSnapshot(snap *apis.LVMSnapshot) error {
 	snapVolume := snap.Spec.VolGroup + "/" + getLVMSnapName(snap.Name)
 
 	args := buildLVMSnapCreateArgs(snap)
-	cmd := exec.Command(LVCreate, args...)
-	out, err := cmd.CombinedOutput()
+	out, _, err := RunCommandSplit(LVCreate, args...)
 
 	if err != nil {
 		klog.Errorf("lvm: could not create snapshot %s cmd %v error: %s", snapVolume, args, string(out))
@@ -524,8 +540,7 @@ func DestroySnapshot(snap *apis.LVMSnapshot) error {
 	}
 
 	args := buildLVMSnapDestroyArgs(snap)
-	cmd := exec.Command(LVRemove, args...)
-	out, err := cmd.CombinedOutput()
+	out, _, err := RunCommandSplit(LVRemove, args...)
 
 	if err != nil {
 		klog.Errorf("lvm: could not remove snapshot %s cmd %v error: %s", snapVolume, args, string(out))
@@ -639,34 +654,13 @@ func getIntFieldValue(fieldName, fieldValue string) int {
 // serving vgs or other lvm utility.
 func ReloadLVMMetadataCache() error {
 	args := []string{"--cache"}
-	cmd := exec.Command(PVScan, args...)
-	output, err := cmd.CombinedOutput()
+	output, _, err := RunCommandSplit(PVScan, args...)
 	if err != nil {
 		klog.Errorf("lvm: reload lvm metadata cache: %v - %v", string(output), err)
 		return err
 	}
-	return nil
-}
-
-// RunCommandSplit is a wrapper function to run a command and receive its
-// STDERR and STDOUT streams in separate []byte vars.
-func RunCommandSplit(command string, args ...string) ([]byte, []byte, error) {
-	var cmdStdout bytes.Buffer
-	var cmdStderr bytes.Buffer
-
-	cmd := exec.Command(command, args...)
-	cmd.Stdout = &cmdStdout
-	cmd.Stderr = &cmdStderr
-	err := cmd.Run()
-
-	output := cmdStdout.Bytes()
-	error_output := cmdStderr.Bytes()
-
-	if len(error_output) > 0 {
-		klog.Warningf("lvm: said into stderr: %s", error_output)
-	}
 
-	return output, error_output, err
+	return nil
 }
 
 // ListLVMVolumeGroup invokes `vgs` to list all the available volume
@@ -880,12 +874,12 @@ func ListLVMLogicalVolume() ([]LogicalVolume, error) {
 		"--reportformat", "json",
 		"--units", "b",
 	}
-	cmd := exec.Command(LVList, args...)
-	output, err := cmd.CombinedOutput()
+	output, _, err := RunCommandSplit(LVList, args...)
 	if err != nil {
 		klog.Errorf("lvm: error while running command %s %v: %v", LVList, args, err)
 		return nil, err
 	}
+
 	return decodeLvsJSON(output)
 }
 
@@ -902,12 +896,12 @@ func ListLVMPhysicalVolume() ([]PhysicalVolume, error) {
 		"--reportformat", "json",
 		"--units", "b",
 	}
-	cmd := exec.Command(PVList, args...)
-	output, err := cmd.CombinedOutput()
+	output, _, err := RunCommandSplit(PVList, args...)
 	if err != nil {
 		klog.Errorf("lvm: error while running command %s %v: %v", PVList, args, err)
 		return nil, err
 	}
+
 	return decodePvsJSON(output)
 }
 
@@ -1029,20 +1023,19 @@ func decodePvsJSON(raw []byte) ([]PhysicalVolume, error) {
 
 // lvThinExists verifies if thin pool/volume already exists for given volumegroup
 func lvThinExists(vg string, name string) bool {
-	cmd := exec.Command("lvs", vg+"/"+name, "--noheadings", "-o", "lv_name")
-	out, err := cmd.CombinedOutput()
+	out, _, err := RunCommandSplit("lvs", vg+"/"+name, "--noheadings", "-o", "lv_name")
 	if err != nil {
 		klog.Errorf("failed to list existing volumes:%v", err)
 		return false
 	}
+
 	return name == strings.TrimSpace(string(out))
 }
 
 // snapshotExists checks if a snapshot volume exists for the given volumegroup
 // and snapshot name.
 func isSnapshotExists(vg, snapVolumeName string) (bool, error) {
-	cmd := exec.Command("lvs", vg+"/"+snapVolumeName, "--noheadings", "-o", "lv_name")
-	out, err := cmd.CombinedOutput()
+	out, _, err := RunCommandSplit("lvs", vg+"/"+snapVolumeName, "--noheadings", "-o", "lv_name")
 	if err != nil {
 		return false, err
 	}
@@ -1051,8 +1044,7 @@ func isSnapshotExists(vg, snapVolumeName string) (bool, error) {
 
 // getVGSize get the size in bytes for given volumegroup name
 func getVGSize(vgname string) string {
-	cmd := exec.Command("vgs", vgname, "--noheadings", "-o", "vg_free", "--units", "b", "--nosuffix")
-	out, err := cmd.CombinedOutput()
+	out, _, err := RunCommandSplit("vgs", vgname, "--noheadings", "-o", "vg_free", "--units", "b", "--nosuffix")
 	if err != nil {
 		klog.Errorf("failed to list existing volumegroup:%v , %v", vgname, err)
 		return ""