Implement pod-network-reload #8571
Merged
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
openshift-ci-robot
added
the
do-not-merge/work-in-progress
Luap99
force-pushed
the
podman-network-reload
branch
from
34990a4 to
d471398
Compare
Luap99
commented
Dec 3, 2020
libpod/networking_linux.go
Outdated
Comment on lines
884
to
886
| // FIXME: There two bad logrus.Error in the ocicni lib we should remove them | ||
| // https://github.com/cri-o/ocicni/blob/5243a9f23f8a9c1ab706cf1632760cb49eb2277f/pkg/ocicni/ocicni.go#L640 | ||
| // https://github.com/cri-o/ocicni/blob/5243a9f23f8a9c1ab706cf1632760cb49eb2277f/pkg/ocicni/ocicni.go#L774 |
There was a problem hiding this comment.
$ sudo bin/podman network reload --all
ERRO[0000] Error deleting network: running [/usr/sbin/iptables -t nat -D POSTROUTING -s 10.88.0.48 -j CNI-2210851933fa8e49e3ef4f38 -m comment --comment name: "podman" id: "b1b538e8bc4078fc3ee1c95b666ebc7449b9a97bacd15bcbe464a29e1be59c1c" --wait]: exit status 2: iptables v1.8.4 (legacy): Couldn't load target `CNI-2210851933fa8e49e3ef4f38':No such file or directory
Try `iptables -h' or 'iptables --help' for more information.
ERRO[0000] Error while removing pod from CNI network "podman": running [/usr/sbin/iptables -t nat -D POSTROUTING -s 10.88.0.48 -j CNI-2210851933fa8e49e3ef4f38 -m comment --comment name: "podman" id: "b1b538e8bc4078fc3ee1c95b666ebc7449b9a97bacd15bcbe464a29e1be59c1c" --wait]: exit status 2: iptables v1.8.4 (legacy): Couldn't load target `CNI-2210851933fa8e49e3ef4f38':No such file or directory
Try `iptables -h' or 'iptables --help' for more information.
ERRO[0000] Error deleting network: running [/usr/sbin/iptables -t nat -D POSTROUTING -s 10.88.0.47 -j CNI-520223cf89670efdff5d88d8 -m comment --comment name: "podman" id: "fe7e8eca56f844ec33af10f0aa3b31b44a172776e3277b9550a623ed5d96e72b" --wait]: exit status 2: iptables v1.8.4 (legacy): Couldn't load target `CNI-520223cf89670efdff5d88d8':No such file or directory
Try `iptables -h' or 'iptables --help' for more information.
ERRO[0000] Error while removing pod from CNI network "podman": running [/usr/sbin/iptables -t nat -D POSTROUTING -s 10.88.0.47 -j CNI-520223cf89670efdff5d88d8 -m comment --comment name: "podman" id: "fe7e8eca56f844ec33af10f0aa3b31b44a172776e3277b9550a623ed5d96e72b" --wait]: exit status 2: iptables v1.8.4 (legacy): Couldn't load target `CNI-520223cf89670efdff5d88d8':No such file or directory
Try `iptables -h' or 'iptables --help' for more information.
b1b538e8bc4078fc3ee1c95b666ebc7449b9a97bacd15bcbe464a29e1be59c1c
fe7e8eca56f844ec33af10f0aa3b31b44a172776e3277b9550a623ed5d96e72b
These errors are terrible the end user doesn't care and the command still works
There was a problem hiding this comment.
I opened cri-o/ocicni#83 to remove these errors
Luap99
force-pushed
the
podman-network-reload
branch
from
d471398 to
de988cc
Compare
|
@mheon PTAL |
Luap99
force-pushed
the
podman-network-reload
branch
from
de988cc to
a081180
Compare
edsantiago
reviewed
Dec 3, 2020
Luap99
force-pushed
the
podman-network-reload
branch
from
a081180 to
eef9267
Compare
Luap99
commented
Dec 3, 2020
Luap99
force-pushed
the
podman-network-reload
branch
from
eef9267 to
de0c479
Compare
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Luap99 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
the
approved
mheon
reviewed
Dec 7, 2020
libpod/networking_linux.go
Outdated
| // teardownCNI will error if the iptables rules do not exists and this is the case after | ||
| // a firewall reload. The purpose of network reload is to recreate the rules if they do | ||
| // not exists so we should ignore possible errors. | ||
| _ = r.teardownCNI(ctr) |
There was a problem hiding this comment.
Should we be ignoring the error here? Feels like we should at least logrus it.
There was a problem hiding this comment.
see #8571 (comment) I don't want to display this for users by default. Maybe logrus.Info is good enough?
There was a problem hiding this comment.
Maybe we should string-parse the error and discard any No such file or directory errors, and treat others as real errors?
|
One nit, otherwise LGTM |
|
Thanks for taking this one over @Luap99 |
This adds a new command, 'podman network reload', to reload the networks of existing containers, forcing recreation of firewall rules after e.g. `firewall-cmd --reload` wipes them out. Under the hood, this works by calling CNI to tear down the existing network, then recreate it using identical settings. We request that CNI preserve the old IP and MAC address in most cases (where the container only had 1 IP/MAC), but there will be some downtime inherent to the teardown/bring-up approach. The architecture of CNI doesn't really make doing this without downtime easy (or maybe even possible...). At present, this only works for root Podman, and only locally. I don't think there is much of a point to adding remote support (this is very much a local debugging command), but I think adding rootless support (to kill/recreate slirp4netns) could be valuable. Signed-off-by: Matthew Heon <[email protected]> Signed-off-by: Paul Holzinger <[email protected]>
Luap99
force-pushed
the
podman-network-reload
branch
from
de0c479 to
b0286d6
Compare
|
LGTM! |
openshift-ci-robot
removed
the
do-not-merge/work-in-progress
|
/lgtm Nice work @Luap99 |
This was referenced Dec 24, 2020
Luap99
pushed a commit
to Luap99/libpod
that referenced
this pull request
Jan 14, 2021
The changes from cri-o/ocicni#83 are needed to improve the user experience when using the new network reload command. see: containers#8571 (comment) Signed-off-by: Paul Holzinger <[email protected]>
github-actions
bot
added
the
locked - please file new issue/PR
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
This adds a new command, 'podman network reload', to reload the
networks of existing containers, forcing recreation of firewall
rules after e.g.
firewall-cmd --reloadwipes them out.Under the hood, this works by calling CNI to tear down the
existing network, then recreate it using identical settings. We
request that CNI preserve the old IP and MAC address in most
cases (where the container only had 1 IP/MAC), but there will be
some downtime inherent to the teardown/bring-up approach. The
architecture of CNI doesn't really make doing this without
downtime easy (or maybe even possible...).
At present, this only works for root Podman, and only locally.
I don't think there is much of a point to adding remote support
(this is very much a local debugging command), but I think adding
rootless support (to kill/recreate slirp4netns) could be
valuable.