Repository: code-ready/crc
Pull request #1837 (2021-01-14 14:54:51)

Use only config object and don't recreate it for each api request
Follow-up of https://github.com/code-ready/crc/pull/1558

Since #1558 we recreate the config object each time. This is painful and implies to reconstruct the API handler for each request. 
This PR fixes that by only using viper for reading configuration. Writing is done manually. 

* Get() is now called every time.
* 2 processes can now share the same configuration without reloading

The intention of this is to simplify the API in a future PR.

openshift-ci-robot: [APPROVALNOTIFIER] This PR is **APPROVED**

This pull-request has been approved by: *<a href="https://github.com/code-ready/crc/pull/1837#" title="Author self-approved">guillaumerose</a>*

The full list of commands accepted by this bot can be found [here](https://go.k8s.io/bot-commands?repo=code-ready%2Fcrc).

The pull request process is described [here](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process)

<details >
Needs approval from an approver in each of these files:

- ~~[OWNERS](https://github.com/code-ready/crc/blob/master/OWNERS)~~ [guillaumerose]

Approvers can indicate their approval by writing `/approve` in a comment
Approvers can cancel approval by writing `/approve cancel` in a comment
</details>
<!-- META={"approvers":[]} -->
openshift-ci[bot]: @guillaumerose: The following test **failed**, say `/retest` to rerun all failed tests:

Test name | Commit | Details | Rerun command
--- | --- | --- | ---
ci/prow/images | b8b0529b14528ee54b39be3bc0ea2034e115e047 | [link](https://prow.ci.openshift.org/view/gs/origin-ci-test/pr-logs/pull/code-ready_crc/1837/pull-ci-code-ready-crc-master-images/1349731987241308160) | `/test images`

[Full PR test history](https://prow.ci.openshift.org/pr-history?org=code-ready&repo=crc&pr=1837). [Your PR dashboard](https://prow.ci.openshift.org/pr?query=is:pr+state:open+author:guillaumerose).

<details>

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md).  If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository. I understand the commands that are listed [here](https://go.k8s.io/bot-commands).
</details>
<!-- test report -->
openshift-ci[bot]: @guillaumerose: The following tests **failed**, say `/retest` to rerun all failed tests:

Test name | Commit | Details | Rerun command
--- | --- | --- | ---
ci/prow/images | b8b0529b14528ee54b39be3bc0ea2034e115e047 | [link](https://prow.ci.openshift.org/view/gs/origin-ci-test/pr-logs/pull/code-ready_crc/1837/pull-ci-code-ready-crc-master-images/1349731987241308160) | `/test images`
ci/prow/e2e-crc | b8b0529b14528ee54b39be3bc0ea2034e115e047 | [link](https://prow.ci.openshift.org/view/gs/origin-ci-test/pr-logs/pull/code-ready_crc/1837/pull-ci-code-ready-crc-master-e2e-crc/1349731987182587904) | `/test e2e-crc`

[Full PR test history](https://prow.ci.openshift.org/pr-history?org=code-ready&repo=crc&pr=1837). [Your PR dashboard](https://prow.ci.openshift.org/pr?query=is:pr+state:open+author:guillaumerose).

<details>

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md).  If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository. I understand the commands that are listed [here](https://go.k8s.io/bot-commands).
</details>
<!-- test report -->
openshift-ci[bot]: @guillaumerose: The following tests **failed**, say `/retest` to rerun all failed tests:

Test name | Commit | Details | Rerun command
--- | --- | --- | ---
ci/prow/images | b8b0529b14528ee54b39be3bc0ea2034e115e047 | [link](https://prow.ci.openshift.org/view/gs/origin-ci-test/pr-logs/pull/code-ready_crc/1837/pull-ci-code-ready-crc-master-images/1349731987241308160) | `/test images`
ci/prow/e2e-crc | b8b0529b14528ee54b39be3bc0ea2034e115e047 | [link](https://prow.ci.openshift.org/view/gs/origin-ci-test/pr-logs/pull/code-ready_crc/1837/pull-ci-code-ready-crc-master-e2e-crc/1349731987182587904) | `/test e2e-crc`

[Full PR test history](https://prow.ci.openshift.org/pr-history?org=code-ready&repo=crc&pr=1837). [Your PR dashboard](https://prow.ci.openshift.org/pr?query=is:pr+state:open+author:guillaumerose).

<details>

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md).  If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository. I understand the commands that are listed [here](https://go.k8s.io/bot-commands).
</details>
<!-- test report -->
guillaumerose: Still looking at it. I am testing if removing viper is better.
guillaumerose: This is ready for another round. I found it better without viper.
gbraad: > > Still looking at it. I am testing if removing viper is better.

> This is ready for another round. I found it better without viper.

Based on which conclusions?
openshift-ci-robot: @guillaumerose: PR needs rebase.

<details>

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md).  If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.
</details>
guillaumerose: I simplified the change and kept viper for the moment. The major benefit of this PR is that a single config object is used everywhere (and thus a single machine client).
Follow-up will include a way to deny/order machine client calls (https://github.com/code-ready/crc/compare/master...guillaumerose:syncmachine?expand=1) and then, I would like to have an http.Handler for machine client.
It is far more easier with the same handler all the time than if I need to recreate a handler for each request.
cfergeau: > With this change, each config.Get() / config.Set() will require a mutex lock + reading/writing of the whole config file. It's not really obvious why such a heavyweight approach is required so details would be welcome. I understand the goal is thread safe Get/Set, but hopefully this can be achieved with just locks?

I've tried to make the commit log somehow more explicit, not sure it's fully accurate
```
Since commit 04832bdd3f and because of limitations in viper API [1], for each request to the API server, we create a new Config instance. This ensures that the config is up to date, even if modifications were made by external processes.
This use/recreation of short-lived config objects is suboptimal, and complicates the API server code.

To avoid this, this commit changes the ViperStorage implementation, and re-reads or rewrites the config file every time a Get/Set/Unset operation is done.
This also adds locking so that the config API can be accessed concurrently from multiple go routines, which is needed in the api server case.

[1] viper.Set() not doing exactly what the name indicates, viper.WatchConfig() being racy)
```
This tries to add a description of what the initial problem is, and what it's doing. After doing this though, I wonder if the locking itself should be split, and if we could achieve something similar by rewriting viper.WatchConfig without the concurrency issues.
openshift-ci-robot: [APPROVALNOTIFIER] This PR is **APPROVED**

This pull-request has been approved by: *<a href="https://github.com/code-ready/crc/pull/1837#pullrequestreview-587575949" title="Approved">gbraad</a>*, *<a href="https://github.com/code-ready/crc/pull/1837#" title="Author self-approved">guillaumerose</a>*

The full list of commands accepted by this bot can be found [here](https://go.k8s.io/bot-commands?repo=code-ready%2Fcrc).

The pull request process is described [here](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process)

<details >
Needs approval from an approver in each of these files:

- ~~[OWNERS](https://github.com/code-ready/crc/blob/master/OWNERS)~~ [gbraad,guillaumerose]

Approvers can indicate their approval by writing `/approve` in a comment
Approvers can cancel approval by writing `/approve cancel` in a comment
</details>
<!-- META={"approvers":[]} -->
openshift-ci-robot: New changes are detected. LGTM label has been removed.

Patch:
diff --git a/cmd/crc/cmd/daemon.go b/cmd/crc/cmd/daemon.go
index ab76563ae..18a13eea9 100644
--- 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
index caca9406c..9bb02180f 100644
--- 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
index 565862e0a..76ed78f6c 100644
--- 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/types.go b/pkg/crc/api/types.go
index 827658758..b26e79098 100644
--- 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/config/viper_config.go b/pkg/crc/config/viper_config.go
index 12b0e9fda..d6d964e11 100644
--- a/pkg/crc/config/viper_config.go
+++ b/pkg/crc/config/viper_config.go
@@ -7,43 +7,67 @@ import (
 	"os"
 	"path/filepath"
 	"strings"
+	"sync"
 
 	"github.com/spf13/pflag"
 	"github.com/spf13/viper"
 )
 
 type ViperStorage struct {
-	viper      *viper.Viper
+	storeLock *sync.Mutex
+
+	// store flag values
+	flagSet *pflag.FlagSet
+
 	configFile string
+	envPrefix  string
 }
 
 func NewViperStorage(configFile, envPrefix string) (*ViperStorage, error) {
-	if err := ensureConfigFileExists(configFile); err != nil {
+	return &ViperStorage{
+		storeLock:  &sync.Mutex{},
+		configFile: configFile,
+		envPrefix:  envPrefix,
+	}, nil
+}
+
+func (c *ViperStorage) viperInstance() (*viper.Viper, error) {
+	if err := ensureConfigFileExists(c.configFile); err != nil {
 		return nil, err
 	}
 	v := viper.New()
-	v.SetConfigFile(configFile)
+	v.SetConfigFile(c.configFile)
 	v.SetConfigType("json")
-	v.SetEnvPrefix(envPrefix)
+	v.SetEnvPrefix(c.envPrefix)
 	// Replaces '-' in flags with '_' in env variables
 	v.SetEnvKeyReplacer(strings.NewReplacer("-", "_"))
 	v.AutomaticEnv()
 	v.SetTypeByDefaultValue(true)
-	err := v.ReadInConfig()
-	if err != nil {
-		return nil, fmt.Errorf("error reading configuration file '%s': %v", configFile, err)
+	if err := v.ReadInConfig(); err != nil {
+		return nil, fmt.Errorf("error reading configuration file '%s': %v", c.configFile, err)
 	}
-	return &ViperStorage{
-		viper:      v,
-		configFile: configFile,
-	}, nil
+	if c.flagSet == nil {
+		return v, nil
+	}
+	return v, v.BindPFlags(c.flagSet)
 }
 
 func (c *ViperStorage) Get(key string) interface{} {
-	return c.viper.Get(key)
+	c.storeLock.Lock()
+	defer c.storeLock.Unlock()
+	viperInstance, err := c.viperInstance()
+	if err != nil {
+		return nil
+	}
+	return viperInstance.Get(key)
 }
 
 func (c *ViperStorage) Set(key string, value interface{}) error {
+	c.storeLock.Lock()
+	defer c.storeLock.Unlock()
+	if err := ensureConfigFileExists(c.configFile); err != nil {
+		return err
+	}
 	in, err := ioutil.ReadFile(c.configFile)
 	if err != nil {
 		return err
@@ -57,13 +81,15 @@ func (c *ViperStorage) Set(key string, value interface{}) error {
 	if err != nil {
 		return err
 	}
-	if err := atomicWrite(bin, c.configFile); err != nil {
-		return err
-	}
-	return c.viper.ReadInConfig()
+	return atomicWrite(bin, c.configFile)
 }
 
 func (c *ViperStorage) Unset(key string) error {
+	c.storeLock.Lock()
+	defer c.storeLock.Unlock()
+	if err := ensureConfigFileExists(c.configFile); err != nil {
+		return err
+	}
 	in, err := ioutil.ReadFile(c.configFile)
 	if err != nil {
 		return err
@@ -77,15 +103,15 @@ func (c *ViperStorage) Unset(key string) error {
 	if err != nil {
 		return err
 	}
-	if err := atomicWrite(bin, c.configFile); err != nil {
-		return err
-	}
-	return c.viper.ReadInConfig()
+	return atomicWrite(bin, c.configFile)
 }
 
 // BindFlagset binds a flagset to their respective config properties
 func (c *ViperStorage) BindFlagSet(flagSet *pflag.FlagSet) error {
-	return c.viper.BindPFlags(flagSet)
+	c.storeLock.Lock()
+	defer c.storeLock.Unlock()
+	c.flagSet = flagSet
+	return nil
 }
 
 // ensureConfigFileExists creates the viper config file if it does not exists
diff --git a/pkg/crc/machine/client.go b/pkg/crc/machine/client.go
index c974d47d2..c7d98e091 100644
--- 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
index 45a5ccc95..540ef40dc 100644
--- 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
index cac9fe1e3..dce756a47 100644
--- 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,


Test patch:
diff --git a/pkg/crc/api/api_test.go b/pkg/crc/api/api_test.go
index 4811efa4f..e97f8119a 100644
--- 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,7 +198,7 @@ func TestGetconfigApi(t *testing.T) {
 	}, getconfigRes)
 }
 
-func setupNewInMemoryConfig() (config.Storage, error) {
+func setupNewInMemoryConfig() config.Storage {
 	storage := config.NewEmptyInMemoryStorage()
 	cfg := config.New(&skipPreflights{
 		storage: storage,
@@ -209,7 +206,7 @@ func setupNewInMemoryConfig() (config.Storage, error) {
 	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/config/viper_config_test.go b/pkg/crc/config/viper_config_test.go
index 88819e8d7..5002d02a2 100644
--- a/pkg/crc/config/viper_config_test.go
+++ b/pkg/crc/config/viper_config_test.go
@@ -235,3 +235,87 @@ func TestCannotGetWithWrongType(t *testing.T) {
 
 	assert.True(t, config.Get(CPUs).Invalid)
 }
+
+func TestTwoInstancesSharingSameConfiguration(t *testing.T) {
+	dir, err := ioutil.TempDir("", "cfg")
+	require.NoError(t, err)
+	defer os.RemoveAll(dir)
+	configFile := filepath.Join(dir, "crc.json")
+
+	config1, err := newTestConfig(configFile, "CRC")
+	require.NoError(t, err)
+
+	config2, err := newTestConfig(configFile, "CRC")
+	require.NoError(t, err)
+
+	_, err = config1.Set(CPUs, 5)
+	require.NoError(t, err)
+
+	bin, err := ioutil.ReadFile(configFile)
+	assert.NoError(t, err)
+	assert.JSONEq(t, `{"cpus":5}`, string(bin))
+
+	assert.Equal(t, SettingValue{
+		Value:     5,
+		IsDefault: false,
+	}, config2.Get(CPUs))
+	assert.Equal(t, SettingValue{
+		Value:     5,
+		IsDefault: false,
+	}, config1.Get(CPUs))
+}
+
+func TestTwoInstancesWriteSameConfiguration(t *testing.T) {
+	dir, err := ioutil.TempDir("", "cfg")
+	require.NoError(t, err)
+	defer os.RemoveAll(dir)
+	configFile := filepath.Join(dir, "crc.json")
+
+	config1, err := newTestConfig(configFile, "CRC")
+	require.NoError(t, err)
+
+	config2, err := newTestConfig(configFile, "CRC")
+	require.NoError(t, err)
+
+	_, err = config1.Set(CPUs, 5)
+	require.NoError(t, err)
+
+	_, err = config2.Set(NameServer, "1.1.1.1")
+	require.NoError(t, err)
+
+	bin, err := ioutil.ReadFile(configFile)
+	assert.NoError(t, err)
+	assert.JSONEq(t, `{"cpus":5, "nameservers":"1.1.1.1"}`, string(bin))
+
+	assert.Equal(t, 5, config2.Get(CPUs).Value)
+	assert.Equal(t, 5, config1.Get(CPUs).Value)
+
+	assert.Equal(t, "1.1.1.1", config2.Get(NameServer).Value)
+	assert.Equal(t, "1.1.1.1", config1.Get(NameServer).Value)
+}
+
+func TestTwoInstancesSetAndUnsetSameConfiguration(t *testing.T) {
+	dir, err := ioutil.TempDir("", "cfg")
+	require.NoError(t, err)
+	defer os.RemoveAll(dir)
+	configFile := filepath.Join(dir, "crc.json")
+
+	config1, err := newTestConfig(configFile, "CRC")
+	require.NoError(t, err)
+
+	config2, err := newTestConfig(configFile, "CRC")
+	require.NoError(t, err)
+
+	_, err = config1.Set(CPUs, 5)
+	require.NoError(t, err)
+
+	_, err = config2.Unset(CPUs)
+	require.NoError(t, err)
+
+	bin, err := ioutil.ReadFile(configFile)
+	assert.NoError(t, err)
+	assert.JSONEq(t, `{}`, string(bin))
+
+	assert.Equal(t, 4, config2.Get(CPUs).Value)
+	assert.Equal(t, 4, config1.Get(CPUs).Value)
+}
diff --git a/test/e2e/crcsuite/collect.go b/test/e2e/crcsuite/collect.go
index 85297cd61..5822abcde 100644
--- 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