Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Preliminary support for DataVolumes using populators internally #9812

Merged
merged 2 commits into from
Jun 9, 2023
Merged

Preliminary support for DataVolumes using populators internally #9812

merged 2 commits into from
Jun 9, 2023

Conversation

mhenriks
Copy link
Member

@mhenriks mhenriks commented May 27, 2023 •
edited

What this PR does / why we need it:

Very soon, DataVolumes will use Volume Populators[1] internally. But before that happens, we have to merge this PR. Otherwise, if KubeVirt consumes a version of CDI with populator support, WaitForFirstConsumer PVCs will be broken.

Basically, KubeVirt has to know that it is okay to start VMs with DataVolumes in PendingPopulation phase. Nice thing about PendingPopulation is that we avoid having to create "doppelganger" virt-launcher pods simply to bind WFFC PVCs. See #4025

[1] https://kubernetes.io/blog/2022/05/16/volume-populators-beta/

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:

Once CDI releases with populator support, there will be another PR here with functional tests

Release note:

Handle DataVolume PendingPopulation phase

@kubevirt-bot 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/XXL labels May 27, 2023
@mhenriks
Copy link
Member Author

/retest-required

ShellyKa13
ShellyKa13 reviewed May 31, 2023
View reviewed changes
Copy link
Contributor

@ShellyKa13 ShellyKa13 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Over all looks good, a few questions and a suggestion

pkg/storage/snapshot/restore.go Outdated
waitingDV = waitingDV ||
(dv.Status.Phase != v1beta1.Succeeded &&
dv.Status.Phase != v1beta1.WaitForFirstConsumer &&
dv.Status.Phase != v1beta1.PendingPopulation)
continue
}
if createdDV, err = t.createDataVolume(dvt); err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good catch by adding "waitingDV = waitingDV ||" dont we need to do in "createdDV = createdDV || result of t.createDataVolume(dvt)" by the same logic?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah this code is pretty wonky, can also remove this unnecessary statement at the end of the loop:

...
		}
		if !createdDV {
			continue
		}
	}

pkg/storage/types/dv.go
if dv.Status.Phase != cdiv1.Succeeded {
isBound := dvConditions.HasConditionWithStatus(dv, cdiv1.DataVolumeBound, v1.ConditionTrue)
// WFFC + plus unbound is not provisioning
if isBound || dv.Status.Phase != cdiv1.WaitForFirstConsumer {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe then just add dv.Status.Phase == cdiv1.WaitForFirstConsumer to the line where you check succeeded and pendingPopulation, if its WFFC it can be bound anyways right?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We want WFFC and bound to be "provisioning" so cannot add WFFC to prev check

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how can it be bound and WFFC?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Small time between doppelganger pod created/running and DV phase is updated

pkg/virt-controller/watch/vm_test.go
@@ -320,6 +320,11 @@ var _ = Describe("VirtualMachine", func() {
createCount := 0
shouldExpectDataVolumeCreation(vm.UID, map[string]string{"kubevirt.io/created-by": string(vm.UID), "my": "label"}, map[string]string{"my": "annotation"}, &createCount)

vmInterface.EXPECT().UpdateStatus(context.Background(), gomock.Any()).Times(1).Do(func(ctx context.Context, obj interface{}) {
objVM := obj.(*virtv1.VirtualMachine)
Expect(objVM.Status.PrintableStatus).To(Equal(virtv1.VirtualMachineStatusProvisioning))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder why now this test returns that the dv is provisioning and before it didnt

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Before it wasn't calling updateStatus at all because unset dv phase and unbound condition was not considered provisioning. I think that's incorrect.

pkg/virt-controller/watch/vm_test.go
@@ -2997,11 +3003,16 @@ var _ = Describe("VirtualMachine", func() {
Expect(objVM.Status.PrintableStatus).To(Equal(status))
})

if running {
vmiInterface.EXPECT().Create(context.Background(), gomock.Any()).Return(vmi, nil)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder why before create call was not expected

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

because we were not setting the dv phase before

ShellyKa13
ShellyKa13 reviewed Jun 1, 2023
View reviewed changes
Copy link
Contributor

@ShellyKa13 ShellyKa13 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice
/lgtm

@kubevirt-bot kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label Jun 1, 2023
@mhenriks
Preliminary support for DataVolumes using populators internally
7e52234 
Signed-off-by: Michael Henriksen <mhenriks@redhat.com>
@mhenriks
further cleanup of restore DV handling
9ad2454 
Signed-off-by: Michael Henriksen <mhenriks@redhat.com>
@kubevirt-bot kubevirt-bot removed the lgtm Indicates that a PR is ready to be merged. label Jun 5, 2023
@mhenriks
Copy link
Member Author

mhenriks commented Jun 6, 2023

/retest-required

1 similar comment
@mhenriks
Copy link
Member Author

mhenriks commented Jun 6, 2023

/retest-required

@ShellyKa13
Copy link
Contributor

/lgtm

@kubevirt-bot kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label Jun 6, 2023
@ShellyKa13 ShellyKa13 mentioned this pull request Jun 8, 2023
Merged
@alicefr
Copy link
Member

alicefr commented Jun 9, 2023

Looks great!
/approve

@kubevirt-bot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alicefr

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:

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

@kubevirt-bot kubevirt-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 9, 2023
@kubevirt-bot kubevirt-bot merged commit 251b1a4 into kubevirt:main Jun 9, 2023
35 checks passed
@kubevirt-bot kubevirt-bot mentioned this pull request Jul 26, 2023
Open
@mhenriks mhenriks mentioned this pull request Aug 24, 2023
Open
@akalenyu akalenyu mentioned this pull request Oct 4, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ShellyKa13 ShellyKa13 ShellyKa13 left review comments

@alicefr alicefr Awaiting requested review from alicefr

@vladikr vladikr Awaiting requested review from vladikr

Assignees

@ShellyKa13 ShellyKa13

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. dco-signoff: yes Indicates the PR's author has DCO signed all their commits. lgtm Indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/XXL
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@mhenriks @ShellyKa13 @alicefr @kubevirt-bot