Refactor sbd loading (#145)

* Refactor cluster package to expose a LoadSbdConfig function * Adjust SBDConfig Gatherer to use cluster.LoadSbdConfig function
trento-project · Nov 22, 2022 · f76e000 · f76e000
1 parent 391be77
commit f76e000
Show file tree

Hide file tree

Showing 5 changed files with 53 additions and 58 deletions.
diff --git a/internal/cluster/sbd.go b/internal/cluster/sbd.go
@@ -2,12 +2,12 @@ package cluster
 
 import (
 	"fmt"
-	"io"
 	"os"
 	"regexp"
 	"strconv"
 	"strings"
 
+	"github.com/hashicorp/go-envparse"
 	log "github.com/sirupsen/logrus"
 
 	"github.com/pkg/errors"
@@ -25,7 +25,7 @@ const (
 type SBD struct {
 	cluster string
 	Devices []*SBDDevice
-	Config  map[string]interface{}
+	Config  map[string]string
 }
 
 type SBDDevice struct {
@@ -58,10 +58,10 @@ func NewSBD(executor utils.CommandExecutor, cluster, sbdPath, sbdConfigPath stri
 	var s = SBD{
 		cluster: cluster,
 		Devices: nil, // TODO check me, no slice of pointers needed
-		Config:  map[string]interface{}{},
+		Config:  map[string]string{},
 	}
 
-	c, err := getSBDConfig(sbdConfigPath)
+	c, err := LoadSbdConfig(sbdConfigPath)
 	s.Config = c
 
 	if err != nil {
@@ -70,7 +70,7 @@ func NewSBD(executor utils.CommandExecutor, cluster, sbdPath, sbdConfigPath stri
 		return s, fmt.Errorf("could not find SBD_DEVICE entry in sbd config file")
 	}
 
-	sbdDevice, ok := c["SBD_DEVICE"].(string)
+	sbdDevice, ok := c["SBD_DEVICE"]
 	if !ok {
 		return s, fmt.Errorf("could not cast sbd device to string, %v", c["SBD_DEVICE"])
 	}
@@ -86,23 +86,25 @@ func NewSBD(executor utils.CommandExecutor, cluster, sbdPath, sbdConfigPath stri
 	return s, nil
 }
 
-func getSBDConfig(sbdConfigPath string) (map[string]interface{}, error) {
-	sbdConfFile, err := os.Open(sbdConfigPath)
+func LoadSbdConfig(sbdConfigPath string) (map[string]string, error) {
+	sbdConfigFile, err := os.Open(sbdConfigPath)
 	if err != nil {
 		return nil, errors.Wrap(err, "could not open sbd config file")
 	}
 
-	defer sbdConfFile.Close()
-
-	sbdConfigRaw, err := io.ReadAll(sbdConfFile)
+	defer func() {
+		err := sbdConfigFile.Close()
+		if err != nil {
+			log.Error(err)
+		}
+	}()
 
+	conf, err := envparse.Parse(sbdConfigFile)
 	if err != nil {
-		return nil, errors.Wrap(err, "could not read sbd config file")
+		return nil, errors.Wrap(err, "could not parse sbd config file")
 	}
 
-	configMap := utils.FindMatches(`(?m)^(\w+)=(\S[^#\s]*)`, sbdConfigRaw)
-
-	return configMap, nil
+	return conf, nil
 }
 
 func NewSBDDevice(executor utils.CommandExecutor, sbdPath, device string) SBDDevice {

diff --git a/internal/cluster/sbd_test.go b/internal/cluster/sbd_test.go
@@ -277,10 +277,11 @@ func (suite *SbdTestSuite) TestLoadDeviceDataError() {
 	suite.EqualError(err, "sbd dump command error: error;sbd list command error: error")
 }
 
-func (suite *SbdTestSuite) TestGetSBDConfig() {
-	sbdConfig, err := getSBDConfig(helpers.GetFixturePath("discovery/cluster/sbd/sbd_config"))
+func (suite *SbdTestSuite) TestLoadSbdConfig() {
+	sbdConfig, err := LoadSbdConfig(helpers.GetFixturePath("discovery/cluster/sbd/sbd_config"))
 
-	expectedConfig := map[string]interface{}{
+	expectedConfig := map[string]string{
+		"SBD_OPTS":                "",
 		"SBD_PACEMAKER":           "yes",
 		"SBD_STARTMODE":           "always",
 		"SBD_DELAY_START":         "no",
@@ -298,15 +299,24 @@ func (suite *SbdTestSuite) TestGetSBDConfig() {
 	suite.NoError(err)
 }
 
-func (suite *SbdTestSuite) TestGetSBDConfigError() {
-	sbdConfig, err := getSBDConfig("notexist")
+func (suite *SbdTestSuite) TestLoadSbdConfigError() {
+	sbdConfig, err := LoadSbdConfig("notexist")
 
-	expectedConfig := map[string]interface{}(nil)
+	expectedConfig := map[string]string(nil)
 
 	suite.Equal(expectedConfig, sbdConfig)
 	suite.EqualError(err, "could not open sbd config file: open notexist: no such file or directory")
 }
 
+func (suite *SbdTestSuite) TestLoadSbdConfigParsingError() {
+	sbdConfig, err := LoadSbdConfig(helpers.GetFixturePath("discovery/cluster/sbd/sbd_config_invalid"))
+
+	expectedConfig := map[string]string(nil)
+
+	suite.Equal(expectedConfig, sbdConfig)
+	suite.EqualError(err, "could not parse sbd config file: error on line 1: missing =")
+}
+
 func (suite *SbdTestSuite) TestNewSBD() {
 	mockCommand := new(mocks.CommandExecutor)
 	mockCommand.On("Exec", "/bin/sbd", "-d", "/dev/vdc", "dump").Return(mockSbdDump(), nil)
@@ -318,7 +328,8 @@ func (suite *SbdTestSuite) TestNewSBD() {
 
 	expectedSbd := SBD{
 		cluster: "mycluster",
-		Config: map[string]interface{}{
+		Config: map[string]string{
+			"SBD_OPTS":                "",
 			"SBD_PACEMAKER":           "yes",
 			"SBD_STARTMODE":           "always",
 			"SBD_DELAY_START":         "no",
@@ -402,7 +413,8 @@ func (suite *SbdTestSuite) TestNewSBDError() {
 
 	expectedSbd := SBD{ //nolint
 		cluster: "mycluster",
-		Config: map[string]interface{}{
+		Config: map[string]string{
+			"SBD_OPTS":                "",
 			"SBD_PACEMAKER":           "yes",
 			"SBD_STARTMODE":           "always",
 			"SBD_DELAY_START":         "no",
@@ -428,7 +440,8 @@ func (suite *SbdTestSuite) TestNewSBDUnhealthyDevices() {
 
 	expectedSbd := SBD{
 		cluster: "mycluster",
-		Config: map[string]interface{}{
+		Config: map[string]string{
+			"SBD_OPTS":                "",
 			"SBD_PACEMAKER":           "yes",
 			"SBD_STARTMODE":           "always",
 			"SBD_DELAY_START":         "no",

diff --git a/internal/factsengine/gatherers/sbd.go b/internal/factsengine/gatherers/sbd.go
@@ -1,10 +1,6 @@
 package gatherers
 
 import (
-	"os"
-
-	"github.com/hashicorp/go-envparse"
-
 	log "github.com/sirupsen/logrus"
 	"github.com/trento-project/agent/internal/cluster"
 	"github.com/trento-project/agent/internal/factsengine/entities"
@@ -21,11 +17,6 @@ var (
 		Message: "error reading sbd configuration file",
 	}
 
-	SBDConfigDecodingError = entities.FactGatheringError{
-		Type:    "sbd-config-decoding-error",
-		Message: "error decoding configuration file",
-	}
-
 	SBDConfigValueNotFoundError = entities.FactGatheringError{
 		Type:    "sbd-config-value-not-found",
 		Message: "requested field value not found",
@@ -50,10 +41,10 @@ func (g *SBDGatherer) Gather(factsRequests []entities.FactRequest) ([]entities.F
 	facts := []entities.Fact{}
 	log.Infof("Starting SBD config Facts gathering")
 
-	conf, err := loadSbdConf(g.configFile)
+	conf, err := cluster.LoadSbdConfig(g.configFile)
 
 	if err != nil {
-		return nil, err
+		return nil, SBDConfigFileError.Wrap(err.Error())
 	}
 
 	for _, requestedFact := range factsRequests {
@@ -71,24 +62,3 @@ func (g *SBDGatherer) Gather(factsRequests []entities.FactRequest) ([]entities.F
 
 	return facts, nil
 }
-
-func loadSbdConf(sbdConfigPath string) (map[string]string, error) {
-	sbdConfigFile, err := os.Open(sbdConfigPath)
-	if err != nil {
-		return nil, SBDConfigFileError.Wrap(err.Error())
-	}
-
-	defer func() {
-		err := sbdConfigFile.Close()
-		if err != nil {
-			log.Error(err)
-		}
-	}()
-
-	conf, err := envparse.Parse(sbdConfigFile)
-	if err != nil {
-		return nil, SBDConfigDecodingError.Wrap(err.Error())
-	}
-
-	return conf, nil
-}
diff --git a/internal/factsengine/gatherers/sbd_test.go b/internal/factsengine/gatherers/sbd_test.go
@@ -27,7 +27,7 @@ func (suite *SBDGathererTestSuite) TestConfigFileCouldNotBeRead() {
 	expectedError := entities.FactGatheringError{
 		Type: "sbd-config-file-error",
 		Message: "error reading sbd configuration file: " +
-			"open /path/to/some-non-existent-sbd-config: no such file or directory",
+			"could not open sbd config file: open /path/to/some-non-existent-sbd-config: no such file or directory",
 	}
 
 	suite.EqualError(err, expectedError.Error())
@@ -40,8 +40,8 @@ func (suite *SBDGathererTestSuite) TestInvalidConfigFile() {
 	gatheredFacts, err := gatherer.Gather([]entities.FactRequest{})
 
 	expectedError := &entities.FactGatheringError{
-		Type:    "sbd-config-decoding-error",
-		Message: "error decoding configuration file: error on line 1: missing =",
+		Type:    "sbd-config-file-error",
+		Message: "error reading sbd configuration file: could not parse sbd config file: error on line 1: missing =",
 	}
 
 	suite.EqualError(err, expectedError.Error())
@@ -65,6 +65,11 @@ func (suite *SBDGathererTestSuite) TestSBDGatherer() {
 			Gatherer: "sbd_config",
 			Argument: "AN_INTEGER",
 		},
+		{
+			Name:     "sbd_empty_value",
+			Gatherer: "sbd_config",
+			Argument: "SBD_OPTS",
+		},
 		{
 			Name:     "sbd_unexistent",
 			Gatherer: "sbd_config",
@@ -89,6 +94,10 @@ func (suite *SBDGathererTestSuite) TestSBDGatherer() {
 			Name:  "sbd_integer_value",
 			Value: &entities.FactValueInt{Value: 42},
 		},
+		{
+			Name:  "sbd_empty_value",
+			Value: &entities.FactValueString{Value: ""},
+		},
 		{
 			Name: "sbd_unexistent",
 			Error: &entities.FactGatheringError{

diff --git a/test/fixtures/discovery/cluster/expected_published_cluster_discovery.json b/test/fixtures/discovery/cluster/expected_published_cluster_discovery.json
@@ -1018,6 +1018,7 @@
         }
       ],
       "Config": {
+        "SBD_OPTS": "",
         "SBD_DELAY_START": "no",
         "SBD_DEVICE": "/dev/vdc;/dev/vdb",
         "SBD_MOVE_TO_ROOT_CGROUP": "auto",