Merge pull request #18 from reecetech/feature/lsblk-refactor

(feat): Refactor lsblk -J to lsblk -P + LVM Validation Test Cases

internal/config/validator.go

@@ -9,6 +9,10 @@ import (
 	"github.com/reecetech/ebs-bootstrap/internal/service"
 )
 
+const (
+	LvmWikiDocumentationUrl = "https://github.com/reecetech/ebs-bootstrap/wiki/LVM"
+)
+
 type Validator interface {
 	Validate(c *Config) error
 }
@@ -48,6 +52,9 @@ func (fsv *FileSystemValidator) Validate(c *Config) error {
 		if fs == model.Unformatted {
 			return fmt.Errorf("🔴 %s: Must provide a supported file system", name)
 		}
+		if fs == model.Lvm {
+			return fmt.Errorf("🔴 %s: Refer to %s on how to manage LVM file systems", name, LvmWikiDocumentationUrl)
+		}
 	}
 	return nil
 }

internal/config/validator_test.go

@@ -207,6 +207,17 @@ func TestFileSystemValidator(t *testing.T) {
 			},
 			ExpectedError: fmt.Errorf("🔴 /dev/xvdf: Must provide a supported file system"),
 		},
+		{
+			Name: "LVM File System",
+			Config: &Config{
+				Devices: map[string]Device{
+					"/dev/xvdf": {
+						Fs: model.Lvm,
+					},
+				},
+			},
+			ExpectedError: fmt.Errorf("🔴 /dev/xvdf: Refer to %s on how to manage LVM file systems", LvmWikiDocumentationUrl),
+		},
 	}
 	for _, subtest := range subtests {
 		fsv := NewFileSystemValidator()

internal/service/device.go

@@ -1,14 +1,18 @@
 package service
 
 import (
-	"encoding/json"
 	"fmt"
+	"regexp"
 	"strconv"
+	"strings"
 
 	"github.com/reecetech/ebs-bootstrap/internal/model"
 	"github.com/reecetech/ebs-bootstrap/internal/utils"
 )
 
+var deviceNameRegex = regexp.MustCompile(`^NAME="(.*)"`)
+var devicePropertiesRegex = regexp.MustCompile(`^LABEL="(.*)" FSTYPE="(.*)" MOUNTPOINT="(.*)"`)
+
 type DeviceService interface {
 	GetSize(name string) (uint64, error) // bytes
 	GetBlockDevices() ([]string, error)
@@ -21,15 +25,6 @@ type LinuxDeviceService struct {
 	runnerFactory utils.RunnerFactory
 }
 
-type LsblkBlockDeviceResponse struct {
-	BlockDevices []struct {
-		Name       *string `json:"name"`
-		Label      *string `json:"label"`
-		FsType     *string `json:"fstype"`
-		MountPoint *string `json:"mountpoint"`
-	} `json:"blockdevices"`
-}
-
 func NewLinuxDeviceService(rc utils.RunnerFactory) *LinuxDeviceService {
 	return &LinuxDeviceService{
 		runnerFactory: rc,
@@ -51,46 +46,45 @@ func (du *LinuxDeviceService) GetSize(name string) (uint64, error) {
 
 func (du *LinuxDeviceService) GetBlockDevices() ([]string, error) {
 	r := du.runnerFactory.Select(utils.Lsblk)
-	output, err := r.Command("--nodeps", "-o", "NAME", "-J")
+	output, err := r.Command("--nodeps", "-o", "NAME", "-P")
 	if err != nil {
 		return nil, err
 	}
-	lbd := &LsblkBlockDeviceResponse{}
-	err = json.Unmarshal([]byte(output), lbd)
-	if err != nil {
-		return nil, fmt.Errorf("🔴 Failed to decode lsblk response: %v", err)
-	}
-	d := make([]string, len(lbd.BlockDevices))
-	for i := range d {
-		d[i] = "/dev/" + utils.Safe(lbd.BlockDevices[i].Name)
+
+	lines := strings.Split(strings.TrimSuffix(output, "\n"), "\n")
+	d := make([]string, len(lines))
+
+	for i, line := range lines {
+		matches := deviceNameRegex.FindStringSubmatch(line)
+		if len(matches) != 2 {
+			return nil, fmt.Errorf("🔴 Failed to decode lsblk response")
+		}
+		d[i] = "/dev/" + matches[1]
 	}
 	return d, nil
 }
 
 func (du *LinuxDeviceService) GetBlockDevice(name string) (*model.BlockDevice, error) {
 	r := du.runnerFactory.Select(utils.Lsblk)
-	output, err := r.Command("--nodeps", "-o", "LABEL,FSTYPE,MOUNTPOINT", "-J", name)
+	output, err := r.Command("--nodeps", "-o", "LABEL,FSTYPE,MOUNTPOINT", "-P", name)
 	if err != nil {
 		return nil, err
 	}
-	lbd := &LsblkBlockDeviceResponse{}
-	err = json.Unmarshal([]byte(output), lbd)
-	if err != nil {
-		return nil, fmt.Errorf("🔴 Failed to decode lsblk response: %v", err)
-	}
-	if len(lbd.BlockDevices) != 1 {
-		return nil, fmt.Errorf("🔴 %s: An unexpected number of block devices were returned: Expected=1 Actual=%d", name, len(lbd.BlockDevices))
+
+	matches := devicePropertiesRegex.FindStringSubmatch(output)
+	if len(matches) != 4 {
+		return nil, fmt.Errorf("🔴 Failed to decode lsblk response")
 	}
-	fst := utils.Safe(lbd.BlockDevices[0].FsType)
-	fs, err := model.ParseFileSystem(fst)
+
+	fs, err := model.ParseFileSystem(matches[2])
 	if err != nil {
 		return nil, fmt.Errorf("🔴 %s: %s", name, err)
 	}
 	return &model.BlockDevice{
 		Name:       name,
-		Label:      utils.Safe(lbd.BlockDevices[0].Label),
+		Label:      matches[1],
 		FileSystem: fs,
-		MountPoint: utils.Safe(lbd.BlockDevices[0].MountPoint),
+		MountPoint: matches[3],
 	}, nil
 }
 

internal/service/device_test.go

@@ -72,34 +72,32 @@ func TestGetBlockDevices(t *testing.T) {
 		ExpectedError  error
 	}{
 		{
-			Name:         "lsblk=success + json=valid",
+			Name:         "lsblk=success",
 			RunnerBinary: utils.Lsblk,
-			RunnerArgs:   []string{"--nodeps", "-o", "NAME", "-J"},
-			RunnerOutput: `{"blockdevices": [
-				{"name":"nvme1n1"},
-				{"name":"nvme0n1"}
-			]}`,
+			RunnerArgs:   []string{"--nodeps", "-o", "NAME", "-P"},
+			RunnerOutput: `NAME="nvme1n1"
+NAME="nvme0n1"`,
 			RunnerError:    nil,
 			ExpectedOutput: []string{"/dev/nvme1n1", "/dev/nvme0n1"},
 			ExpectedError:  nil,
 		},
 		{
-			Name:           "lsblk=success + json=invalid",
+			Name:           "lsblk=success + malformed response",
 			RunnerBinary:   utils.Lsblk,
-			RunnerArgs:     []string{"--nodeps", "-o", "NAME", "-J"},
-			RunnerOutput:   `{"invalid_json"}`,
+			RunnerArgs:     []string{"--nodeps", "-o", "NAME", "-P"},
+			RunnerOutput:   `malformed`,
 			RunnerError:    nil,
 			ExpectedOutput: nil,
-			ExpectedError:  fmt.Errorf("🔴 Failed to decode lsblk response: *"),
+			ExpectedError:  fmt.Errorf("🔴 Failed to decode lsblk response"),
 		},
 		{
 			Name:           "lsblk=failure",
 			RunnerBinary:   utils.Lsblk,
-			RunnerArgs:     []string{"--nodeps", "-o", "NAME", "-J"},
+			RunnerArgs:     []string{"--nodeps", "-o", "NAME", "-P"},
 			RunnerOutput:   "",
-			RunnerError:    fmt.Errorf("🔴 lsblk: invalid option -- 'J'"),
+			RunnerError:    fmt.Errorf("🔴 lsblk: invalid option -- 'P'"),
 			ExpectedOutput: nil,
-			ExpectedError:  fmt.Errorf("🔴 lsblk: invalid option -- 'J'"),
+			ExpectedError:  fmt.Errorf("🔴 lsblk: invalid option -- 'P'"),
 		},
 	}
 	for _, subtest := range subtests {
@@ -128,11 +126,9 @@ func TestGetBlockDevice(t *testing.T) {
 			Name:         "lsblk=success",
 			Device:       "/dev/nvme1n1",
 			RunnerBinary: utils.Lsblk,
-			RunnerArgs:   []string{"--nodeps", "-o", "LABEL,FSTYPE,MOUNTPOINT", "-J", "/dev/nvme1n1"},
-			RunnerOutput: `{"blockdevices": [
-				{"name":"nvme1n1", "label":"external-vol", "fstype":"xfs", "mountpoint":"/mnt/app"}
-			]}`,
-			RunnerError: nil,
+			RunnerArgs:   []string{"--nodeps", "-o", "LABEL,FSTYPE,MOUNTPOINT", "-P", "/dev/nvme1n1"},
+			RunnerOutput: `LABEL="external-vol" FSTYPE="xfs" MOUNTPOINT="/mnt/app"`,
+			RunnerError:  nil,
 			ExpectedOutput: &model.BlockDevice{
 				Name:       "/dev/nvme1n1",
 				Label:      "external-vol",
@@ -145,11 +141,9 @@ func TestGetBlockDevice(t *testing.T) {
 			Name:         "lsblk=success + device=unformatted",
 			Device:       "/dev/nvme1n1",
 			RunnerBinary: utils.Lsblk,
-			RunnerArgs:   []string{"--nodeps", "-o", "LABEL,FSTYPE,MOUNTPOINT", "-J", "/dev/nvme1n1"},
-			RunnerOutput: `{"blockdevices": [
-				{"name":"nvme1n1", "label":null, "fstype":null, "mountpoint":null}
-			]}`,
-			RunnerError: nil,
+			RunnerArgs:   []string{"--nodeps", "-o", "LABEL,FSTYPE,MOUNTPOINT", "-P", "/dev/nvme1n1"},
+			RunnerOutput: `LABEL="" FSTYPE="" MOUNTPOINT=""`,
+			RunnerError:  nil,
 			ExpectedOutput: &model.BlockDevice{
 				Name:       "/dev/nvme1n1",
 				Label:      "",
@@ -158,75 +152,31 @@ func TestGetBlockDevice(t *testing.T) {
 			},
 			ExpectedError: nil,
 		},
-		/*	Reference [https://docs.aws.amazon.com/AWSEC2/latest/UserGuide/device_naming.html]
-			---
-			For example, if /dev/sdb is renamed /dev/xvdf, then /dev/sdc is renamed /dev/xvdg.
-			Amazon Linux creates a symbolic link for the name you specified to the renamed device.
-			Other operating systems could behave differently.
-			--
-			With this behaviour established on Amazon Linux, it is possible for lsblk to return a
-			device name that might not reflect the one that was provided to lsblk. To simplify the
-			output of ebs-bootstrap, lets force the BlockDevice to house the name that was provided
-			to the lsblk call
-		*/
 		{
-			Name:         "lsblk=success + symlink=true",
-			Device:       "/dev/sdb",
-			RunnerBinary: utils.Lsblk,
-			RunnerArgs:   []string{"--nodeps", "-o", "LABEL,FSTYPE,MOUNTPOINT", "-J", "/dev/sdb"},
-			RunnerOutput: `{"blockdevices": [
-				{"name":"xvdf", "label":"external-vol", "fstype":"xfs", "mountpoint":"/mnt/app"}
-			]}`,
-			RunnerError: nil,
-			ExpectedOutput: &model.BlockDevice{
-				Name:       "/dev/sdb",
-				Label:      "external-vol",
-				FileSystem: model.Xfs,
-				MountPoint: "/mnt/app",
-			},
-			ExpectedError: nil,
-		},
-		{
-			Name:           "lsblk=success + json=invalid",
+			Name:           "lsblk=success + malformed response",
 			Device:         "/dev/sdb",
 			RunnerBinary:   utils.Lsblk,
-			RunnerArgs:     []string{"--nodeps", "-o", "LABEL,FSTYPE,MOUNTPOINT", "-J", "/dev/sdb"},
-			RunnerOutput:   `{"invalid_json"}`,
-			RunnerError:    nil,
-			ExpectedOutput: nil,
-			ExpectedError:  fmt.Errorf("🔴 Failed to decode lsblk response: *"),
-		},
-		{
-			Name:         "lsblk=success + filesystem=unsupported",
-			Device:       "/dev/sdb",
-			RunnerBinary: utils.Lsblk,
-			RunnerArgs:   []string{"--nodeps", "-o", "LABEL,FSTYPE,MOUNTPOINT", "-J", "/dev/sdb"},
-			RunnerOutput: `{"blockdevices": [
-				{"name":"sdb", "label":null, "fstype":"jfs", "mountpoint":"/mnt/app"}
-			]}`,
+			RunnerArgs:     []string{"--nodeps", "-o", "LABEL,FSTYPE,MOUNTPOINT", "-P", "/dev/sdb"},
+			RunnerOutput:   "malformed",
 			RunnerError:    nil,
 			ExpectedOutput: nil,
-			ExpectedError:  fmt.Errorf("🔴 /dev/sdb: File system 'jfs' is not supported"),
+			ExpectedError:  fmt.Errorf("🔴 Failed to decode lsblk response"),
 		},
-		/*	I haven't encountered a scenario where lsblk successfully returns an empty array
-			Typically, if it cannot find a specific block device, it would produce an error.
-			Nevertheless, lets test this scenario...
-		*/
 		{
-			Name:           "lsblk=success + len(devices)==0",
+			Name:           "lsblk=success + filesystem=unsupported",
 			Device:         "/dev/sdb",
 			RunnerBinary:   utils.Lsblk,
-			RunnerArgs:     []string{"--nodeps", "-o", "LABEL,FSTYPE,MOUNTPOINT", "-J", "/dev/sdb"},
-			RunnerOutput:   `{"blockdevices": []}`,
+			RunnerArgs:     []string{"--nodeps", "-o", "LABEL,FSTYPE,MOUNTPOINT", "-P", "/dev/sdb"},
+			RunnerOutput:   `LABEL="" FSTYPE="jfs" MOUNTPOINT="/mnt/app"`,
 			RunnerError:    nil,
 			ExpectedOutput: nil,
-			ExpectedError:  fmt.Errorf("🔴 /dev/sdb: An unexpected number of block devices were returned: Expected=1 Actual=0"),
+			ExpectedError:  fmt.Errorf("🔴 /dev/sdb: File system 'jfs' is not supported"),
 		},
 		{
 			Name:           "lsblk=failure",
 			Device:         "/dev/sdc",
 			RunnerBinary:   utils.Lsblk,
-			RunnerArgs:     []string{"--nodeps", "-o", "LABEL,FSTYPE,MOUNTPOINT", "-J", "/dev/sdc"},
+			RunnerArgs:     []string{"--nodeps", "-o", "LABEL,FSTYPE,MOUNTPOINT", "-P", "/dev/sdc"},
 			RunnerOutput:   "",
 			RunnerError:    fmt.Errorf("🔴 lsblk: /dev/sdc: not a block device"),
 			ExpectedOutput: nil,

internal/service/filesystem.go

@@ -42,7 +42,7 @@ func (fsf *LinuxFileSystemServiceFactory) Select(fs model.FileSystem) (FileSyste
 		return NewXfsService(fsf.RunnerFactory), nil
 	case model.Lvm:
 		//lint:ignore ST1005 This Error Message Is Supposed to Be Prepended With A Device Name
-		return nil, fmt.Errorf("Refer to https://github.com/reecetech/ebs-bootstrap/wiki/LVM on how to manage LVM file systems")
+		return nil, fmt.Errorf("A Physical Volume cannot be queried/modified")
 	case model.Unformatted:
 		//lint:ignore ST1005 This Error Message Is Supposed to Be Prepended With A Device Name
 		return nil, fmt.Errorf("An unformatted file system can not be queried/modified")

internal/service/filesystem_test.go

@@ -48,6 +48,13 @@ func TestFileSystemFactory(t *testing.T) {
 			ExpectedOutput: nil,
 			ExpectedError:  fmt.Errorf("An unformatted file system can not be queried/modified"),
 		},
+		{
+			Name:           "LVM",
+			FileSystem:     model.Lvm,
+			CmpOption:      cmp.AllowUnexported(),
+			ExpectedOutput: nil,
+			ExpectedError:  fmt.Errorf("A Physical Volume cannot be queried/modified"),
+		},
 	}
 	for _, subtest := range subtests {
 		t.Run(subtest.Name, func(t *testing.T) {

internal/service/nvme.go

@@ -90,6 +90,8 @@ type nvmeIdentifyController struct {
 	Vs        nvmeIdentifyControllerAmznVS
 }
 
+var instanceStoreRegex = regexp.MustCompile(`^(ephemeral[0-9]):(sd[a-z]|none)`)
+
 type NVMeIoctlResult struct {
 	Name   string
 	IdCtrl nvmeIdentifyController
@@ -162,12 +164,11 @@ func (ns *AwsNitroNVMeService) getBlockDeviceMapping(nir *NVMeIoctlResult) (stri
 	if ns.isInstanceStoreVolume(nir) {
 		// Vendor Specfic (vs)
 		vs := strings.TrimRightFunc(string(nir.IdCtrl.Vs.Bdev[:]), ns.trimBlockDevice)
-		// Regex Block device Mapping
-		rebdm := regexp.MustCompile(`^(ephemeral[0-9]):(sd[a-z]|none)`)
+
 		// Match Block Device Mapping
-		mbdm := rebdm.FindStringSubmatch(vs)
+		mbdm := instanceStoreRegex.FindStringSubmatch(vs)
 		if len(mbdm) != 3 {
-			return "", fmt.Errorf("🔴 %s: Instance-store vendor specific metadata did not match pattern. Pattern=%s, Actual=%s", nir.Name, rebdm.String(), vs)
+			return "", fmt.Errorf("🔴 %s: Instance-store vendor specific metadata did not match pattern. Pattern=%s, Actual=%s", nir.Name, instanceStoreRegex.String(), vs)
 		}
 		// If the block device mapping is "none", then lets default to assigning the
 		// the block device mapping to the match result from ephemeral[0-9]