-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Issue 8044: generic restore - allow to ignore delay binding for WaitForFirstConsumer #8550
Issue 8044: generic restore - allow to ignore delay binding for WaitForFirstConsumer #8550
Conversation
505f06b
to
5b1f3bd
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #8550 +/- ##
==========================================
- Coverage 59.18% 59.16% -0.02%
==========================================
Files 370 370
Lines 39591 39605 +14
==========================================
+ Hits 23433 23434 +1
- Misses 14677 14689 +12
- Partials 1481 1482 +1 ☔ View full report in Codecov by Sentry. |
5b1f3bd
to
423065a
Compare
Signed-off-by: Lyndon-Li <[email protected]>
423065a
to
a56b06b
Compare
storageClass, err = storageClient.StorageClasses().Get(ctx, *tmpPVC.Spec.StorageClassName, metav1.GetOptions{}) | ||
if err != nil { | ||
return false, errors.Wrapf(err, "error to get storage class %s", *tmpPVC.Spec.StorageClassName) | ||
if !ignoreConsume { |
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 have a trivial comment, if ignoreConsume == true it seems we don't need to enter the Pollxx
function, and the code will be slightly easier to read:
tmpPVC, err := pvcGetter.PersistentVolumeClaims(namespace).Get(ctx, pvc, metav1.GetOptions{})
if err != nil {
return false, errors.Wrapf(err, "error to get pvc %s/%s", namespace, pvc)
}
if ignoreConsume {
return "", tmpPVC, nil
}
err := wait.PollUntilContextTimeout(ctx, waitInternal, timeout, true, func(ctx context.Context) (bool, error) {
if tmpPVC.Spec.StorageClassName != nil && storageClass == nil {
.....
updated = tmpPVC
return true, nil
})
return selectedNode, updated, 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.
There is a side benefit with the current code --- we could probe if the targetPVC exists by always calling pvcGetter.PersistentVolumeClaims(namespace).Get
. If the targetPVC doesn't exists, we could fail the restore earlier.
For sure, to achieve the same purpose, we could realign the code for better understanding --- adding another pvcGetter.PersistentVolumeClaims(namespace).Get
outside of Pollxx
, but that will add redundant code.
So let's keep it as is for now.
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.
Approved with a trivial comment.
It can be merged if you don't want to make the change.
Fix issue #8044, allow users to ignore delay binding the restorePVC of data mover when it is in WaitForFirstConsumer mode