NewSystemd handles UnitExists when starting units #290
Conversation
mmerkes
force-pushed
the
unitexists
branch
2 times, most recently
from
699e348 to
8565c75
Compare
mmerkes
changed the title
|
@kolyshkin Let me know if you have feedback here since you opened the original issue and made the runc change. Tx! |
There was a problem hiding this comment.
Alas, I am not a maintainer and can't say which uses/users this method has.
But nevertheless a fix needs to be made, because the current behavior (of not adding the container into a proper cgroup) is totally unacceptable (and can, in some circumstances, be qualified as a security issue).
So, I'm adding my non-binding LGTM, but this needs a review from the actual maintainers of this package.
.gitignore
Outdated
| @@ -1,2 +1,5 @@ | |||
| example/example | |||
| cmd/cgctl/cgctl | |||
|
|
|||
| *.swp | |||
| coverage.txt | |||
There was a problem hiding this comment.
These settings should be in your home directory
As done in opencontainers/runc#3782, UnitExists errors are no longer ignored when starting units. This change makes the logic more robust like in runc: 1. Attempts to reset a failed unit if it already exists 2. Verifies via the unit status that it successfully starts 3. Waits longer for unit to start 4. Continues to ignore unit existing when pid is -1 to accommodate kubelet use case 5. Otherwise, returns an error if it already exists Signed-off-by: Matt Merkes <[email protected]> Update .gitignore Signed-off-by: Matt <[email protected]>
Updated. Falsely assumed that the merge would squash. @AkihiroSuda |
|
@AkihiroSuda This good to be merged? |
As done in this runc PR
opencontainers/runc#3782,
UnitExists errors are no longer ignored when starting units. If it already exists, we attempt to reset the failed unit and retry once. Otherwise, we fail. See #279 for more context.
Fixes #279
In generally, I'm following runc's example and making this code more robust. See code and commit for more details. Ideally, we'd reuse the code, but don't currently have a dependency on
runcas a library here and I don't have enough context to know if that would be reasonable or not.