Allow ImmediateBinding annotation when using populators #2765

ShellyKa13 · 2023-06-21T13:41:19Z

In case of using PVC with populators if the PVC has this annotation we prevent from waiting for it to be schedueled and we proceed with the process.
When using datavolumes with populators in case the dv has the annotation it will be passed to the PVC. we prevent from being in pendingPopulation in case the created pvc has the annotaion.
Plus when having honorWaitForFirstConsumer feature gate disabled we will put on the target PVC the immediateBinding annotation. Now we allow to use populators when having the annotation the the feature gate disabled.

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:
Integration of clone datavolumes with populators is in a PR: #2750
once merged will need to add to the datavolume clone controller the ImmediateBinding annotation in case of honorWaitForFirstConsumer feature gate disabled

Release note:

Allow ImmediateBinding annotation when using populators

mhenriks · 2023-06-22T00:21:25Z

pkg/controller/datavolume/upload-controller.go

+	}
+	if !globalHonorWaitForFirstConsumer {
+		cc.AddAnnotation(pvc, cc.AnnImmediateBinding, "")
+	}


Maybe put this code in a function that can be shared all the other controllers?

alromeros

The code looks good to me, do you plan to add more functional tests?

mhenriks · 2023-06-23T14:19:14Z

tests/datavolume_test.go

+				expectedPVCPhase = v1.ClaimPending
+			}
+			By(fmt.Sprintf("waiting for pvc to match phase %s", string(expectedPVCPhase)))
+			err = utils.WaitForPersistentVolumeClaimPhase(f.K8sClient, pvc.Namespace, expectedPVCPhase, pvc.Name)


I get that these existing DataVolume tests are implicitly testing "immediate bind on populators". May be helpful to have a couple explicit tests as well

ShellyKa13 · 2023-06-26T10:51:30Z

/hold
waiting to PR #2750 to be merged to add the clone controller part and test
@mhenriks @alromeros added functional test for import and upload for populators with pvc only, any other test you expected?

mhenriks · 2023-06-28T00:19:02Z

/approve

kubevirt-bot · 2023-06-28T00:19:10Z

[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

awels · 2023-06-30T12:36:04Z

/retest-required
/hold cancel
required PR was merged.

awels · 2023-06-30T12:45:16Z

/hold
Oops missed the we need some extra tests. putting hold back

In case of using PVC with populators if the PVC has this annotation we prevent from waiting for it to be schedueled and we proceed with the process. When using datavolumes with populators in case the dv has the annotation it will be passed to the PVC. we prevent from being in pendingPopulation in case the created pvc has the annotaion. Plus when having honorWaitForFirstConsumer feature gate disabled we will put on the target PVC the immediateBind annotation. Now we allow to use populators when having the annotation the the feature gate disabled. Signed-off-by: Shelly Kagan <skagan@redhat.com>

Signed-off-by: Shelly Kagan <skagan@redhat.com>

ShellyKa13 · 2023-07-03T14:44:35Z

/unhold
required PR was merged, added relevant code to clone populator and datavolume clone, added required tests

ShellyKa13 · 2023-07-05T07:51:18Z

/retest

mhenriks · 2023-07-05T13:55:27Z

pkg/controller/clone/planner.go

+	if targetStorageClass.VolumeBindingMode != nil &&
+		*targetStorageClass.VolumeBindingMode == storagev1.VolumeBindingWaitForFirstConsumer &&
+		cc.ImmediateBindingRequested(args.TargetClaim) {
+		args.Log.V(3).Info("Immediate binding requested, need to fall back to host assisted")


This should not be necessary. The prep phase can bind pvc' to a random node.

you mean leave without the fallback and create the pvc' without waiting for selected node and it will just get bound with the pod of the clone? or need to implement something for that?

Yeah shouldn't have to make any changes to the planner

Signed-off-by: Shelly Kagan <skagan@redhat.com>

mhenriks · 2023-07-06T19:10:57Z

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

mhenriks · 2023-07-06T19:11:49Z

/lgtm

mhenriks · 2023-07-06T19:41:09Z

/test pull-cdi-apidocs

alromeros · 2023-07-07T08:20:52Z

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

awels · 2023-07-07T13:29:24Z

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

awels · 2023-07-07T17:03:28Z

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

awels · 2023-07-07T20:15:37Z

I think this might be a real failure, if you look at the 1_dv logs file, you will see the DV in state pending while we are waiting for the WFFC or population pending state.

akalenyu · 2023-07-09T08:50:29Z

I think this might be a real failure, if you look at the 1_dv logs file, you will see the DV in state pending while we are waiting for the WFFC or population pending state.

Yeah I think this is the same one I am handling in #2784

akalenyu · 2023-07-09T08:55:42Z

Note the istio failure is exclusive to this PR:
https://search.ci.kubevirt.io/?search=6498&maxAge=336h&context=1&type=bug%2Bissue%2Bjunit&name=containerized-data-importer&excludeName=&maxMatches=5&maxBytes=20971520&groupBy=job

This annotations are used for the import/upload/clone pods to define netork configurations. Signed-off-by: Shelly Kagan <skagan@redhat.com>

awels · 2023-07-09T23:04:47Z

/lgtm

awels · 2023-07-09T23:05:43Z

/hold

ShellyKa13 · 2023-07-10T07:26:48Z

/cherrypick release-v1.57

kubevirt-bot · 2023-07-10T07:27:42Z

@ShellyKa13: new pull request created: #2793

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.

awels · 2023-07-10T11:49:20Z

oh I was too slow with the hold, I thought there were some questions remaining

ShellyKa13 · 2023-07-10T11:55:46Z

@awels yeah :) never mind, I did the cherry pick so if we decide that its ok we can get the backport in faster and do the release.
But we will talk in grooming and see if we should create a fixup PR

I find myself going back a lot to this function and I noticed some of the fallback reasons are outdated following: kubevirt#2765 kubevirt#2873 Signed-off-by: Alex Kalenyuk <akalenyu@redhat.com>

I find myself going back a lot to this function and I noticed some of the fallback reasons are outdated following: #2765 #2873 Signed-off-by: Alex Kalenyuk <akalenyu@redhat.com>

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. labels Jun 21, 2023

kubevirt-bot requested review from awels and maya-r June 21, 2023 13:41

kubevirt-bot added the size/M label Jun 21, 2023

mhenriks reviewed Jun 22, 2023

View reviewed changes

ShellyKa13 force-pushed the populators-immediate-bind branch from 4a30a96 to 88f1bda Compare June 22, 2023 08:59

alromeros reviewed Jun 22, 2023

View reviewed changes

mhenriks reviewed Jun 23, 2023

View reviewed changes

kubevirt-bot added size/L and removed size/M labels Jun 25, 2023

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

ShellyKa13 mentioned this pull request Jun 27, 2023

Add documentation for cdi populators #2776

Merged

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

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

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

ShellyKa13 added 2 commits July 2, 2023 12:04

Add functional tests to population using PVCs

1888be4

Signed-off-by: Shelly Kagan <skagan@redhat.com>

ShellyKa13 force-pushed the populators-immediate-bind branch 2 times, most recently from 8fde694 to 1d4d715 Compare July 3, 2023 14:43

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

mhenriks reviewed Jul 5, 2023

View reviewed changes

Support immediate binding with clone datavolume

875dee0

Signed-off-by: Shelly Kagan <skagan@redhat.com>

kubevirt-bot assigned mhenriks Jul 6, 2023

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

kubevirt-bot assigned alromeros Jul 7, 2023

Pass allowed annotations from target pvc to pvc prime

c5ae705

This annotations are used for the import/upload/clone pods to define netork configurations. Signed-off-by: Shelly Kagan <skagan@redhat.com>

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

kubevirt-bot assigned awels Jul 9, 2023

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

kubevirt-bot merged commit 533a725 into kubevirt:main Jul 9, 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 9, 2023

kubevirt-bot mentioned this pull request Jul 10, 2023

[release-v1.57] Allow ImmediateBinding annotation when using populators #2793

Merged

akalenyu mentioned this pull request Sep 21, 2023

Update doc of shouldUseCDIPopulator to reflect changes #2904

Merged

akalenyu mentioned this pull request Dec 21, 2023

Stop skipping advanced clone method tests with immediate bind annotation #3039

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow ImmediateBinding annotation when using populators #2765

Allow ImmediateBinding annotation when using populators #2765

ShellyKa13 commented Jun 21, 2023

mhenriks Jun 22, 2023

alromeros left a comment

mhenriks Jun 23, 2023

ShellyKa13 commented Jun 26, 2023

mhenriks commented Jun 28, 2023

kubevirt-bot commented Jun 28, 2023

awels commented Jun 30, 2023

awels commented Jun 30, 2023

ShellyKa13 commented Jul 3, 2023

ShellyKa13 commented Jul 5, 2023

mhenriks Jul 5, 2023

ShellyKa13 Jul 6, 2023

mhenriks Jul 6, 2023

ShellyKa13 Jul 6, 2023

mhenriks commented Jul 6, 2023

mhenriks commented Jul 6, 2023

mhenriks commented Jul 6, 2023

alromeros commented Jul 7, 2023

awels commented Jul 7, 2023

awels commented Jul 7, 2023

awels commented Jul 7, 2023

akalenyu commented Jul 9, 2023

akalenyu commented Jul 9, 2023

awels commented Jul 9, 2023

awels commented Jul 9, 2023

ShellyKa13 commented Jul 10, 2023

kubevirt-bot commented Jul 10, 2023

awels commented Jul 10, 2023

ShellyKa13 commented Jul 10, 2023

Allow ImmediateBinding annotation when using populators #2765

Allow ImmediateBinding annotation when using populators #2765

Conversation

ShellyKa13 commented Jun 21, 2023

mhenriks Jun 22, 2023

Choose a reason for hiding this comment

alromeros left a comment

Choose a reason for hiding this comment

mhenriks Jun 23, 2023

Choose a reason for hiding this comment

ShellyKa13 commented Jun 26, 2023

mhenriks commented Jun 28, 2023

kubevirt-bot commented Jun 28, 2023

awels commented Jun 30, 2023

awels commented Jun 30, 2023

ShellyKa13 commented Jul 3, 2023

ShellyKa13 commented Jul 5, 2023

mhenriks Jul 5, 2023

Choose a reason for hiding this comment

ShellyKa13 Jul 6, 2023

Choose a reason for hiding this comment

mhenriks Jul 6, 2023

Choose a reason for hiding this comment

ShellyKa13 Jul 6, 2023

Choose a reason for hiding this comment

mhenriks commented Jul 6, 2023

mhenriks commented Jul 6, 2023

mhenriks commented Jul 6, 2023

alromeros commented Jul 7, 2023

awels commented Jul 7, 2023

awels commented Jul 7, 2023

awels commented Jul 7, 2023

akalenyu commented Jul 9, 2023

akalenyu commented Jul 9, 2023

awels commented Jul 9, 2023

awels commented Jul 9, 2023

ShellyKa13 commented Jul 10, 2023

kubevirt-bot commented Jul 10, 2023

awels commented Jul 10, 2023

ShellyKa13 commented Jul 10, 2023