Improve DataVolume status reporting with populators #2928

alromeros · 2023-10-16T09:38:08Z

What this PR does / why we need it:

With the addition of populators into CDI, we introduced a minor divergence in the DataVolume status reporting between the new and the old flow to avoid checking PVC Prime from the legacy controllers.

Though it would be ideal to ignore the PVC Prime in the DV layer completely, there are certain cases where the reported status does not match the current behavior (check https://bugzilla.redhat.com/show_bug.cgi?id=2241637 for an example).

This PR aims to update the status reporting mechanism in import and upload controllers so both the old flow and the populator flow match.

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 # https://bugzilla.redhat.com/show_bug.cgi?id=2241637

Special notes for your reviewer:

It would be ideal to completely ignore the PVC Prime from the DV layer, but I think this solution will help us with all the specific cases where we are reporting a status that doesn't match CDI behavior. Let me know what you think.

Release note:

Bugfix: Improve DataVolume status reporting with populators

alromeros · 2023-10-16T09:43:14Z

/cc @mhenriks
I've been trying to avoid checking the PVC Prime but I think this is a good solution for all the corner cases where we report ImportScheduled or UploadScheduled with an indefinitely pending PVC Prime. Let me know what you think.

alromeros · 2023-10-17T16:12:55Z

I'm wondering how to address the error that's popping up in the tests. There's this specific case where we test a failed import in which the PVC Prime gets bound after the target PVC is updated for the last time. Since we don't trigger reconciles for PVC Prime updates, we don't update the status phase to ImportScheduled. It's just a minor cosmetic divergence from the non-populator flow, but still a bummer since all this PR is to address these divergences. Don't know if there's a way to solve this unless we add a watch to reconcile PVC Primes, which seems overkill for this specific case. Will continue with it tomorrow.

mhenriks · 2023-10-17T20:13:28Z

pkg/controller/datavolume/upload-controller.go

@@ -196,19 +196,37 @@ func isPVCUploadPopulation(pvc *corev1.PersistentVolumeClaim) bool {
 	return populators.IsPVCDataSourceRefKind(pvc, cdiv1.VolumeUploadSourceRef)
 }

+func (r *UploadReconciler) shouldUpdateStatusPhase(pvc *corev1.PersistentVolumeClaim, dv *cdiv1.DataVolume) (bool, error) {


Is this function duplicated in import controller?

There's common code but one function uses isPVCImportPopulation and AnnImportPod and the other the upload equivalent. I can try to have one shared function but I think this practice is somewhat common in our controllers.

alromeros · 2023-10-18T13:20:22Z

pkg/controller/populators/import-populator.go

+		_, ok := pvc.Annotations[cc.AnnPopulatorProgress]
+		// Initialize the progress once PVC Prime is bound
+		if !ok && cc.IsBound(pvcPrime) {
+			cc.AddAnnotation(pvc, cc.AnnPopulatorProgress, "N/A")


This is a nasty patch to force a reconcile in the DV controller when the PVC' gets bound disguised as an annotation to initialize progress. It should fix the error in the tests, and it matches the behavior in the DV controller. After spending hours on this I think this is the best-looking solution.

With the addition of populators into CDI, we introduced a minor divergence in the DV status reporting between the new and the old flow to avoid checking PVC Prime. Though it would be ideal to completely ignore the PVC Prime in the DV layer, this divergence might confuse some users. This commit updates the status reporting mechanism so both the old flow and the populator flow match. Signed-off-by: Alvaro Romero <alromero@redhat.com>

alromeros · 2023-10-20T08:24:04Z

/retest-required

alromeros · 2023-10-20T16:07:05Z

/test pull-containerized-data-importer-e2e-nfs

alromeros · 2023-10-23T07:59:52Z

/test pull-containerized-data-importer-e2e-nfs

mhenriks · 2023-10-30T20:07:48Z

/lgtm
/approve

Thanks @alromeros

kubevirt-bot · 2023-10-30T20:07:56Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mhenriks

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~~ [mhenriks]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

alromeros · 2023-10-31T08:30:40Z

/cherrypick release-v1.57

kubevirt-bot · 2023-10-31T08:31:22Z

@alromeros: #2928 failed to apply on top of branch "release-v1.57":

Applying: Check PVC Prime before updating DV status to improve status reporting
Using index info to reconstruct a base tree...
M	pkg/controller/datavolume/import-controller.go
M	pkg/controller/datavolume/import-controller_test.go
M	pkg/controller/datavolume/upload-controller_test.go
M	pkg/controller/populators/import-populator.go
M	pkg/controller/populators/import-populator_test.go
Falling back to patching base and 3-way merge...
Auto-merging pkg/controller/populators/import-populator_test.go
Auto-merging pkg/controller/populators/import-populator.go
Auto-merging pkg/controller/datavolume/upload-controller_test.go
Auto-merging pkg/controller/datavolume/import-controller_test.go
Auto-merging pkg/controller/datavolume/import-controller.go
CONFLICT (content): Merge conflict in pkg/controller/datavolume/import-controller.go
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 Check PVC Prime before updating DV status to improve status reporting
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

In response to this:

/cherrypick release-v1.57

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/test-infra repository.

akalenyu · 2023-10-31T10:45:51Z

/cherrypick release-v1.58

kubevirt-bot · 2023-10-31T10:46:29Z

@akalenyu: new pull request created: #2953

In response to this:

/cherrypick release-v1.58

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/test-infra repository.

kubevirt-bot added release-note Denotes a PR that will be considered when it comes time to generate release notes. dco-signoff: yes Indicates the PR's author has DCO signed all their commits. size/L labels Oct 16, 2023

alromeros changed the title ~~Check PVC Prime before updating DataVolume status to improve status reporting~~ Improve DataVolume status reporting with populators Oct 16, 2023

kubevirt-bot requested review from maya-r and mhenriks October 16, 2023 09:38

mhenriks reviewed Oct 17, 2023

View reviewed changes

alromeros force-pushed the fix-populator-bug branch 2 times, most recently from 455dee9 to ad7a682 Compare October 18, 2023 13:15

alromeros commented Oct 18, 2023

View reviewed changes

alromeros requested a review from mhenriks October 18, 2023 14:10

alromeros force-pushed the fix-populator-bug branch 2 times, most recently from bba8e23 to 1af88b0 Compare October 19, 2023 09:02

alromeros force-pushed the fix-populator-bug branch from 1af88b0 to adb502e Compare October 19, 2023 12:50

kubevirt-bot assigned mhenriks Oct 30, 2023

kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label Oct 30, 2023

kubevirt-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 30, 2023

kubevirt-bot merged commit 7392a86 into kubevirt:main Oct 31, 2023
18 checks passed

alromeros mentioned this pull request Oct 31, 2023

[release-v1.57] Check PVC Prime before updating DV status to improve status reporting #2951

Merged

kubevirt-bot mentioned this pull request Oct 31, 2023

[release-v1.58] Improve DataVolume status reporting with populators #2953

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve DataVolume status reporting with populators #2928

Improve DataVolume status reporting with populators #2928

alromeros commented Oct 16, 2023

alromeros commented Oct 16, 2023

alromeros commented Oct 17, 2023

mhenriks Oct 17, 2023

alromeros Oct 18, 2023

alromeros Oct 18, 2023

alromeros commented Oct 20, 2023

alromeros commented Oct 20, 2023

alromeros commented Oct 23, 2023

mhenriks commented Oct 30, 2023

kubevirt-bot commented Oct 30, 2023

alromeros commented Oct 31, 2023

kubevirt-bot commented Oct 31, 2023

akalenyu commented Oct 31, 2023

kubevirt-bot commented Oct 31, 2023

Improve DataVolume status reporting with populators #2928

Improve DataVolume status reporting with populators #2928

Conversation

alromeros commented Oct 16, 2023

alromeros commented Oct 16, 2023

alromeros commented Oct 17, 2023

mhenriks Oct 17, 2023

Choose a reason for hiding this comment

alromeros Oct 18, 2023

Choose a reason for hiding this comment

alromeros Oct 18, 2023

Choose a reason for hiding this comment

alromeros commented Oct 20, 2023

alromeros commented Oct 20, 2023

alromeros commented Oct 23, 2023

mhenriks commented Oct 30, 2023

kubevirt-bot commented Oct 30, 2023

alromeros commented Oct 31, 2023

kubevirt-bot commented Oct 31, 2023

akalenyu commented Oct 31, 2023

kubevirt-bot commented Oct 31, 2023