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

Fix host networking issues and rename incidental issue #3344

Merged
merged 6 commits into from
Aug 23, 2024

Conversation

apostasie
Copy link
Contributor

@apostasie apostasie commented Aug 22, 2024

Purported to:

Does NOT fix yet #355

The take-away are:

  1. the modification to make network=host use the host hostname as a default is straightforward
  2. host networking previously did not call Acquire on the hosts store, causing Update to fail during a rename operation. Note that the same is true for none. Rename has been modified to not hard fail anymore on hostsstore Update failures. Another PR (Rename rollback on error #3342) is also making rename able to rollback in case of failure
  3. for the same reason as above, extraHosts were not added - this has now been done in SetupNetworking and CleanupNetworking for the hostNetworkManager

Note that calls to CleanupNetworking and SetupNetworking were previously gated by a platform check to apply only on Windows (although not everywhere).
Now, this does not seem to serve much of a purpose, as these methods did nothing for any of the networkManager (with the exception of the Windows implementation of CNI).
Is this some kind of unfinished refactor?

Anyhow, I did remove the platform checks altogether, since the code decides already if something should happen.

@apostasie apostasie changed the title [WIP] Fix host networking issues and rename incidental issue Fix host networking issues and rename incidental issue Aug 22, 2024
@apostasie apostasie marked this pull request as ready for review August 22, 2024 07:16
@AkihiroSuda AkihiroSuda added this to the v2.0.0 milestone Aug 22, 2024
@apostasie
Copy link
Contributor Author

Note that we will not clean-up meta.json if the container exits on its own.

This is similar to #2993 and #1829 and belongs to a more general class of problem with our network / container lifecycle management (be it CNI or not).

@fahedouch proposal on containerd never went anywhere unfortunately (containerd/containerd#8163).

I think we need to re-evaluate our strategy wrt lifecycle cleanup.

Copy link
Member

@AkihiroSuda AkihiroSuda left a comment

Choose a reason for hiding this comment

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

Thanks

@AkihiroSuda AkihiroSuda merged commit 3511911 into containerd:main Aug 23, 2024
22 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment