Skip to content

Conversation

ratnam915
Copy link
Contributor

What type of PR is this?

(feature/bug/documentation/other)

What this PR does / Why we need it?

Special notes for your reviewer

Test Coverage

Guidelines for CAD investigations

  • New investgations should be accompanied by unit tests and/or step-by-step manual tests in the investigation README.
  • Actioning investigations should be locally tested in staging, and E2E testing is desired. See README for more info on investigation graduation process.

Test coverage checks

  • Added tests
  • Created jira card to add unit test
  • This PR may not need unit tests

Pre-checks (if applicable)

  • Ran unit tests locally
  • Validated the changes in a cluster
  • Included documentation changes with PR

@codecov-commenter
Copy link

codecov-commenter commented Jul 29, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 33.64%. Comparing base (47a8a8e) to head (31a74e4).

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             main     #517   +/-   ##
=======================================
  Coverage   33.64%   33.64%           
=======================================
  Files          37       37           
  Lines        2476     2476           
=======================================
  Hits          833      833           
  Misses       1583     1583           
  Partials       60       60           
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Comment on lines 31 to 46
## ONLY FOR LOCAL TESTING, THIS CONFIGURATION HAS TO BE REVERTED BACK BEFORE COMMIT AND PUSHING TO THE REPOSITORY

Comment out #56,57 in configuration_anomaly_detection_tests.go and replace with the following code:

ocme2eCli, err = ocme2e.New(ctx, ocmToken, clientID, clientSecret, ocmEnv)
Expect(err).ShouldNot(HaveOccurred(), "Unable to setup E2E OCM Client")

ocmCli, err = ocm.New(cadOcmFilePath)
Expect(err).ShouldNot(HaveOccurred(), "Unable to setup ocm anomaly detection client")

Add below statements in #50,#53 respectively

ocmToken := os.Getenv("OCM_TOKEN")
Expect(ocmToken).NotTo(BeEmpty(), "OCM_TOKEN must be set")

## "!! PLEASE NOTE THAT SINCE OCM_TOKEN IS NOW DEPRECATED ABOVE LINES OF CODE HAVE TO BE REMOVED AND #56,57 HAVE TO BE UNCOMMENTED !!"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we ensure we don't need to modify the code to run this locally? That seems like a workaround ;) Ideally, running e2e tests with a target cluster should work programmatically. If you could provide a short script here, that would be very beneficial.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @typeid : The reason this workaround is being proposed from us is for OCM_CLIENT_ID and OCM_CLIENT_SECRET to work from a local machine a service account is required, if the service account is available we would not need to do this workaround and the test would work fine.

The script would not be required in that case, please let us know on how to proceed with this.

I can put a note in with a link to the SOP to create service account and enable it to be able to test the code in local.

Copy link
Member

@typeid typeid Aug 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about attempting to load the existing ocm token, and falling back to clientid and clientsecret?
That way it would work for both envs, correct?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, how do we do this change after building the e2e?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

E.g.

import (
	ocmConfig "github.com/openshift-online/ocm-common/pkg/ocm/config"
	ocmConnBuilder "github.com/openshift-online/ocm-common/pkg/ocm/connection-builder"
)
...

		cfg, err := ocmConfig.Load()
		if err != nil {
			clientID := os.Getenv("OCM_CLIENT_ID")
			Expect(clusterID).NotTo(BeEmpty(), "OCM_CLIENT_ID must be set")

			clientSecret := os.Getenv("OCM_CLIENT_SECRET")
			Expect(clusterID).NotTo(BeEmpty(), "OCM_CLIENT_SECRET must be set")

			ocme2eCli, err = ocme2e.New(ctx, "", clientID, clientSecret, ocmEnv)
			Expect(err).ShouldNot(HaveOccurred(), "Unable to setup E2E OCM Client")
		} else {
			// Build connection based on local config
			connection, err := ocmConnBuilder.NewConnection().Config(cfg).AsAgent("cad-local-e2e-tests").Build()
			ocme2eCli = &ocme2e.Client{Connection: connection}
			Expect(err).ShouldNot(HaveOccurred(), "Unable to setup E2E OCM Client")
		}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @typeid, this has significantly reduced overhead, the README file is fairly simple now.

I made the code changes and carried out local testing and it ran fine.

Also in other news i was comparing the Pagerduty alerts that we received https://redhat.pagerduty.com/service-directory/P4BLYHK/activity to the runtimes of the E2E pipeline -> https://prow.ci.openshift.org/job-history/gs/test-platform-results/logs/periodic-ci-openshift-osde2e-main-nightly-4.19-rosa-classic-sts

It looks like they are matching, however i don't see any logs from our test cases niether passing nor failing, should be consider closing the task for the environment variables syncing with E2E?

Comment on lines 23 to 25
7. Enable Debug Mode
This enables detailed logging during test execution:
export CAD_DEBUG=true
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is this variable for if we're not running the cadctl afterwards?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is again required for local testing and can be removed if we are following the service account mode

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CAD_DEBUG? That variable doesn't even exist anymore, and it should have had no influence on e2e :)

Comment on lines 12 to 15
5. AWS Credentials
These are needed for interacting with the cluster. You can find them in the ~/.aws/credentials file.
export AWS_ACCESS_KEY_ID=<your AWS access key ID>
export AWS_SECRET_ACCESS_KEY=<your AWS secret access key>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we make this more generic to work on "target stage cluster"? The credentials in this file will not always match the cluster's AWS creds.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added another line to ensure cluster is created with a default profile and the credentials for the same profile are set in the environment variables in this step

@@ -9,6 +9,42 @@ To do this, following steps are recommended

ocm get /api/clusters_mgmt/v1/clusters/(cluster-id)/credentials | jq -r .kubeconfig > /(path-to)/kubeconfig
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not part of this PR, but the sentence above:

. Deploy your new version of operator in a test cluster

That doesn't seem to apply here.

@ratnam915
Copy link
Contributor Author

"/label tide/merge-method-squash"


3. Create a ROSA Cluster which would be used for E2E testing using the below command:

rosa create cluster --cluster-name=<cluster-name> --profile <default-aws-profile>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

polish: we need to export the cluster id as OCM_CLUSTER_ID here

@RaphaelBut
Copy link
Contributor

I ran through these steps while developing another test case and it worked fine.
Thanks for adding this!

Copy link
Contributor

openshift-ci bot commented Aug 12, 2025

@ratnam915: all tests passed!

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-sigs/prow repository. I understand the commands that are listed here.

Copy link
Member

@typeid typeid left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm
/approve

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Aug 13, 2025
Copy link
Contributor

openshift-ci bot commented Aug 13, 2025

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ratnam915, typeid

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:

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

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 13, 2025
@openshift-merge-bot openshift-merge-bot bot merged commit 2d9a751 into openshift:main Aug 13, 2025
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants