-
Notifications
You must be signed in to change notification settings - Fork 258
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.
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
Check for pvc populated before doing any clone validations and actions #2375
Conversation
@@ -808,6 +806,9 @@ func (r *DatavolumeReconciler) ensureExtendedToken(pvc *corev1.PersistentVolumeC | |||
func (r *DatavolumeReconciler) selectCloneStrategy(datavolume *cdiv1.DataVolume, pvcSpec *corev1.PersistentVolumeClaimSpec) (cloneStrategy, error) { | |||
preferredCloneStrategy, err := r.getCloneStrategy(datavolume) | |||
if err != nil { | |||
if k8serrors.IsNotFound(err) { | |||
return NoClone, nil |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe it would be better if getCloneStrategy()
handles this case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what should be returned in that case? preferredCloneStrategy=nil and err=nil? I would still need to add here a check if preferredCloneStrategy=nil and return NoClone. or create a new CDICloneStrategy-> cdiv1.CloneStrategyNoClone? I would still need to add here a check of if preferredCloneStrategy=cdiv1.CloneStrategyNoClone and return NoClone?
I dont mind doing either but doesnt seem any cleaner or simpler then this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Anyway, why do we want to ignore notFound Err?
For example if source is not found it can return notFoundError, and for CDI it means that it needs to exit and retry later. And this is what happens when we return NoClone, err
f err != nil {
return reconcile.Result{}, err
}
And how it behaves If we return NoClone, nil
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this behaves correctly then I am ok with the rest.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@brybacki I think it behaves as expected: If the source PVC doesn't exist but the target is already populated, we are just updating the DV without triggering any errors. However, if the source PVC doesn't exist and the target is not populated, it'll be handled by validateCloneAndSourcePVC
, which will exit without errors to retry once the source is created:
containerized-data-importer/pkg/controller/datavolume-controller.go
Lines 673 to 682 in 083ee01
if pvcPopulated || prePopulated { | |
return r.reconcileDataVolumeStatus(datavolume, pvc, selectedCloneStrategy) | |
} | |
// Check if source PVC exists and do proper validation before attempting to clone | |
if done, err := r.validateCloneAndSourcePVC(datavolume); err != nil { | |
return reconcile.Result{}, err | |
} else if !done { | |
return reconcile.Result{}, nil | |
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok
ffac790
to
4ed66bc
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good!
@@ -2836,6 +2836,7 @@ func (r *DatavolumeReconciler) validateCloneAndSourcePVC(datavolume *cdiv1.DataV | |||
if err != nil { | |||
// Clone without source | |||
if k8serrors.IsNotFound(err) { | |||
r.recorder.Eventf(datavolume, corev1.EventTypeWarning, ErrUnableToClone, "Source pvc %s not found", datavolume.Spec.Source.PVC.Name) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall looks very good, but wouldn't this event be a little bit redundant? We are just recording a very similar event in the line below
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just moved it from the get clone strategy to here since it didnt seem related there, you added the event so you proabably know best if its redundant, if it is Ill delete :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The first event says "Source pvc %s not found"
while the second one is "The source PVC doesn't exist"
, so I think they are too similar to record them at the same time. Maybe it's a good idea to merge the two so the second event also displays the PVC name, but I'd just use one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
@@ -1904,7 +1905,6 @@ func (r *DatavolumeReconciler) getCloneStrategy(dataVolume *cdiv1.DataVolume) (* | |||
sourcePvc, err := r.findSourcePvc(dataVolume) | |||
if err != nil { | |||
if k8serrors.IsNotFound(err) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just in case, you left an empty condition here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
right thanks :)
4ed66bc
to
cc119fa
Compare
/test pull-containerized-data-importer-e2e-hpp-latest |
tests/cloner_test.go
Outdated
targetName, | ||
"1Gi", | ||
map[string]string{ | ||
controller.AnnPodPhase: "Succeeded", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What happens if this annotation is missing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
checked it, the test still passes, should I remove this annotation from the test?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed
cc119fa
to
7dd01a0
Compare
7dd01a0
to
083ee01
Compare
/test pull-containerized-data-importer-e2e-ceph |
_, err = utils.CreateDataVolumeFromDefinition(f.CdiClient, f.Namespace.Name, cloneDV) | ||
Expect(err).ToNot(HaveOccurred()) | ||
By("Wait for clone DV Succeeded phase") | ||
err = utils.WaitForDataVolumePhaseWithTimeout(f, f.Namespace.Name, cdiv1.Succeeded, targetName, cloneCompleteTimeout) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we check to make sure the clone didn't happen? Maybe check for some logging in the controller? Or maybe check if the annCloneType annotation is not set.
083ee01
to
e0e6ef9
Compare
In case our pvc is already populated no need to check for source PVC unknown size etc. Signed-off-by: Shelly Kagan <skagan@redhat.com>
e0e6ef9
to
d3ed3b9
Compare
/test pull-containerized-data-importer-e2e-ceph-gc |
/lgtm |
[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:
Approvers can indicate their approval by writing |
/retest |
1 similar comment
/retest |
/hold |
/unhold |
/retest |
opened a fix for the test flake: #2386 |
/cherrypick release-v1.53 |
@ShellyKa13: #2375 failed to apply on top of branch "release-v1.53":
In response to this:
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#2375) In case our pvc is already populated no need to check for source PVC unknown size etc. Signed-off-by: Shelly Kagan <skagan@redhat.com> Signed-off-by: Shelly Kagan <skagan@redhat.com>
* Status reporting for CSI & Smart clones with WFFC storage (#2364) * Fix logging level so we respect it in controllers/operator Signed-off-by: Alex Kalenyuk <akalenyu@redhat.com> * Fix CSI & Smart clones with WFFC storage status reporting Signed-off-by: Alex Kalenyuk <akalenyu@redhat.com> * Check for pvc populated before doing any clone validations and actions (#2375) In case our pvc is already populated no need to check for source PVC unknown size etc. Signed-off-by: Shelly Kagan <skagan@redhat.com> Signed-off-by: Shelly Kagan <skagan@redhat.com> Signed-off-by: Alex Kalenyuk <akalenyu@redhat.com> Signed-off-by: Shelly Kagan <skagan@redhat.com> Co-authored-by: Shelly Kagan <78472213+ShellyKa13@users.noreply.github.com>
In case our pvc is already populated no need to check for source PVC
unknown size etc.
Missing tests
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 #
This should fix bug: https://bugzilla.redhat.com/show_bug.cgi?id=2104479
Special notes for your reviewer:
Release note: