Respect wffc override for blank block disks #2917

akalenyu · 2023-10-04T11:26:37Z

What this PR does / why we need it:
We currently don't support the wffc override for blank block disks,
while there may be some use cases where that is desired.

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 #2915

Special notes for your reviewer:

Release note:

BugFix: wffc override not respected for blank block disks

alromeros · 2023-10-04T12:12:55Z

tests/import_test.go

+		Entry("[test_id:4972] with first consumer", true),
+		Entry("with bind immediate annotation", false),
+	)
+	It("Should create blank raw image for block PV", func() {


Code leftover, looks good otherwise. Thanks for the PR!

awels · 2023-10-04T12:35:01Z

pkg/controller/import-controller.go

-		pvc.GetAnnotations()[cc.AnnPodPhase] = string(corev1.PodSucceeded)
-		if err := r.updatePVC(pvc, log); err != nil {
-			return reconcile.Result{}, errors.WithMessage(err, fmt.Sprintf("could not update pvc %q annotation and/or label", pvc.Name))
+		if pvc.Status.Phase == corev1.ClaimBound {


So I am not sure about this. We purposely mark the PVC succeeded in the blank block case regardless of if it is bound or not. This is so that KubeVirt won't think it can't use the PVC. Once we have a consumer kubernetes will bind the PVC and everything resolves itself.

This was pre-populator logic. Now I don't really see how populators would affect blank block volumes at all, since there is no population. Maybe I am missing something.

This is so that KubeVirt won't think it can't use the PVC

KubeVirt should be fine with WFFC Pending PVCs/DV:
kubevirt/kubevirt#9812

Right, so I am trying to understand why we want this particular check here. I don't think we even want to use populators, unless maybe pre-allocation is set to true.

The problem is if we stop using populators with this flow, we're back at the doppelganger pod solution

For the blank volume without pre-allocation case, there is no need for the doppel ganger. It is 'populated' already considering there is nothing we have to do. It just gets bound when the VM starts, but it is still exactly the way we want it. With pre-allocation, I would expect our populator to attempt the pr-eallocation at the appropriate time.

For the blank volume without pre-allocation case, there is no need for the doppel ganger

Yeah but kubevirt is not aware of that, it starts it anyway

But generally, I see your points, plus the test I added is broken because immediate.bind with blank block disk
doesn't do anything. Have to take another look

In my case, the PVC is not in bound state. It is in WFFC.
If the issue is because I didn't enable HonorWaitForFirstConsumer feature gate, is there any side effect or behavior change of enabling this feature gate?

So we've been going with this feature for quite a few releases now,
it basically means we respect WFFC binding by not creating any worker pods until PVCs get scheduled.

Is there any particular use case you have that would require bypassing WFFC behavior,
and bind PVCs randomly? Or will these blank disks be consumed by a VM?

akalenyu · 2023-10-04T15:01:48Z

/hold

Taking a step back. This works with HonorWaitForFirstConsumer enabled creating a bind pod.
The only thing that is broken is using bind.immediate with block blank DVs (because no consumer will be created)
Maybe we should just document this? or should we create a dummy bind pod for that case?

akalenyu · 2023-10-09T17:07:54Z

/hold cancel

akalenyu · 2023-10-09T17:08:05Z

@lzang @awels PTAL

We currently don't support the wffc override for blank block disks, while there may be some use cases where that is desired. Signed-off-by: Alex Kalenyuk <akalenyu@redhat.com>

awels · 2023-10-11T15:31:18Z

/lgtm

mhenriks · 2023-10-11T15:36:41Z

/approve

kubevirt-bot · 2023-10-11T15:37:04Z

[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

lzang · 2023-10-11T19:40:16Z

pkg/controller/import-controller.go

-			return reconcile.Result{}, errors.WithMessage(err, fmt.Sprintf("could not update pvc %q annotation and/or label", pvc.Name))
-		}
-		return reconcile.Result{}, nil
-	}


I don't get why this is not needed any more from looking at the CL. Could you please clarify why?
Does this logic work for both populator case and non-populator case?

Essentially this was a short cut for one particular case (blank disk, no preallocation) and removing it will cause a pod to be created, but per the rest of this PR, that pod won't do anything. What it will do is follow the rest of the logic when creating a population pod, which should work for both populator and non populator cases, as well as HonorWaitForFirstConsumer set or not.

akalenyu · 2024-04-08T12:54:33Z

/cherrypick release-v1.57

kubevirt-bot · 2024-04-08T12:55:54Z

@akalenyu: new pull request created: #3170

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.

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 Oct 4, 2023

kubevirt-bot requested review from awels and maya-r October 4, 2023 11:26

kubevirt-bot added the size/M label Oct 4, 2023

akalenyu force-pushed the blank-wffc-import-false-succeeded branch from f7839c4 to 623248e Compare October 4, 2023 11:51

kubevirt-bot added size/S and removed size/M labels Oct 4, 2023

alromeros reviewed Oct 4, 2023

View reviewed changes

akalenyu force-pushed the blank-wffc-import-false-succeeded branch from 623248e to 9dc43b2 Compare October 4, 2023 12:16

awels reviewed Oct 4, 2023

View reviewed changes

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

akalenyu force-pushed the blank-wffc-import-false-succeeded branch from 9dc43b2 to e6e318e Compare October 9, 2023 17:06

kubevirt-bot added size/M and removed size/S labels Oct 9, 2023

akalenyu changed the title ~~Report succeeded phase for block blank import more accurately~~ Respect wffc override for blank block disks Oct 9, 2023

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

Respect wffc override for blank block disks

84500d0

We currently don't support the wffc override for blank block disks, while there may be some use cases where that is desired. Signed-off-by: Alex Kalenyuk <akalenyu@redhat.com>

akalenyu force-pushed the blank-wffc-import-false-succeeded branch from e6e318e to 84500d0 Compare October 10, 2023 09:35

kubevirt-bot assigned awels Oct 11, 2023

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

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

lzang reviewed Oct 11, 2023

View reviewed changes

kubevirt-bot merged commit 02af1da into kubevirt:main Oct 11, 2023
18 checks passed

kubevirt-bot mentioned this pull request Apr 8, 2024

[release-v1.57] Respect wffc override for blank block disks #3170

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Respect wffc override for blank block disks #2917

Respect wffc override for blank block disks #2917

akalenyu commented Oct 4, 2023 •

edited

Loading

alromeros Oct 4, 2023

akalenyu Oct 4, 2023

awels Oct 4, 2023

akalenyu Oct 4, 2023

awels Oct 4, 2023

akalenyu Oct 4, 2023

awels Oct 4, 2023

akalenyu Oct 4, 2023 •

edited

Loading

lzang Oct 4, 2023

akalenyu Oct 5, 2023

akalenyu commented Oct 4, 2023 •

edited

Loading

akalenyu commented Oct 9, 2023

akalenyu commented Oct 9, 2023

awels commented Oct 11, 2023

mhenriks commented Oct 11, 2023

kubevirt-bot commented Oct 11, 2023

lzang Oct 11, 2023

awels Oct 12, 2023

akalenyu commented Apr 8, 2024

kubevirt-bot commented Apr 8, 2024

Respect wffc override for blank block disks #2917

Respect wffc override for blank block disks #2917

Conversation

akalenyu commented Oct 4, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akalenyu Oct 4, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akalenyu commented Oct 4, 2023 • edited Loading

akalenyu commented Oct 9, 2023

akalenyu commented Oct 9, 2023

awels commented Oct 11, 2023

mhenriks commented Oct 11, 2023

kubevirt-bot commented Oct 11, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akalenyu commented Apr 8, 2024

kubevirt-bot commented Apr 8, 2024

akalenyu commented Oct 4, 2023 •

edited

Loading

akalenyu Oct 4, 2023 •

edited

Loading

akalenyu commented Oct 4, 2023 •

edited

Loading