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

[WFLY-15849] OpenTelemetry QS on OpenShift #872

Merged
merged 2 commits into from
Feb 1, 2024

Conversation

emmartins
Copy link
Contributor

@emmartins emmartins commented Jan 30, 2024

Clean up YAML
Add instructions for deploying otel collector on OS
OepnShift CI setup

Issue: WFLY-15849, WFLY-18943

@emmartins
Copy link
Contributor Author

Superseeds #870

@kabir
Copy link
Contributor

kabir commented Jan 30, 2024

@emmartins For some reason the opentelemetry tests are not getting picked up. See https://gcsweb-ci.apps.ci.l2s4.p1.openshiftapps.com/gcs/test-platform-results/pr-logs/pull/wildfly_quickstart/872/pull-ci-wildfly-quickstart-main-wildfly-quickstart-e2e/1752305685376274432/artifacts/wildfly-quickstart-e2e/quickstart-build/build-log.txt

Right at the end

Parsed test directories, and determined tests should be run for the following directories:
	
Running tests...
All tests run in 0m0s.
\033[0;32mAll tests passed!\033[0m

@emmartins
Copy link
Contributor Author

emmartins commented Jan 30, 2024 via email

@kabir
Copy link
Contributor

kabir commented Jan 30, 2024

It might be because the GH Action workflow is called quickstart_opentelemetry_ci.yml while the quickstart folder is called opentelemetry-tracing, which would cause it to miss this check https://github.com/wildfly/quickstart/blob/main/.ci/openshift-ci/build-root/scripts/openshift-test-runner.sh#L151-L153

Either the quickstart_opentelemetry_ci.yml file needs to be called quickstart_opentelemetry-tracing_ci.yml, or the quickstart folder needs to be renamed to opentelemetry

@jasondlee
Copy link
Contributor

Either the quickstart_opentelemetry_ci.yml file needs to be called quickstart_opentelemetry-tracing_ci.yml, or the quickstart folder needs to be renamed to opentelemetry

I can address that issue here: #871

Build with openshift profile
Clean up YAML
Modify how WF is configured under OS build
Add instructions for deploying otel collector on OS
@emmartins
Copy link
Contributor Author

It might be because the GH Action workflow is called quickstart_opentelemetry_ci.yml while the quickstart folder is called opentelemetry-tracing, which would cause it to miss this check https://github.com/wildfly/quickstart/blob/main/.ci/openshift-ci/build-root/scripts/openshift-test-runner.sh#L151-L153

Either the quickstart_opentelemetry_ci.yml file needs to be called quickstart_opentelemetry-tracing_ci.yml, or the quickstart folder needs to be renamed to opentelemetry

fixed, added functions override and remove test exclusion

Copy link
Contributor

@jasondlee jasondlee left a comment

Choose a reason for hiding this comment

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

Looks good to me. I'll close my PR in favor of this one.

Copy link
Contributor

@kabir kabir left a comment

Choose a reason for hiding this comment

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

I've checked the OpenShift tests, and they are running

@emmartins emmartins changed the title [WFLY-15849] OpenTelemetry QS under OpenShift [WFLY-15849] OpenTelemetry QS on OpenShift Jan 30, 2024
@kstekovi
Copy link
Contributor

kstekovi commented Feb 1, 2024

The OpenShift guide have unorganized steps compare to bootabale jar.
bootable-jar:

  • build
  • start app
  • start integration tests
  • stop

OpenShift:

  • prerequisites (start the opentelemetry)
  • deploy QS
  • uninstall
  • start integration tests

@emmartins
Copy link
Contributor Author

emmartins commented Feb 1, 2024 via email


function cleanPrerequisites() {
echo "Deleting all OpenTelemetry Collector resources"
oc delete -f charts/opentelemetry-collector.yaml
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this step should as additional step in undeploy

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The proper solution for this requires an additional extension point for shared-docs, similar to helm-install-prerequisites, and that will affect all QS so I have created https://issues.redhat.com/browse/WFLY-18986 to deliver it (and more general shared improvements). For now the uninstall is referenced as a note at https://github.com/wildfly/quickstart/pull/872/files#diff-437f0901bdbc94e992d4b6b75f355240a557c4879683f47d27dad1c51f27dc4aR19

@emmartins
Copy link
Contributor Author

emmartins commented Feb 1, 2024 via email

@kstekovi
Copy link
Contributor

kstekovi commented Feb 1, 2024

It seem to be similar issues with steps organization. So I think this could be fixed in that scope. #872 (comment)

I did think of that, but we will need to add another extension point on the shared-docs, similar to what we have for extra pre requisites install. Due to tight schedule for now I opted for a note on the pre requisites install content, but definitely in the future I would like to add such undeploy extension point and have mp reactive, micrometer and opentelemetry QS to provide an impl for that. If you agree of course, your call, you have the right to request that now if you want :-)

On Thu, 1 Feb 2024 at 10:02, Krystof Stekovic @.> wrote: @.* commented on this pull request. ------------------------------ In .ci/openshift-ci/build-root/scripts/qs-overrides/opentelemetry-tracing/overridable-functions.sh <#872 (comment)>: > @@ -0,0 +1,9 @@ +function runPostHelmInstallCommands() { + echo "Applying all OpenTelemetry Collector resources" + oc apply -f charts/opentelemetry-collector.yaml +} + +function cleanPrerequisites() { + echo "Deleting all OpenTelemetry Collector resources" + oc delete -f charts/opentelemetry-collector.yaml I think this step should as additional step in undeploy — Reply to this email directly, view it on GitHub <#872 (review)>, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAEDY6ECDHVNBEZT5XFT7OTYRNR3JAVCNFSM6AAAAABCRDS3OCVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMYTQNJWGA3DSNZSHE . You are receiving this because you were mentioned.Message ID: @.***>

@emmartins
Copy link
Contributor Author

The OpenShift guide have unorganized steps compare to bootabale jar. bootable-jar:

  • build
  • start app
  • start integration tests
  • stop

OpenShift:

  • prerequisites (start the opentelemetry)
  • deploy QS
  • uninstall
  • start integration tests

fixed

@kstekovi
Copy link
Contributor

kstekovi commented Feb 1, 2024

OpenShift steps works as is expected. Ready to merge. Thanks.

@emmartins
Copy link
Contributor Author

thank you @kstekovi

@emmartins emmartins merged commit 9dcbe39 into wildfly:main Feb 1, 2024
20 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants