Conversation
Owner
tis24dev
commented
Feb 12, 2026
tis24dev
commented
- Enhance prefiltering and PBS staged restore
- Add PBS notifications backup and API restore
- Make PBS restore behavior interactive (UI-driven)
- Add ctx cancellation and refactor restore code
- Refactor restore functions, tighten error checks
- Add prefilter-manual CLI and test adjustments
- Add context cancellation support, refactor switches
- Refactor function signatures & simplify logging
Refine backup prefiltering: track detailed stats, skip symlinks, and avoid normalizing known structured config paths (etc/proxmox-backup, etc/pve, etc/ssh, etc/pam.d, etc/systemd/system). Normalize helpers now return a (changed,bool) flag; config normalization is limited to safe text normalization (no sorting). Tests updated and a new structured-config test added. In orchestrator, add recovery for malformed/flattened proxmox-backup datastore.cfg by detecting duplicate keys and attempting to restore from pbs_datastore_inventory.json (including a lightweight inventory parser and duplicate-key detector). Update tests to cover recovery behavior.
Add support for backing up PBS notifications and for applying PBS config via the proxmox-backup-manager API. Key changes: - Collector: add BackupPBSNotifications and BackupPBSNotificationsPriv flags, include notifications.cfg and notifications-priv.cfg in PBS collection logic, and write a notifications_summary.json report; update tests. - New file: collector_pbs_notifications_summary.go implements JSON summaries for notification targets, matchers and endpoints. - Config: add RESTORE_PBS_APPLY_MODE and RESTORE_PBS_STRICT settings with parsing and template defaults; add PBS notifications config flags to parsing and defaults. - Orchestrator: add pbs_api_apply.go implementing API-based restore/apply of PBS categories (with strict 1:1 reconciliation and an auto fallback to file-based apply) and wire collector overrides for notifications. - Categories: update PBS category definitions to include command outputs and notification-related files. These changes enable selective backup of PBS notification data and provide an API-first restore path (with configurable behavior and strict reconciliation option).
Move PBS restore reconciliation out of backup.env and into an interactive choice during restores. Introduces a PBSRestoreBehavior enum (Merge vs Clean 1:1) and threads it through RestorePlan; removes RESTORE_PBS_APPLY_MODE/RESTORE_PBS_STRICT config parsing and env template entries. Staged PBS apply logic now uses the selected behavior: API-based applies are preferred and file-based fallbacks are only allowed in Clean mode, while Merge mode skips destructive/API-unavailable actions. Updated CLI and TUI to prompt for the behavior, adjusted notification/apply helpers, tests, and documentation to reflect the interactive selection and new default apply semantics.
Collector: simplify Flush error assignment and add context cancellation checks (ctx.Err()) in aggregateBackupHistory and aggregateReplicationStatus, including periodic checks inside loops to allow early cancellation. Restore/Access Control: ensure ctx is non-nil and check ctx.Err() early in applyPBSAccessControlFromStage and applyPVEAccessControlFromStage. Remove unused logger parameter from applyPBSACLSectionFormat and applyPBSACLLineFormat (they no longer accept a logger). Restore/SDN: reserve ctx (_ = ctx) for future use and tighten error handling when stat'ing staged SDN (remove redundant nil check in the else-if condition). Restore/TUI: simplify NIC repair branch by removing a redundant nil check on plan.Mapping and delete the unused promptOkTUI helper.
Simplify and tighten various restore-related internals: - Consolidate service timeout variable formatting. - Replace redundant "err != nil && !errors.Is(...)" checks with a single "!errors.Is(err, os.ErrNotExist)" in fs_atomic.go and restore_firewall.go. - Improve confirmRestoreAction destination handling: clean and display the actual restore destination (root vs a subdirectory) and clarify warnings. - Remove unused parameters: detectNodeForVM no longer accepts vmEntry and restartPVEFirewallService/extractHardlink no longer accept a logger; update callers accordingly. - Adjust extractHardlink signature and callers/tests to match; update tests to remove logger construction and assert behavior against the new signatures. - Minor test updates and formatting tweaks. These changes reduce unused parameters, improve user-facing messaging during restore, and simplify error checks. Tests were updated to reflect the new function signatures.
Introduce a new cmd/prefilter-manual command to run the backup prefilter manually with configurable root, max-size and log-level flags. Tighten and clarify unit tests: adjust assertion message in collector_collectall_test.go, reformat table-driven cases in orchestrator/decrypt_test.go, and add a lint ignore for an intentional nil-context test in copyRawArtifactsToWorkdir.
Add context handling and cancellation checks across orchestrator operations: createBundle and decrypt TUI now accept/guard nil contexts and check ctx.Err(), io.Copy uses a context-aware reader (contextReader) to allow cancellation. Update tests to use context.TODO and rename one test accordingly. Refactor multiple if/else branches to switch statements for systemType in categories, compatibility and restore UI code, and adjust logging to include step index in logStep. Miscellaneous: reformat category literals, simplify an error check in restore_ha, adjust bundle test to use a switch on header names, and remove an unused renderEnvValue helper from config/upgrade.go.
Remove logger coupling from several APIs and tidy up related logic. - Remove logger parameter from decryptArchiveWithSecretPrompt, buildNetworkPlanReport, targetNetworkEndpointFromConfig, applyPBSNodeCfgViaAPI, and validatePBSDatastoreReadOnly and update all callers. - Drop unused defaultNetworkHealthOptions and remove applyPBSHostConfigsFromStage (cleanup of dead/unused code). - Make guardMountPoint resilient to nil/expired contexts by ensuring ctx is non-nil and returning ctx.Err() early. - Adjust NewWithDeps to avoid copying the full Config into base (only set DryRun) to reduce unintended coupling. - Replace if/else with switch in RecreateDirectoriesFromConfig and ensure PBS datastore readonly warnings check logger != nil before logging. These changes simplify APIs by decoupling logging responsibilities, fix a nil-context issue, and remove some unused helpers to make call sites clearer.
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Pull request overview
Syncs dev to main by expanding PBS restore support (UI-driven behavior selection and API-based staged apply), improving prefilter safety for structured configs, and adding broader context-cancellation/robustness refactors across backup/restore flows.
Changes:
- Add interactive PBS restore reconciliation mode (Merge vs Clean 1:1) and wire it through restore planning + staged apply.
- Implement PBS API-based staged apply for multiple PBS categories (including notifications), with controlled fallbacks and new backup inventory/summary artifacts.
- Refactor/strengthen restore + backup utilities (ctx cancellation, safer prefiltering, signature tweaks, logging/flow cleanup) and update tests/docs accordingly.
Reviewed changes
Copilot reviewed 57 out of 57 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/orchestrator/workflow_ui_tui_restore.go | Add TUI prompt hook for PBS restore behavior. |
| internal/orchestrator/workflow_ui_cli.go | Add CLI prompt for PBS restore behavior. |
| internal/orchestrator/workflow_ui.go | Extend RestoreWorkflowUI with PBS behavior selection. |
| internal/orchestrator/selective.go | Refactor restore mode menu branching to switch. |
| internal/orchestrator/restore_workflow_ui_helpers_test.go | Fake UI updated for new PBS behavior prompt. |
| internal/orchestrator/restore_workflow_ui.go | Prompt PBS behavior and include in RestorePlan. |
| internal/orchestrator/restore_tui.go | Add PBS behavior TUI screen; minor safety tweak. |
| internal/orchestrator/restore_test.go | Update tests for signature changes (hardlink/node). |
| internal/orchestrator/restore_sdn.go | Minor ctx placeholder + stat error handling tweak. |
| internal/orchestrator/restore_plan.go | Add PBSRestoreBehavior field to plan. |
| internal/orchestrator/restore_notifications.go | PBS notifications apply: API-first with behavior-based fallback. |
| internal/orchestrator/restore_ha.go | Simplify stat error handling. |
| internal/orchestrator/restore_firewall.go | Refactor firewall restart signature + stat error handling. |
| internal/orchestrator/restore_errors_test.go | Update hardlink tests for new signature. |
| internal/orchestrator/restore_access_control.go | Add ctx guards; simplify ACL apply helpers’ signatures. |
| internal/orchestrator/restore.go | Improve destination messaging; simplify hardlink/node helpers usage. |
| internal/orchestrator/pbs_staged_apply_test.go | Add test for datastore.cfg inventory recovery. |
| internal/orchestrator/pbs_staged_apply.go | Major: PBS staged apply behavior + API apply + inventory recovery. |
| internal/orchestrator/pbs_restore_behavior.go | New enum defining Merge vs Clean PBS restore behavior. |
| internal/orchestrator/pbs_notifications_api_apply_test.go | New test coverage for PBS notifications API apply. |
| internal/orchestrator/pbs_notifications_api_apply.go | New: apply PBS notifications via proxmox-backup-manager. |
| internal/orchestrator/pbs_api_apply.go | New: shared PBS proxmox-backup-manager apply helpers. |
| internal/orchestrator/orchestrator.go | Add ctx cancellation in bundling; extend collector overrides. |
| internal/orchestrator/network_plan.go | Remove logger from plan generation helpers. |
| internal/orchestrator/network_health.go | Remove unused default options helper. |
| internal/orchestrator/network_apply_workflow_ui.go | Update calls to new network plan signatures. |
| internal/orchestrator/mount_guard.go | Add ctx guards in mount guard. |
| internal/orchestrator/fs_atomic.go | Simplify ErrNotExist checks. |
| internal/orchestrator/directory_recreation.go | Adjust datastore read-only validation signature; switch refactor. |
| internal/orchestrator/deps.go | Stop setting base.Config directly; rely on SetConfig path. |
| internal/orchestrator/decrypt_workflow_ui.go | Remove unused logger parameter from secret prompt decrypt helper. |
| internal/orchestrator/decrypt_tui.go | Add ctx nil/Err checks in TUI decrypt loop. |
| internal/orchestrator/decrypt_test.go | Reformat tests; adjust context test semantics. |
| internal/orchestrator/decrypt.go | Align decrypt flow with updated helper signature. |
| internal/orchestrator/compatibility.go | Switch refactor for system type handling. |
| internal/orchestrator/categories.go | Add PBS inventory/snapshot paths; switch refactors for filtering. |
| internal/orchestrator/bundle_test.go | Switch refactor for expected bundle file mapping. |
| internal/config/upgrade.go | Remove unused env rendering helper. |
| internal/config/templates/backup.env | Add PBS notifications collection toggles. |
| internal/config/config.go | Add config flags + parsing for PBS notifications backup. |
| internal/backup/optimizations_test.go | Adjust expectations: keep configs semantically intact. |
| internal/backup/optimizations_structured_test.go | New test ensuring structured configs are skipped by prefilter. |
| internal/backup/optimizations_helpers_test.go | Update tests for new normalize/minify return signatures. |
| internal/backup/optimizations.go | Prefilter: skip structured paths/symlinks; safer normalization; stats. |
| internal/backup/collector_pve.go | Improve flush error handling; add ctx checks in aggregators. |
| internal/backup/collector_pbs_notifications_summary.go | New: generate PBS notification snapshot summary report. |
| internal/backup/collector_pbs_commands_coverage_test.go | Expect notifications summary output in coverage test. |
| internal/backup/collector_pbs.go | Add notifications cfg collection + snapshots + summary. |
| internal/backup/collector_collectall_test.go | Fix assertion wording and redundant nil check. |
| internal/backup/collector.go | Add collector config flags + defaults for PBS notifications. |
| docs/RESTORE_TECHNICAL.md | Document PBS staged apply behavior + API coverage. |
| docs/RESTORE_GUIDE.md | Document PBS merge/clean behavior and datastore recovery notes. |
| docs/RESTORE_DIAGRAMS.md | Add PBS API apply note for staged categories. |
| docs/CONFIGURATION.md | Document PBS restore behavior + prefilter semantics + new env vars. |
| docs/CLI_REFERENCE.md | Note PBS restore behavior selection in CLI docs. |
| docs/BACKUP_ENV_MAPPING.md | Map new PBS notifications vars; note behavior not in env. |
| cmd/prefilter-manual/main.go | New manual utility to run prefilter on a directory. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
139
to
143
| if id == "" { | ||
| for _, v := range row { | ||
| if s, ok := v.(string); ok { | ||
| id = strings.TrimSpace(s) | ||
| break |
There was a problem hiding this comment.
parsePBSListIDs() falls back to picking the “first string” value from a map when none of the candidateKeys are present. Map iteration order is nondeterministic, so this can yield inconsistent IDs and (in strict mode) may remove/update the wrong PBS objects. Consider removing the fallback and returning an error when no expected key is found (or require callers to pass a key that must exist).
Comment on lines
+158
to
+163
| if strict { | ||
| out, err := runPBSManager(ctx, "notification", "endpoint", typ, "list", "--output-format=json") | ||
| if err != nil { | ||
| return err | ||
| } | ||
| current, err := parsePBSListIDs(out, "name", "id") |
There was a problem hiding this comment.
Strict-mode cleanup removes notification endpoints before matchers. If matchers reference endpoints, endpoint removal may fail due to existing matchers, and there’s no retry after matchers are removed—so “Clean 1:1” may leave extra endpoints behind. Consider removing matchers first (or retrying endpoint removals after matcher cleanup) when strict=true.
parsePBSListIDs: sanitize and validate candidate keys, error when none provided, and make row parsing stricter and more informative. The function now trims empty keys up-front, iterates using the cleaned key list, ensures values are non-empty strings, and returns a descriptive error (including row index and available keys) instead of silently skipping unparseable rows. applyPBSNotificationsViaAPI: move matcher cleanup earlier in strict mode so matchers are removed before endpoints (preventing endpoint cleanup from being blocked by references). The matcher handling was consolidated (deduplicated) and matcher names are sorted for deterministic processing.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.