Repository: code-ready/crc
Pull request #1984 (2021-02-11 09:38:33)

Add error type to segment
It will be easy to create filter using error type for telemetry side.



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

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

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]: @praveenkumar: The following test **failed**, say `/retest` to rerun all failed tests:

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

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

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

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

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

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

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/pkg/crc/cluster/cluster.go b/pkg/crc/cluster/cluster.go
index eee417580..acfbb4b9d 100644
--- a/pkg/crc/cluster/cluster.go
+++ b/pkg/crc/cluster/cluster.go
@@ -320,7 +320,7 @@ func WaitForAPIServer(ocConfig oc.Config) error {
 		logging.Debug(stdout)
 		return nil
 	}
-	return errors.RetryAfter(2*time.Minute, waitForAPIServer, time.Second)
+	return errors.RetryAfter(3*time.Minute, waitForAPIServer, time.Second)
 }
 
 func DeleteOpenshiftAPIServerPods(ocConfig oc.Config) error {
diff --git a/pkg/crc/preflight/preflight_checks_common.go b/pkg/crc/preflight/preflight_checks_common.go
index 0c0a56eb0..6d7663c85 100644
--- a/pkg/crc/preflight/preflight_checks_common.go
+++ b/pkg/crc/preflight/preflight_checks_common.go
@@ -17,6 +17,15 @@ import (
 	"github.com/pkg/errors"
 )
 
+var bundleCheck = Check{
+	configKeySuffix:  "check-bundle-extracted",
+	checkDescription: "Checking if CRC bundle is extracted in '$HOME/.crc'",
+	check:            checkBundleExtracted,
+	fixDescription:   "Extracting bundle from the CRC executable",
+	fix:              fixBundleExtracted,
+	flags:            SetupOnly,
+}
+
 var genericPreflightChecks = [...]Check{
 	{
 		configKeySuffix:  "check-podman-cached",
@@ -32,14 +41,6 @@ var genericPreflightChecks = [...]Check{
 		fixDescription:   "Caching admin-helper executable",
 		fix:              fixAdminHelperExecutableCached,
 	},
-	{
-		configKeySuffix:  "check-bundle-extracted",
-		checkDescription: "Checking if CRC bundle is extracted in '$HOME/.crc'",
-		check:            checkBundleExtracted,
-		fixDescription:   "Extracting bundle from the CRC executable",
-		fix:              fixBundleExtracted,
-		flags:            SetupOnly,
-	},
 	{
 		configKeySuffix:  "check-ram",
 		checkDescription: "Checking minimum RAM requirements",
diff --git a/pkg/crc/preflight/preflight_darwin.go b/pkg/crc/preflight/preflight_darwin.go
index cb551ce6c..c8eef86a8 100644
--- a/pkg/crc/preflight/preflight_darwin.go
+++ b/pkg/crc/preflight/preflight_darwin.go
@@ -126,5 +126,6 @@ func getPreflightChecks(experimentalFeatures bool, mode network.Mode) []Check {
 		checks = append(checks, traySetupChecks[:]...)
 	}
 
+	checks = append(checks, bundleCheck)
 	return checks
 }
diff --git a/pkg/crc/preflight/preflight_linux.go b/pkg/crc/preflight/preflight_linux.go
index b2820c42c..76a9f3850 100644
--- a/pkg/crc/preflight/preflight_linux.go
+++ b/pkg/crc/preflight/preflight_linux.go
@@ -252,6 +252,7 @@ func getPreflightChecksForDistro(distro *linux.OsRelease, networkMode network.Mo
 	if networkMode == network.DefaultMode {
 		checks = append(checks, libvirtNetworkPreflightChecks[:]...)
 	}
+	checks = append(checks, bundleCheck)
 	return checks
 }
 
diff --git a/pkg/crc/preflight/preflight_windows.go b/pkg/crc/preflight/preflight_windows.go
index 5ac5f376d..e39adbb45 100644
--- a/pkg/crc/preflight/preflight_windows.go
+++ b/pkg/crc/preflight/preflight_windows.go
@@ -155,5 +155,6 @@ func getPreflightChecks(experimentalFeatures bool, networkMode network.Mode) []C
 		checks = append(checks, traySetupChecks[:]...)
 	}
 
+	checks = append(checks, bundleCheck)
 	return checks
 }
diff --git a/pkg/crc/segment/segment.go b/pkg/crc/segment/segment.go
index 9a053166a..1e49cc49d 100644
--- a/pkg/crc/segment/segment.go
+++ b/pkg/crc/segment/segment.go
@@ -2,6 +2,8 @@ package segment
 
 import (
 	"context"
+	"errors"
+	"fmt"
 	"io/ioutil"
 	"net"
 	"os"
@@ -85,7 +87,8 @@ func (c *Client) Upload(ctx context.Context, action string, duration time.Durati
 		Set("duration", duration.Milliseconds()).
 		Set("tty", crcos.RunningInTerminal())
 	if err != nil {
-		properties = properties.Set("error", err.Error())
+		properties = properties.Set("error", err.Error()).
+			Set("error-type", errorType(err))
 	}
 
 	return c.segmentClient.Enqueue(analytics.Track{
@@ -140,3 +143,11 @@ func (l *loggingAdapter) Logf(format string, args ...interface{}) {
 func (l *loggingAdapter) Errorf(format string, args ...interface{}) {
 	logging.Errorf(format, args...)
 }
+
+func errorType(err error) string {
+	wrappedErr := errors.Unwrap(err)
+	if wrappedErr != nil {
+		return fmt.Sprintf("%T", wrappedErr)
+	}
+	return fmt.Sprintf("%T", err)
+}


Test patch:
diff --git a/pkg/crc/preflight/preflight_linux_test.go b/pkg/crc/preflight/preflight_linux_test.go
index 020da1455..6bc0b3b3c 100644
--- a/pkg/crc/preflight/preflight_linux_test.go
+++ b/pkg/crc/preflight/preflight_linux_test.go
@@ -65,7 +65,6 @@ var checkListForDistros = []checkListForDistro{
 			{cleanup: adminhelper.CleanHostsFile},
 			{check: checkPodmanExecutableCached},
 			{check: checkAdminHelperExecutableCached},
-			{check: checkBundleExtracted},
 			{configKeySuffix: "check-ram"},
 			{cleanup: removeCRCMachinesDir},
 			{cleanup: removeOldLogs},
@@ -86,6 +85,7 @@ var checkListForDistros = []checkListForDistro{
 			{check: checkCrcNetworkManagerDispatcherFile},
 			{check: checkLibvirtCrcNetworkAvailable},
 			{check: checkLibvirtCrcNetworkActive},
+			{check: checkBundleExtracted},
 		},
 	},
 	{
@@ -96,7 +96,6 @@ var checkListForDistros = []checkListForDistro{
 			{cleanup: adminhelper.CleanHostsFile},
 			{check: checkPodmanExecutableCached},
 			{check: checkAdminHelperExecutableCached},
-			{check: checkBundleExtracted},
 			{configKeySuffix: "check-ram"},
 			{cleanup: removeCRCMachinesDir},
 			{cleanup: removeOldLogs},
@@ -110,6 +109,7 @@ var checkListForDistros = []checkListForDistro{
 			{check: checkMachineDriverLibvirtInstalled},
 			{cleanup: removeCrcVM},
 			{check: checkVsock},
+			{check: checkBundleExtracted},
 		},
 	},
 	{
@@ -120,7 +120,6 @@ var checkListForDistros = []checkListForDistro{
 			{cleanup: adminhelper.CleanHostsFile},
 			{check: checkPodmanExecutableCached},
 			{check: checkAdminHelperExecutableCached},
-			{check: checkBundleExtracted},
 			{configKeySuffix: "check-ram"},
 			{cleanup: removeCRCMachinesDir},
 			{cleanup: removeOldLogs},
@@ -140,6 +139,7 @@ var checkListForDistros = []checkListForDistro{
 			{check: checkCrcDnsmasqConfigFile},
 			{check: checkLibvirtCrcNetworkAvailable},
 			{check: checkLibvirtCrcNetworkActive},
+			{check: checkBundleExtracted},
 		},
 	},
 	{
@@ -150,7 +150,6 @@ var checkListForDistros = []checkListForDistro{
 			{cleanup: adminhelper.CleanHostsFile},
 			{check: checkPodmanExecutableCached},
 			{check: checkAdminHelperExecutableCached},
-			{check: checkBundleExtracted},
 			{configKeySuffix: "check-ram"},
 			{cleanup: removeCRCMachinesDir},
 			{cleanup: removeOldLogs},
@@ -164,6 +163,7 @@ var checkListForDistros = []checkListForDistro{
 			{check: checkMachineDriverLibvirtInstalled},
 			{cleanup: removeCrcVM},
 			{check: checkVsock},
+			{check: checkBundleExtracted},
 		},
 	},
 	{
@@ -174,7 +174,6 @@ var checkListForDistros = []checkListForDistro{
 			{cleanup: adminhelper.CleanHostsFile},
 			{check: checkPodmanExecutableCached},
 			{check: checkAdminHelperExecutableCached},
-			{check: checkBundleExtracted},
 			{configKeySuffix: "check-ram"},
 			{cleanup: removeCRCMachinesDir},
 			{cleanup: removeOldLogs},
@@ -194,6 +193,7 @@ var checkListForDistros = []checkListForDistro{
 			{check: checkCrcDnsmasqConfigFile},
 			{check: checkLibvirtCrcNetworkAvailable},
 			{check: checkLibvirtCrcNetworkActive},
+			{check: checkBundleExtracted},
 		},
 	},
 	{
@@ -204,7 +204,6 @@ var checkListForDistros = []checkListForDistro{
 			{cleanup: adminhelper.CleanHostsFile},
 			{check: checkPodmanExecutableCached},
 			{check: checkAdminHelperExecutableCached},
-			{check: checkBundleExtracted},
 			{configKeySuffix: "check-ram"},
 			{cleanup: removeCRCMachinesDir},
 			{cleanup: removeOldLogs},
@@ -218,6 +217,7 @@ var checkListForDistros = []checkListForDistro{
 			{check: checkMachineDriverLibvirtInstalled},
 			{cleanup: removeCrcVM},
 			{check: checkVsock},
+			{check: checkBundleExtracted},
 		},
 	},
 	{
@@ -228,7 +228,6 @@ var checkListForDistros = []checkListForDistro{
 			{cleanup: adminhelper.CleanHostsFile},
 			{check: checkPodmanExecutableCached},
 			{check: checkAdminHelperExecutableCached},
-			{check: checkBundleExtracted},
 			{configKeySuffix: "check-ram"},
 			{cleanup: removeCRCMachinesDir},
 			{cleanup: removeOldLogs},
@@ -250,6 +249,7 @@ var checkListForDistros = []checkListForDistro{
 			{check: checkCrcNetworkManagerDispatcherFile},
 			{check: checkLibvirtCrcNetworkAvailable},
 			{check: checkLibvirtCrcNetworkActive},
+			{check: checkBundleExtracted},
 		},
 	},
 	{
@@ -260,7 +260,6 @@ var checkListForDistros = []checkListForDistro{
 			{cleanup: adminhelper.CleanHostsFile},
 			{check: checkPodmanExecutableCached},
 			{check: checkAdminHelperExecutableCached},
-			{check: checkBundleExtracted},
 			{configKeySuffix: "check-ram"},
 			{cleanup: removeCRCMachinesDir},
 			{cleanup: removeOldLogs},
@@ -275,6 +274,7 @@ var checkListForDistros = []checkListForDistro{
 			{cleanup: removeCrcVM},
 			{configKeySuffix: "check-apparmor-profile-setup"},
 			{check: checkVsock},
+			{check: checkBundleExtracted},
 		},
 	},
 }
diff --git a/pkg/crc/segment/segment_test.go b/pkg/crc/segment/segment_test.go
index af65b96f3..59b6ff053 100644
--- a/pkg/crc/segment/segment_test.go
+++ b/pkg/crc/segment/segment_test.go
@@ -15,6 +15,7 @@ import (
 
 	cmdConfig "github.com/code-ready/crc/cmd/crc/cmd/config"
 	crcConfig "github.com/code-ready/crc/pkg/crc/config"
+	crcErr "github.com/code-ready/crc/pkg/crc/errors"
 	"github.com/code-ready/crc/pkg/crc/logging"
 	"github.com/code-ready/crc/pkg/crc/telemetry"
 	"github.com/code-ready/crc/pkg/crc/version"
@@ -30,9 +31,10 @@ type segmentResponse struct {
 			ExperimentalFeatures bool   `json:"enable-experimental-features"`
 		} `json:"traits"`
 		Properties struct {
-			Error   string `json:"error"`
-			Version string `json:"version"`
-			CPUs    int    `json:"cpus"`
+			Error     string `json:"error"`
+			ErrorType string `json:"error-type"`
+			Version   string `json:"version"`
+			CPUs      int    `json:"cpus"`
 		} `json:"properties"`
 		Type string `json:"type"`
 	} `json:"batch"`
@@ -69,7 +71,41 @@ func newTestConfig(value string) (*crcConfig.Config, error) {
 	return config, nil
 }
 
-func TestClientUploadWithConsent(t *testing.T) {
+func TestClientUploadWithConsentAndWithSerializableError(t *testing.T) {
+	body, server := mockServer()
+	defer server.Close()
+	defer close(body)
+
+	dir, err := ioutil.TempDir("", "cfg")
+	require.NoError(t, err)
+	defer os.RemoveAll(dir)
+
+	config, err := newTestConfig("yes")
+	require.NoError(t, err)
+
+	c, err := newCustomClient(config, filepath.Join(dir, "telemetry"), server.URL)
+	require.NoError(t, err)
+
+	require.NoError(t, c.Upload(context.Background(), "start", time.Minute, crcErr.ToSerializableError(crcErr.VMNotExist)))
+	require.NoError(t, c.Close())
+
+	select {
+	case x := <-body:
+		s := segmentResponse{}
+		require.NoError(t, json.Unmarshal(x, &s))
+		require.Equal(t, s.Batch[0].Type, "identify")
+		require.Equal(t, s.Batch[0].Traits.OS, runtime.GOOS)
+		require.Equal(t, s.Batch[0].Traits.ExperimentalFeatures, true)
+		require.Equal(t, s.Batch[1].Type, "track")
+		require.Equal(t, s.Batch[1].Properties.Error, crcErr.VMNotExist.Error())
+		require.Equal(t, s.Batch[1].Properties.ErrorType, "errors.vmNotExist")
+		require.Equal(t, s.Batch[1].Properties.Version, version.GetCRCVersion())
+	default:
+		require.Fail(t, "server should receive data")
+	}
+}
+
+func TestClientUploadWithConsentAndWithoutSerializableError(t *testing.T) {
 	body, server := mockServer()
 	defer server.Close()
 	defer close(body)
@@ -96,6 +132,7 @@ func TestClientUploadWithConsent(t *testing.T) {
 		require.Equal(t, s.Batch[0].Traits.ExperimentalFeatures, true)
 		require.Equal(t, s.Batch[1].Type, "track")
 		require.Equal(t, s.Batch[1].Properties.Error, "an error occurred")
+		require.Equal(t, s.Batch[1].Properties.ErrorType, "*errors.errorString")
 		require.Equal(t, s.Batch[1].Properties.Version, version.GetCRCVersion())
 	default:
 		require.Fail(t, "server should receive data")
diff --git a/test/e2e/features/basic.feature b/test/e2e/features/basic.feature
index dd0cffbc3..6753f3832 100644
--- a/test/e2e/features/basic.feature
+++ b/test/e2e/features/basic.feature
@@ -125,7 +125,7 @@ Feature: Basic test
     Scenario: Monitoring stack check
         Given checking that CRC is running
         When executing "eval $(crc oc-env)" succeeds
-        And setting kubeconfig context to "crc-admin" succeeds
+        And login to the oc cluster succeeds
         And executing "oc get pods -n openshift-monitoring" succeeds
         Then stdout matches ".*cluster-monitoring-operator-\w+-\w+\ *2/2\ *Running.*"
         And unsetting config property "enable-cluster-monitoring" succeeds
@@ -135,7 +135,7 @@ Feature: Basic test
     Scenario: Monitoring stack check
         Given checking that CRC is running
         And executing "crc oc-env | Invoke-Expression" succeeds
-        And setting kubeconfig context to "crc-admin" succeeds
+        And login to the oc cluster succeeds
         And executing "oc get pods -n openshift-monitoring" succeeds
         Then stdout matches ".*cluster-monitoring-operator-\w+-\w+\ *2/2\ *Running.*"
         And unsetting config property "enable-cluster-monitoring" succeeds