FullSync: remove temporary success workaround #65
Conversation
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
This commit introduces the changes to use only the include list paths
in RSYNC CLI as source paths.
- If IncludeList is configured for a configured path, while
syncing the path(during full sync and periodic sync) rsync will
use only the paths configured in the includeList as source paths.
- Whereas while immediate sync, data-sync will be able to know the
exact modified path and hence the modified path will be used as
source path by rsync, instead of using all paths in includeList.
In data-sync, the IncludeList will help the users to configure path
names in IncludeList if they want to monitor only few number of data
paths inside a directory and rest of the files don't need to monitor.
Note : Although rsync supports the `--filter` option to selectively
include paths, it requires explicitly including all parent directories
as well, and excluding the rest. This makes the CLI command
more complex. Hence, the above approach has been chosen.
Change-Id: Ieb668e0a1670f8082ca1615b53f5e78678269e65
Signed-off-by: Arya K Padman <[email protected]>
This commit introduces retry support in syncData to make the
data sync process more reliable in case of rsync failures
If rsync fails, the system waits for retryInterval seconds and
tries again, up to the maximum number of retry attempts. Each
retry is tracked using retryCount
For error code 24 (vanished source file), we fetch the corrected
source path and attempt to sync using the updated path
For other error codes, the retry continues with the original path
If all retry attempts are exhausted and sync still fails, a clear
failure message is logged and the function returns false to
indicate the failure
Tested:
Scenario 1: Retry on generic rsync failure (code 12)
- Initial attempt failed with:
rsync: connection unexpectedly closed (0 bytes received so far)
rsync error: error in rsync protocol data stream (code 12)
- Retry triggered after 30 seconds
- Sync succeeded on retry
- Logs confirmed retry count, error code, delay, and sync success
Scenario 2: Retry with vanished source file (code 24)
- Initial attempt failed with a vanished file error
- Detected code 24 in rsync output
- Extracted the available path Retry performed with the
corrected path
- Sync completed successfully
Change-Id: Ia2e79be4af4268f5328e26d967dfa18bbe0f2011
Signed-off-by: Manish Tiwari <[email protected]>
This commit adds support to handle IncludeList sync failures by retrying with a framed rsync command built for vanished paths If rsync reports vanished paths, the system now rebuilds the CLI with proper '--include' and '--exclude' filters to sync only the required IncludeList paths, This avoids retrying the full parent directory and ensures only the listed paths are retried For other errors, the retry continues normally using the same IncludeList Tested: Verified retry behavior with IncludeList paths vanished path case rebuilds framed rsync and syncs only the listed paths Change-Id: I899cdead98b54649fd1ebdf51725fb18bcd510d1 Signed-off-by: Manish Tiwari <[email protected]>
this commit update improves how we handle retries when multiple syncs happen at the same time if an inotify event comes for a file or folder that’s already being in processe, we now skip that request, we simply check if the path exists in 'syncInProgress' if yes, we don’t trigger another sync. This avoids redundant sync attempts and prevents unnecessary new sync request for the same file while a retry is ongoing Tested: Verified that no redundant sync is triggered if a file is already syncing or retrying Change-Id: Ie48946359a2424b6cf5a9f20ca17ad1d07448aa0 Signed-off-by: Manish Tiwari <[email protected]>
This commit introduces a change to abort ongoing sync operations if the disable_sync property is set to true during a sync failure The syncData now checks the disable_sync flag early in its execution If set, it immediately returns false without attempting to continue with further sync actions Change-Id: I189cffb0cdc82bf124f0e6928a5ad667a830afe7 Signed-off-by: Manish Tiwari <[email protected]>
This commit adds a unit test to simulate a vanished file scenario during sync. Initially, the given source path doesn’t exist and rsync fails with exit code 24 In the retry attempt, the logic identifies the valid parent path and rebuilds the sync command with it. Once the parent path is available, the sync succeeds as expected. This test validates: - Retry flow when rsync reports a vanished file - Fallback mechanism using the valid parent path - Successful sync after retry Change-Id: I134bf98ad3714154f08b37c1e2366ca33b35b0e8 Signed-off-by: Manish Tiwari <[email protected]>
We had added a quick workaround that always marked FullSync as success, even when data sync failed, It was just to unblock testing at that time Now since retry support is in place, this workaround is not needed anymore. If the source path is missing, retry will pick the nearest valid parent path and complete the sync properly - Removed forced SyncEventsHealth 'Ok' after sync attempt - Restored normal error reporting - Enabled the failure test checks again Tested: - Verified sync passes with retry when source path is missing - Retry picks parent path and completes successfully ``` phosphor-rbmc-data-sync-mgr[1647]: exit=24; retry with switching SRC [/var/lib/phosphor-settings-manager/settings/] -> vanishedPath [/var/lib/phosphor-settings-manager] phosphor-rbmc-data-sync-mgr[1647]: Full Sync completed successfully phosphor-rbmc-data-sync-mgr[1647]: Elapsed time for full sync: [20]s ``` Change-Id: Ifa384f288a2d0de941e2539fa047a044a13281e1 Signed-off-by: Manish Tiwari <[email protected]>
manishtwari
changed the title
RameshIyyar
reviewed
Oct 9, 2025
| // Add destination data path if configured | ||
| // TODO: Change the default destPath to empty once remote sync is enabled. | ||
| syncCmd.append(dataSyncCfg._destPath.value_or(fs::path(" /")).string()); | ||
| syncCmd.append(" "s + |
Contributor
Author
There was a problem hiding this comment.
This change is needed when a destination path is configured
without the space, the src and dest paths get merged (e.g., /a/b/c/data/backup)
so adding the space ensures they remain separate (/a/b/c/ /data/backup)
| #else | ||
| // TODO Support for remote (i,e sibling BMC) copying needs to be added. | ||
| #endif | ||
|
|
Collaborator
There was a problem hiding this comment.
Why new commit? You could just revert fb9588a commit, right?
Collaborator
There was a problem hiding this comment.
Did some other code go in that enables the sync to work on skiboards?
aryakpadman
approved these changes
Oct 30, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
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.
We had added a quick workaround that always marked FullSync as
success, even when data sync failed, It was just to unblock
testing at that time
Now since retry support is in place, this workaround is not needed
anymore. If the source path is missing, retry will pick the nearest
valid parent path and complete the sync properly
This commit have dependency on other commits, please review the commit
FullSync: remove temporary success workaround