PVC Clone Populator #2709

mhenriks · 2023-05-09T03:47:39Z

What this PR does / why we need it:

Overview

VolumeCloneSource populator for PersistentVolumeClaim sources. VolumeSnapshot coming soon.

The populator will do pretty much exactly the same work as a DataVolume with a PVC source. Efficient clone with snapshots if possible. CSI clone if specifically configured. Host "copy" clone otherwise. HOWEVER we can now do WaitForFirstConsumer snapshot/csi clones which is a nice new feature brought to you by the glory of populators.

API

apiVersion: cdi.kubevirt.io/v1beta1
kind: VolumeCloneSource
metadata:
  name: my-clone-source
spec:
  source:
    kind: PersistentVolumeClaim
    name: my-pvc-source
---
apiVersion: v1
kind: PersistentVolumeClaim
metadata:
  name:  my-cloned-pvc
spec:
  dataSourceRef:
    apiGroup: cdi.kubevirt.io
    kind: VolumeCloneSource
    name: my-clone-source
  accessModes:
  - ReadWriteOnce
  resources:
    requests:
      storage: 10Gi

Implementation Details

Reviewers, focus your attention here: https://github.com/mhenriks/containerized-data-importer/tree/clone-pop/pkg/controller/clone

And here: https://github.com/mhenriks/containerized-data-importer/blob/clone-pop/pkg/controller/populators/clone-populator.go

I attempted to break the clone operation into two processes, planning and execution. The entrypoint for those processes is the Planner. ChooseStrategy evaluates the source, target, and chooses the best strategy (snapshot, csi-clone, or copy).

Plan takes a given source, target, strategy and returns a chain of Phases. A Phase is typically responsible for doing a single operation like snapshot a PVC. When a Phase is complete, it returns nil, nil and the caller (clone populator) will execute the next phase in the chain. When all phases execute successfully, the controller will wait for the target PVC to be bound then clean up any temp resources created.

For a snapshot clone, the planner will return the following chain of phases:

snapshot -> snap-clone -> prep-claim -> rebind

For csi-clone, the planner will return:

csi-clone -> prep-claim -> rebind

copy looks like this

host-clone -> rebind

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:

AGAIN

Reviewers, focus your attention here: https://github.com/mhenriks/containerized-data-importer/tree/clone-pop/pkg/controller/clone

And here: https://github.com/mhenriks/containerized-data-importer/blob/clone-pop/pkg/controller/populators/clone-populator.go

Release note:

VolumeCloneSource Populator

akalenyu · 2023-05-09T14:49:50Z

pkg/controller/datavolume/smart-clone-controller.go

-		if restoreSize.IsZero() {
-			return nil, fmt.Errorf("unable to determine restore size of PVC")
-		}
+		reqSize := targetPvcSpec.Resources.Requests[corev1.ResourceStorage]


i would sleep better at night if you use the copy here targetPvcSpecCopy

The smart clone controller is not long for this world in subsequent PR I will be deleting it then we will all sleep better

alromeros

Good job! I really like the planning/phase logic. Currently just some questions and silly comments about styling. Once I start working on the clone from snapshot I'll review it again with better insight.

alromeros · 2023-05-10T12:22:31Z

cmd/cdi-controller/controller.go

@@ -280,6 +280,10 @@ func start() {
 		klog.Errorf("Unable to setup upload populator: %v", err)
 		os.Exit(1)
 	}
+	if _, err := populators.NewClonePopulator(ctx, mgr, log, clonerImage, pullPolicy, installerLabels); err != nil {
+		klog.Errorf("Unable to setup import populator: %v", err)


Typo: import populator

alromeros · 2023-05-10T12:52:49Z

pkg/controller/clone/csi-clone.go

+	return "CSIClone"
+}
+
+// Reconcile ensures a snapshot is created correctly


Typo: Not snapshot

pkg/controller/clone/host-clone.go

alromeros · 2023-05-10T13:01:11Z

pkg/controller/clone/prep-claim.go

+	return "PrepClaim"
+}
+
+// Reconcile ensures a snapshot is created correctly


Typo: Not snapshot

alromeros · 2023-05-10T13:15:30Z

pkg/controller/datavolume/pvc-clone-controller.go

 	case HostAssistedClone:
-		return "network"
+		return "copy"


I think "copy" is representative of what a hostAssistedClone does, but I feel like we should avoid having several names to describe the same operation. Why not simply use host-assisted-clone?

Changed to be consistent with API types: https://github.com/mhenriks/containerized-data-importer/blob/clone-pop/staging/src/kubevirt.io/containerized-data-importer-api/pkg/apis/core/v1beta1/types.go#L829

It should never have been "network"

alromeros · 2023-05-10T14:30:35Z

pkg/controller/clone/planner.go

+)
+
+// Planner plans clone operations
+type Planner struct {


So, do we plan to eventually reuse the planning/phase structure in the old clone flow? I assumed a lot of this new logic would be reused when we discussed this PR yesterday.

Don't have the details nailed down yet, but it would be nice if datavolume controller can call Plan. Strategy will always be host assisted in that case for pvc or snapshot source. The main difference is that datavolume controller will not get a rebind phase

alromeros · 2023-05-10T14:58:52Z

pkg/controller/populators/clone-populator.go

+		Strategy:    *cs,
+	}
+
+	phases, err := r.planner.Plan(ctx, args)


Really like this design, but I can't help but wonder how other users/contributors will figure out what's happening here. I think this would benefit by having more comments or even by dividing reconcilePending into smaller functions.

I'll think about how it can be clearer.

alromeros · 2023-05-10T16:01:00Z

pkg/controller/populators/clone-populator.go

+	}
+
+	clonePopulator, err := controller.New(clonePopulatorName, mgr, controller.Options{
+		MaxConcurrentReconciles: 5,


First time I see us specifying this, why 5?

IMO it is a bug that we are only using one thread in other controllers. Especially controllers that poll status from pods that are coming/going. Things can get jammed up

mhenriks · 2023-05-18T13:53:29Z

/retest-required

alromeros

Looks very good! Still want to do another review but I'll continue tomorrow.

alromeros · 2023-05-18T15:10:25Z

tests/clone-populator_test.go

+		}
+	})
+
+	It("should should do filesystem to filesystem clone", func() {


Typo: Repeated "should" (all cases)

alromeros · 2023-05-18T15:18:29Z

tests/clone-populator_test.go

+		Expect(targetHash).To(Equal(sourceHash))
+	})
+
+	It("should should do csi clone if possible", func() {


Maybe add a test case to explicitly test smart clone?

alromeros · 2023-05-18T16:15:03Z

pkg/controller/clone/planner.go

+}
+
+// AddCoreWatches watches "core" types
+func (p *Planner) AddCoreWatches(log logr.Logger) error {


Why do we add this watch here instead of adding it to the clone populator? Why do we need to watch core types if we already have watches for PVCs? I'm missing something here.

Yeah this is a little wonky. The idea is that the clone populator would be ignorant of the types of resources created by the phases so the planner would handle creating the watches. This includes an additional PVC watch because the one in base populator does not cover PVCs with ownership label.

BUT the clone populator currently knows what types are created because they are needed for the cleanup function here: https://github.com/mhenriks/containerized-data-importer/blob/clone-pop/pkg/controller/populators/clone-populator.go#L274-L278

I think it would be better if the clone controller either 1) gets the types to cleanup from the planner or 2) calls a function on the planner to do the actual cleanup .

alromeros · 2023-05-18T16:16:44Z

pkg/controller/populators/clone-populator.go

+	}
+	reconciler.planner = planner
+
+	if err := addCommonPopulatorsWatches(mgr, clonePopulator, log, cdiv1.VolumeCloneSourceRef, &cdiv1.VolumeCloneSource{}); err != nil {


Maybe we need to add a watch for clones without source, so we retrigger the reconcile once the source PVC is created.

Currently will requeue if the planner returns nil. This covers the source not existing case. https://github.com/mhenriks/containerized-data-importer/blob/clone-pop/pkg/controller/populators/clone-populator.go#L207

There is a functional test for this case.

Replacing the requeue with a watch is doable but it is a kind of complicated. Will require an index on the source field of VolumeCloneSource. Maybe a subsequent PR

Signed-off-by: Michael Henriksen <mhenriks@redhat.com>

only supports PVC source now snapshot coming soon Signed-off-by: Michael Henriksen <mhenriks@redhat.com>

Signed-off-by: Michael Henriksen <mhenriks@redhat.com>

other review comments verifier pod should bount readonly Signed-off-by: Michael Henriksen <mhenriks@redhat.com>

mhenriks · 2023-05-19T13:47:34Z

/test pull-cdi-linter

synchronize get hash calls Signed-off-by: Michael Henriksen <mhenriks@redhat.com>

Signed-off-by: Michael Henriksen <mhenriks@redhat.com>

alromeros

I'm unable to see any errors/missing stuff in the functionality, but I still have some suggestions about styling and naming. Let me know what you think!

pkg/controller/clone/common.go

alromeros · 2023-05-22T10:13:24Z

pkg/controller/clone/common.go

+}
+
+// IsSourceClaimReady checks that PVC exists, is bound, and is not being used
+func IsSourceClaimReady(ctx context.Context, c client.Client, namespace, name string) (bool, error) {


Another minor complaint, but since we consistently use PVC or PersistentVolumeClaim in our code, I think it can be inconsistent to use claim in all these new functions. I think it wouldn't hurt to change Claim to PVC, wdyt?

Yeah I get that we use PVC a lot but I think Claim is better and I think more in line with kubernetes/kubernetes. Rather than enforcing naming schemes for the whole repo, how do you feel about doing it on a package level?

I prefer to be consistent across the whole repo, but if Kubernetes is using claim in its code then I'm ok if we start using it in new code.

alromeros · 2023-05-22T10:20:40Z

pkg/controller/clone/common.go

+}
+
+// MustGetStorageClassForClaim returns the storageclass for a PVC
+func MustGetStorageClassForClaim(ctx context.Context, c client.Client, pvc *corev1.PersistentVolumeClaim) (*storagev1.StorageClass, error) {


I think this function name is a little bit awkward. I also think it doesn't make that much sense to have it since we still need to check its error return (we are only saving two checks in planner.go).

alromeros · 2023-05-22T10:21:30Z

pkg/controller/clone/common.go

+	return sc, nil
+}
+
+// GetCompatibleVolumeSnapshotClass returns a VolumeSNapshotClass name that works for all PVCs


Minor typo: Uppercase N in VolumeSNapshotClass.

alromeros · 2023-05-22T10:25:53Z

pkg/controller/clone/common.go

+	}
+}
+
+func claimBoundOrWFFC(ctx context.Context, c client.Client, pvc *corev1.PersistentVolumeClaim) (bool, error) {


I think either isClaimBoundOrWFFC or isPVCBoundOrWFFC is more consistent with the rest of our code.

alromeros · 2023-05-22T10:30:05Z

pkg/controller/clone/planner.go

+	}
+
+	if !exists {
+		// TODO EVENT


Why not add the event now?

alromeros · 2023-05-22T10:34:49Z

pkg/controller/clone/planner.go

+	return nil
+}
+
+func (p *Planner) strategyForSourcePVC(ctx context.Context, args *ChooseStrategyArgs) (*cdiv1.CDICloneStrategy, error) {


Probably just preference but I miss having the verb in several function names. For example, I think getStrategyForSourcePVC sounds better and it's more informative than just strategyForSourcePVC.

perhaps this is a bit pedantic, but I like the prefix get/set for simple operations. simply returning a value that is already set/stored somewhere for example. This is actually computing a value that is not stored anywhere. I will add 'compute' prefix

alromeros · 2023-05-22T10:47:48Z

pkg/controller/clone/planner.go

+		Recorder:        p.Recorder,
+	}
+
+	rp := &RebindPhase{


Maybe it's not necessary, but since we are assembling these phases in a bunch of functions what do you think of adding a constructor function for each phase? In that way we can do something like:

rb := createRebindPhase(source, target, client, log, recorder)

I think it can be cleaner and easier to understand (also, less room for mistakes when setting parameters).

I actually think functions have more opportunity for error especially when there are multiple parameters of the same type.

ShellyKa13

Great work! I think the phases and planner really help simplify the clone process.
I do have an issue with the planner being populator inclined but it is not in the populators package.
I have a comment regarding the desiredPVC that is not clear to me.
I understand that we might want to use the planner and the phases in the regular flow but as is it really seems like it has very specific populators behavior, so why not move the whole new 'clone' directory under the populators package?
btw havn't reviewed the UT and tests yet.. I need a break :)

ShellyKa13 · 2023-05-18T14:57:31Z

pkg/controller/clone/common.go

+}
+
+// GetVolumeCloneSource returns the VolumeCloneSource dataSourceRef
+func GetVolumeCloneSource(ctx context.Context, c client.Client, pvc *corev1.PersistentVolumeClaim) (*cdiv1.VolumeCloneSource, error) {


this function is only used in pkg/controller/populators/clone-populator.go so why not put it there? also with IsDataSourceVolumeClone function

okay, I'll get rid of or move this

ShellyKa13 · 2023-05-21T14:09:26Z

pkg/controller/clone/common.go

+
+// GetVolumeCloneSource returns the VolumeCloneSource dataSourceRef
+func GetVolumeCloneSource(ctx context.Context, c client.Client, pvc *corev1.PersistentVolumeClaim) (*cdiv1.VolumeCloneSource, error) {
+	if !IsDataSourceVolumeClone(pvc.Spec.DataSourceRef) {


there is an existing function to check a kind of datasourceref: populators.IsPVCDataSourceRefKind(pvc, cdiv1.VolumeCloneSourceRef)

ShellyKa13 · 2023-05-21T14:16:03Z

pkg/controller/clone/common.go

+	}
+
+	sc := &storagev1.StorageClass{}
+	exists, err := getResource(ctx, c, "", *pvc.Spec.StorageClassName, sc)


just making sure if you dont want to use existing: GetStorageClassByName instead of new code

GetStorageClassByName will return the default storageclass if storageclassname is nil and I do not want that

ShellyKa13 · 2023-05-21T14:19:50Z

pkg/controller/clone/common.go

+}
+
+func claimBoundOrWFFC(ctx context.Context, c client.Client, pvc *corev1.PersistentVolumeClaim) (bool, error) {
+	if pvc.Spec.VolumeName != "" {


you can use existing function: !IsUnbound(pvc)

You love this function, huh

I just think if we have it lets use it, if you dont like the double negative you can create IsBound function, its just a check done a lot in the code, and I think it explains what volumename == "" means

ShellyKa13 · 2023-05-21T14:54:44Z

pkg/controller/populators/populator-base.go

@@ -170,65 +178,6 @@ func (r *ReconcilerBase) getPV(volName string) (*corev1.PersistentVolume, error)
 	return pv, nil
 }

-func (r *ReconcilerBase) rebindPV(targetPVC, pvcPrime *corev1.PersistentVolumeClaim) error {
-	pv, err := r.getPV(pvcPrime.Spec.VolumeName)


getPV is only used here, delete it if you dont use it anymore. or use it where you rewrote Rebind

Yeah but is used in upload populator unit tests

it was used in the UT since it already existed. since now it is not used in the code (not sure why not using it is better but ok) it should not be in the code, can be move to the UT framework instead..

ShellyKa13 · 2023-05-22T10:30:02Z

pkg/controller/clone/prep-claim.go

+			return nil, fmt.Errorf("actual PVC size missing")
+		}
+
+		p.Log.V(3).Info("prep pod required to force bind")


same for this log, its not required to 'force bind' more like for 'resize'

no for WFFC need to force bind even if source/target are the same size

my mistake I confused it with rebind which as I mentioned in previous commit is not relevant to prep-claim phase

ShellyKa13 · 2023-05-22T10:32:26Z

pkg/controller/clone/rebind.go

+
+// RebindPhase binds a PV from one PVC to another
+type RebindPhase struct {
+	SourceNamespace string


I think the name 'source' is confusing with the source of the clone, the rebind is used only to rebind the pv from pvcPrime to targetPVC, so I think we can be explicit about it in the rebind phase.

I disagree. To me, Phases stand on their own. And when you think of a rebind operation in isolation, source and target makes sense

ShellyKa13 · 2023-05-22T10:36:21Z

pkg/controller/clone/host-clone.go

+	}
+
+	if !exists {
+		actualClaim, err = p.createClaim(ctx)


here no need to check IsSourceClaimReady ?

No that is handled in clone controller

ShellyKa13 · 2023-05-22T10:42:21Z

pkg/controller/common/util.go

+}
+
+// Rebind binds the PV of source to target
+func Rebind(ctx context.Context, c client.Client, source, target *corev1.PersistentVolumeClaim) error {


seems weird for me that a function used only in populators context is not in the populators package..

It is used in the clone package and the populators package

ShellyKa13 · 2023-05-22T10:45:13Z

pkg/controller/clone/planner.go

@@ -344,13 +345,20 @@ func (p *Planner) planHostAssistedFromPVC(ctx context.Context, args *PlanArgs) (
 		desiredClaim.Spec.Resources.Requests[corev1.ResourceStorage] = is
 	}

+	ct := cdiv1.DataVolumeKubeVirt


you can use: cc.GetContentType(string(uploadSource.Spec.ContentType))

This is outdated code, contentType was removed

… exist Signed-off-by: Michael Henriksen <mhenriks@redhat.com>

mhenriks · 2023-05-22T13:09:19Z

Great work! I think the phases and planner really help simplify the clone process. I do have an issue with the planner being populator inclined but it is not in the populators package. I have a comment regarding the desiredPVC that is not clear to me. I understand that we might want to use the planner and the phases in the regular flow but as is it really seems like it has very specific populators behavior, so why not move the whole new 'clone' directory under the populators package? btw havn't reviewed the UT and tests yet.. I need a break :)

Plan is to expand the clone package to usable by datavolume controller as well. But I haven't figured out all the details yet. Having it's own package gives us flexibility.

Signed-off-by: Michael Henriksen <mhenriks@redhat.com>

ShellyKa13 · 2023-05-23T09:52:26Z

pkg/controller/populators/util.go

+	}
+
+	if *pvc.Spec.StorageClassName == "" {
+		return false, "", fmt.Errorf("PVC %s/%s has empty storage class name", pvc.Namespace, pvc.Name)


so incase storageclass name is empty we want to return error and not go by the default storageclass?

default storageclass is irrelevent. PVC is created and storageclass has been assigned. This is not like pvc definition in a datavolume where there is flexibility

ShellyKa13 · 2023-05-23T09:54:14Z

pkg/controller/populators/populator-base.go

@@ -335,7 +273,7 @@ func (r *ReconcilerBase) reconcileCommon(pvc *corev1.PersistentVolumeClaim, popu
 		return nil, err
 	}
 	// Check storage class
-	ready, waitForFirstConsumer, err := r.handleStorageClass(pvc)
+	ready, nodeName, err := claimReadyForPopulation(context.TODO(), r.client, pvc)


why is it better to return nodeName instead of a boolean if its != ""

to me it is a more useful api with nodename

you are only using it again once to check != "". I dont see the value, but I wont insist

ShellyKa13 · 2023-05-23T13:17:17Z

/lgtm

alromeros · 2023-05-23T16:57:30Z

/lgtm

awels

Looks good a few nitpicks/typos and one ask.

pkg/controller/clone-controller.go

pkg/controller/clone/csi-clone.go

awels · 2023-05-23T19:53:55Z

pkg/controller/clone/planner.go

+	return nil, fmt.Errorf("unknown strategy/source %s", string(args.Strategy))
+}
+
+func (p *Planner) watchSnapshots(ctx context.Context, log logr.Logger) error {


Yeah maybe name this addSnapshotWatches or something

awels · 2023-05-23T20:02:05Z

pkg/controller/clone/planner.go

+		}
+
+		if n == nil {
+			strategy = cdiv1.CloneStrategyHostAssisted


Can we emit an event on the target claim here that explains we switched to host assisted clone because no compatible volume snapshot class could be found?

awels · 2023-05-23T20:05:55Z

pkg/controller/clone/planner.go

+func (p *Planner) validateAdvancedClonePVC(ctx context.Context, args *ChooseStrategyArgs, sourceClaim *corev1.PersistentVolumeClaim) (bool, error) {
+	if !SameVolumeMode(sourceClaim, args.TargetClaim) {
+		args.Log.V(3).Info("volume modes not compatible for advanced clone")
+		return false, nil


Does it make sense to emit an event here indicating why we switched to host assisted clone? In most cases we are not logging at V(3) and this piece of information gets lost. I would settle for a V(1) log honestly.

awels · 2023-05-23T20:06:06Z

pkg/controller/clone/planner.go

+	}
+
+	if srcCapacity.Cmp(targetRequest) < 0 && !allowExpansion {
+		args.Log.V(3).Info("advanced clone not possible, no volume expansion")


Does it make sense to emit an event here indicating why we switched to host assisted clone? In most cases we are not logging at V(3) and this piece of information gets lost. I would settle for a V(1) log honestly.

awels · 2023-05-23T20:15:55Z

pkg/controller/clone/snap-clone.go

+		return nil, fmt.Errorf("snapshot missing restoresize")
+	}
+
+	// 0 restore size is a special case, provisioners that do that seem to all restoring to bigger pvcs


typo restoring should be restore

"allow restoring"

Signed-off-by: Michael Henriksen <mhenriks@redhat.com>

awels · 2023-05-23T21:08:01Z

/lgtm
/approve

kubevirt-bot · 2023-05-23T21:08:15Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: awels

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

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

awels · 2023-05-24T00:33:50Z

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

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 May 9, 2023

kubevirt-bot requested review from awels and maya-r May 9, 2023 03:47

kubevirt-bot added the size/XXL label May 9, 2023

mhenriks force-pushed the clone-pop branch from 34364e5 to 7c403ff Compare May 9, 2023 03:52

akalenyu reviewed May 9, 2023

View reviewed changes

mhenriks changed the title ~~[Draft] Clone populator~~ [WIP] PVC Clone Populator May 9, 2023

kubevirt-bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 9, 2023

alromeros reviewed May 10, 2023

View reviewed changes

mhenriks force-pushed the clone-pop branch from 6d0ca19 to b2c15d8 Compare May 11, 2023 03:23

mhenriks changed the title ~~[WIP] PVC Clone Populator~~ PVC Clone Populator May 18, 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 May 18, 2023

mhenriks force-pushed the clone-pop branch from bdab669 to a9990f9 Compare May 18, 2023 00:12

alromeros reviewed May 18, 2023

View reviewed changes

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

mhenriks added 5 commits May 18, 2023 16:30

touch up zero restoresize snapshot

1bac742

Signed-off-by: Michael Henriksen <mhenriks@redhat.com>

clone populator

fd84df9

only supports PVC source now snapshot coming soon Signed-off-by: Michael Henriksen <mhenriks@redhat.com>

more unit tests

3e1d894

Signed-off-by: Michael Henriksen <mhenriks@redhat.com>

unit test for clone populator

188e448

Signed-off-by: Michael Henriksen <mhenriks@redhat.com>

func tests for clone populator

a5ac4cb

Signed-off-by: Michael Henriksen <mhenriks@redhat.com>

mhenriks force-pushed the clone-pop branch from d6dba3c to b5f075d Compare May 18, 2023 20:38

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

move clone populator cleanup function to planner

aa2dd2f

other review comments verifier pod should bount readonly Signed-off-by: Michael Henriksen <mhenriks@redhat.com>

mhenriks force-pushed the clone-pop branch from b5f075d to aa2dd2f Compare May 18, 2023 23:28

mhenriks force-pushed the clone-pop branch from bc2308f to b835445 Compare May 19, 2023 13:55

add readonly flag to test executor pods

855b8dc

synchronize get hash calls Signed-off-by: Michael Henriksen <mhenriks@redhat.com>

mhenriks force-pushed the clone-pop branch from b835445 to 855b8dc Compare May 19, 2023 14:28

mhenriks added 2 commits May 19, 2023 11:03

increase linter timeout

55e56db

Signed-off-by: Michael Henriksen <mhenriks@redhat.com>

better/explicit readonly support for test pods

c7be492

Signed-off-by: Michael Henriksen <mhenriks@redhat.com>

alromeros reviewed May 22, 2023

View reviewed changes

ShellyKa13 reviewed May 22, 2023

View reviewed changes

check pv for driver info before looking up storageclass as it may not…

eeb5ec9

… exist Signed-off-by: Michael Henriksen <mhenriks@redhat.com>

addressed review comments

ec33442

Signed-off-by: Michael Henriksen <mhenriks@redhat.com>

mhenriks force-pushed the clone-pop branch from d8a407f to ec33442 Compare May 22, 2023 17:36

alromeros mentioned this pull request May 22, 2023

Add clone from snapshot functionalities to clone-populator #2724

Merged

ShellyKa13 reviewed May 23, 2023

View reviewed changes

kubevirt-bot assigned ShellyKa13 May 23, 2023

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

kubevirt-bot assigned alromeros May 23, 2023

awels reviewed May 23, 2023

View reviewed changes

chooseStrategy shoud generate more events

23935e1

Signed-off-by: Michael Henriksen <mhenriks@redhat.com>

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

kubevirt-bot assigned awels May 23, 2023

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

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

mhenriks mentioned this pull request May 24, 2023

Integration of Data volume using CDI populators #2722

Merged

kubevirt-bot merged commit f88fab6 into kubevirt:main May 24, 2023

alromeros mentioned this pull request Jun 14, 2023

New PR and commit guidelines #2752

Closed

PVC Clone Populator #2709

PVC Clone Populator #2709

Conversation

mhenriks commented May 9, 2023 • edited Loading

Overview

API

Implementation Details

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alromeros left a 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

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

mhenriks commented May 18, 2023

alromeros left a 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

Choose a reason for hiding this comment

mhenriks May 18, 2023 • edited Loading

Choose a reason for hiding this comment

mhenriks commented May 19, 2023

alromeros left a 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

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

alromeros May 22, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alromeros May 22, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ShellyKa13 left a 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

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

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

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

mhenriks commented May 22, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mhenriks commented May 9, 2023 •

edited

Loading

mhenriks May 18, 2023 •

edited

Loading

alromeros May 22, 2023 •

edited

Loading

alromeros May 22, 2023 •

edited

Loading

ShellyKa13 May 23, 2023 •

edited

Loading