cgroups-rs: fix verify_path() logic#159
Conversation
src/fs/mod.rs
Outdated
|
|
||
| fn verify_path(&self) -> Result<()> { | ||
| if self.get_path().starts_with(self.get_base()) { | ||
| if self.get_path().is_absolute() { |
There was a problem hiding this comment.
Well, this is the common part of cgroupfs. I think we only need to make sure whether the get_path() exists or not.
There was a problem hiding this comment.
Actually, verify_path() is in front of the cgroupfs directory creation process, so we cannot verify whether get_path() exists prior to its execution.
There was a problem hiding this comment.
IMHO, there is no relation between the path and the base. According to the current codebase, the path is the mount point, and the base is the mount root. Both of them are read from /proc/self/mountinfo. Without DinD, self.get_path().starts_with(self.get_base()) works coincidentally. The typical root is /, and controllers are under /sys/fs/cgroup. With DinD, a mount namespace might be applied. The mount root differs from container engines and is unpredictable. In your case, it is /docker/{xxxx}. So my idea is that we can remove verify_path() function completely. WDYT?
There was a problem hiding this comment.
I found that the verify_path was introduced in commit c63dad4. Before commit 4005ad8, the root and base are the same one, see 4005ad8#diff-f058debe54b01b2b04c284d1b5ec01250c4cfce0fd4508222079b87dbec9416eL433-R436. IMHO, verify_path never throws errors previously and is completely useless. Removing it is reasonable to me.
There was a problem hiding this comment.
Need some input from @lifupan @Tim-Zhang @quanweiZhou
There was a problem hiding this comment.
Agreed with removing it. In version 0.4.0, the mount point behavior differs between Pouch and Docker environments - the former is /, while the latter is /docker/$cid. The behavior of this function is indeed somewhat unpredictable. cc @quanweiZhou
There was a problem hiding this comment.
Need some input from @lifupan @Tim-Zhang @quanweiZhou
From the current information, this validation doesn't seem to serve much purpose and could actually introduce problems. I support removing this validation.
There was a problem hiding this comment.
Apologies for the delay. After removing verify_path(), during regression testing of our project, we encountered a sporadic issue where creating cgroups occasionally fails. Once I identify the root cause and find an appropriate fix, I'll update the patch here.
|
Hey @Lu-yq, how is the progress going? We are going to release a new version soon, the next version might be a couple of months after that. /cc @Tim-Zhang @pmores |
|
Hi @Lu-yq , We got a CI error: Please fix it, thanks |
Lu-yq
force-pushed
the
fix-verify-path
branch
from
293423c to
cca6819
Compare
remove the verify_path() logic because the semantics change of controller.base. Signed-off-by: Lu-yq <443471302@qq.com>
Lu-yq
force-pushed
the
fix-verify-path
branch
from
611f468 to
c1a99ff
Compare
|
I have removed the verify_path() now! plz have a look @justxuewei @Tim-Zhang cc @quanweiZhou |
Bump major(minor actually) version for incompatible changes in kata-containers#154. Changelog: - kata-containers#152 - kata-containers#154 - kata-containers#159 Signed-off-by: Tim Zhang <tim@hyper.sh>
4 participants
Because the semantics of controller.base has changed, the logic for verify_path should be modified.
Fixes: #158