Skip to content
This repository has been archived by the owner on Dec 7, 2023. It is now read-only.

Teardown IPMasq rules for all actual configured bridges instead of using the hardcoded default string #461

Merged
merged 1 commit into from
Sep 27, 2019

Conversation

stealthybox
Copy link
Contributor

@stealthybox stealthybox commented Sep 26, 2019

Followup #460

This logic seems to work well.
I added a trace line.

This protects us from a class of network removal bugs when stopping vm's that were created on the previous network names or CNI networks that are user configured.

You can create a vm with an old version of ignite on the 172.18/16 network, stop it with this new version without error.
If you remove the CNI configuration and then start it again it will be on the new 10.61/16 network.

This gives us our migration plan.

@stealthybox stealthybox requested a review from twelho as a code owner September 26, 2019 23:30
@stealthybox stealthybox requested review from chanwit and removed request for twelho September 26, 2019 23:32
@stealthybox
Copy link
Contributor Author

stealthybox commented Sep 26, 2019

++ @najeal

@stealthybox stealthybox added area/networking Issues related to networking kind/bug Categorizes issue or PR as related to a bug. labels Sep 26, 2019
@stealthybox stealthybox added this to the v0.6.1 milestone Sep 26, 2019
pkg/network/cni/cni.go Show resolved Hide resolved
pkg/network/cni/cni.go Show resolved Hide resolved
pkg/network/cni/cni.go Outdated Show resolved Hide resolved
@stealthybox stealthybox force-pushed the generic-bridge-teardown branch from dba82ba to 32a5341 Compare September 27, 2019 06:32
@stealthybox
Copy link
Contributor Author

@chanwit I consolidated the errors.

I moved this cleanup into function.
It's now called before fetching the netNSPath which means cleanup of the chains will still happen if the container no longer exists.

The error is wrapped with the util defer helper.

Copy link
Member

@chanwit chanwit left a comment

Choose a reason for hiding this comment

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

lgtm

return err
}

cleanupErr := plugin.cleanupBridges(containerID)
if cleanupErr != nil {
defer util.DeferErr(&err, func() error {
Copy link
Contributor

Choose a reason for hiding this comment

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

why do you defer the return error?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can still try to remove the network if the cleanup fails.

@stealthybox
Copy link
Contributor Author

stealthybox commented Sep 27, 2019

PR description edit:

This protects us from a class of network removal bugs when stopping vm's that were created on the previous network names or CNI networks that are user configured.

You can create a vm with an old version of ignite on the 172.18/16 network, stop it with this new version without error.
If you remove the CNI configuration and then start it again it will be on the new 10.61/16 network.

This gives us our migration plan.

@stealthybox stealthybox merged commit 809db18 into weaveworks:master Sep 27, 2019
@stealthybox stealthybox deleted the generic-bridge-teardown branch September 27, 2019 19:20
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area/networking Issues related to networking kind/bug Categorizes issue or PR as related to a bug.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants