-
Notifications
You must be signed in to change notification settings - Fork 264
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
Xo tasks @xo/backups #8012
base: backup-XO-Tasks
Are you sure you want to change the base?
Xo tasks @xo/backups #8012
Conversation
@xen-orchestra/backups/_runners/_writers/IncrementalRemoteWriter.mjs
Outdated
Show resolved
Hide resolved
@xen-orchestra/backups/_runners/_writers/IncrementalXapiWriter.mjs
Outdated
Show resolved
Hide resolved
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 don't know if this is normal, since the work will be split into multiple PRs, but log.tasks
doesn't exist anymore, and this breaks the UI.
{
"data": {
"mode": "delta",
"reportWhen": "failure"
},
"id": "1727783877606",
"jobId": "d78a6445-1c59-4932-9851-d6c642e46c21",
"jobName": "8.2 non smart mode",
"message": "backup",
"scheduleId": "464e7258-3559-4716-aa3e-743c85bd89c9",
"start": 1727783877606,
"status": "success",
"end": 1727783897166
}
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.
please rework the retry task handling
) | ||
) | ||
} | ||
|
||
if (job.xoMetadata !== undefined && settings.retentionXoMetadata !== 0) { | ||
promises.push( | ||
runTask( | ||
Task.run( |
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.
shouldn't we use runInside here to ensure we don't lose an error ?
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 only difference using runInside
would make is that the task would stay pending if it doesn't have an error, so it's not a solution.
If we want the backup to fail when one subtask fails, we would have to remove the .catch(noop)
, but it would need some testing as it may have other consequences.
Yes, backup tasks and vates tasks handle logs differently, it will be dealt with later |
@@ -110,22 +110,32 @@ export const VmsRemote = class RemoteVmsBackupRunner extends Abstract { | |||
taskByVmId[vmUuid] = new Task(taskStart) | |||
} | |||
const task = taskByVmId[vmUuid] | |||
// error has to be caught in the taskto prevent in failure, but handled outside the task to execute another task.run() |
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.
// error has to be caught in the taskto prevent in failure, but handled outside the task to execute another task.run() | |
// error has to be caught in the task to prevent in failure, but handled outside the task to execute another task.run() |
}) | ||
) | ||
.then(result => { | ||
if (taskError) { |
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 (taskError) { | |
if (taskError !== undefined) { |
.runInside(async () => | ||
vmBackup.run().catch(error => { | ||
taskError = error | ||
}) | ||
) | ||
.then(result => { | ||
if (taskError) { | ||
if (isLastRun) { | ||
throw error | ||
// ending the task with error | ||
return task.run(() => { | ||
throw taskError | ||
}) | ||
} else { | ||
Task.warning(`Retry the VM mirror backup due to an error`, { | ||
// don't end the task | ||
task.warning(`Retry the VM mirror backup due to an error`, { | ||
attempt: nTriesByVmId[vmUuid], | ||
error: error.message, | ||
error: taskError.message, | ||
}) | ||
queue.add(vmUuid) | ||
} | ||
} else { | ||
// ending the task with success | ||
task.run(() => result) |
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.
To avoid nested if/else
, i suggest something like that:
// NOT TESTED
return task.runInside(async () => {
let result
try {
result = await vmBackup.run()
} catch (error) {
taskError = error
}
if (taskError === undefined) {
return task.run(() => result)
}
if (isLastRun) {
return task.run(() => {
throw taskError
})
}
task.warning(`Retry the VM mirror backup due to an error`, {
attempt: nTriesByVmId[vmUuid],
error: taskError.message,
})
queue.add(vmUuid)
})
86a2f13
to
5f37c87
Compare
Description
Replacing backup tasks by tasks from
@vates/task
in@xen-orchestra/backups
.Merge this to branch
backup-XO-Tasks
instead ofmaster
, as it must not be merged before further changes are made.Checklist
Fixes #007
,See xoa-support#42
,See https://...
)Introduced by
CHANGELOG.unreleased.md
Review process
Notes: