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.

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

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

Merged
merged 3 commits into from
Feb 28, 2024

Conversation

fbeauchamp
Copy link
Collaborator

@fbeauchamp fbeauchamp commented Feb 20, 2024

Do not squash

Description

fix healthcheck when no data have been transferred , adding a info for the user

clarifying documentation on healthcheck in case of mirror backups

image

From : https://help.vates.tech/#ticket/zoom/21585

Checklist

  • Commit
    • Title follows commit conventions
    • Reference the relevant issue (Fixes #007, See xoa-support#42, See https://...)
    • If bug fix, add Introduced by
  • Changelog
    • If visible by XOA users, add changelog entry
    • Update "Packages to release" in CHANGELOG.unreleased.md
  • PR
    • If UI changes, add screenshots
    • If not finished or not tested, open as Draft

@fbeauchamp fbeauchamp requested review from MathieuRA and removed request for MathieuRA February 20, 2024 16:28
@fbeauchamp
Copy link
Collaborator Author

bug is still here : https://xcp-ng.org/forum/post/71850

@fbeauchamp fbeauchamp force-pushed the fix_mirror_full_healtcheck branch 2 times, most recently from a3df7b7 to da89109 Compare February 21, 2024 11:10
@fbeauchamp
Copy link
Collaborator Author

Copy link
Member

@MathieuRA MathieuRA left a comment

Choose a reason for hiding this comment

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

If the PR should not be squashed, write it in the PR description and fix the typo in your last commit name.

IMO, a changelog entry should be added for the healhcheck fix

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.

@julien-f julien-f merged commit ccdd6e2 into master Feb 28, 2024
1 check passed
@julien-f julien-f deleted the fix_mirror_full_healtcheck branch February 28, 2024 09:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants