Data upload controller #6337
Data upload controller #6337
Conversation
qiuming-best
added
the
kind/changelog-not-required
qiuming-best
force-pushed
the
data-upload-controller
branch
5 times, most recently
from
24aae49 to
e4b8859
Compare
qiuming-best
force-pushed
the
data-upload-controller
branch
2 times, most recently
from
5f98bc2 to
9a957d3
Compare
| } | ||
|
|
||
| if du.Spec.DataMover != "" && du.Spec.DataMover != dataMoverType { | ||
| log.WithField("Data mover", du.Spec.DataMover).Info("it is not one built-in data mover which is not supported by Velero") |
There was a problem hiding this comment.
This message looks like trivial, so suggest to change it to debug log
|
|
||
| ep, ok := r.snapshotExposerList[du.Spec.SnapshotType] | ||
| if !ok { | ||
| return ctrl.Result{}, fmt.Errorf("%v type of snapshot exposer is not exist", du.Spec.SnapshotType) |
There was a problem hiding this comment.
Do we need to call errorOut so as to mark the CR as failed?
| log.Info("Data upload is accepted") | ||
|
|
||
| exposeParam := r.setupExposeParam(&du) | ||
| if err := ep.Expose(ctx, getOwnerObject(&du), du.Spec.CSISnapshot.VolumeSnapshot, du.Spec.OperationTimeout.Duration, exposeParam); err != nil { |
There was a problem hiding this comment.
Since the snapshot type for the du is arbitrary here, it is not a good practice to refer to u.Spec.CSISnapshot
There was a problem hiding this comment.
What about we change the Expose interface to Expose(context.Context, corev1.ObjectReference, interface{}) error
for every specific snapshot type, they could get the parameter values from the interface{}?
There was a problem hiding this comment.
Yes, we can put the snapshot ID to the parameter interface
There was a problem hiding this comment.
I'm not sure why we need snapshot ID as parameter?
There was a problem hiding this comment.
Yes, we can put the snapshot ID to the parameter interface
Sorry for the typo, it should be "snapshot name". I see the current change that move snapshot name and timeout to parameter, it is fine.
| log.WithError(err).Error("error updating data upload into canceling status") | ||
| return ctrl.Result{}, err | ||
| } | ||
| ep.CleanUp(ctx, getOwnerObject(&du), du.Spec.CSISnapshot.VolumeSnapshot, du.Spec.SourceNamespace) |
There was a problem hiding this comment.
Don't call the CleanUp here, by setting the phase to DataUploadPhaseCanceling, it only means the data mover is notified for cancel, it doesn't mean the data mover has cancelled and the snapshot is released by the data mover.
If we call CleanUp here, it is possible that the snapshot is deleted where the data mover is still trying to access it.
The CleanUp should be called inside the Cancel callback.
qiuming-best
force-pushed
the
data-upload-controller
branch
2 times, most recently
from
9c86e7a to
79b0da3
Compare
qiuming-best
force-pushed
the
data-upload-controller
branch
from
79b0da3 to
6155110
Compare
config/rbac/role.yaml
Outdated
| - apiGroups: | ||
| - velero.io | ||
| resources: | ||
| - dataupload |
There was a problem hiding this comment.
This doesn't look correct, we've already have the role declaration for datauploads in the same file. And datauploads looks to be the correct resource name instead of dataupload
There was a problem hiding this comment.
yes, I've removed it
qiuming-best
force-pushed
the
data-upload-controller
branch
2 times, most recently
from
95fe9dd to
0efdc38
Compare
| // The fresh new DataUpload CR first created will trigger to create one pod (long time, maybe failure or unknown status) by one of the dataupload controllers | ||
| // then the request will get out of the Reconcile queue immediately by not blocking others' CR handling, in order to finish the rest data upload process we need to | ||
| // re-enqueue the previous related request once the related pod is in running status to keep going on the rest logic. and below logic will avoid handling the unwanted | ||
| // pod status and also avoid black others CR handling |
qiuming-best
force-pushed
the
data-upload-controller
branch
from
0efdc38 to
857598f
Compare
Signed-off-by: Ming <[email protected]>
qiuming-best
force-pushed
the
data-upload-controller
branch
from
857598f to
b3e99a7
Compare
Thank you for contributing to Velero!
Please add a summary of your change
Does your change fix a particular issue?
Fixes #(issue)
#6119
Please indicate you've done the following:
/kind changelog-not-requiredas a comment on this pull request.site/content/docs/main.