Improve Dockerman icon caching. #1146
Conversation
| global $docroot, $dockerManPaths; | ||
| $imgUrl = $this->getTemplateValue($Repository, 'Icon','all',$contName); | ||
| if (!$imgUrl) $imgUrl = $tmpIconUrl; | ||
| $imgUrl = $iconUrl ?: $this->getTemplateValue($Repository, 'Icon','all',$contName); |
There was a problem hiding this comment.
Prefer iconUrl specified by container label if it exists.
| $iconRAM = sprintf('%s/%s-%s.png', $dockerManPaths['images-ram'], $contName, 'icon'); | ||
| $icon = sprintf('%s/%s-%s.png', $dockerManPaths['images'], $contName, 'icon'); | ||
| $imgUrlHash = sha1($imgUrl); | ||
| $iconFile = sprintf('%s-%s.png', 'icon', $imgUrlHash); |
There was a problem hiding this comment.
Instead of storing the icons by container name only, this change uses a sha1 hash of the imgUrl. This way a change to the icon url will automatically invalidate the cached icon.
| $iconFile = sprintf('%s-%s.png', 'icon', $imgUrlHash); | ||
|
|
||
| $iconRAM = sprintf('%s/%s-%s', $dockerManPaths['images-ram'], $contName, $iconFile); | ||
| $icon = sprintf('%s/%s', $dockerManPaths['images'], $iconFile); |
There was a problem hiding this comment.
In images-ram the icon file name contains both the container name and the urlHash so that it is unique for each container.
In images the icons are store by urlHash alone. This has the benefit of storing only a single copy of identical icons.
| return (is_file($iconRAM)) ? str_replace($docroot, '', $iconRAM) : ''; | ||
| } | ||
|
|
||
| public function purgeUnusedIconFiles($contName, $keepIcon='') { |
There was a problem hiding this comment.
This new function better handles the more complex removal process for this caching mechanism.
| if ( ($keepIcon === '') || !(strpos($filename, $keepIcon) !== false) ) { | ||
| @unlink($filename); | ||
| } | ||
| } |
There was a problem hiding this comment.
The first block removes all icons associated with contName in images-ram except the one specified by keepIcon.
| @unlink($filename); | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
The next block removes all icons stored in the old style from the images directory. Ideally this would only need to be run once when the caching mechanism was first upgraded to purge all old icons. Unfortunately i could not think of a good way to make that happen so i settled for always running this check.
| @unlink($filename); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
The final block removes any of the shared icons in images that share the same hash as icons removed in the first block that are not referenced by another icon in images-ram.
| $imgUrl = $iconUrl ?: $this->getTemplateValue($Repository, 'Icon','all',$contName); | ||
| if (!$imgUrl || trim($imgUrl) == "/plugins/dynamix.docker.manager/images/question.png") return ''; | ||
|
|
||
| $imageName = $contName ?: $name; |
There was a problem hiding this comment.
I removed this line specifically because i could not figure out what it was doing. So far as i can tell $name is never set anywhere.
| $iconPath = $DockerTemplates->getIcon($Repository,$Name); | ||
| @unlink("$docroot/$iconPath"); | ||
| @unlink("{$dockerManPaths['images']}/".basename($iconPath)); | ||
| $DockerTemplates->purgeUnusedIconFiles($Name); |
There was a problem hiding this comment.
Since removing an icon from the cache is a bit more complex I use a new function specifically for that purpose.
|
My unRAID server has ~42 containers running with about ~30 unique icons. In my testing the changes to the caching mechanism do not seem to greatly effect the execution time of the getAllInfo function. |
| if ($ct['Icon']) $tmp['icon'] = $ct['Icon']; | ||
| if ( ! $communityApplications ) { | ||
| if (!is_file($tmp['icon']) || $reload) $tmp['icon'] = $this->getIcon($image,$name,$tmp['icon']); | ||
| if (!is_file($tmp['icon']) || $reload) $tmp['icon'] = $this->getIcon($image,$name,$ct['Icon']); |
There was a problem hiding this comment.
If this change is not made then subsequent calls to getAllInfo will invalidate the cache by passing in the icon file path via tmp['icon'] instead of the icon url.
|
It'd be great to get some eyes on this whenever someone has time - I've got between 50 and 60 containers on each of my unraid servers, and loading the docker tab's gotten to feeling pretty clunky at this point, so any performance improvement would be great! |
dcflachs
force-pushed
the
dockerman_icon_patch
branch
from
0365a1e to
a5bd646
Compare
|
Rebased changes onto latest rev |
|
I would love to get some feedback on this issue and proposed fix. |
|
Not my call, but since this is from 2022 you might want to retest the base system on 7.x and resubmit (even though there are no merge conflicts). Quite probable that since it is a very old PR that it's not going to get looked at since there have been numerous changes in DockerClient. |
WalkthroughThe changes refactor the handling of Docker container icon files within the container management logic. The update centralizes the logic for cleaning up unused icon files by introducing a new method, Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CreateDocker
participant DockerTemplates
User->>CreateDocker: Submit container creation/update
CreateDocker->>DockerTemplates: getIcon(repository, name, iconUrl)
DockerTemplates->>DockerTemplates: Generate SHA1 icon filename
DockerTemplates->>DockerTemplates: Download/copy icon if needed
DockerTemplates->>DockerTemplates: purgeUnusedIconFiles(name, keepIcon)
DockerTemplates->>CreateDocker: Return icon path
CreateDocker->>DockerTemplates: purgeUnusedIconFiles(name)
CreateDocker->>User: Complete container creation/update
Poem
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
emhttp/plugins/dynamix.docker.manager/include/DockerClient.php (1)
347-376: 🛠️ Refactor suggestionAvoid “only variables should be passed by reference” notice
Inside
purgeUnusedIconFiles()the suffix is extracted with:$suffix = end(explode('-', $ramFile));
end()expects its argument by reference; passing the result ofexplode()triggers
Only variables should be passed by referenceon PHP 7+ (and still emits a notice on earlier versions).
Assign to a variable first:- $suffix = end(explode('-', $ramFile)); + $parts = explode('-', $ramFile); + $suffix = end($parts);This eliminates the notice and is forward-compatible with PHP 8’s stricter reference handling.
🧹 Nitpick comments (3)
emhttp/plugins/dynamix.docker.manager/include/CreateDocker.php (1)
85-91: Centralised icon-cleanup call looks correct, but consider preserving the current icon when possibleReplacing the hand-written
unlink()sequence with$DockerTemplates->purgeUnusedIconFiles($Name)is a nice consolidation and keeps the template layer in charge of file housekeeping.
One small follow-up: at this point you already know the hash-based filename thatgetIcon()will eventually keep (it’sicon-<sha1>.png) once the new icon is downloaded. If you pass that value as the second argument you avoid a brief window where both the RAM and persistent copies are removed, forcing a re-download on the next page refresh:$keep = sprintf('icon-%s.png', sha1($_POST['contIcon'])); $DockerTemplates->purgeUnusedIconFiles($Name, $keep);Not strictly required, but it shaves an extra HTTP request for users who reopen the Docker tab immediately after clicking Apply.
emhttp/plugins/dynamix.docker.manager/include/DockerClient.php (2)
355-364: Directory creation race – use@mkdirwith the recursive flag
mkdir(dirname($iconRAM), 0755, true)is fine when multiple concurrent calls are unlikely, but if twogetIcon()invocations land at the same time the second one will throw a warning because the directory now exists.
You’re already suppressing IO warnings elsewhere, so consider the same pattern here:- if (!is_dir(dirname($iconRAM))) mkdir(dirname($iconRAM), 0755, true); - if (!is_dir(dirname($icon))) mkdir(dirname($icon), 0755, true); +@mkdir(dirname($iconRAM), 0755, true); +@mkdir(dirname($icon), 0755, true);It keeps the intent while removing a harmless but noisy warning in the logs.
378-406: Pattern mismatch may leave orphaned persistent icons
purgeUnusedIconFiles()deletes persistent icons using the glob pattern
%s/%s*.png(i.e.<contName>*.png).
Since persistent icons are now stored asicon-<sha1>.pngwithout the container name, they are never caught by this first pass and rely solely on the suffix-cleanup logic below.That works, but it’s indirect and means the initial
glob()does unnecessary work.
You could simplify (and speed up) the method by deleting from the persistent directory based on the computed$keepIconinstead:- $icon_glob = sprintf('%s/%s*.png', $dockerManPaths['images'], $contName); + $icon_glob = sprintf('%s/icon-*.png', $dockerManPaths['images']);and keep the “suffix” reconciliation for safety.
Not a correctness bug, but trimming the extra filesystem pass reduces IO when many containers share the same icon.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
emhttp/plugins/dynamix.docker.manager/include/CreateDocker.php(1 hunks)emhttp/plugins/dynamix.docker.manager/include/DockerClient.php(3 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
emhttp/plugins/dynamix.docker.manager/include/CreateDocker.php (1)
emhttp/plugins/dynamix.docker.manager/include/DockerClient.php (1)
purgeUnusedIconFiles(378-406)
|
Bump. Really need this. |
|
I agree. I didn't know what was happening when I added a second container of the same image and they had 2 different icons but came from the same registry. Even if there was a way to only do it via the command line, that would work for me |
6 participants
In 6.10.1+ the dockerman icon caching mechanism does not work correctly for icons specified by the net.unraid.docker.icon container label on containers without a dockerman template. For containers created with this label outside of dockerman the caching mechanism initially downloads and displays the correct icon. If the icon url is changed however the new icon is never downloaded because the old icon remains cached and there is no mechanism in place for invalidating the icon in the absence of a dockerman template. To fix this i propose the following changes to dockerman.
Summary by CodeRabbit
Bug Fixes
Refactor