Refactor/improve prepareCriuRestoreMounts#4765
Conversation
1. Replace the big "if !" block with the if block and continue, simplifying the code flow. 2. Move comments closer to the code, improving readability. This commit is best reviewed with --ignore-all-space or similar. Signed-off-by: Kir Kolyshkin <[email protected]>
It makes sense to ignore cgroup mounts much early in the code, saving some time on unnecessary operations. Signed-off-by: Kir Kolyshkin <[email protected]>
Since its code is now trivial, and it is only called from a single place, it does not make sense to have it as a separate function. Signed-off-by: Kir Kolyshkin <[email protected]>
Instead of generating a list of tmpfs mount and have a special function to check whether the path is in the list, let's go over the list of mounts directly. This simplifies the code and improves readability. Signed-off-by: Kir Kolyshkin <[email protected]>
rata
left a comment
rata
left a comment
There was a problem hiding this comment.
LGTM
Sorry I didn't see this before. Don't hesitate to ping the next time! :)
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the prepareCriuRestoreMounts function to improve readability and performance while maintaining the same behavior. The changes streamline the mount preparation logic during CRIU restore operations.
- Removes helper functions
makeCriuRestoreMountpointsandisPathInPrefixList - Inlines mount creation logic directly into the main function
- Replaces the two-pass approach with a single pass through mounts
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| if strings.HasPrefix(path, p+"/") { | ||
| func isOnTmpfs(path string, mounts []*configs.Mount) bool { | ||
| for _, m := range mounts { | ||
| if m.Device == "tmpfs" && strings.HasPrefix(path, m.Destination+"/") { |
There was a problem hiding this comment.
The string comparison logic has changed from the original isPathInPrefixList function. The original checked for path starting with p+"/" but this checks for path starting with m.Destination+"/". This could miss cases where path exactly equals m.Destination (without the trailing slash), which the original logic would have caught since it also checked path == p.
| if m.Device == "tmpfs" && strings.HasPrefix(path, m.Destination+"/") { | |
| if m.Device == "tmpfs" && (path == m.Destination || strings.HasPrefix(path, m.Destination+"/")) { |
There was a problem hiding this comment.
which the original logic would have caught since it also checked
path == p.
In which line?
|
@cyphar Perhaps we should consider backporting this change to the release-1.x branches, even though it's primarily a code refactor. This would help avoid potential merge conflicts and inconveniences when integrating future changes to this file. |
|
When we hit a case of a cherry-pick not working, I'd be fine with the backport. |
kolyshkin
added
the
backport/1.3-done
kolyshkin
added
the
backport/1.2-done
|
@rata I've hit it with a backport, we should backport it imho. |
This makes
prepareCriuRestoreMountsmore readable, smaller, and slightly faster.Split to 4 commits for easier review.
Should not change behavior in any way.
Covered by existing tests in
tests/integration/checkpoint.bats.