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

Do not check logs in serverless tests #2267

Merged
merged 1 commit into from
Jan 10, 2025

Conversation

fedinskiy
Copy link
Contributor

Summary

Follow-up to #2266

Please select the relevant options.

  • Bug fix (non-breaking change which fixes an issue)
  • Dependency update
  • Refactoring
  • Backport
  • New scenario (non-breaking change which adds functionality)
  • This change requires a documentation update
  • This change requires execution against OCP (use run tests phrase in comment)

Checklist:

  • Methods and classes used in PR scenarios are meaningful
  • Commits are well encapsulated and follow the best practices

These apps do not have them
@fedinskiy fedinskiy requested a review from mocenas January 9, 2025 14:51
@fedinskiy fedinskiy self-assigned this Jan 9, 2025
@fedinskiy
Copy link
Contributor Author

run tests

@@ -19,4 +20,9 @@ public class ServerlessExtensionOpenShiftHttpMinimumReactiveIT extends HttpMinim
protected RequestSpecification givenSpec() {
return HTTPS_CLIENT_SPEC;
}

@Override
@Disabled("Serverless deployments do not have logs which we can check")
Copy link
Member

Choose a reason for hiding this comment

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

wow, this is surprising

Can you share why is that?

app.getLogs() can't retrieve them or even with oc tool there are no logs?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If you run oc status, there would be no pods to read logs from (which is, I guess, expected). Calling app.getLogs returns empty list.

Copy link
Member

Choose a reason for hiding this comment

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

ok, I guess we would need to actively collect logs in FW for any serverless deployment before it gets scaled to zero.

Copy link
Contributor Author

@fedinskiy fedinskiy Jan 10, 2025

Choose a reason for hiding this comment

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

BTW, here how such deployment looks like:

Warning: apps.openshift.io/v1 DeploymentConfig is deprecated in v4.14+, unavailable in v4.10000+
In project fd-demonstration on server https://api.ocp4-17<...>

svc/app-00001 - <...> ports 80->8012, 443->8112
svc/app - kourier-internal.knative-serving-ingress.svc.cluster.local

svc/app-00001-private - <...> ports 80->8012, 443->8112, 9090->http-autometric, 9091->http-usermetric, 8022, 8012
  deployment/app-00001-deployment deploys image-registry.openshift-image-registry.svc:5000/fd-demonstration/app@sha256:<...>,registry.redhat.io/openshift-serverless-1/serving-queue-rhel8@sha256:<...>
    deployment #1 running for 22 minutes

bc/app source builds uploaded code on istag/openjdk-17:latest
  -> istag/app:1.0.0-SNAPSHOT
  build #1 succeeded 22 minutes ago


1 info identified, use 'oc status --suggest' to see details.

mvn clean verify -Dit.test=ServerlessExtensionOpenShiftHttpMinimumReactiveIT -Popenshift -Dts.openshift.delete.project.after.all=false -Dts.openshift.ephemeral.namespaces.enabled=false -Doc.reruns=0

from http/http-minimum-reactive

Copy link
Contributor

Choose a reason for hiding this comment

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

When I tried this, with @fedinskiy 's command, the pod were present in the openshift while tests were searching for logs and remained there for quite a while even after the test failed. Logs from the pod were manually readable no problem and contained all the lines tests are searching for.

IDK why tests were not able to read those. But I agree that for now it is best to disable it investigate the issue with logs reading.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mocenas was it "build pod" or "app pod"? They can be distinguished by "Completed" status (or lack thereof). I have only seen the former.

Copy link
Contributor

Choose a reason for hiding this comment

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

@mocenas was it "build pod" or "app pod"? They can be distinguished by "Completed" status (or lack thereof). I have only seen the former.

I was app pod. It was in state Running. (also build pod had "build" in name).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, now I see temporary pod, which lives for roughly 80 seconds, but the last line in its logs contains "11:26:56,497 Installed features: [cdi, kubernetes, rest, rest-jackson, smallrye-context-propagation, vertx]", so no notification about interception there.

Maybe it is a bug in Quarkus interception, but for now we cann't check it automatically anyway

Copy link
Member

@michalvavrik michalvavrik Jan 14, 2025

Choose a reason for hiding this comment

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

Let's consider that you really can't inspect Knative serving function logs (which makes it basically unusable in production, I suppose they just export it automatically to grafana or something like that we don't have) - you still have running application that returns response. Can you set quarkus.log.file.enable and quarkus.log.file.path and read it from the file system inside Quarkus application resource method? Here you have operator resource that you call https://github.com/quarkus-qe/quarkus-test-suite/blob/main/http/http-minimum-reactive/src/test/java/io/quarkus/ts/http/minimum/reactive/HttpMinimumReactiveIT.java#L99, is there something that stops you from adding a new method that returns logs for certain category (class - so that it is short)?

Interestingly, this can be implemented as a FW feature as well when at least one extension is based on Vert.x HTTP (so 99 of 100 deployments), but I suspect that there is proper solution possible and it just needs a time that we don't have.

Copy link
Contributor

@mocenas mocenas left a comment

Choose a reason for hiding this comment

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

LGTM

@fedinskiy fedinskiy merged commit e527a5c into quarkus-qe:main Jan 10, 2025
9 checks passed
@fedinskiy fedinskiy deleted the fix/weekly branch January 10, 2025 11:33
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