Skip to content

Fix/backup-restore-watch-control-files - #4150

Open
1L7V5101V wants to merge 2 commits into
volcengine:mainfrom
1L7V5101V:fix/backup-exclude-watch-control-files
Open

Fix/backup-restore-watch-control-files#4150
1L7V5101V wants to merge 2 commits into
volcengine:mainfrom
1L7V5101V:fix/backup-exclude-watch-control-files

Conversation

@1L7V5101V

Copy link
Copy Markdown

Description

Backup of a workspace with an active watch task fails to round-trip: ov backup packs viking://resources/.watch_tasks.json (and .bak), but ov restore rejects the resulting pack with cannot import watch task control file, so any user with a watch task can neither backup nor restore.

Two root causes, both fixed:

  • Backup runs under a ROOT maintenance context, which bypasses the watch-task control file filter in _is_accessible, so the control files end up in the pack.
  • Restore validates every member with validate_import_target_uri, which unconditionally rejects watch control files, failing the entire pack.

Fix both sides:

  • Backup: exclude watch-task control files from the backup walk (_backup_entries).
  • Restore: skip (instead of reject) watch control members when restoring a backup, so packs created by older versions still round-trip. Import keeps rejecting them.

Human Involvement

  • A human participated in the implementation or review loop
  • This PR was generated entirely by AI agents without human participation in the loop

Related Issue

Fixes #4139

Type of Change

  • Bug fix (non-breaking change that fixes an issue)
  • New feature (non-breaking change that adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation update
  • Refactoring (no functional changes)
  • Performance improvement
  • Test update

Changes Made

  • openviking/storage/ovpack/operations.py: skip is_watch_task_control_uri entries in _backup_entries; pass skip_watch_control_files=True from restore_ovpack.
  • openviking/storage/ovpack/validation.py: add skip_watch_control_files keyword to validated_import_members; when set, log a warning and skip watch control members instead of validating/rejecting them.
  • tests/misc/test_ovpack_import_policy.py: test_backup_excludes_watch_task_control_files (regression for the backup side) and test_restore_skips_watch_task_control_files (a pre-fix pack containing .watch_tasks.json restores successfully, skipping the control file).

Testing

  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • I have tested this on the following platforms:
    • Linux
    • Windows

Local verification on Windows:

Checklist

  • My code follows the project's coding style
  • I have performed a self-review of my code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • Any dependent changes have been merged and published

Screenshots (if applicable)

N/A

Additional Notes

The fix is additive and non-breaking: the default behavior of validated_import_members (used by import_ovpack) is unchanged — import still rejects watch control files; only the backup-restore path opts into skipping them.

Backup runs with a ROOT maintenance context, which bypasses the watch-task
control file filter in _is_accessible, so packs can carry
viking://resources/.watch_tasks.json and .bak. Restore then rejected the
whole pack via validate_import_target_uri. Exclude the files on the backup
side and skip (instead of reject) them on the restore side so packs created
by older versions round-trip.

Fixes volcengine#4139
@hachimin-wh

Copy link
Copy Markdown
Contributor

this covers both suggested fixes. I verified that
validate_manifest_content only checks zip↔manifest consistency, so it doesn't preempt the
skip on poisoned packs (the skipped entry is still hash-verified), and keeping strict
rejection for scoped ov import is the right call. Two small suggestions: (1) the control
file comes as a triple (.json / .bak / .tmp) — the predicate covers all three, but
tests only exercise the main file; (2) a one-line doc note that watch tasks don't migrate
with backups would help.

Address review feedback on volcengine#4150: exercise .watch_tasks.json.bak and
.tmp alongside the main file in both the backup-exclusion and the
restore-skip tests, and add a doc note that watch task schedules are
instance-local and do not migrate with backups.
@1L7V5101V

Copy link
Copy Markdown
Author

Thanks for the review! Both suggestions are addressed in the latest push (f5919bc):

  1. Triple coverage: WatchTaskBackupVikingFS now exposes .watch_tasks.json, .watch_tasks.json.bak, and .watch_tasks.json.tmp; test_backup_excludes_watch_task_control_files asserts none of the three land in the pack, and test_restore_skips_watch_task_control_files builds a pre-fix pack containing all three and asserts restore skips each of them (only README.md is written).

  2. Doc note: added a one-liner to backup_ovpack in docs/en/api/14-ovpack.md (and the Chinese mirror) that watch task schedules are instance-local and do not migrate with backups.

Both new tests pass and ruff check is clean.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Backlog

Development

Successfully merging this pull request may close these issues.

[Bug]: Backup packs .watch_tasks.json but restore rejects it — backup/restore round-trip broken whenever a watch task exists

2 participants