Unconditionally remove conmon files before starting #4065
Conversation
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: mheon The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci-robot
added
approved
|
LGTM |
|
LGTM |
|
Testing is not happy though. |
|
This broke starting containers on my Workstation 29 where things were somehow working regardless of the warning about |
|
Oh, wait, wait, wait. This is happening too late in the process. Needs to be attached to |
We've been seeing a lot of issues (ref: containers#4061, but there are others) where Podman hiccups on trying to start a container, because some temporary files have been retained and Conmon will not overwrite them. If we're calling start() we can safely assume that we really want those files gone so the container starts without error, so invoke the cleanup routine. It's relatively cheap (four file removes) so it shouldn't hurt us that much. Also contains a small simplification to the removeConmonFiles logic - we don't need to stat-then-remove when ignoring ENOENT is fine. Signed-off-by: Matthew Heon <[email protected]>
mheon
force-pushed
the
unconditional_conmon_rm
branch
from
65a7db2 to
407fba4
Compare
Ok, so my Workstation 29 is back to being as it used to be. Let me now see if it unbreaks |
|
Next step on the logs is to get a print statement in stating our default log level, to see if it changed |
|
As before, the first failed |
| return errors.Wrapf(err, "error removing container %s exit file", c.ID()) | ||
| } | ||
| if err := os.Remove(exitFile); err != nil && !os.IsNotExist(err) { | ||
| return errors.Wrapf(err, "error removing container %s exit file", c.ID()) |
There was a problem hiding this comment.
Going to throw out the question as I'm not sure. Would it be better to not return an error for each of these attempts? I.e. If one fails, note it, then try removing the next file, if that fails note it, then try next? Then at the end report all of the errors encountered? Otherwise, if there are multiple things wrong, will the user be able to clean up/investigate fully?
|
@vrothberg I don't know if this one actually solved the issue, which is why I'm sitting on it - @debarshiray indicates that it didn't, which makes me concerned that I hit a symptom and the underlying problem is unaddressed. |
vrothberg
changed the title
openshift-ci-robot
added
the
do-not-merge/work-in-progress
|
Added a |
If you look very closely at #4061 and read between the lines, then it seems that this pull request at least made things move further. |
Ok, I have a better understanding of #4061 now. I don't think we should block this pull request just because it doesn't address #4061 fully. The remaining issue in #4061 is unrelated to this PR in my opinion. |
|
/hold cancel |
mheon
changed the title
openshift-ci-robot
removed
the
do-not-merge/work-in-progress
|
/lgtm |
github-actions
bot
added
the
locked - please file new issue/PR
We've been seeing a lot of issues (ref: #4061, but there are others) where Podman hiccups on trying to start a container, because some temporary files have been retained and Conmon will not overwrite them.
If we're calling start() we can safely assume that we really want those files gone so the container starts without error, so invoke the cleanup routine. It's relatively cheap (four file removes) so it shouldn't hurt us that much.
Also contains a small simplification to the removeConmonFiles logic - we don't need to stat-then-remove when ignoring ENOENT is fine.