[4.7] libct/cg/sd: reconnect and retry on dbus connection error #8
Conversation
In case it takes more than 1 second for systemd to create a unit, startUnit() times out with a warning and then runc proceeds (to create cgroups using fs manager and so on). Now runc and systemd are racing, and multiple scenarios are possible. In one such scenario, by the time runc calls systemd manager's Apply() the unit is not yet created, the dbusConnection.SetUnitProperties() call fails with "unit xxx.scope not found", and the whole container start also fails. To eliminate the race, we need to return an error in case the timeout is hit. To reduce the chance to fail, increase the timeout from 1 to 30 seconds, to not error out too early on a busy/slow system (and times like 3-5 seconds are not unrealistic). While at it, as the timeout is quite long now, make sure to not leave a stray timer. Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com> (cherry picked from commit 3844789) Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
As the caller of this function just logs the error, it does not make sense to pass it. Instead, log it (once) and return -1. This is a preparation for the second user. Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com> (cherry picked from commit eee425f) [Minor merge conflict due to missing "return" removed by a hunk from commit 978fa6e] Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
[@kolyshkin: documentation nits] Signed-off-by: Shiming Zhang <wzshiming@foxmail.com> Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com> (cherry picked from commit cdbed6f) [minor merge conflict due to missing upstream commit 73f22e7] Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Generalize isUnitExists as isDbusError, and use errors.As while at it (which can handle wrapped errors as well). Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com> (cherry picked from commit bacfc2c) Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
[@kolyshkin: doc nits, use dbus.ErrClosed and isDbusError] Signed-off-by: Shiming Zhang <wzshiming@foxmail.com> Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com> (cherry picked from commit 15fee98) Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Signed-off-by: Shiming Zhang <wzshiming@foxmail.com> Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com> (cherry picked from commit 6122bc8) Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Instead of reconnecting to dbus after some failed operations, and returning an error (so a caller has to retry), reconnect AND retry in place for all such operations. This should fix issues caused by a stale dbus connection after e.g. a dbus daemon restart. Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com> (cherry picked from commit 47ef9a1) [Minor merge conflicts due to missing upstream commits 52390d6 and af521ed.] Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
|
@kolyshkin: No Bugzilla bug is referenced in the title of this pull request. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
1 similar comment
|
@kolyshkin: No Bugzilla bug is referenced in the title of this pull request. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
kolyshkin
changed the title
openshift-ci-robot
added
bugzilla/severity-medium
|
@kolyshkin: This pull request references Bugzilla bug 1941456, which is invalid:
Comment In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
/bugzilla refresh |
|
@kolyshkin: This pull request references Bugzilla bug 1941456, which is invalid:
Comment In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
kolyshkin
changed the title
|
@kolyshkin: No Bugzilla bug is referenced in the title of this pull request. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
openshift-ci-robot
removed
bugzilla/severity-medium
openshift-ci-robot
added
the
do-not-merge/work-in-progress
|
This also needs opencontainers#2936 / opencontainers#2937 backported. |
Commit 47ef9a1 forgot to wrap GetManagerProperty("ControlGroup") into retryOnDisconnect. Since there's one other user of GetManagerProperty, add getManagerProperty wrapper and use it. Fixes: 47ef9a1 Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com> (cherry picked from commit 99c5c50) Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Using per cgroup manager dbus connection instances means that every cgroup manager instance gets a new connection, and those connections are never closed, ultimately resulting in file descriptors limit being hit. Revert back to using a single global dbus connection for everything, without changing the callers. NOTE that it is assumed a runtime can't use both root and rootless dbus at the same time. If this happens, we panic. Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com> (cherry picked from commit c7f847e) Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
|
Added a backport of opencontainers#2937 (modulo test) |
|
@kolyshkin: No Bugzilla bug is referenced in the title of this pull request. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
|
@kolyshkin: No Bugzilla bug is referenced in the title of this pull request. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
1 similar comment
|
@kolyshkin: No Bugzilla bug is referenced in the title of this pull request. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
This also needs opencontainers#2997 |
This fixes isDbusError function, introduced by commit bacfc2c. Due to a type error it was not working at all. This also fixes the whole "retry on dbus disconnect" logic. This also fixes a regression in startUnit (and cgroupManager.Apply()), which should never return "unit already exists" error but it did. Fixes: bacfc2c Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com> (cherry picked from commit b2d28c5, modulo test) Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Noticed that the check of trying to use both rootful and rootless in NewDbusConnManager never worked, as we never set dbusInited to true. Do that. While at it, protect this with the mutex (against the case of two goroutines simultaneously calling NewDbusConnManager). This is a rare call, so taking read-only then read-write mutex does not make sense. Fixes: c7f847e Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com> (cherry picked from commit d06bda6) Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
|
@kolyshkin: No Bugzilla bug is referenced in the title of this pull request. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
Issues go stale after 90d of inactivity. Mark the issue as fresh by commenting If this issue is safe to close now please do so with /lifecycle stale |
openshift-ci
bot
added
the
lifecycle/stale
|
Stale issues rot after 30d of inactivity. Mark the issue as fresh by commenting If this issue is safe to close now please do so with /lifecycle rotten |
openshift-ci
bot
added
lifecycle/rotten
|
Rotten issues close after 30d of inactivity. Reopen the issue by commenting /close |
|
@openshift-bot: Closed this PR. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
This is a backport of upstream PRs
to
release-4.7branch, to be consumed by cri-o 1.20 in order to fix https://bugzilla.redhat.com/show_bug.cgi?id=1941456.This also includes backports of
mostly to make the merge simpler.
Original description follows
n case the dbus daemon is ever restarted, the connection is no longer valid and every operation
fails. This is a minor concern for short-lived runc, but much more of a issue in case there is
a long-running daemon (e.g.
cri-o) is using runc's libcontainer, as the connection is neverretried and the only remedy is to restart the daemon.
The solution to the above is to check the errors returned for
dbus: connection closed by usererror, and try to re-connect on that. This is what PR opencontainers#2862 does.
This is a carry of opencontainers#2862, implementing the idea of retry-in-place (first described
at opencontainers#2862 (comment) and opencontainers#2862 (comment)) on top of what it does.
For more info, see commit messages as well as opencontainers#2862.
Fixes: