Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

RHOAIENG-2508 ODS-2600 Validate DSC creates all Serverless CRs #1259

Conversation

asanzgom
Copy link
Contributor

@asanzgom asanzgom commented Mar 6, 2024

RHOAIENG-2508
The goal of this Test Case is to automate manual testing on the Platform scrum team

@asanzgom asanzgom added needs testing Needs to be tested in Jenkins new test New test(s) added (PR will be listed in release-notes) labels Mar 6, 2024
@asanzgom asanzgom self-assigned this Mar 6, 2024


*** Test Cases ***
Validate DSC creates all Serverless CRs

Check warning

Code scanning / Robocop

Test case '{{ test_name }}' has too many keywords inside ({{ keyword_count }}/{{ max_allowed_count }}) Warning test

Test case 'Validate DSC creates all Serverless CRs' has too many keywords inside (12/10)
Copy link
Contributor

github-actions bot commented Mar 6, 2024

Robot Results

✅ Passed ❌ Failed ⏭️ Skipped Total Pass %
455 0 0 455 100

@asanzgom
Copy link
Contributor Author

asanzgom commented Mar 6, 2024

Triggered rhods-ci-pr-test/2573

Executed successfully

@asanzgom asanzgom requested a review from FedeAlonso March 7, 2024 09:39
[Documentation] Assign vars related to product
[Arguments] ${product}
IF "${product}" == "RHODS"
Set Suite Variable ${OPERATOR_APPNAME} Red Hat OpenShift AI

Check notice

Code scanning / Robocop

{{ set_variable_keyword }} can be replaced with VAR Note test

Set Suite Variable can be replaced with VAR
Set Suite Variable ${OPERATOR_NAME} Red Hat OpenShift AI
Set Suite Variable ${OPERATOR_NAMESPACE} redhat-ods-operator
ELSE IF "${product}" == "ODH"
Set Suite Variable ${OPERATOR_APPNAME} Open Data Hub Operator

Check notice

Code scanning / Robocop

{{ set_variable_keyword }} can be replaced with VAR Note test

Set Suite Variable can be replaced with VAR
@asanzgom
Copy link
Contributor Author

asanzgom commented Mar 7, 2024

Triggered rhods-ci-pr-test/2576

Executed successfully

@asanzgom asanzgom requested review from jstourac and zdtsw March 7, 2024 11:05
Copy link
Member

@jstourac jstourac left a comment

Choose a reason for hiding this comment

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

I put a few comments, Adrian. TBH, I would welcome if somebody more experienced with the whole our TS review this too.

[Documentation] Checks that all conditions Reconciled Succesfully
[Arguments] ${namespace} ${dsc_name}
${rc} ${out}= Run And Return Rc And Output
... oc get DataScienceCluster ${dsc_name} -n ${namespace} -o jsonpath='{.status.conditions[].reason}'
Copy link
Member

Choose a reason for hiding this comment

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

How exactly we check the condition here? 🤔

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 meant to be displayed as it is very prone to errors depending on the installation

Copy link
Member

@jstourac jstourac Mar 7, 2024

Choose a reason for hiding this comment

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

Yeah, the status of the components isn't stable... this is something I think that operator could also improve... the statuses are regularly changing even in case that all is fine (Ready vs Unknown). Again, I am new to the OpenShift world, so my assumption can be wrong.

But wanted to say that this doesn't check anything... it just prints some info... so if that is desired and won't be changed, please rename the keyword appropriately so it's not confusing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

changed

Copy link
Member

Choose a reason for hiding this comment

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

Just realized that we are checking the condition of just one component - then... shouldn't we want it to be in ReconcileReady state? It's just one component here, it should be ready and working.

Copy link
Contributor

Choose a reason for hiding this comment

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

Should Be Equal As Integers ${rc} ${0} are you sure this is actually checking what we want? it would check that the the oc get command has worked without error (i.e., status.conditions[].reason exists in the DSC).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

changed the keyword to wait for Conditions on DSC to be Reconciled successfully or timeout

${pod_logs}= Oc Get Pod Logs name=${operator_name} namespace=${operator_namespace} container=rhods-operator
${error_present}= Run Keyword And Return Status Should Contain ${pod_logs} ERROR
IF ${error_present}
Log message=Check Pod Logs, ERROR level logs found. level=WARN
Copy link
Member

Choose a reason for hiding this comment

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

Just warning here? Errors in operator logs are just fine?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There are ERROR level logs at the start because of Reconciliation sync, then it is fine

Copy link
Member

Choose a reason for hiding this comment

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

Yes, so shouldn't we limit to check logs after some time when the operator is stable? Otherwise, do you plan to open these test results even when they are green and search for the content of this?

Also, it would be nice if operator would be smart enough to try to understand the context and reduce emitting errors at the beginning when components are starting. But I'm new to the OpenShift world, so my assumption may be just wrong.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@zdtsw This is something that maybe could be improved on the operator side
In the meanwhile, we need a test in place that is not prone to errors

Copy link

Choose a reason for hiding this comment

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

which part need to be done in operator?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not printing ERROR logs due to Reconcile if afterward everything goes fine
e.g.

2024-03-07T10:12:03Z	ERROR	Reconciler error	{"controller": "secret-generator-controller", "controllerGroup": "", "controllerKind": "Secret", "Secret": {"name":"dashboard-oauth-config","namespace":"redhat-ods-applications"}, "namespace": "redhat-ods-applications", "name": "dashboard-oauth-config", "reconcileID": "65761c63-8b59-44bc-8a57-4471f6163df0", "error": "Secret \"dashboard-oauth-config-generated\" not found"}
sigs.k8s.io/controller-runtime/pkg/internal/controller.(*Controller).reconcileHandler
	/remote-source/operator/deps/gomod/pkg/mod/sigs.k8s.io/controller-runtime@v0.14.6/pkg/internal/controller/controller.go:329
sigs.k8s.io/controller-runtime/pkg/internal/controller.(*Controller).processNextWorkItem
	/remote-source/operator/deps/gomod/pkg/mod/sigs.k8s.io/controller-runtime@v0.14.6/pkg/internal/controller/controller.go:274
sigs.k8s.io/controller-runtime/pkg/internal/controller.(*Controller).Start.func2.2
	/remote-source/operator/deps/gomod/pkg/mod/sigs.k8s.io/controller-runtime@v0.14.6/pkg/internal/controller/controller.go:235

I see this error everytime Operator gets installed

Copy link
Member

Choose a reason for hiding this comment

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

@asanzgom regarding this test - so no plan to wait until operator is stable and check only the portion of the logs after that?

Also, looks like it just prints this warning message but doesn't print any actual error in the log... it's kind of useless, isn't it? 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We need to check the Operator's Pod Logs for errors. There is one Test Case that could be refactored into a reusable Keyword for this
ods_ci/tests/Tests/100__deploy/130__operators/130__rhods_operator/135__rhods_operator_logs_verification.robot

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree with Jan's points.
Check For Errors On Operator Logs so I would expect it failing if any error is found. I understand that ERRORS at the startup are accepted/expected, but what is the benefit we got from having this check?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Refactored to fail when ERROR reg ex is found on logs and added ProductBug Tag

@asanzgom
Copy link
Contributor Author

asanzgom commented Mar 7, 2024

I put a few comments, Adrian. TBH, I would welcome if somebody more experienced with the whole our TS review this too.

@jstourac This TS is new

@asanzgom asanzgom dismissed stale reviews from jstourac and jgarciao via 43f0a2d April 2, 2024 10:43
Resource Should Exist
[Documentation] Check resource existson a namespace
[Arguments] ${resource} ${resource_name} ${namespace}
${rc} ${out}= Run and Return Rc And Output

Check warning

Code scanning / Robocop

Keyword name '{{ keyword_name }}' does not follow case convention Warning test

Keyword name 'Run and Return Rc And Output' does not follow case convention
ods_ci/tests/Resources/OCP.resource Fixed Show fixed Hide fixed
ods_ci/tests/Resources/OCP.resource Fixed Show fixed Hide fixed
ods_ci/tests/Resources/OCP.resource Fixed Show fixed Hide fixed
ods_ci/tests/Resources/OCP.resource Fixed Show fixed Hide fixed
ods_ci/tests/Resources/OCP.resource Fixed Show fixed Hide fixed
ods_ci/tests/Resources/OCP.resource Fixed Show fixed Hide fixed
Log ${status} console=True
Should Be Equal ${status} ${expected_status} msg=${resource} is not in Ready status

Check For Errors On Operator Logs
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you modify this keyword (in this PR or a follow-up on) so it can be used on any pod and not just the operator pod?

Verify Pod Logs Does Not Contain
[Arguments]    ${pod_name}  ${label_selector}  ${all_containers}=${FALSE}  ${container_name}=${EMPTY}    ${namespace}    ${since}  ${since-time}  ${regex_pattern}

And use the oc command instead of Oc Get. If you run oc logs -h you'll see that it has most of the parameters

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok Jorge I modify it

Assign Vars According To Product
[Documentation] Assign vars related to product
[Arguments] ${product}
IF "${product}" == "RHODS"
Copy link
Contributor

Choose a reason for hiding this comment

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

It should be ${PRODUCT}, as it is a global variable

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 Jorge, I change it

*** Variables ***
${KNATIVESERVING_NS} knative-serving
${ISTIO_NS} istio-system
${regex_pattern} ERROR

Check warning

Code scanning / Robocop

Section variable '{{ variable_name }}' name should be uppercase Warning test

Section variable '${regex_pattern}' name should be uppercase
${ISTIO_NS} istio-system
${regex_pattern} ERROR
${LABEL_SELECTOR} name=rhods-operator

Check warning

Code scanning / Robocop

Invalid number of empty lines between sections ({{ empty_lines }}/{{ allowed_empty_lines }}) Warning test

Invalid number of empty lines between sections (1/2)
[Arguments] ${PRODUCT}
IF "${PRODUCT}" == "RHODS"
Set Suite Variable ${OPERATOR_APPNAME} Red Hat OpenShift AI
Set Suite Variable ${OPERATOR_NAME} Red Hat OpenShift AI

Check notice

Code scanning / Robocop

{{ set_variable_keyword }} can be replaced with VAR Note test

Set Suite Variable can be replaced with VAR
[Arguments] ${pod_name} ${pod_namespace} ${regex_pattern} ${container}
${pod_logs}= Oc Get Pod Logs name=${pod_name} namespace=${pod_namespace} container=${container}
${match_list} Get Regexp Matches ${pod_logs} ${regex_pattern}
${entry_msg} Remove Duplicates ${match_list}

Check warning

Code scanning / Robocop

The assignment sign is not consistent within the file. Expected '{{ expected_sign }}' but got '{{ actual_sign }}' instead Warning test

The assignment sign is not consistent within the file. Expected '=' but got '' instead
${pod_logs}= Oc Get Pod Logs name=${pod_name} namespace=${pod_namespace} container=${container}
${match_list} Get Regexp Matches ${pod_logs} ${regex_pattern}
${entry_msg} Remove Duplicates ${match_list}
${length} Get Length ${entry_msg}

Check warning

Code scanning / Robocop

The assignment sign is not consistent within the file. Expected '{{ expected_sign }}' but got '{{ actual_sign }}' instead Warning test

The assignment sign is not consistent within the file. Expected '=' but got '' instead
ods_ci/tests/Resources/OCP.resource Dismissed Show dismissed Hide dismissed
Comment on lines +226 to +227
IF ${length} != ${0} FAIL Pod ${pod_name} logs should not contain regexp ${regex_pattern}. Matching log entry: ${entry_msg}
... ELSE Log message=Pod ${pod_name} logs does not contain regexp ${regex_pattern} level=INFO

Check warning

Code scanning / Robocop

Avoid splitting inline IF to multiple lines Warning test

Avoid splitting inline IF to multiple lines
${match_list} Get Regexp Matches ${pod_logs} ${regex_pattern}
${entry_msg} Remove Duplicates ${match_list}
${length} Get Length ${entry_msg}
IF ${length} != ${0} FAIL Pod ${pod_name} logs should not contain regexp ${regex_pattern}. Matching log entry: ${entry_msg}

Check warning

Code scanning / Robocop

Trailing whitespace at the end of line Warning test

Trailing whitespace at the end of line
jgarciao
jgarciao previously approved these changes Apr 4, 2024
Copy link

sonarqubecloud bot commented Apr 4, 2024

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
2.6% Duplication on New Code

See analysis details on SonarCloud

Set Suite Variable ${OPERATOR_NAME} Red Hat OpenShift AI
ELSE IF "${PRODUCT}" == "ODH"
Set Suite Variable ${OPERATOR_APPNAME} Open Data Hub Operator
Set Suite Variable ${OPERATOR_NAME} Open Data Hub Operator

Check notice

Code scanning / Robocop

{{ set_variable_keyword }} can be replaced with VAR Note test

Set Suite Variable can be replaced with VAR
@asanzgom asanzgom requested a review from jgarciao April 4, 2024 11:02
@asanzgom asanzgom merged commit cdc6695 into red-hat-data-services:master Apr 4, 2024
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs testing Needs to be tested in Jenkins new test New test(s) added (PR will be listed in release-notes)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants