Don't recreate each time a config, a machine client and a handler

guillaumerose · guillaumerose · commit f8669faeffe9 · 2021-02-10T11:52:42.000-08:00
Just call the config.Get() when it is necessary.
diff --git a/cmd/crc/cmd/daemon.go b/cmd/crc/cmd/daemon.go
@@ -13,7 +13,6 @@ import (
 	"time"
 
 	"github.com/code-ready/crc/pkg/crc/api"
-	crcConfig "github.com/code-ready/crc/pkg/crc/config"
 	"github.com/code-ready/crc/pkg/crc/constants"
 	"github.com/code-ready/gvisor-tap-vsock/pkg/transport"
 	"github.com/code-ready/gvisor-tap-vsock/pkg/types"
@@ -142,15 +141,10 @@ func run(configuration *types.Configuration, endpoints []string) error {
 	}
 }
 
-func newConfig() (crcConfig.Storage, error) {
-	config, _, err := newViperConfig()
-	return config, err
-}
-
 func runDaemon() error {
 	// Remove if an old socket is present
 	os.Remove(constants.DaemonSocketPath)
-	apiServer, err := api.CreateServer(constants.DaemonSocketPath, newConfig, newMachineWithConfig)
+	apiServer, err := api.CreateServer(constants.DaemonSocketPath, config, newMachine())
 	if err != nil {
 		return err
 	}
diff --git a/cmd/crc/cmd/root.go b/cmd/crc/cmd/root.go
@@ -164,13 +164,7 @@ func newViperConfig() (*crcConfig.Config, *crcConfig.ViperStorage, error) {
 }
 
 func newMachine() machine.Client {
-	return newMachineWithConfig(config)
-}
-
-func newMachineWithConfig(config crcConfig.Storage) machine.Client {
-	networkMode := network.ParseMode(config.Get(cmdConfig.NetworkMode).AsString())
-	enableMonitoring := config.Get(cmdConfig.EnableClusterMonitoring).AsBool()
-	return machine.NewClient(constants.DefaultName, networkMode, enableMonitoring)
+	return machine.NewClient(constants.DefaultName, isDebugLog(), config)
 }
 
 func addForceFlag(cmd *cobra.Command) {
diff --git a/pkg/crc/api/api.go b/pkg/crc/api/api.go
@@ -6,10 +6,12 @@ import (
 	"fmt"
 	"net"
 
+	crcConfig "github.com/code-ready/crc/pkg/crc/config"
 	"github.com/code-ready/crc/pkg/crc/logging"
+	"github.com/code-ready/crc/pkg/crc/machine"
 )
 
-func CreateServer(socketPath string, config newConfigFunc, machine newMachineFunc) (Server, error) {
+func CreateServer(socketPath string, config crcConfig.Storage, machine machine.Client) (Server, error) {
 	listener, err := net.Listen("unix", socketPath)
 	if err != nil {
 		logging.Error("Failed to create socket: ", err.Error())
@@ -18,19 +20,13 @@ func CreateServer(socketPath string, config newConfigFunc, machine newMachineFun
 	return createServerWithListener(listener, config, machine)
 }
 
-func createServerWithListener(listener net.Listener, config newConfigFunc, machine newMachineFunc) (Server, error) {
+func createServerWithListener(listener net.Listener, config crcConfig.Storage, machine machine.Client) (Server, error) {
 	apiServer := Server{
 		listener:               listener,
 		clusterOpsRequestsChan: make(chan clusterOpsRequest, 10),
-		handlerFactory: func() (RequestHandler, error) {
-			cfg, err := config()
-			if err != nil {
-				return nil, err
-			}
-			return &Handler{
-				Config:        cfg,
-				MachineClient: &Adapter{Underlying: machine(cfg)},
-			}, nil
+		handler: &Handler{
+			Config:        config,
+			MachineClient: &Adapter{Underlying: machine},
 		},
 	}
 	return apiServer, nil
@@ -61,33 +57,25 @@ func (api Server) handleRequest(req commandRequest, conn net.Conn) {
 	defer conn.Close()
 	var result string
 
-	handler, err := api.handlerFactory()
-	if err != nil {
-		logging.Error(err.Error())
-		result = encodeErrorToJSON(fmt.Sprintf("Failed to initialize new config store: %v", err))
-		writeStringToSocket(conn, result)
-		return
-	}
-
 	switch req.Command {
 	case "start":
-		result = handler.Start(req.Args)
+		result = api.handler.Start(req.Args)
 	case "stop":
-		result = handler.Stop()
+		result = api.handler.Stop()
 	case "status":
-		result = handler.Status()
+		result = api.handler.Status()
 	case "delete":
-		result = handler.Delete()
+		result = api.handler.Delete()
 	case "version":
-		result = handler.GetVersion()
+		result = api.handler.GetVersion()
 	case "setconfig":
-		result = handler.SetConfig(req.Args)
+		result = api.handler.SetConfig(req.Args)
 	case "unsetconfig":
-		result = handler.UnsetConfig(req.Args)
+		result = api.handler.UnsetConfig(req.Args)
 	case "getconfig":
-		result = handler.GetConfig(req.Args)
+		result = api.handler.GetConfig(req.Args)
 	case "webconsoleurl":
-		result = handler.GetWebconsoleInfo()
+		result = api.handler.GetWebconsoleInfo()
 	default:
 		result = encodeErrorToJSON(fmt.Sprintf("Unknown command supplied: %s", req.Command))
 	}
diff --git a/pkg/crc/api/api_test.go b/pkg/crc/api/api_test.go
@@ -13,7 +13,6 @@ import (
 
 	cmdConfig "github.com/code-ready/crc/cmd/crc/cmd/config"
 	"github.com/code-ready/crc/pkg/crc/config"
-	"github.com/code-ready/crc/pkg/crc/machine"
 	"github.com/code-ready/crc/pkg/crc/machine/fakemachine"
 	"github.com/code-ready/crc/pkg/crc/preflight"
 	"github.com/code-ready/crc/pkg/crc/version"
@@ -32,9 +31,7 @@ func TestApi(t *testing.T) {
 	require.NoError(t, err)
 
 	client := fakemachine.NewClient()
-	api, err := createServerWithListener(listener, setupNewInMemoryConfig, func(_ config.Storage) machine.Client {
-		return client
-	})
+	api, err := createServerWithListener(listener, setupNewInMemoryConfig(), client)
 	require.NoError(t, err)
 	go func() {
 		if err := api.Serve(); err != nil {
@@ -201,15 +198,15 @@ func TestGetconfigApi(t *testing.T) {
 	}, getconfigRes)
 }
 
-func setupNewInMemoryConfig() (config.Storage, error) {
+func setupNewInMemoryConfig() config.Storage {
 	storage := config.NewEmptyInMemoryStorage()
 	cfg := config.New(&skipPreflights{
 		storage: storage,
 	})
 	cmdConfig.RegisterSettings(cfg)
 	preflight.RegisterSettings(cfg)
 
-	return cfg, nil
+	return cfg
 }
 
 func setupAPIServer(t *testing.T) (string, func()) {
@@ -221,9 +218,7 @@ func setupAPIServer(t *testing.T) (string, func()) {
 	require.NoError(t, err)
 
 	client := fakemachine.NewClient()
-	api, err := createServerWithListener(listener, setupNewInMemoryConfig, func(_ config.Storage) machine.Client {
-		return client
-	})
+	api, err := createServerWithListener(listener, setupNewInMemoryConfig(), client)
 	require.NoError(t, err)
 	go func() {
 		if err := api.Serve(); err != nil {
diff --git a/pkg/crc/api/types.go b/pkg/crc/api/types.go
@@ -3,21 +3,14 @@ package api
 import (
 	"encoding/json"
 	"net"
-
-	"github.com/code-ready/crc/pkg/crc/config"
-	"github.com/code-ready/crc/pkg/crc/machine"
 )
 
-type newHandlerFunc func() (RequestHandler, error)
-type newConfigFunc func() (config.Storage, error)
-type newMachineFunc func(config.Storage) machine.Client
-
 type commandError struct {
 	Err string
 }
 
 type Server struct {
-	handlerFactory         newHandlerFunc
+	handler                RequestHandler
 	listener               net.Listener
 	clusterOpsRequestsChan chan clusterOpsRequest
 }
diff --git a/pkg/crc/machine/client.go b/pkg/crc/machine/client.go
@@ -1,6 +1,8 @@
 package machine
 
 import (
+	cmdConfig "github.com/code-ready/crc/cmd/crc/cmd/config"
+	crcConfig "github.com/code-ready/crc/pkg/crc/config"
 	"github.com/code-ready/crc/pkg/crc/network"
 	"github.com/code-ready/machine/libmachine/state"
 )
@@ -20,16 +22,16 @@ type Client interface {
 }
 
 type client struct {
-	name              string
-	networkMode       network.Mode
-	monitoringEnabled bool
+	name   string
+	debug  bool
+	config crcConfig.Storage
 }
 
-func NewClient(name string, networkMode network.Mode, monitoringEnabled bool) Client {
+func NewClient(name string, debug bool, config crcConfig.Storage) Client {
 	return &client{
-		name:              name,
-		networkMode:       networkMode,
-		monitoringEnabled: monitoringEnabled,
+		name:   name,
+		debug:  debug,
+		config: config,
 	}
 }
 
@@ -38,5 +40,13 @@ func (client *client) GetName() string {
 }
 
 func (client *client) useVSock() bool {
-	return client.networkMode == network.VSockMode
+	return client.networkMode() == network.VSockMode
+}
+
+func (client *client) networkMode() network.Mode {
+	return network.ParseMode(client.config.Get(cmdConfig.NetworkMode).AsString())
+}
+
+func (client *client) monitoringEnabled() bool {
+	return client.config.Get(cmdConfig.EnableClusterMonitoring).AsBool()
 }
diff --git a/pkg/crc/machine/start.go b/pkg/crc/machine/start.go
@@ -116,7 +116,7 @@ func (client *client) Start(startConfig StartConfig) (*StartResult, error) {
 			CPUs:        startConfig.CPUs,
 			Memory:      startConfig.Memory,
 			DiskSize:    startConfig.DiskSize,
-			NetworkMode: client.networkMode,
+			NetworkMode: client.networkMode(),
 		}
 
 		crcBundleMetadata, err = getCrcBundleInfo(startConfig.BundlePath)
@@ -261,7 +261,7 @@ func (client *client) Start(startConfig StartConfig) (*StartResult, error) {
 		IP:        instanceIP,
 		// TODO: should be more finegrained
 		BundleMetadata: *crcBundleMetadata,
-		NetworkMode:    client.networkMode,
+		NetworkMode:    client.networkMode(),
 	}
 
 	// Run the DNS server inside the VM
@@ -335,7 +335,7 @@ func (client *client) Start(startConfig StartConfig) (*StartResult, error) {
 		return nil, errors.Wrap(err, "Failed to update cluster ID")
 	}
 
-	if client.monitoringEnabled {
+	if client.monitoringEnabled() {
 		logging.Info("Enabling cluster monitoring operator...")
 		if err := cluster.StartMonitoring(ocConfig); err != nil {
 			return nil, errors.Wrap(err, "Cannot start monitoring stack")
@@ -406,7 +406,7 @@ func (client *client) IsRunning() (bool, error) {
 }
 
 func (client *client) validateStartConfig(startConfig StartConfig) error {
-	if client.monitoringEnabled && startConfig.Memory < minimumMemoryForMonitoring {
+	if client.monitoringEnabled() && startConfig.Memory < minimumMemoryForMonitoring {
 		return fmt.Errorf("Too little memory (%s) allocated to the virtual machine to start the monitoring stack, %s is the minimum",
 			units.BytesSize(float64(startConfig.Memory)*1024*1024),
 			units.BytesSize(minimumMemoryForMonitoring*1024*1024))
diff --git a/pkg/crc/machine/status.go b/pkg/crc/machine/status.go
@@ -61,7 +61,7 @@ func (client *client) Status() (*ClusterStatusResult, error) {
 	}
 	return &ClusterStatusResult{
 		CrcStatus:        state.Running,
-		OpenshiftStatus:  getOpenShiftStatus(sshRunner, client.monitoringEnabled),
+		OpenshiftStatus:  getOpenShiftStatus(sshRunner, client.monitoringEnabled()),
 		OpenshiftVersion: crcBundleMetadata.GetOpenshiftVersion(),
 		DiskUse:          diskUse,
 		DiskSize:         diskSize,
diff --git a/test/e2e/crcsuite/collect.go b/test/e2e/crcsuite/collect.go
@@ -13,10 +13,10 @@ import (
 	"sync"
 	"time"
 
+	crcConfig "github.com/code-ready/crc/pkg/crc/config"
 	"github.com/code-ready/crc/pkg/crc/constants"
 	"github.com/code-ready/crc/pkg/crc/logging"
 	"github.com/code-ready/crc/pkg/crc/machine"
-	"github.com/code-ready/crc/pkg/crc/network"
 	"github.com/code-ready/crc/pkg/crc/ssh"
 	"github.com/pkg/errors"
 	"golang.org/x/sync/semaphore"
@@ -177,7 +177,7 @@ type VMCommandCollector struct {
 }
 
 func (collector *VMCommandCollector) Collect(w Writer) error {
-	client := machine.NewClient(constants.DefaultName, network.DefaultMode, false)
+	client := machine.NewClient(constants.DefaultName, true, crcConfig.New(crcConfig.NewEmptyInMemoryStorage()))
 	ip, err := client.IP()
 	if err != nil {
 		return err
@@ -198,7 +198,7 @@ type ContainerLogCollector struct {
 }
 
 func (collector *ContainerLogCollector) Collect(w Writer) error {
-	client := machine.NewClient(constants.DefaultName, network.DefaultMode, false)
+	client := machine.NewClient(constants.DefaultName, true, crcConfig.New(crcConfig.NewEmptyInMemoryStorage()))
 	ip, err := client.IP()
 	if err != nil {
 		return err

Original file line number	Diff line number	Diff line change
`@@ -164,13 +164,7 @@ func newViperConfig() (crcConfig.Config, crcConfig.ViperStorage, error) {`
`164`	`164`	`}`
`165`	`165`
`166`	`166`	`func newMachine() machine.Client {`
`167`		`- return newMachineWithConfig(config)`
`168`		`-}`
`169`		`-`
`170`		`-func newMachineWithConfig(config crcConfig.Storage) machine.Client {`
`171`		`- networkMode := network.ParseMode(config.Get(cmdConfig.NetworkMode).AsString())`
`172`		`- enableMonitoring := config.Get(cmdConfig.EnableClusterMonitoring).AsBool()`
`173`		`- return machine.NewClient(constants.DefaultName, networkMode, enableMonitoring)`
	`167`	`+ return machine.NewClient(constants.DefaultName, isDebugLog(), config)`
`174`	`168`	`}`
`175`	`169`
`176`	`170`	`func addForceFlag(cmd *cobra.Command) {`
Original file line number	Diff line number	Diff line change
`@@ -61,7 +61,7 @@ func (client client) Status() (ClusterStatusResult, error) {`
`61`	`61`	`}`
`62`	`62`	`return &ClusterStatusResult{`
`63`	`63`	`CrcStatus: state.Running,`
`64`		`- OpenshiftStatus: getOpenShiftStatus(sshRunner, client.monitoringEnabled),`
	`64`	`+ OpenshiftStatus: getOpenShiftStatus(sshRunner, client.monitoringEnabled()),`
`65`	`65`	`OpenshiftVersion: crcBundleMetadata.GetOpenshiftVersion(),`
`66`	`66`	`DiskUse: diskUse,`
`67`	`67`	`DiskSize: diskSize,`