Support multi-part backup #109
Conversation
ushitora-anqou
force-pushed
the
split-diff
branch
2 times, most recently
from
cca83a9 to
0af1089
Compare
ushitora-anqou
changed the title
ushitora-anqou
force-pushed
the
split-diff
branch
7 times, most recently
from
ff16d2d to
055f549
Compare
ushitora-anqou
force-pushed
the
split-diff
branch
4 times, most recently
from
65a1818 to
fd22a0f
Compare
Signed-off-by: Ryotaro Banno <[email protected]>
ushitora-anqou
force-pushed
the
split-diff
branch
from
fd22a0f to
aa50aaf
Compare
Signed-off-by: Ryotaro Banno <[email protected]>
ushitora-anqou
force-pushed
the
split-diff
branch
from
aa50aaf to
d724dbf
Compare
…and startUpload() Signed-off-by: Ryotaro Banno <[email protected]>
ushitora-anqou
force-pushed
the
split-diff
branch
from
d724dbf to
09ac92a
Compare
Signed-off-by: Ryotaro Banno <[email protected]>
Signed-off-by: Ryotaro Banno <[email protected]>
Signed-off-by: Ryotaro Banno <[email protected]>
Signed-off-by: Ryotaro Banno <[email protected]>
ushitora-anqou
force-pushed
the
split-diff
branch
from
09ac92a to
b331276
Compare
…s correct Signed-off-by: Ryotaro Banno <[email protected]>
Signed-off-by: Ryotaro Banno <[email protected]>
Signed-off-by: Ryotaro Banno <[email protected]>
Signed-off-by: Ryotaro Banno <[email protected]>
Signed-off-by: Ryotaro Banno <[email protected]>
Signed-off-by: Ryotaro Banno <[email protected]>
ushitora-anqou
force-pushed
the
split-diff
branch
from
b331276 to
cf528a5
Compare
…-size is changed during the backup Signed-off-by: Ryotaro Banno <[email protected]>
Signed-off-by: Ryotaro Banno <[email protected]>
Signed-off-by: Ryotaro Banno <[email protected]>
Signed-off-by: Ryotaro Banno <[email protected]>
Signed-off-by: Ryotaro Banno <[email protected]>
Signed-off-by: Ryotaro Banno <[email protected]>
…liation Signed-off-by: Ryotaro Banno <[email protected]>
Signed-off-by: Ryotaro Banno <[email protected]>
ushitora-anqou
force-pushed
the
split-diff
branch
from
cf528a5 to
5c8946a
Compare
ushitora-anqou
changed the title
| TransferPartSize *resource.Quantity `json:"transferPartSize,omitempty"` | ||
|
|
||
| // largestCompletedExportPartNum indicates the largest part number | ||
| // that has been successfully exported (i.e., `rbd export-diff`). |
There was a problem hiding this comment.
| // that has been successfully exported (i.e., `rbd export-diff`). | |
| // that has been successfully exported by `rbd export-diff`. |
| LargestCompletedUploadPartNum *int `json:"largestCompletedUploadPartNum,omitempty"` | ||
|
|
||
| // largestCompletedImportPartNum indicates the largest part number | ||
| // that has been successfully imported (i.e., `rbd import-diff`). |
There was a problem hiding this comment.
| // that has been successfully imported (i.e., `rbd import-diff`). | |
| // that has been successfully imported by `rbd import-diff`. |
| labelComponentExportJob, | ||
| MantleExportJobPrefix, | ||
| func(job *batchv1.Job, partNum int) error { | ||
| if backup.Status.LargestCompletedExportPartNum == nil || *backup.Status.LargestCompletedExportPartNum < partNum { |
There was a problem hiding this comment.
Please add an assertion to verify *backup.Status.LargestCompletedExportPartNum +1 == partNum at the beginning of this block. By this assertion, we can detect the existence of multiple running export jobs for one MantleBackup. It implies serious bugs.
| return nil | ||
| } | ||
|
|
||
| if err := r.patchStatusTransferPartSizeIfEmpty(ctx, targetBackup); err != nil { |
There was a problem hiding this comment.
| if err := r.patchStatusTransferPartSizeIfEmpty(ctx, targetBackup); err != nil { | |
| if err := r.addStatusTransferPartSizeIfEmpty(ctx, targetBackup); err != nil { |
add is more suitable for in this case because add explains what this function directly. On the other hand, patch doesn't tell what kind patch will be applied.
| return pv.Spec.CSI.VolumeAttributes["pool"], pv.Spec.CSI.VolumeAttributes["imageName"], nil | ||
| } | ||
|
|
||
| func (r *MantleBackupReconciler) getNumberOfParts(backup *mantlev1.MantleBackup) (int64, error) { |
There was a problem hiding this comment.
How about returining int value rathert than int64 value? The return value seems to be casted to int in all cases.
| if backup.Status.LargestCompletedExportPartNum != nil { | ||
| partNum = *backup.Status.LargestCompletedExportPartNum | ||
| } | ||
| return partNum+1 >= int(limit), nil |
There was a problem hiding this comment.
We don't expect partNum+1 > int(limit). So how about adding an assertion to meet partNum + 1 == int(limit)
| labelComponentUploadJob, | ||
| MantleUploadJobPrefix, | ||
| func(job *batchv1.Job, partNum int) error { | ||
| if backup.Status.LargestCompletedUploadPartNum == nil || *backup.Status.LargestCompletedUploadPartNum < partNum { |
There was a problem hiding this comment.
It's better to add an assertion similar to the above comment about handleCompletedExportJobs.
| if backup.Status.LargestCompletedExportPartNum != nil { | ||
| partNum = *backup.Status.LargestCompletedExportPartNum | ||
| } | ||
| return partNum+1 >= int(limit), nil |
There was a problem hiding this comment.
Please return true iff partNum + 1 == int(limit) because partNum + 1 > int(limit) implies serious bugs.
| labelComponentUploadJob, | ||
| MantleUploadJobPrefix, | ||
| func(job *batchv1.Job, partNum int) error { | ||
| if backup.Status.LargestCompletedUploadPartNum == nil || *backup.Status.LargestCompletedUploadPartNum < partNum { |
There was a problem hiding this comment.
The current code might result in regarding all upload jobs has been completed by mistake.
Assuming the following cases.
- There is a RBD snapshot S
- Mantle divide this into four parts, S0,...,S3.
- All export jobs have been finished.
- Mantle creates four upload jobs UJ_S0,...,UJ_S3.
- Only UJ_S3 has been finished.
- Mantle calls handleCompletedUploadJobs(). Then it regards all upload jobs has been finished.
Probably what we need to do is just counting the number of completed jobs and whether the number is equal to the number of created jobs. In other words, status.largestCompletedUploadPartNum is not necessary. WDYT?
| return nil | ||
| } | ||
|
|
||
| if err := r.createOrUpdateUploadJobs(ctx, targetBackup); err != nil { |
There was a problem hiding this comment.
createOrUpdateUploadJobs might create multiple jobs. So we get the maximum number of new jobs we can create. We would get this value possibly from canNewUploadJobBeCreted().
No description provided.