Make size optional when cloning using the Storage API

[alromeros]

What this PR does / why we need it:

When cloning a Data Volume, the size of the target can be potentially obtainable via the source PVC. This discards the need to explicitly specify it.

Considering that, this PR aims to modify the cloning process in order to allow omitting the resources.request.storage field when cloning a PVC, which will be obtained from the source instead.

Special notes for your reviewer:

Release note:

Make size optional when cloning using Storage API

[kubevirt-bot]

Hi @alromeros. Thanks for your PR.

I'm waiting for a kubevirt member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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]

When you create a commit make sure to add -s to your command like this git commit -s this will automatically sign the commit so the DCO bot won't complain. To do this with an existing commit do git commit --amend -s to amend the commit message with the signing

[akalenyu]

/ok-to-test

[ShellyKa13]

if I understand correctly at this point hasTargetRequest can't be false right? maybe return an error if somehow it is?

[alromeros]

Yes, It shouldn't be, but I wanted to make sure. I'll later add proper error handling, thanks!

[ShellyKa13]

over all looks good! had some questions. great job!

[brybacki]

Very good direction.
👍

[alromeros]

/retest

[alromeros]

/test pull-containerized-data-importer-e2e-hpp-latest

[brybacki]

good, I cannot find the new test but I think it is ready to go
/lgtm

But it is a big PR so it would be good to have another reviewer do the last check.
@awels WDYT?

[brybacki]

I think that in detectCloneSize, the branch when source is block needs additional logic

		targetSize, err = inflateSizeWithOverhead(r.client, imgSize, pvcSpec)

[awels]

When seeing this function this was screaming, use me in all 3 variations.
The csi clone, the snapshot clone, and the no target size clone. I would call it something like isSourceReadyToClone that returns a boolean and error. The error is to catch the normal errors like now. When calling this function I would do it something like this for the csi clone and smart clone options.

if readyToClone, err := r.isSourceReadyToClone(dv, cloneStrategy); err != nil {
  return reconcile.Result{}, err
} else if !readyToClone {
  return reconcile.Result{Requeue: true},
    r.updateCloneStatusPhase(cdiv1.CloneScheduled, datavolume, nil, selectedCloneStrategy)
}

For the size less clone option something like this

else if !done {
  if readyToClone, err := r.isSourceReadyToClone(dv, cloneStrategy); err != nil {
    return reconcile.Result{}, err
  } else if !readyToClone {
    return reconcile.Result{Requeue: true},
      r.updateCloneStatusPhase(cdiv1.CloneScheduled, datavolume, nil, cloneStrategy)
  }
  return reconcile.Result{}, nil
}

[awels]

So if you are skipping due to that bug, I would remove the manual skip code above and make this a PEntry (which will skip the entry).

[alromeros]

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

[brybacki]

/lgtm

[awels]

/approve

[kubevirt-bot]

[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