feat: Add Mover Status (7.4) - #1042
Conversation
|
Example View
|
|
Very nice! |
|
tom mortensen (@limetech) these changes will be pending your mover and move changes needed to made mover.ini in state dir. |
|
Gist for my current test mover script https://gist.github.com/SimonFair/91d54c983a8c9b09c0d803baa25f01ac |
…air/webgui into Mover-Progress-NCHAN-rc3+
|
it's baaaaaaaaaaack |
|
tom mortensen (@limetech) Started creating a php version of the mover script to allow status to show file name and current action. But I still need to look at is the progress for each file. Can you provide details of now to get the ioctl from move so I can update the nchan for parity_list to get the progress. Or if you have another way you would like to progress let me know. WIP https://gist.github.com/SimonFair/5e1f318b20e43afc01dcf29a78aee2c1
|
|
any info from tom mortensen (@limetech) when or if this PR get merged? |
|
Wow this looks amazing! |
|
Bump, this would be a great feature |
|
Any news on this one? |
|
is it possible to have this notify the apprise api of its progress ? |
|
Is there any reason, why this hasnt been merged yet? Would love to see this feature implemented in unraid 7.1 |
|
Can someone solve the conflicts and merge it into 7 please? |
Unlikely at this point as backend changes are required for pool to pool and progress. These are just the GUI changes. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
Included review availability: Your plan includes up to 2 reviews per rolling hour; 1 remains after this review. WalkthroughAdds configurable mover progress tracking. ChangesMover Progress Feature
Estimated code review effort: 4 (Complex) | ~60 minutes Merge Risk: ⚪ Minimal · up to No actionable merge-blocking risk remains; the PR is merge-ready after normal checks and review. Sequence Diagram(s)sequenceDiagram
participant mover as sbin/mover
participant mover_ini as mover.ini
participant parity_list
participant Browser
participant mover_rows as mover rows
mover->>mover: calculate_totals()
mover->>mover_ini: write progress status
parity_list->>mover_ini: read mover status
parity_list->>Browser: publish mover payload
Browser->>mover_rows: update and show selected rows
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Really hoping this makes it in soon, seems like a great addition with most of the heavy lifting already done |
|
Can we potentially get an update on this? |
|
+1 for this, this would be fantastic |
|
Would love this to be added soon! |
|
+1 |
|
Came across this when searching for a way to monitor mover progress - this would be a great feature! |
|
SimonFair do you want to rebase this |
🔧 PR Test Plugin AvailableA test plugin has been generated for this PR that includes the modified files. Version: 📥 Installation Instructions:Install via Unraid Web UI:
Alternative: Direct Download
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@emhttp/plugins/dynamix/ArrayOperation.page`:
- Around line 517-528: The issue is that when moverlines decreases from a higher
value, previously visible rows above the new limit remain visible instead of
being hidden, because only rows 0 through moverlines-1 are updated in the
visibility loop. Before the main loop starting with `for (let i = 0; i <
moverlines; i++)`, add a reset loop to hide all mover rows (from 0 through
maxmoverlines) to ensure that any rows now outside the updated range are
properly hidden regardless of their previous visibility state.
In `@emhttp/plugins/dynamix/MoverSettings.page`:
- Around line 137-145: The form control for `shareMoverProgress` is defined in
the MoverSettings.page file but lacks backend implementation. You need to add
code to read the `shareMoverProgress` setting from the configuration storage,
pass this value to the mover process or service that handles progress display
functionality, and implement the conditional logic to enable or disable progress
display based on whether the setting is "enabled" or "disabled". This ensures
the toggle state actually affects the mover's behavior rather than just
persisting without effect.
In `@sbin/mover`:
- Around line 75-81: The PID file creation using the echo statement that writes
to /var/run/mover.pid needs to be moved to execute before the sleep 5 command
rather than after. Move the line containing echo $$ >/var/run/mover.pid to
execute immediately after the writestatus call and before the sleep 5 statement
to ensure the PID file is created early and prevent a race condition where a
second mover instance could start during the sleep window before the first
instance has written its PID.
In `@sbin/mover.old`:
- Around line 48-55: The stop and status command sections (at lines 134-143 and
163-165) do not validate that the PID in the file actually belongs to the mover
process before acting on it, unlike the startup section which checks with ps and
grep. Add the same PID ownership validation check (ps h $(cat $PIDFILE) | grep
mover) to both the stop function before calling killtree and the status function
before reporting the process as running, ensuring that only the actual mover
process is terminated or reported, preventing the termination of unrelated
processes with reused or stale PIDs.
- Around line 99-103: The EMPTYING variable can be blank, which causes the for
loop over the arr array to iterate with an empty DISK value, resulting in
scanning /mnt directly. Add a guard condition to check if the EMPTYING variable
is not empty before entering the for DISK in "${arr[@]}" loop. If EMPTYING is
empty, skip the entire disk iteration to prevent unintended operations on the
/mnt root directory.
In `@sbin/moverprogress`:
- Around line 144-145: The variables $DUARGSI and $DUARGSO containing file paths
are being directly interpolated into shell commands passed to shell_exec(),
which creates a shell injection vulnerability. Escape these variables using the
escapeshellarg() function before concatenating them into the shell commands to
prevent malicious input from breaking or manipulating the du commands. Apply
this same escapeshellarg() protection to all other instances mentioned in the
comment at lines 172, 177, 184-185, 217, 222, and 229-230 where file paths are
being concatenated into shell commands.
- Around line 72-76: In the startMover() function where the PID file content is
checked, add process liveness verification before refusing startup. Instead of
immediately exiting when the PID file exists, first check if the process with
that PID is actually running using a system call like kill -0 or equivalent.
Only exit with the "already running" message if the process is confirmed to be
alive. If the process is not running, allow the startup to proceed and
optionally clean up the stale PID file to prevent future blocks.
- Around line 263-275: The stopMover() function reads the process ID from the
PID file but never actually terminates the running mover process before removing
the PID file. After validating that $pid exists, you need to actually kill the
process using a system call like posix_kill() or exec() to send a termination
signal to the process with the retrieved $pid before sleeping and removing the
/var/run/mover.pid file. This ensures the mover process is properly terminated
rather than just orphaning it while the UI reports it as stopped.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: eb2452a6-b999-4cc9-8b2e-831134540ab0
📒 Files selected for processing (7)
emhttp/languages/en_US/helptext.txtemhttp/plugins/dynamix/ArrayOperation.pageemhttp/plugins/dynamix/MoverSettings.pageemhttp/plugins/dynamix/nchan/parity_listsbin/moversbin/mover.oldsbin/moverprogress
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@sbin/mover`:
- Around line 271-279: The calculate_totals function is being called
unconditionally even when mover progress is disabled, causing unnecessary tree
scanning. Wrap the calculate_totals function call and the related writestatus
calls (the ones before and after it) in a conditional block that checks if
mover_progress_enabled is true, so that the expensive tree walk is skipped when
progress tracking is disabled.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 4c08eb94-80c2-47cd-91ef-b381b752a3b4
📒 Files selected for processing (3)
emhttp/plugins/dynamix/ArrayOperation.pageemhttp/plugins/dynamix/nchan/parity_listsbin/mover
🚧 Files skipped from review as they are similar to previous changes (1)
- emhttp/plugins/dynamix/ArrayOperation.page
Eli Bosley (elibosley)
added
7.4.0
Eli Bosley (elibosley)
changed the title
18 participants
Initial code for 6.10+
Summary by CodeRabbit