containerized-data-importer
containerized-data-importer copied to clipboard
kubevirt
forklift: fix metrics report
-
Use default implementation for metric registration, otherwise it was not registered and the metrics weren't reported
-
Rename kubevirt_cdi_ovirt_progress_total to kubevirt_cdi_ovirt_populator_progress_total for consistency
What this PR does / why we need it:
Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #
Special notes for your reviewer:
Release note:
Fix progress reporting for forklift populators
Hi @bennyz. Thanks for your PR.
PRs from untrusted users cannot be marked as trusted with /ok-to-test in this repo meaning untrusted PR authors can never trigger tests themselves. Collaborators can still trigger tests on the PR using /test all.
I understand the commands that are listed here.
Instructions 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.
[APPROVALNOTIFIER] This PR is NOT APPROVED
This pull-request has been approved by: Once this PR has been reviewed and has the lgtm label, please assign mhenriks for approval. For more information see the Kubernetes Code Review Process.
The full list of commands accepted by this bot can be found here.
Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment
This needs to be a well though decision. CDI leverages controller-runtime to create and manage the endpoint to expose metrics to Prometheus, if we decide to stop using the controller runtime registry we need to have 2 things in consideration:
-
We need to add a handler to serve the metrics from the Prometheus registry, such as in https://github.com/kubevirt/kubevirt/blob/d42feddf52dcdb0e1c9a5eed6633eb229a10d553/pkg/virt-controller/watch/application.go#L496
-
We would lose some
controller-runtime"free" metrics: https://github.com/kubernetes-sigs/controller-runtime/blob/main/pkg/internal/controller/metrics/metrics.go
@bennyz why is this change needed to fix metrics report?
/hold
This needs to be a well though decision. CDI leverages
controller-runtimeto create and manage the endpoint to expose metrics to Prometheus, if we decide to stop using the controller runtime registry we need to have 2 things in consideration:
- We need to add a handler to serve the metrics from the Prometheus registry, such as in kubevirt/kubevirt@
d42fedd/pkg/virt-controller/watch/application.go#L496- We would lose some
controller-runtime"free" metrics: kubernetes-sigs/controller-runtime@main/pkg/internal/controller/metrics/metrics.go@bennyz why is this change needed to fix metrics report?
/hold
The problem is that without this change our custom metric was simply not reported and it seemed like it wasn't registered at all... this a workaround we arrived at with @arnongilboa
I'm open to any suggestions
I was taking a look, and these are 2 separate executables and I see that CDI has a custom implementation to start a Prometheus handler in each, https://github.com/kubevirt/containerized-data-importer/blob/dd07461d2b82942249424b0b2c65f5632e84c583/cmd/ovirt-populator/ovirt-populator.go#L58 and https://github.com/kubevirt/containerized-data-importer/blob/dd07461d2b82942249424b0b2c65f5632e84c583/cmd/openstack-populator/openstack-populator.go#L104
this should be fine, but can you please add an e2e test to ensure the metrics are scrappable?
/unhold
This needs to be a well though decision. CDI leverages
controller-runtimeto create and manage the endpoint to expose metrics to Prometheus, if we decide to stop using the controller runtime registry we need to have 2 things in consideration:
- We need to add a handler to serve the metrics from the Prometheus registry, such as in https://github.com/kubevirt/kubevirt/blob/d42feddf52dcdb0e1c9a5eed6633eb229a10d553/pkg/virt-controller/watch/application.go#L496
- We would lose some
controller-runtime"free" metrics: https://github.com/kubernetes-sigs/controller-runtime/blob/main/pkg/internal/controller/metrics/metrics.go@bennyz why is this change needed to fix metrics report?
/hold
These apps are not controller- runtime based at all (they're not controllers), they're just ephemeral containers that know how to populate data from ovirt/openstack
coverage: 58.482% (+0.007%) from 58.475% when pulling 1fdbacf06d33b3099436ad96bbd5b549b11d0527 on bennyz:forklift-fix-metrics into dd07461d2b82942249424b0b2c65f5632e84c583 on kubevirt:main.
@bennyz: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:
| Test name | Commit | Details | Required | Rerun command |
|---|---|---|---|---|
| pull-containerized-data-importer-e2e-destructive | 1fdbacf06d33b3099436ad96bbd5b549b11d0527 | link | true | /test pull-containerized-data-importer-e2e-destructive |
| pull-containerized-data-importer-e2e-istio | 1fdbacf06d33b3099436ad96bbd5b549b11d0527 | link | true | /test pull-containerized-data-importer-e2e-istio |
| pull-cdi-verify-go-mod | 1fdbacf06d33b3099436ad96bbd5b549b11d0527 | link | false | /test pull-cdi-verify-go-mod |
| pull-cdi-generate-verify | 1fdbacf06d33b3099436ad96bbd5b549b11d0527 | link | false | /test pull-cdi-generate-verify |
| pull-containerized-data-importer-e2e-upg | 1fdbacf06d33b3099436ad96bbd5b549b11d0527 | link | true | /test pull-containerized-data-importer-e2e-upg |
Instructions 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.
PR needs rebase.
Instructions 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.
Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.
If this issue is safe to close now please do so with /close.
/lifecycle stale
Stale issues rot after 30d of inactivity.
Mark the issue as fresh with /remove-lifecycle rotten.
Rotten issues close after an additional 30d of inactivity.
If this issue is safe to close now please do so with /close.
/lifecycle rotten
Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.
/close
@kubevirt-bot: Closed this PR.
In response to this:
Rotten issues close after 30d of inactivity. Reopen the issue with
/reopen. Mark the issue as fresh with/remove-lifecycle rotten./close
Instructions 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.