Add kuttl tests for lightspeed-app-server /metrics endpoint - #22
Add kuttl tests for lightspeed-app-server /metrics endpoint#22jancervenka wants to merge 3 commits into
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: jancervenka The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
Hi @jancervenka. Thanks for your PR. I'm waiting for a openstack-k8s-operators member to verify that this patch is reasonable to test. If it is, they should reply with Regular contributors should join the org to skip this step. Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions 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. |
b371d7d to
4fef44e
Compare
The kuttl test asserts that the endpoint is reachable and responds with a valid Prometheus format.
4fef44e to
3c85e10
Compare
|
/ok-to-test |
| - kind: ServiceAccount | ||
| name: ${SA_NAME} | ||
| namespace: ${NAMESPACE} | ||
| EOF |
There was a problem hiding this comment.
These ClusterRole and ClusterRoleBinding are beeing created but I don't see them being cleaned up at the end, so they will stay in the cluster as leftovers artifacts.
Also, I think we can create them in a declarative way ? Instead of inside the TestAssert
There was a problem hiding this comment.
@umago Thank you Lucas! I moved the creation to create-openstack-lightspeed-instance.yaml and also added it to cleanup-openstanck-lightspeed-instance.yaml and errors-openstack-lightspeed-instance.yaml.
| echo "Response (first 500 chars):" | ||
| echo "$METRICS_OUTPUT" | head -c 500 | ||
| exit 1 | ||
| fi |
There was a problem hiding this comment.
I would to hear @lpiwowar and/or @malingatembo opinion on using kuttl tests this ^
It seems like it fits more into the new test framework than kuttl. Because this is asserting some behavior from our API endpoint rather than just asserting the state of the cluster (which I believe kuttl is better designed for).
There was a problem hiding this comment.
It seems like it fits more into the new test framework than kuttl. ...
issue (blocking): I agree with this. If we did not have the foundation for the test framework laid out by @malingatembo, then I would consider this as an exception, maybe. But since the foundation is laid out, I think we should start adding tests there. It will force us to polish the test framework, and it will make the usage of it much easier later since the relevant tests will already be there and we won't be forced to migrate them there.
| echo "$METRICS_OUTPUT" | head -c 500 | ||
| exit 1 | ||
| fi | ||
|
|
There was a problem hiding this comment.
Also, if we are OK with the kuttl tests running these type of tests, I think we should also check for at least one lightspeed_ specific metric here ? Making sure it's the right endpoint and not just any other prometheus endpoint
There was a problem hiding this comment.
I added a check for the presence of the ls_llm_calls_total metric. 👍
lpiwowar
left a comment
lpiwowar
left a comment
There was a problem hiding this comment.
Thanks for the PR:) 🎉
I just agree with @umago on this one. I'm not sure whether we agreed to implement the tests for the /metrics endpoint in the Kuttl tests while I was gone. If so then I won't fight it. Otherwise, I think they should be added into the test framework created by @malingatembo.
| echo "Response (first 500 chars):" | ||
| echo "$METRICS_OUTPUT" | head -c 500 | ||
| exit 1 | ||
| fi |
There was a problem hiding this comment.
It seems like it fits more into the new test framework than kuttl. ...
issue (blocking): I agree with this. If we did not have the foundation for the test framework laid out by @malingatembo, then I would consider this as an exception, maybe. But since the foundation is laid out, I think we should start adding tests there. It will force us to polish the test framework, and it will make the usage of it much easier later since the relevant tests will already be there and we won't be forced to migrate them there.
|
@jancervenka: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions 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. |
|
/hold We need to agree on whether we want to include the /metrics endpoint test in the Kuttl tests. As of now, the agreement seems to be for the test to be present in @malingatembo's framework. |
3 participants
The kuttl test asserts that the endpoint is reachable and responds with a valid Prometheus format.