Repository: code-ready/crc
Pull request #1801 (2020-12-24 09:49:29)

Use udev rule to configure the permission and group for /dev/vsock
This would persist the permissions and group change after reboot

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

This pull-request has been approved by: *<a href="https://github.com/code-ready/crc/pull/1801#" title="Author self-approved">anjannath</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)~~ [anjannath]

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: [APPROVALNOTIFIER] This PR is **APPROVED**

This pull-request has been approved by: *<a href="https://github.com/code-ready/crc/pull/1801#" title="Author self-approved">anjannath</a>*, *<a href="https://github.com/code-ready/crc/pull/1801#pullrequestreview-562427683" title="Approved">cfergeau</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)~~ [anjannath,cfergeau]

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":[]} -->

Patch:
diff --git a/cmd/crc/cmd/root.go b/cmd/crc/cmd/root.go
index 36e7a13b2..1c2b96adf 100644
--- a/cmd/crc/cmd/root.go
+++ b/cmd/crc/cmd/root.go
@@ -5,6 +5,7 @@ import (
 	"io/ioutil"
 	"os"
 	"strings"
+	"time"
 
 	cmdConfig "github.com/code-ready/crc/cmd/crc/cmd/config"
 	crcConfig "github.com/code-ready/crc/pkg/crc/config"
@@ -175,13 +176,12 @@ func addForceFlag(cmd *cobra.Command) {
 func executeWithLogging(fullCmd string, input func(cmd *cobra.Command, args []string) error) func(cmd *cobra.Command, args []string) error {
 	return func(cmd *cobra.Command, args []string) error {
 		logging.Debugf("Running '%s'", fullCmd)
-		if err := input(cmd, args); err != nil {
-			if serr := segmentClient.Upload(fullCmd, err); serr != nil {
-				logging.Debugf("Cannot send data to telemetry: %v", serr)
-			}
-			return err
+		startTime := time.Now()
+		err := input(cmd, args)
+		if serr := segmentClient.Upload(fullCmd, time.Since(startTime), err); serr != nil {
+			logging.Debugf("Cannot send data to telemetry: %v", serr)
 		}
-		return nil
+		return err
 	}
 }
 
diff --git a/pkg/crc/preflight/preflight_checks_network_linux.go b/pkg/crc/preflight/preflight_checks_network_linux.go
index bc78d6802..ca22b3ea9 100644
--- a/pkg/crc/preflight/preflight_checks_network_linux.go
+++ b/pkg/crc/preflight/preflight_checks_network_linux.go
@@ -72,22 +72,27 @@ server=/crc.testing/192.168.130.11
 dns=dnsmasq
 `
 
-	crcNetworkManagerDispatcherPath   = filepath.Join(crcNetworkManagerRootPath, "dispatcher.d", "pre-up.d", "99-crc.sh")
-	crcNetworkManagerDispatcherConfig = `#!/bin/sh
+	crcNetworkManagerOldDispatcherPath = filepath.Join(crcNetworkManagerRootPath, "dispatcher.d", "pre-up.d", "99-crc.sh")
+	crcNetworkManagerDispatcherPath    = filepath.Join(crcNetworkManagerRootPath, "dispatcher.d", "99-crc.sh")
+	crcNetworkManagerDispatcherConfig  = `#!/bin/sh
 # This is a NetworkManager dispatcher script to configure split DNS for
 # the 'crc' libvirt network.
-# The corresponding crc bridge is recreated each time the system reboots, so
-# it cannot be configured permanently through NetworkManager.
-# Changing DNS settings with nmcli requires the connection to go down/up,
-# so we directly make the change using resolvectl
+#
+# The corresponding crc bridge is not created through NetworkManager, so
+# it cannot be configured permanently through NetworkManager. We make the
+# change directly using resolvectl instead.
+#
+# NetworkManager will overwrite this resolvectl configuration every time a
+# network connection goes up/down, so we run this script on each of these events
+# to restore our settings. This is a NetworkManager bug which is fixed in
+# version 1.26.6 by this commit:
+# https://cgit.freedesktop.org/NetworkManager/NetworkManager/commit/?id=ee4e679bc7479de42780ebd8e3a4d74afa2b2ebe
 
 export LC_ALL=C
 
-if [ "$1" = crc ]; then
-        resolvectl domain "$1" ~testing
-        resolvectl dns "$1" 192.168.130.11
-        resolvectl default-route "$1" false
-fi
+resolvectl domain crc ~testing
+resolvectl dns crc 192.168.130.11
+resolvectl default-route crc false
 
 exit 0
 `
@@ -266,6 +271,10 @@ func checkCrcNetworkManagerDispatcherFile() error {
 
 func fixCrcNetworkManagerDispatcherFile() error {
 	logging.Debug("Fixing NetworkManager dispatcher configuration")
+
+	// Remove dispatcher script which was used in crc 1.20 - it's been moved to a new location
+	_ = removeNetworkManagerConfigFile(crcNetworkManagerOldDispatcherPath)
+
 	err := fixNetworkManagerConfigFile(crcNetworkManagerDispatcherPath, crcNetworkManagerDispatcherConfig, 0755)
 	if err != nil {
 		return err
@@ -276,6 +285,9 @@ func fixCrcNetworkManagerDispatcherFile() error {
 }
 
 func removeCrcNetworkManagerDispatcherFile() error {
+	// Remove dispatcher script which was used in crc 1.20 - it's been moved to a new location
+	_ = removeNetworkManagerConfigFile(crcNetworkManagerOldDispatcherPath)
+
 	return removeNetworkManagerConfigFile(crcNetworkManagerDispatcherPath)
 }
 
diff --git a/pkg/crc/preflight/preflight_linux.go b/pkg/crc/preflight/preflight_linux.go
index f2e7ff45f..e2587c8c1 100644
--- a/pkg/crc/preflight/preflight_linux.go
+++ b/pkg/crc/preflight/preflight_linux.go
@@ -8,6 +8,7 @@ import (
 	"strings"
 	"syscall"
 
+	crcErrors "github.com/code-ready/crc/pkg/crc/errors"
 	"github.com/code-ready/crc/pkg/crc/logging"
 	"github.com/code-ready/crc/pkg/crc/network"
 	crcos "github.com/code-ready/crc/pkg/os"
@@ -97,13 +98,20 @@ var libvirtNetworkPreflightChecks = [...]Check{
 }
 
 var vsockPreflightChecks = Check{
-	configKeySuffix:  "check-vsock",
-	checkDescription: "Checking if vsock is correctly configured",
-	check:            checkVsock,
-	fixDescription:   "Checking if vsock is correctly configured",
-	fix:              fixVsock,
+	configKeySuffix:    "check-vsock",
+	checkDescription:   "Checking if vsock is correctly configured",
+	check:              checkVsock,
+	fixDescription:     "Checking if vsock is correctly configured",
+	fix:                fixVsock,
+	cleanupDescription: "Removing vsock configuration",
+	cleanup:            removeVsockCrcSettings,
 }
 
+const (
+	vsockUdevRulesPath          = "/usr/lib/udev/rules.d/99-crc-vsock.rules"
+	vsockModuleAutoLoadConfPath = "/etc/modules-load.d/vhost_vsock.conf"
+)
+
 func checkVsock() error {
 	executable, err := os.Executable()
 	if err != nil {
@@ -126,7 +134,7 @@ func checkVsock() error {
 			return err
 		}
 		if group.Name != "libvirt" {
-			return errors.New("/dev/vsock is not is the right group")
+			return errors.New("/dev/vsock is not in the right group")
 		}
 	} else {
 		return errors.New("cannot cast info")
@@ -146,21 +154,39 @@ func fixVsock() error {
 	if err != nil {
 		return err
 	}
-	_, _, err = crcos.RunWithPrivilege("modprobe vhost_vsock", "modprobe", "vhost_vsock")
+
+	udevRule := `KERNEL=="vsock", MODE="0660", OWNER="root", GROUP="libvirt"`
+	err = crcos.WriteToFileAsRoot("Create udev rule for /dev/vsock", udevRule, vsockUdevRulesPath, 0644)
 	if err != nil {
 		return err
 	}
-	_, _, err = crcos.RunWithPrivilege("chown /dev/vsock", "chown", "root:libvirt", "/dev/vsock")
+	err = crcos.WriteToFileAsRoot(fmt.Sprintf("Create file %s", vsockModuleAutoLoadConfPath), "vhost_vsock", vsockModuleAutoLoadConfPath, 0644)
 	if err != nil {
 		return err
 	}
-	_, _, err = crcos.RunWithPrivilege("chmod /dev/vsock", "chmod", "g+rw", "/dev/vsock")
+	_, _, err = crcos.RunWithPrivilege("modprobe vhost_vsock", "modprobe", "vhost_vsock")
 	if err != nil {
 		return err
 	}
 	return nil
 }
 
+func removeVsockCrcSettings() error {
+	var mErr crcErrors.MultiError
+	_, _, err := crcos.RunWithPrivilege(fmt.Sprintf("rm %s", vsockUdevRulesPath), "rm", "-f", vsockUdevRulesPath)
+	if err != nil {
+		mErr.Collect(err)
+	}
+	_, _, err = crcos.RunWithPrivilege(fmt.Sprintf("rm %s", vsockModuleAutoLoadConfPath), "rm", "-f", vsockModuleAutoLoadConfPath)
+	if err != nil {
+		mErr.Collect(err)
+	}
+	if len(mErr.Errors) == 0 {
+		return nil
+	}
+	return mErr
+}
+
 func getAllPreflightChecks() []Check {
 	checks := getPreflightChecksForDistro(distro(), network.DefaultMode)
 	checks = append(checks, vsockPreflightChecks)
diff --git a/pkg/crc/segment/segment.go b/pkg/crc/segment/segment.go
index 254dfb386..e9ac423c4 100644
--- a/pkg/crc/segment/segment.go
+++ b/pkg/crc/segment/segment.go
@@ -4,7 +4,9 @@ import (
 	"io/ioutil"
 	"os"
 	"path/filepath"
+	"runtime"
 	"strings"
+	"time"
 
 	"github.com/code-ready/crc/cmd/crc/cmd/config"
 	crcConfig "github.com/code-ready/crc/pkg/crc/config"
@@ -53,24 +55,35 @@ func (c *Client) Close() error {
 	return c.segmentClient.Close()
 }
 
-func (c *Client) Upload(action string, err error) error {
+func (c *Client) Upload(action string, duration time.Duration, err error) error {
 	if !c.config.Get(config.ConsentTelemetry).AsBool() {
 		return nil
 	}
-	logging.Debug("Uploading the error to segment")
 
 	anonymousID, uerr := getUserIdentity(c.telemetryFilePath)
 	if uerr != nil {
 		return uerr
 	}
 
-	t := analytics.NewTraits().
-		Set("action", action).
-		Set("error", err.Error())
+	if err := c.segmentClient.Enqueue(analytics.Identify{
+		AnonymousId: anonymousID,
+		Traits: analytics.NewTraits().
+			Set("os", runtime.GOOS),
+	}); err != nil {
+		return err
+	}
+
+	properties := analytics.NewProperties().
+		Set("success", err == nil).
+		Set("duration", duration.Milliseconds())
+	if err != nil {
+		properties = properties.Set("error", err.Error())
+	}
 
-	return c.segmentClient.Enqueue(analytics.Identify{
+	return c.segmentClient.Enqueue(analytics.Track{
 		AnonymousId: anonymousID,
-		Traits:      t,
+		Event:       action,
+		Properties:  properties,
 	})
 }
 


Test patch:
diff --git a/pkg/crc/segment/segment_test.go b/pkg/crc/segment/segment_test.go
index cdf4faa22..abc799f97 100644
--- a/pkg/crc/segment/segment_test.go
+++ b/pkg/crc/segment/segment_test.go
@@ -8,7 +8,9 @@ import (
 	"net/http/httptest"
 	"os"
 	"path/filepath"
+	"runtime"
 	"testing"
+	"time"
 
 	cmdConfig "github.com/code-ready/crc/cmd/crc/cmd/config"
 	crcConfig "github.com/code-ready/crc/pkg/crc/config"
@@ -22,8 +24,11 @@ type segmentResponse struct {
 		AnonymousID string `json:"anonymousId"`
 		MessageID   string `json:"messageId"`
 		Traits      struct {
-			Error string `json:"error"`
+			OS string `json:"os"`
 		} `json:"traits"`
+		Properties struct {
+			Error string `json:"error"`
+		} `json:"properties"`
 		Type string `json:"type"`
 	} `json:"batch"`
 	Context struct {
@@ -81,14 +86,17 @@ func TestClientUploadWithConsent(t *testing.T) {
 	c, err := newCustomClient(config, filepath.Join(dir, "telemetry"), server.URL)
 	require.NoError(t, err)
 
-	require.NoError(t, c.Upload("start", errors.New("an error occurred")))
+	require.NoError(t, c.Upload("start", time.Minute, errors.New("an error occurred")))
 	require.NoError(t, c.Close())
 
 	select {
 	case x := <-body:
 		s := segmentResponse{}
 		require.NoError(t, json.Unmarshal(x, &s))
-		require.Equal(t, s.Batch[0].Traits.Error, "an error occurred")
+		require.Equal(t, s.Batch[0].Type, "identify")
+		require.Equal(t, s.Batch[0].Traits.OS, runtime.GOOS)
+		require.Equal(t, s.Batch[1].Type, "track")
+		require.Equal(t, s.Batch[1].Properties.Error, "an error occurred")
 		require.Equal(t, s.Context.App.Name, "crc")
 		require.Equal(t, s.Context.App.Version, version.GetCRCVersion())
 	default:
@@ -111,7 +119,7 @@ func TestClientUploadWithOutConsent(t *testing.T) {
 	c, err := newCustomClient(config, filepath.Join(dir, "telemetry"), server.URL)
 	require.NoError(t, err)
 
-	require.NoError(t, c.Upload("start", errors.New("an error occurred")))
+	require.NoError(t, c.Upload("start", time.Second, errors.New("an error occurred")))
 	require.NoError(t, c.Close())
 
 	select {