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

fix: set telemetry config value, patch service, update tests #185

Merged
merged 3 commits into from
Feb 13, 2024

Conversation

DnPlas
Copy link
Contributor

@DnPlas DnPlas commented Feb 7, 2024

This commit ensures the configuration value for the telemetry setting is correctly passed to the workload configuration value. With this we ensure the workload is correctly exposing metrics in the desired endpoint so they can be scraped by prometheus. With this change we also must ensure that the K8s Service correctly exposes the ports of the endpoints that this workload has (for metrics and the actual dex service). Finally, a bit of refactoring work had to be made in the unit test to match the recent changes and the kubernetes_service_patch library this charm uses has been bumped v0 -> v1.

Part of canonical/bundle-kubeflow#563

Testing

Please refer to the steps to reproduce in this comment, just deploying this app. After deploying this app and cos-lite, relations and dependencies, and waiting a couple minutes (10 min) none of the alerts should fire (for this app only).

TODO

This commit ensures the configuration value for the telemetry setting is correctly passed to the
workload configuration value. With this we ensure the workload is correctly exposing metrics in the
desired endpoint so they can be scraped by prometheus.
With this change we also must ensure that the K8s Service correctly exposes the ports of the endpoints
that this workload has (for metrics and the actual dex service).
Finally, a bit of refactoring work had to be made in the unit test to match the recent changes and the
kubernetes_service_patch library this charm uses has been bumped v0 -> v1.

Part of canonical/bundle-kubeflow#563
The test_prometheus_grafana_integration test case was doing queries to prometheus
and checking the request returned successfully and that the application name and model
was listed correctly. To make this test case more accurately, we can add an assertion that
also checks that the unit is available, this way we avoid issues like the one described in
canonical/bundle-kubeflow#564.

Part of canonical/bundle-kubeflow#564
Copy link
Contributor

@orfeas-k orfeas-k left a comment

Choose a reason for hiding this comment

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

Thank you @DnPlas, left a minor question. I 'll soon be testing the deployment and if everything is fine, I 'll aprove

tests/unit/test_charm.py Show resolved Hide resolved
Copy link
Contributor

@orfeas-k orfeas-k left a comment

Choose a reason for hiding this comment

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

Thank you Daniela

@DnPlas DnPlas merged commit e449575 into main Feb 13, 2024
7 checks passed
@DnPlas DnPlas deleted the KF-1647-fix-alerts-firing branch February 13, 2024 12:36
DnPlas added a commit that referenced this pull request Feb 13, 2024
* fix: set telemetry config value, patch service, update tests

This commit ensures the configuration value for the telemetry setting is correctly passed to the
workload configuration value. With this we ensure the workload is correctly exposing metrics in the
desired endpoint so they can be scraped by prometheus.
With this change we also must ensure that the K8s Service correctly exposes the ports of the endpoints
that this workload has (for metrics and the actual dex service).
Finally, a bit of refactoring work had to be made in the unit test to match the recent changes and the
kubernetes_service_patch library this charm uses has been bumped v0 -> v1.

Part of canonical/bundle-kubeflow#563

* tests: add an assertion for checking unit is available

The test_prometheus_grafana_integration test case was doing queries to prometheus
and checking the request returned successfully and that the application name and model
was listed correctly. To make this test case more accurately, we can add an assertion that
also checks that the unit is available, this way we avoid issues like the one described in
canonical/bundle-kubeflow#564.

Part of canonical/bundle-kubeflow#564
DnPlas added a commit that referenced this pull request Feb 13, 2024
…186)

* fix: set telemetry config value, patch service, update tests

This commit ensures the configuration value for the telemetry setting is correctly passed to the
workload configuration value. With this we ensure the workload is correctly exposing metrics in the
desired endpoint so they can be scraped by prometheus.
With this change we also must ensure that the K8s Service correctly exposes the ports of the endpoints
that this workload has (for metrics and the actual dex service).
Finally, a bit of refactoring work had to be made in the unit test to match the recent changes and the
kubernetes_service_patch library this charm uses has been bumped v0 -> v1.

Part of canonical/bundle-kubeflow#563

* tests: add an assertion for checking unit is available

The test_prometheus_grafana_integration test case was doing queries to prometheus
and checking the request returned successfully and that the application name and model
was listed correctly. To make this test case more accurately, we can add an assertion that
also checks that the unit is available, this way we avoid issues like the one described in
canonical/bundle-kubeflow#564.

Part of canonical/bundle-kubeflow#564
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants