Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

libnetwork: fix sandbox restore #45654

Merged
merged 2 commits into from May 30, 2023

Conversation

corhere
Copy link
Contributor

@corhere corhere commented May 30, 2023

- What I did
Fixed passing the list of interfaces to restore.

- How I did it
By passing it as structured data.

- How to verify it
Live-restored containers can resolve other restored containers attached to the same networks over DNS.

docker stats shows nonzero NET I/O stats for restored containers.

- Description for the changelog

  • Fixed an issue which caused container networking of live-restored containers to malfunction in various ways after restarting the daemon

- A picture of a cute animal (not mandatory but encouraged)

Signed-off-by: Cory Snider <csnider@mirantis.com>
The method to restore a network namespace takes a collection of
interfaces to restore with the options to apply. The interface names are
structured data, tuples of (SrcName, DstPrefix) but for whatever reason
are being passed into Restore() serialized to strings. A refactor,
f0be4d1, accidentally broke the
serialization by dropping the delimiter. Rather than fix the
serialization and leave the time-bomb for someone else to trip over,
pass the interface names as structured data.

Signed-off-by: Cory Snider <csnider@mirantis.com>
Copy link
Member

@akerouanton akerouanton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Member

@neersighted neersighted left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like this regressed in f0be4d1 🙈

@corhere
Copy link
Contributor Author

corhere commented May 30, 2023

@neersighted indeed. I already referenced the offending commit in my commit message.

@neersighted
Copy link
Member

Yup, just extremely late to the party -- I was looking at the diff locally trying to figure out how this even happened. Of course, after tracking it down in the blame, I noticed your comment 😆

I guess forgetting to check the commit message is even sillier 😅

@corhere corhere merged commit a254346 into moby:master May 30, 2023
101 of 102 checks passed
@corhere corhere deleted the libn/fix-embedded-resolver-live-reload branch May 30, 2023 19:46
@thaJeztah
Copy link
Member

Looks like this regressed in f0be4d1 🙈

Yes. Took me a minute to spot my mistake even after Cory posted the line where my mistake was 🥹.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

docker stats net I/O do not work after 24.0.x upgrade, works after container restart
4 participants