kubevirt / containerized-data-importer

Data Import Service for kubernetes, designed with kubevirt in mind.
Apache License 2.0
395 stars 249 forks source link

Fix progress metric registration and parsing #3292

Closed arnongilboa closed 1 month ago

arnongilboa commented 1 month ago

What this PR does / why we need it: Use default metric registration. We shouldn't use the controller-runtime registration as we have no controller here and it will not register the metric correctly.

Add kubevirt_cdi_import_progress_total metric and use it in the importer instead of kubevirt_cdi_clone_progress_total.

Fix the metric parsing to match their new names. Otherwise the DV progress will not be updated until its 100%.

Regression introduced in #3254

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 metric registration and parsing
arnongilboa commented 1 month ago

/hold

Should improve func test.

arnongilboa commented 1 month ago

/unhold

arnongilboa commented 1 month ago

/retest

coveralls commented 1 month ago

Coverage Status

coverage: 58.929% (+0.03%) from 58.904% when pulling 6660e8ad6538183cc784503c9ec1818b4c174e05 on arnongilboa:fix_clone_progress_metric into dd518e770bfc4131aec497386b2920c4a7d8cec9 on kubevirt:main.

akalenyu commented 1 month ago

Note there are a couple more of these regex spread around. Do we want to maybe unify them?

$ (cd pkg; git grep 'regexp.MustCompile("progress'\\\\)
controller/common/util.go:      importRegExp := regexp.MustCompile("progress\\{ownerUID\\=\"" + args.OwnerUID + "\"\\} (\\d{1,3}\\.?\\d*)")
controller/populators/import-populator.go:      importRegExp := regexp.MustCompile("progress\\{ownerUID\\=\"" + string(pvc.UID) + "\"\\} (\\d{1,3}\\.?\\d*)")
arnongilboa commented 1 month ago

Note there are a couple more of these regex spread around. Do we want to maybe unify them?

$ (cd pkg; git grep 'regexp.MustCompile("progress'\\\\)
controller/common/util.go:      importRegExp := regexp.MustCompile("progress\\{ownerUID\\=\"" + args.OwnerUID + "\"\\} (\\d{1,3}\\.?\\d*)")
controller/populators/import-populator.go:      importRegExp := regexp.MustCompile("progress\\{ownerUID\\=\"" + string(pvc.UID) + "\"\\} (\\d{1,3}\\.?\\d*)")

Good catch, I'll sure unify them.

akalenyu commented 1 month ago

Can I rebase https://github.com/kubevirt/containerized-data-importer/pull/3293 on this?

arnongilboa commented 1 month ago

Can I rebase #3293 on this?

I'm doing a small API refactoring, so wait a few mins.

akalenyu commented 1 month ago

/approve

kubevirt-bot commented 1 month ago

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: akalenyu

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files: - ~~[OWNERS](https://github.com/kubevirt/containerized-data-importer/blob/main/OWNERS)~~ [akalenyu] Approvers can indicate their approval by writing `/approve` in a comment Approvers can cancel approval by writing `/approve cancel` in a comment
ShellyKa13 commented 1 month ago

/lgtm