Skip to content
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.

Sign up for GitHub

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

Jump to bottom

fix(backup/mirror): don't run healthcheck if nothing as been transferred #7396

Merged
merged 3 commits into from
Feb 28, 2024
Merged

fix(backup/mirror): don't run healthcheck if nothing as been transferred #7396

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
28b4d4d
fix(backups/AbstractRemoteVmBackupRunner): correctly store settings
fbeauchamp Feb 20, 2024
bec6ab7
fix(backups/MixinRemoteWriter): don't run healthcheck if nothing has …
fbeauchamp Feb 20, 2024
9547090
docs(backups/mirror): health check only applied on the last transferred
fbeauchamp Feb 20, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions @xen-orchestra/backups/_runners/_vmRunners/_AbstractRemote.mjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ export const AbstractRemote = class AbstractRemoteVmBackupRunner extends Abstrac
this.config = config
this.job = job
this.remoteAdapters = remoteAdapters
this._settings = settings
this.scheduleId = schedule.id
this.timestamp = undefined

Expand Down
10 changes: 5 additions & 5 deletions @xen-orchestra/backups/_runners/_writers/_MixinRemoteWriter.mjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,11 +77,11 @@ export const MixinRemoteWriter = (BaseClass = Object) =>
healthCheck() {
const sr = this._healthCheckSr
assert.notStrictEqual(sr, undefined, 'SR should be defined before making a health check')
assert.notStrictEqual(
this._metadataFileName,
undefined,
'Metadata file name should be defined before making a health check'
)
if (this._metadataFileName === undefined) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is an architectural issue:

  • writers where initially developed to run on a single backup
  • this assumption has been broken when re-using the writers for every backups of a VM in mirror backups
  • the introduction of _metadataFileName is a kind of global variable which is set to last wrote backup

We should clean this, either using writers for a single backup, and creating new one as necessary, or accepting they are intended to be called on multiple backups and clean up the state as necessary (e.g. passing metadataFileName as parameter to healthCheck).

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • using a new writer each time will lead to healthchecking each backup of a delta chain, and I think it will cause problem for the first transfer, or we'll have to rework the way we call the healthcheck
  • In theory, the _metadataFileName can be specific to each writer. In practice it should be the same path for all remotewriters, relative to the remote root. The parameter won't have the same meaning with replication writers. I am not favorable to store this in the runner and use it as a parameter
  • the transfer list of mirror backup is computed per remote and may be different, if the user add a new remote, or if a remore failed the previous trasnfer, that means that you can have a healthcheck on a remote, and not on another

As a side note, the current iteration of the backups code comes from the mirror backup feature, 9 months ago https://github.com/vatesfr/xen-orchestra/commits/1005e295b27942fe7986bdbc335ca41f9b400ea2/%40xen-orchestra/backups/Backup.js?browsing_rename_history=true&new_path=@xen-orchestra/backups/Backup.mjs&original_branch=master .

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. I understand that
  2. Fair enough
  3. Makes sense

I know the change was done a long time ago but I did not understand/think about the semantic changes implied for the writers.
I don't think there are currently bugs with this but I fear problems in the future.

// this can happen when making a mirror backup with nothing to transfer
Task.info('no health check, since no backup have been transferred')
return
}
return Task.run(
{
name: 'health check',
Expand Down
2 changes: 2 additions & 0 deletions CHANGELOG.unreleased.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,8 @@
- [New/SR] Fix create button never appearing for `smb iso` SR [#7355](https://github.com/vatesfr/xen-orchestra/issues/7355), [Forum#8417](https://xcp-ng.org/forum/topic/8417) (PR [#7405](https://github.com/vatesfr/xen-orchestra/pull/7405))
- [Pool/Network] Don't allow MTU values that are too small to work (<68) (PR [#7393](https://github.com/vatesfr/xen-orchestra/pull/7393)
- [Import/VMWare] Correctly handle IDE disks
- [Backups/Full] Fix `Cannot read properties of undefined (reading 'healthCheckVmsWithTags')` (PR [#7396](https://github.com/vatesfr/xen-orchestra/pull/7396))
- [Backups/Healthcheck] Don't run health checks after empty mirror backups (PR [#7396](https://github.com/vatesfr/xen-orchestra/pull/7396))

### Packages to release

Expand Down
2 changes: 1 addition & 1 deletion docs/mirror_backup.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ Just go into your "Backup" view, and select Vm Mirror Backup.
Then, select if you want to mirror incremental backups or full backups.
You must have exactly one source remote, you must have one or more destinations. The mirroring speed will be limited by the slower remote.

Most options of the full/incremental backups applies here, like concurrency (number of VM transferred in parallel), report, proxy and speed limit. You can also add a health check on schedules.
Most options of the full/incremental backups applies here, like concurrency (number of VM transferred in parallel), report, proxy and speed limit. You can also add a health check on schedules. Please note that only the last transferred backup (if any) will be checked.

:::tip
If you have full and incremental backups on a remote, you must configure 2 mirror backup jobs, one full and one incremental.
Expand Down
Loading