daemon: handleContainerExit: ignore networking errors #49507
Merged
+52
−1
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
akerouanton
commented
Feb 20, 2025
akerouanton
force-pushed
the
fix-restart-port-already-in-use
branch
from
874796b to
a34be7e
Compare
akerouanton
changed the title
akerouanton
added
area/networking
area/daemon
kind/bugfix
akerouanton
force-pushed
the
fix-restart-port-already-in-use
branch
from
a34be7e to
aa45c80
Compare
vvoland
approved these changes
Feb 20, 2025
daemon/monitor.go
Outdated
| // c.ErrorMsg is set by [daemon.containerStart], and doesn't preserve the | ||
| // error type (because this field is persisted on disk). So, use string | ||
| // matching instead of usual error comparison methods. | ||
| if strings.Contains(c.ErrorMsg, "failed to set up container networking") { |
There was a problem hiding this comment.
For posterity: We could also consider doing that in in general for any c.ErrorMsg != "" error, but for now let's go with a minimal change to get the pre-v28 behavior.
robmry
approved these changes
Feb 20, 2025
daemon/start_linux.go
Outdated
| @@ -34,5 +34,8 @@ func (daemon *Daemon) initializeCreatedTask( | |||
| return errdefs.System(err) | |||
| } | |||
| } | |||
| return daemon.allocateNetwork(ctx, cfg, ctr) | |||
| if err := daemon.allocateNetwork(ctx, cfg, ctr); err != nil { | |||
| return fmt.Errorf("failed to set up container networking: %w", err) | |||
There was a problem hiding this comment.
The test failure should be enough to avoid regressions. But, as these are both in package daemon, could put the error string in a const to make it clear where the message is used?
Prior to commit fe856b9, containers' network sandbox and interfaces were created before the containerd task. Now, it's created after. If this step fails, the containerd task is forcefully deleted, and an event is sent to the c8d event monitor, which triggers `handleContainerExit`. Then this method tries to restart the faulty container. This leads to containers with a published port already in use to be stuck in a tight restart loop (if they're started with `--restart=always`) until the port is available. This is needlessly spamming the daemon logs. Prior to that commit, a published port already in use wouldn't trigger the restart process. This commit adds a check to `handleContainerExit` to ignore exit events if the latest container error is related to networking setup. Signed-off-by: Albin Kerouanton <albinker@gmail.com>
akerouanton
force-pushed
the
fix-restart-port-already-in-use
branch
from
aa45c80 to
ac8b4e3
Compare
vvoland
added
process/cherry-picked
and removed
process/cherry-pick
process/cherry-pick/28.x
labels
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Related to:
- What I did
Prior to commit fe856b9, containers' network sandbox and interfaces were created before the containerd task. Now, it's created after.
If this step fails, the containerd task is forcefully deleted, and an event is sent to the c8d event monitor, which triggers
handleContainerExit. Then this method tries to restart the faulty container.This leads to containers with a published port already in use to be stuck in a tight restart loop (if they're started with
--restart=always) until the port is available. This is needlessly spamming the daemon logs.Prior to that commit, a published port already in use wouldn't trigger the restart process.
- How I did it
This commit adds a check to
handleContainerExitto ignore exit events if the latest container error is related to networking setup.- How to verify it
- Human readable description for the release notes
- A picture of a cute animal (not mandatory but encouraged)