Repository navigation
classifier: skip Docker container processes to fix docker exec failure - #334
Conversation
Qualcomm AI ReviewClick to expand Code ReviewReviewed Commits: b192511
URM was moving Docker container init processes out of their Add isContainerProcess() to detect container init processes by Signed-off-by: Varun Singhal varusing@qti.qualcomm.com This PR adds logic to detect and skip Docker container init processes when deciding whether to classify/move a process, in order to avoid breaking Files Changed:
Overview: The change is functionally reasonable and well-scoped, but the substring-based detection logic for identifying Docker container cgroup scopes is somewhat fragile. No critical/blocking issues were found. [Functionality] Fragile substring matching for container-scope detection - MediumIn Fixed Code Snippet: #include <regex>
...
static const std::regex kDockerScopeRegex(R"(docker-[0-9a-fA-F]+\.scope)");
...
while(std::getline(cgroupFile, line)) {
if(std::regex_search(line, kDockerScopeRegex)) {
return true;
}
} |
|
|
||
| std::ifstream cgroupFile("/proc/" + std::to_string(pid) + "/cgroup"); | ||
| if(cgroupFile.is_open()) { | ||
| std::string line; | ||
| while(std::getline(cgroupFile, line)) { | ||
| if(line.find("docker-") != std::string::npos && | ||
| line.find(".scope") != std::string::npos) { | ||
| return true; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
[Functionality] Fragile substring matching for container-scope detection - Medium
In isContainerProcess, the check line.find("docker-") != std::string::npos && line.find(".scope") != std::string::npos only verifies that both substrings exist somewhere in the line, without validating their relative order or that they form a single contiguous token (e.g., docker-<CID>.scope). This could theoretically produce false positives on unusual cgroup path strings that happen to contain both substrings unrelated to a real docker scope, or false negatives if .scope appears before docker- in some other combined cgroup line format. A regex-based match anchored to the expected pattern (e.g., docker-[A-Za-z0-9]+\.scope) would be more robust.
Fixed Code Snippet
#include <regex>
...
static const std::regex kDockerScopeRegex(R"(docker-[0-9a-fA-F]+\.scope)");
...
while(std::getline(cgroupFile, line)) {
if(std::regex_search(line, kDockerScopeRegex)) {
return true;
}
}URM was moving Docker container init processes out of their docker-<CID>.scope cgroup, causing systemd to remove the empty scope and breaking docker exec with "can't open cgroup" error. Add isContainerProcess() to detect container init processes by reading /proc/<pid>/cgroup and skip them in shouldProcBeIgnored(). CRs-Fixed: 4689720 Signed-off-by: Varun Singhal <varusing@qti.qualcomm.com>
b192511 to
ac90224
Compare
Qualcomm AI ReviewClick to expand Code ReviewReviewed commit: ac90224 "classifier: skip Docker container processes to fix docker exec failure URM was moving Docker container init processes out of their Add isContainerProcess() to detect container init processes by CRs-Fixed: 4689720 Signed-off-by: Varun Singhal varusing@qti.qualcomm.com" This PR adds Docker-container detection to the ContextualClassifier so that container init processes (identified via
Overall the change is small and self-contained. A main concern is the fairly loose substring-based matching logic used to detect the docker scope line, which is worth a second look but is not a blocking correctness issue. [Functionality] Substring-based docker scope detection may produce false positivesThe detection logic checks whether a single cgroup line contains both Consider using a more precise match, e.g., checking that Fixed Code Snippet: // Example of tighter matching using find + suffix check
size_t dockerPos = line.find("docker-");
if(dockerPos != std::string::npos &&
line.compare(line.size() - 6, 6, ".scope") == 0) {
return true;
} |
| // - CRI-O (OpenShift / k8s alternative): cgroup path contains "crio-<CID>.scope" | ||
|
|
||
| std::ifstream cgroupFile("/proc/" + std::to_string(pid) + "/cgroup"); | ||
| if(cgroupFile.is_open()) { | ||
| std::string line; | ||
| while(std::getline(cgroupFile, line)) { | ||
| if(line.find("docker-") != std::string::npos && | ||
| line.find(".scope") != std::string::npos) { | ||
| return true; | ||
| } | ||
| } | ||
| } | ||
| return false; | ||
| } | ||
|
|
||
| int8_t ContextualClassifier::shouldProcBeIgnored(int32_t evType, pid_t pid) { | ||
| if(evType == CC_APP_CLOSE) { | ||
| return false; |
There was a problem hiding this comment.
[Functionality] Substring-based docker scope detection may produce false positives
The detection logic checks whether a single cgroup line contains both "docker-" and ".scope" substrings anywhere in the line, without validating structure (e.g., that they are contiguous as docker-<CID>.scope). A cgroup line with unrelated content that happens to include both substrings (e.g., a path segment named similarly) could be misidentified as a container process, causing legitimate processes to be skipped by URM.
Consider using a more precise match, e.g., checking that .scope immediately follows the docker-<CID> pattern, or using a regex to validate the expected format.
Fixed Code Snippet
// Example of tighter matching using find + suffix check
size_t dockerPos = line.find("docker-");
if(dockerPos != std::string::npos &&
line.compare(line.size() - 6, 6, ".scope") == 0) {
return true;
}| LOGI(CLASSIFIER_TAG, "Successfully loaded static proc data"); | ||
| } | ||
|
|
||
| int8_t ContextualClassifier::isContainerProcess(pid_t pid) { |
There was a problem hiding this comment.
I think the current name-based filtering is more of a Docker-specific workaround. Since systemd-run, systemd-nspawn, and other cgroup-managed workloads may run into the same issue, is there a more generic solution based on cgroup hierarchy rather than process/container names?
There was a problem hiding this comment.
Yes, this is intentionally specific to Docker.
Consider this a hotfix for the issue reported in https://orbit/CR/4689720. A more generic solution (e.g. detecting any external cgroup manager, not just Docker) that we can track through #333 urm issue
| // - containerd (k8s default runtime): cgroup path contains "containerd-<CID>.scope" | ||
| // - CRI-O (OpenShift / k8s alternative): cgroup path contains "crio-<CID>.scope" | ||
|
|
||
| std::ifstream cgroupFile("/proc/" + std::to_string(pid) + "/cgroup"); |
There was a problem hiding this comment.
can't we add this to a exclusion list that classifier already has, why whitelisting code needed?
There was a problem hiding this comment.
-
docker run
→ we Container(repo) created using this command (docker run -dit --name repro python:3.12-slim)
→ New container PID 2223 placed in system.slice/docker-bd04adaedc13.scope ✅ -
URM kicks in
→ Sees PID 2223 (python3 one)
→ Moves it to urm.slice/focused.apps⚠️ -
docker-bd04adaedc13.scope is now EMPTY
→ systemd deletes the scope 🗑️ -
docker exec repro /bin/true
→ runc looks for PID 2223 at docker-bd04adaedc13.scope
→ SCOPE IS GONE 💥
→ "can't open cgroup: no such file or directory" ❌so all docker images type we have add in blocker list. that's why come with this solution.
455f2e5
into
qualcomm:main
URM was moving Docker container init processes out of their docker-.scope cgroup, causing systemd to remove the empty scope and breaking docker exec with "can't open cgroup" error.
Add isContainerProcess() to detect container init processes by reading /proc//cgroup and skip them in shouldProcBeIgnored().