Support multi-stage imports in import populator #2767

alromeros · 2023-06-21T17:42:52Z

What this PR does / why we need it:

This Pull Request aims to update the import populator to support multi-stage imports.

The API and functionality remain the same as with DataVolumes, with the only difference being that the used VolumeImportSource will now require a populated TargetClaim field that refers to the specific PVC to be imported (only mandatory in multistage imports).

For more information: https://github.com/kubevirt/containerized-data-importer/blob/main/doc/datavolumes.md#multi-stage-import

Special notes for your reviewer:

Release note:

Support multi-stage imports in import populator

kubevirt-bot · 2023-06-21T17:42:54Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

alromeros · 2023-06-21T17:46:23Z

/test pull-containerized-data-importer-e2e-ceph
/test pull-containerized-data-importer-non-csi-hpp
/test pull-containerized-data-importer-e2e-hpp-latest

alromeros · 2023-06-26T09:42:10Z

/retest-required

mhenriks

looks pretty good! Some minor comments

staging/src/kubevirt.io/containerized-data-importer-api/pkg/apis/core/v1beta1/types.go

pkg/apiserver/webhooks/populators-validate.go

pkg/controller/datavolume/import-controller.go

ShellyKa13

Over all looks good! some comments and need to add some tests and UT

pkg/apiserver/webhooks/populators-validate.go

staging/src/kubevirt.io/containerized-data-importer-api/pkg/apis/core/v1beta1/types.go

pkg/controller/datavolume/import-controller.go

ShellyKa13 · 2023-06-28T07:19:44Z

pkg/controller/populators/import-populator.go

 	case string(corev1.PodFailed):
 		// We'll get called later once it succeeds
 		r.recorder.Eventf(pvc, corev1.EventTypeWarning, importFailed, messageImportFailed, pvc.Name)
 	case string(corev1.PodSucceeded):
+		if _, multiStageInProgress = pvc.Annotations[cc.AnnCurrentCheckpoint]; multiStageInProgress {


maybe put this in a function since you use it twice and it will be cleaner?

After adding the change I think it's a little bit unintuitive to have a function for this (sometimes I want the value, sometimes I don't). If it's not a must I'll leave the annotation as check as it is.

Had to fix a bug involving this annotation so ended up following your suggestion.

pkg/apiserver/webhooks/populators-validate_test.go

tests/import_test.go

pkg/controller/datavolume/import-controller_test.go

This commit modifies the VolumeImportSource API to support multi-stage imports, adding the following fields: - Checkpoints, to represent the stages of a multistage import - TargetClaim, the name of the specific PVC to be imported - FinalCheckpoint, to indicate that the current Checkpoint is the final one Signed-off-by: Alvaro Romero <alromero@redhat.com>

This commit updates the import populator to support multi-stage imports. The API and functionality remains the same as with DataVolumes, with the only difference that the used VolumeImportSource will now require a populated "TargetClaim" field that reffers to the specific PVC to be populated. The DataVolume controller is also updated to allow using the populator flow with VDDK and ImageIO sources. Signed-off-by: Alvaro Romero <alromero@redhat.com>

alromeros · 2023-07-10T10:11:56Z

tests/datavolume_test.go

@@ -1119,7 +1119,7 @@ var _ = Describe("[vendor:cnv-qe@redhat.com][level:component]DataVolume tests",
 				size:             "1Gi",
 				url:              vcenterURL,
 				dvFunc:           createVddkDataVolume,
-				eventReason:      common.AwaitingVDDK,
+				eventReason:      "Pending",


Note for the reviewer: I changed this eventReason to pending since with populators, DataVolumes won't get boundCondition events.

alromeros · 2023-07-11T12:54:23Z

@mhenriks @ShellyKa13 I hopefully just fixed the last major bug here, would appreciate more reviews :)

Signed-off-by: Alvaro Romero <alromero@redhat.com>

This commit fixes several bugs in the import-populator logic for multi-stage imports. Signed-off-by: Alvaro Romero <alromero@redhat.com>

ShellyKa13

looks great! small question just to make sure, giving lgtm anyways if the question is irrelevant :)
/lgtm

ShellyKa13 · 2023-07-11T07:54:40Z

pkg/controller/datavolume/import-controller.go

+	if isMultiStage {
+		importSource.Spec.TargetClaim = &dv.Name
+		importSource.Spec.Checkpoints = dv.Spec.Checkpoints
+		importSource.Spec.FinalCheckpoint = &dv.Spec.FinalCheckpoint


just making sure FinalCheckpoint cant be nil?

Yeah it can be nil, but it's always checked before dereferencing (and, in that case, we assume its value to be false)

alromeros · 2023-07-11T13:30:25Z

/hold

I think everything is fixed but let's see how well tests behave. I've been struggling with some racy behavior.

alromeros · 2023-07-11T16:37:05Z

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

alromeros · 2023-07-12T11:12:58Z

I'm confident this is working
/unhold

mhenriks · 2023-07-12T14:12:36Z

/approve

kubevirt-bot · 2023-07-12T14:12:45Z

[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

kubevirt-bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. dco-signoff: yes Indicates the PR's author has DCO signed all their commits. labels Jun 21, 2023

kubevirt-bot requested review from aglitke and maya-r June 21, 2023 17:42

kubevirt-bot added do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. size/XL labels Jun 21, 2023

alromeros force-pushed the add-multistage-import-support branch from 603e6c4 to 81aa294 Compare June 23, 2023 11:08

alromeros marked this pull request as ready for review June 23, 2023 11:08

kubevirt-bot requested a review from mhenriks June 23, 2023 11:08

alromeros force-pushed the add-multistage-import-support branch 4 times, most recently from 22b0df3 to e0cdedb Compare June 23, 2023 16:29

alromeros force-pushed the add-multistage-import-support branch from e0cdedb to a4225f5 Compare June 26, 2023 11:14

kubevirt-bot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Jun 26, 2023

ShellyKa13 mentioned this pull request Jun 27, 2023

Add documentation for cdi populators #2776

Merged

mhenriks reviewed Jun 28, 2023

View reviewed changes

staging/src/kubevirt.io/containerized-data-importer-api/pkg/apis/core/v1beta1/types.go Outdated Show resolved Hide resolved

pkg/apiserver/webhooks/populators-validate.go Outdated Show resolved Hide resolved

pkg/controller/datavolume/import-controller.go Show resolved Hide resolved

ShellyKa13 reviewed Jun 28, 2023

View reviewed changes

kubevirt-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 30, 2023

alromeros force-pushed the add-multistage-import-support branch from a4225f5 to cde28c5 Compare June 30, 2023 10:04

kubevirt-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 30, 2023

kubevirt-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 10, 2023

alromeros force-pushed the add-multistage-import-support branch from cde28c5 to 83942fe Compare July 10, 2023 10:01

kubevirt-bot added size/XXL and removed size/XL labels Jul 10, 2023

alromeros force-pushed the add-multistage-import-support branch from 83942fe to 442ae5c Compare July 10, 2023 10:05

kubevirt-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 10, 2023

alromeros force-pushed the add-multistage-import-support branch from 442ae5c to 7566617 Compare July 10, 2023 10:07

alromeros commented Jul 10, 2023

View reviewed changes

alromeros force-pushed the add-multistage-import-support branch 2 times, most recently from c0d2b61 to 95c04c9 Compare July 10, 2023 11:43

alromeros changed the title ~~[WIP] Support multi-stage imports in import populator~~ Support multi-stage imports in import populator Jul 10, 2023

kubevirt-bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 10, 2023

alromeros force-pushed the add-multistage-import-support branch 2 times, most recently from 3d7dded to b4fb700 Compare July 11, 2023 12:49

alromeros added 3 commits July 11, 2023 14:58

Add unit tests for multistage import support in populators

9bc3133

Signed-off-by: Alvaro Romero <alromero@redhat.com>

Add functional tests to test multistage import populator flow

9a76e17

Signed-off-by: Alvaro Romero <alromero@redhat.com>

Fix multi-stage import logic in import-populator and add remaining tests

814b25d

This commit fixes several bugs in the import-populator logic for multi-stage imports. Signed-off-by: Alvaro Romero <alromero@redhat.com>

alromeros force-pushed the add-multistage-import-support branch from b4fb700 to 814b25d Compare July 11, 2023 12:59

ShellyKa13 reviewed Jul 11, 2023

View reviewed changes

kubevirt-bot assigned ShellyKa13 Jul 11, 2023

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

kubevirt-bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 11, 2023

kubevirt-bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 12, 2023

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

kubevirt-bot merged commit 9c443b4 into kubevirt:main Jul 12, 2023
16 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support multi-stage imports in import populator #2767

Support multi-stage imports in import populator #2767

alromeros commented Jun 21, 2023 •

edited

Loading

kubevirt-bot commented Jun 21, 2023

alromeros commented Jun 21, 2023

alromeros commented Jun 26, 2023

mhenriks left a comment

ShellyKa13 left a comment

ShellyKa13 Jun 28, 2023

alromeros Jun 28, 2023

alromeros Jul 10, 2023

alromeros Jul 11, 2023

alromeros Jul 10, 2023

alromeros commented Jul 11, 2023

ShellyKa13 left a comment

ShellyKa13 Jul 11, 2023

alromeros Jul 11, 2023

alromeros commented Jul 11, 2023 •

edited

Loading

alromeros commented Jul 11, 2023

alromeros commented Jul 12, 2023

mhenriks commented Jul 12, 2023

kubevirt-bot commented Jul 12, 2023

Support multi-stage imports in import populator #2767

Support multi-stage imports in import populator #2767

Conversation

alromeros commented Jun 21, 2023 • edited Loading

kubevirt-bot commented Jun 21, 2023

alromeros commented Jun 21, 2023

alromeros commented Jun 26, 2023

mhenriks left a comment

Choose a reason for hiding this comment

ShellyKa13 left a comment

Choose a reason for hiding this comment

ShellyKa13 Jun 28, 2023

Choose a reason for hiding this comment

alromeros Jun 28, 2023

Choose a reason for hiding this comment

alromeros Jul 10, 2023

Choose a reason for hiding this comment

alromeros Jul 11, 2023

Choose a reason for hiding this comment

alromeros Jul 10, 2023

Choose a reason for hiding this comment

alromeros commented Jul 11, 2023

ShellyKa13 left a comment

Choose a reason for hiding this comment

ShellyKa13 Jul 11, 2023

Choose a reason for hiding this comment

alromeros Jul 11, 2023

Choose a reason for hiding this comment

alromeros commented Jul 11, 2023 • edited Loading

alromeros commented Jul 11, 2023

alromeros commented Jul 12, 2023

mhenriks commented Jul 12, 2023

kubevirt-bot commented Jul 12, 2023

alromeros commented Jun 21, 2023 •

edited

Loading

alromeros commented Jul 11, 2023 •

edited

Loading