Add cluster type to telemetry #4763

valaparthvi · 2021-06-01T08:32:25Z

What type of PR is this?
/kind feature

What does this PR do / why we need it:
This PR adds information about cluster type to telemetry data. Currently it detects k8s, openshift4 and openshift3.

Which issue(s) this PR fixes:

Fixes part of #4462

PR acceptance criteria:

How to test changes / Special notes to the reviewer:
Run odo push or odo project create and see if the data is visible on Segment.

girishramnani · 2021-06-01T09:04:19Z

pkg/segment/context/context_test.go

+	}
+}
+
+func fakeClusterVersion(fakeClientset *occlient.FakeClientset) {


you can return a configv1.ClusterVersion here when the user list for clusterversion. if you want to verify what parameters to add here, maybe add a PrependReactor for anything called on the fake client, and then print the arguments. That way you would know which calls you need to mock.

valaparthvi · 2021-06-02T07:30:23Z

pkg/occlient/occlient.go

+func (c *Client) IsOpenshift4() (bool, error) {
+	return c.GetKubeClient().IsResourceSupported("config.openshift.io", "v1", "clusterversions")
+}
+


Is it alright to place this method here?
I can move this function to segment/context/context.go if it might not be used in other places.

We should call it something related to the resource like IsClusterVersionsSupported similar to IsProjectSupported. Please add some comments for the function.

valaparthvi · 2021-06-04T06:30:13Z

pkg/occlient/fakeclient.go

-		client.GetKubeClient().SetDiscoveryInterface(fakeDiscoveryWithRoute)
+		//client.GetKubeClient().SetDiscoveryInterface(fakeDiscoveryWithRoute)


This has been commented out because it gets overriden by the next statement.

We can remove the comments for now

Are you suggesting I keep it as it is for now? I can do that, I have issue #4773 open, I can fix this when I work on that issue.

No, we should remove it for now

mik-dass · 2021-06-04T07:57:02Z

pkg/kclient/kclient.go

@@ -234,6 +234,21 @@ func (c *Client) IsResourceSupported(apiGroup, apiVersion, resourceName string)
 	return supported, nil
 }

+func (c *Client) IsResourceGroupSupported(apiGroup string) (bool, error) {


Please add a comment for the function

I deleted this method. I am no longer using it.

mik-dass · 2021-06-04T08:07:03Z

pkg/segment/context/context.go

+	}
+}
+
+func SetClusterType(ctx context.Context, client *occlient.Client) {


Please add some comments for the function

mik-dass · 2021-06-04T08:10:04Z

pkg/segment/context/context.go

+		if isOC, _ := client.IsProjectSupported(); isOC {
+			if isOC4, _ := client.IsOpenshift4(); isOC4 {


Please handle the errors from these two calls. We should default to kubernetes etc only when there are no errors.

Do you suggest we do error handling for if isOC4, _ := client.IsOpenshift4(); isOC4 { as well?

Do you suggest we do error handling for if isOC4, _ := client.IsOpenshift4(); isOC4 { as well?

Yes

mik-dass · 2021-06-04T08:14:32Z

pkg/segment/segment.go

@@ -205,6 +205,9 @@ func RunningInTerminal() bool {

 // IsTelemetryEnabled returns true if user has consented to telemetry
 func IsTelemetryEnabled(cfg *preference.PreferenceInfo) bool {
+	if cfg == nil {
+		cfg, _ = preference.New()


Please handle the error here

mik-dass · 2021-06-04T08:15:57Z

tests/integration/devfile/cmd_devfile_watch_test.go

+		PIt("should fail", func() {
 			helper.Chdir(commonVar.OriginalWorkingDirectory)
 			helper.CmdShouldPass("odo", "create", "nodejs", "--project", commonVar.Project, "--context", commonVar.Context, cmpName)
 			output := helper.CmdShouldFail("odo", "watch", "--context", commonVar.Context)
 			Expect(output).To(ContainSubstring("component does not exist. Please use `odo push` to create your component"))
 		})

-		It("should error out on devfile flag", func() {
+		PIt("should error out on devfile flag", func() {
 			helper.CmdShouldFail("odo", "watch", "--devfile", "invalid.yaml")
 		})
 	})

 	Context("when executing odo watch after odo push", func() {
-		It("should listen for file changes", func() {
+		XIt("should listen for file changes", func() {


Why are we marking them as pending?

I forgot to remove them. Done.

valaparthvi · 2021-06-04T11:01:26Z

/test psi-unit-test-windows

-unit-pr-build_PR_4763/repo/.gocache
[ssh:Windows 10] ++ GOCACHE='C:\Users\Admin\amqp_ci_rcv_odo-windows-unit-pr-build_PR_4763\repo\.gocache'
[ssh:Windows 10] +++ cygpath -pw /cygdrive/c/Users/Admin/amqp_ci_rcv_odo-windows-unit-pr-build_PR_4763/repo/artifacts
[ssh:Windows 10] ++ CUSTOM_HOMEDIR='C:\Users\Admin\amqp_ci_rcv_odo-windows-unit-pr-build_PR_4763\repo\artifacts'
[ssh:Windows 10] ++ shout 'Setting PATH'
[ssh:Windows 10] ++ set +x
[ssh:Windows 10] ++ export 'PATH=/bin:/cygdrive/c/WINDOWS/system32:/cygdrive/c/WINDOWS:/cygdrive/c/WINDOWS/System32/Wbem:/cygdrive/c/WINDOWS/System32/WindowsPowerShell/v1.0:/cygdrive/c/Go/bin:/cygdrive/c/Program Files/Git/cmd:/cygdrive/c/WINDOWS/System32/OpenSSH:/cygdrive/c/TDM-GCC-64/bin:/cygdrive/c/Users/cyg_server/AppData/Local/Microsoft/WindowsApps:C:\Users\Admin\amqp_ci_rcv_odo-windows-unit-pr-build_PR_4763\repo\bin'
[ssh:Windows 10] ++ PATH='/bin:/cygdrive/c/WINDOWS/system32:/cygdrive/c/WINDOWS:/cygdrive/c/WINDOWS/System32/Wbem:/cygdrive/c/WINDOWS/System32/WindowsPowerShell/v1.0:/cygdrive/c/Go/bin:/cygdrive/c/Program Files/Git/cmd:/cygdrive/c/WINDOWS/System32/OpenSSH:/cygdrive/c/TDM-GCC-64/bin:/cygdrive/c/Users/cyg_server/AppData/Local/Microsoft/WindowsApps:C:\Users\Admin\amqp_ci_rcv_odo-windows-unit-pr-build_PR_4763\repo\bin'

[ssh:Windows 10] + . ./scripts/run_script_unit.sh
[ssh:Windows 10] ++ shout 'Running unit tests'
[ssh:Windows 10] ++ set +x
[ssh:Windows 10] ++ [[ windows == \w\i\n\d\o\w\s ]]
[ssh:Windows 10] ++ GOFLAGS=-mod=vendor
[ssh:Windows 10] ++ make test-windows
[ssh:Windows 10] make: *** [Makefile:155: test-windows] Error 1

[ssh:Windows 10] !!!run failed, skipping!!!

2021/06/04 10:49:16 : failed due to err Failed the test, see logs above ^
{"component":"entrypoint","error":"wrapped process failed: exit status 1","file":"prow/entrypoint/run.go:80","func":"k8s.io/test-infra/prow/entrypoint.Options.Run","level":"error","msg":"Error executing test process","severity":"error","time":"2021-06-04T10:49:16Z"}
error: failed to execute wrapped command: exit status 1

valaparthvi · 2021-06-07T14:13:33Z

/test psi-unit-test-windows

openshift-ci · 2021-06-07T14:13:42Z

@valaparthvi: The /retest command does not accept any targets.
The following commands are available to trigger jobs:

/test psi-kubernetes-integration-e2e
/test psi-unit-test-mac
/test psi-unit-test-windows
/test unit
/test v4.5-images
/test v4.6-images
/test v4.7-images
/test v4.7-integration-e2e
/test v4.8-images

Use /test all to run the following jobs:

pull-ci-openshift-odo-main-psi-kubernetes-integration-e2e
pull-ci-openshift-odo-main-psi-unit-test-mac
pull-ci-openshift-odo-main-psi-unit-test-windows
pull-ci-openshift-odo-main-unit
pull-ci-openshift-odo-main-v4.7-integration-e2e

In response to this:

/retest psi-unit-test-windows

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

valaparthvi · 2021-06-08T10:28:47Z

/retest

valaparthvi · 2021-06-08T11:28:01Z

/retest

valaparthvi · 2021-06-08T11:34:19Z

/override ci/prow/psi-kubernetes-integration-e2e
Reason - #4776

openshift-ci · 2021-06-08T11:34:24Z

@valaparthvi: Overrode contexts on behalf of valaparthvi: ci/prow/psi-kubernetes-integration-e2e

In response to this:

/override ci/prow/psi-kubernetes-integration-e2e
Reason - #4776

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

mik-dass · 2021-06-08T12:39:29Z

pkg/segment/context/context.go

+			value = "not-found"
+		} else {
+			if isOC {
+				if isOC4, _ := client.GetKubeClient().IsCSVSupported(); isOC4 {


Please check the error here as well

I was wondering whether I should, because if it throws an error we still know that it is an openshift cluster. What value do you suggest we assign in case this throws an error?

But in case of an connection error to the cluster, this might lead to false assumptions about the type of openshift cluster.

What value do you suggest we assign in case this throws an error?

I would suggest not setting the property at all or defaulting to just openshift.

I want to go with openshift but then it would mean adding a unit test for it which would mean mocking the IsCSVSupported function to return an error and all of this seems like a stretch to me.
If you still think that it's worth the effort, I'll go with it.

OK, let's default to openshift when an error occurs. We can skip the unit test case for now.

mik-dass

/lgtm

…h odo push

…test

valaparthvi · 2021-06-30T07:31:25Z

/test v4.7-integration-e2e

Error-ed out while collecting console logs.

Gathering console logs for i-07ec01954a84e305a

'ascii' codec can't encode character '\u2026' in position 18189: ordinal not in range(128)
{"component":"entrypoint","error":"wrapped process failed: exit status 1","file":"prow/entrypoint/run.go:80","func":"k8s.io/test-infra/prow/entrypoint.Options.Run","level":"error","msg":"Error executing test process","severity":"error","time":"2021-06-30T06:29:22Z"}
error: failed to execute wrapped command: exit status 1

valaparthvi · 2021-06-30T10:24:50Z

/retest

Failure on v4.7-integration-e2e job looks like a flake-

 ------------------------------
• Failure [116.321 seconds]
odo devfile push command tests
/go/src/github.com/openshift/odo/tests/integration/devfile/cmd_devfile_push_test.go:18
  verify command executions
  /go/src/github.com/openshift/odo/tests/integration/devfile/cmd_devfile_push_test.go:464
    when default build and run commands are defined
[Fail] odo devfile push command tests verify command executions when default build and run commands are defined [It] should execute correct commands 
/go/src/github.com/openshift/odo/tests/integration/devfile/cmd_devfile_push_test.go:514
Ran 188 of 195 Specs in 1055.057 seconds
FAIL! -- 187 Passed | 1 Failed | 1 Pending | 6 Skipped

Same for psi-kubernetes-integration-e2e -

"kubernetes-integration-e2e" test steps failed: ["kubernetes-integration-e2e" pod "kubernetes-integration-e2e-kubernetes-integration-e2e-steps" failed: context canceled
Link to step on registry info site: https://steps.ci.openshift.org/reference/kubernetes-integration-e2e-steps
Link to job on registry info site: https://steps.ci.openshift.org/job?org=openshift&repo=odo&branch=main&test=kubernetes-integration-e2e&variant=psi, cancelled]

dharmit

/approve

pkg/segment/context/context.go

pkg/odo/cli/component/create_helpers.go

openshift-ci · 2021-06-30T12:17:06Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dharmit

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [dharmit]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

openshift-bot · 2021-06-30T17:43:43Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2021-06-30T19:55:42Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2021-06-30T20:20:13Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-ci · 2021-06-30T21:44:34Z

@valaparthvi: The following test failed, say /retest to rerun all failed tests:

Test name	Commit	Details	Rerun command
ci/prow/psi-kubernetes-integration-e2e	`fd86e46`	link	`/test psi-kubernetes-integration-e2e`

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

openshift-ci bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. Required by Prow. kind/feature Categorizes issue as a feature request. For PRs, that means that the PR is the implementation labels Jun 1, 2021

openshift-ci bot requested review from dharmit and prietyc123 June 1, 2021 08:32

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. Required by Prow. label Jun 1, 2021

valaparthvi force-pushed the cluster branch from b8d3985 to 525db08 Compare June 1, 2021 08:37

girishramnani reviewed Jun 1, 2021

View reviewed changes

valaparthvi commented Jun 2, 2021

View reviewed changes

openshift-ci bot removed the approved Indicates a PR has been approved by an approver from all required OWNERS files. Required by Prow. label Jun 3, 2021

valaparthvi changed the title ~~WIP: Add cluster type to telemetry~~ Add cluster type to telemetry Jun 3, 2021

openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. Required by Prow. label Jun 3, 2021

valaparthvi force-pushed the cluster branch from 95ebecf to 131aa59 Compare June 3, 2021 13:09

valaparthvi mentioned this pull request Jun 3, 2021

Track telemetry implementation for odo #4462

Closed

17 tasks

valaparthvi commented Jun 4, 2021

View reviewed changes

mik-dass reviewed Jun 4, 2021

View reviewed changes

valaparthvi force-pushed the cluster branch from 064b21f to 66d9c1e Compare June 8, 2021 10:03

mik-dass reviewed Jun 8, 2021

View reviewed changes

mik-dass approved these changes Jun 9, 2021

View reviewed changes

openshift-ci bot assigned mik-dass Jun 9, 2021

valaparthvi added 16 commits June 30, 2021 10:38

Add cluster type with odo push and project set and component type wit…

214ba28

…h odo push

code cleanup, handle errors and add documentation for functions

8e7ca6f

more error handling and documentation

36fc04d

Remove comments

bda37f1

go mod tidy

fa4d367

change import order

ca478d6

Use CSV instead of clusterversion

2d12194

Attempt at fixing integration tests

6012ad0

Add more error handling

f4b7760

Add better value to componentType on odo push

2958af8

fix componenttype for odo create

31d94e0

Use ExtractComponentType while setting componentType and modify unit …

9e54a70

…test

Cleanup redundant code

a30ae4e

Use a default value when component type is not found

fc52e39

Remove FakeDiscovery related redundant code

4eea85e

Fix ci failures

4045cc4

valaparthvi force-pushed the cluster branch from ca9d94a to 4045cc4 Compare June 30, 2021 05:08

dharmit reviewed Jun 30, 2021

View reviewed changes

pkg/segment/context/context.go Show resolved Hide resolved

pkg/segment/context/context.go Outdated Show resolved Hide resolved

pkg/odo/cli/component/create_helpers.go Outdated Show resolved Hide resolved

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. Required by Prow. label Jun 30, 2021

use constants

fd86e46

dharmit approved these changes Jun 30, 2021

View reviewed changes

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. Required by Prow. label Jun 30, 2021

openshift-merge-robot merged commit f6714b0 into redhat-developer:main Jun 30, 2021

		client.GetKubeClient().SetDiscoveryInterface(fakeDiscoveryWithRoute)
		//client.GetKubeClient().SetDiscoveryInterface(fakeDiscoveryWithRoute)

		if isOC, _ := client.IsProjectSupported(); isOC {
		if isOC4, _ := client.IsOpenshift4(); isOC4 {

Add cluster type to telemetry #4763

Add cluster type to telemetry #4763

Conversation

valaparthvi commented Jun 1, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mik-dass Jun 4, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

valaparthvi commented Jun 4, 2021

valaparthvi commented Jun 7, 2021 • edited Loading

openshift-ci bot commented Jun 7, 2021

valaparthvi commented Jun 8, 2021

valaparthvi commented Jun 8, 2021

valaparthvi commented Jun 8, 2021

openshift-ci bot commented Jun 8, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mik-dass left a comment

Choose a reason for hiding this comment

valaparthvi commented Jun 30, 2021

valaparthvi commented Jun 30, 2021

dharmit left a comment

Choose a reason for hiding this comment

openshift-ci bot commented Jun 30, 2021

openshift-bot commented Jun 30, 2021

openshift-bot commented Jun 30, 2021

openshift-bot commented Jun 30, 2021

openshift-ci bot commented Jun 30, 2021 • edited Loading

mik-dass Jun 4, 2021 •

edited

Loading

valaparthvi commented Jun 7, 2021 •

edited

Loading

openshift-ci bot commented Jun 30, 2021 •

edited

Loading