Do not skip bridge creation if bridge exists #3204

brb · 2017-12-18T13:04:57Z

I recommend to review each commit separately.

This PR does a few things:

Removes create_bridge from the weave script. As a consequence, weave attach $CIDR $CONTAINER is no longer possible without running weaver.
Removes unnecessary create_bridge from weave expose and weave hide.
Makes net/bridge.go:EnsureBridge do not skip creation of the bridge if such already exists, and also makes the function idempotent.

Part of the #3133 fix; going to add netlink subscribe for changes to the bridge in a separate PR.

To include LinkNotFoundError.

Both cmds depend on running weaver which creates the bridge during its initialization. So, calling "create_bridge" during execution of the cmds is redundant.

"weave attach $CIDR $CONTAINER_ID" is no longer possible w/o running weaver. This feature was undocumented and complicated the weave script, as "weave attach" required to call "create_bridge". Removing the feature allows us to eliminate the bridge creation from the script. Running "weave attach" w/o weaver results in the following err: bridge "weave" not present; did you launch weave? Also, this commit simplifies the "attach-container" cmd from weaveutil by removing "mtu" as a param. The cmd is used only by "weave attach", and a mtu is value for an attached container interface is exactly the same as one of the weave bridge.

No longer needed in the weave script and thus, in weaveutil.

EnsureBridge used to skip creation and initialization of the weave bridge and friends if such existed. This caused problems when EnsureBridge failed before completing all init steps and so, subsequent weaver could not work without "weave reset". This change makes EnsureBridge idempotent.

deitch · 2017-12-18T13:19:52Z

As commented on the other issue, thank you very much for this. The “reboot bug” bit us more than once.

deitch · 2017-12-18T13:21:20Z

On my phone, so hard to read, but does this also ensure iptables correctness?

brb · 2017-12-18T15:56:29Z

On my phone, so hard to read, but does this also ensure iptables correctness?

Yep, it should.

If you want to do an early testing, please let me know - I could build images for you.

deitch · 2017-12-19T11:25:08Z

I’d be happy to test. We have an infra sandbox where we setup and tear down clusters all the time (automated through terraform) so more than happy to run tests there if you can provide an image.

brb · 2017-12-22T10:00:48Z

@deitch Cool, thanks! You can start using the fixed version (for testing purpose) by replacing weaveworks/weave-{kube,npc}:latest with weaveworks/weave-{kube,npc}:git-a7ea71bd in https://github.com/weaveworks/weave/blob/master/prog/weave-kube/weave-daemonset-k8s-1.7.yaml

brb · 2017-12-22T10:05:08Z

PTAL

deitch · 2017-12-23T17:38:51Z

Will do @brb, thank you.

deitch · 2017-12-23T17:59:04Z

upgraded weave to the specific image,
First test: manually recreated the problem: weave bridge down and WEAVE rules removed from iptables. Killed pod, let daemonset restore it. Everything came back perfectly.
Second test: Same as first, but reboot. Everything came back perfectly.

Looks good to me! Please do tag version here when it is merged in and released.

brb · 2017-12-28T13:00:54Z

@deitch Thanks! I'm waiting for someone to review the PR.

bboreham · 2017-12-28T21:20:32Z

I have read through the code, and that all seems fine.
I would like to know more about what it will do under different failure conditions, e.g. if some of the iptables rules are present and some are absent.

deitch · 2018-01-02T15:19:34Z

I have read through the code, and that all seems fine

I certainly am excited for this. It makes weave far more resilient. Instead of "works from nothing" and "works when it already works", it becomes, "always ensure good state on startup".

brb · 2018-01-06T13:00:15Z

@bboreham It will restore iptables rules. The filter/FORWARD rules are restored in a correct order. Also, I've extended the 191 integration test to test restoration of iptables rules.

PTAL - two new commits since your review.

net/bridge.go

+	for _, rs := range rulespecs {
+		// If any is missing, then delete all, as we need to preserve the order of
+		// given rules. Ignore errors, as rule might not exist.
+		if !allFound {


bboreham · 2018-01-08T18:06:38Z

I like the table of rules - in future we should be able to use the same table to remove them on weave reset, and take that code out of the shell script.

deitch · 2018-01-09T17:23:18Z

Fabulous! What version will it (did it?) go into?

bboreham · 2018-01-09T17:38:11Z

2.2; I'd like to get #3210 in too

deitch · 2018-01-09T18:07:31Z

Thanks @bboreham. Is there an ETA?

deitch · 2018-01-22T14:23:19Z

Nice #3210 is merged too. So 2.2?

brb · 2018-01-23T11:12:12Z

We are experiencing some integration test failures, so we would like to fix those before doing 2.2.

deitch · 2018-01-25T17:54:33Z

We are experiencing some integration test failures, so we would like to fix those before doing 2.2.

Thanks @brb. Not that I have any cycles, but we have gotten bit by this one several times now. If there is anything we can do to help move it along, let us know?

bboreham · 2018-01-30T16:08:43Z

Weave Net release 2.2 just out https://github.com/weaveworks/weave/releases/tag/v2.2.0

deitch · 2018-01-30T16:16:51Z

Really great! Thanks to all!

Does it automatically get pushed to docker hub?

bboreham · 2018-01-30T16:26:01Z

Yes, it's there on Docker Hub now

deitch · 2018-01-30T16:27:25Z

Ah, got it. By default, it only shows scanned tags, not unscanned.

brb added 6 commits December 14, 2017 16:56

Update netlink dependency

0c249fa

To include LinkNotFoundError.

Do not create bridge upon "weave expose" or "weave hide"

a71e90d

Both cmds depend on running weaver which creates the bridge during its initialization. So, calling "create_bridge" during execution of the cmds is redundant.

Remove "create_bridge" routines

1860f21

No longer needed in the weave script and thus, in weaveutil.

Pass WEAVE_NO_BRIDGED_FASTDP to weaver

a0c8fc2

brb added this to the 2.2 milestone Dec 18, 2017

brb mentioned this pull request Dec 18, 2017

Do something more useful when the weave bridge is DOWN #3133

Closed

Add smoke test for bridge re-creation after restart

14768d6

brb force-pushed the issues/3133-idempotent-bridge branch from f2972fd to 14768d6 Compare December 22, 2017 09:42

brb changed the title ~~WIP: do not skip bridge creation if bridge exists~~ Do not skip bridge creation if bridge exists Dec 22, 2017

Ensure the order of filter/FORWARD iptables rules

921ffef

brb force-pushed the issues/3133-idempotent-bridge branch 5 times, most recently from 7d72764 to bcc8968 Compare January 6, 2018 11:43

Add test case for restoring iptables rules after weaver restart

1d67e2f

brb force-pushed the issues/3133-idempotent-bridge branch from bcc8968 to 1d67e2f Compare January 6, 2018 12:23

bboreham approved these changes Jan 8, 2018

View reviewed changes

net/bridge.go

for _, rs := range rulespecs {

// If any is missing, then delete all, as we need to preserve the order of

// given rules. Ignore errors, as rule might not exist.

if !allFound {

This comment was marked as abuse.

Sign in to view

bboreham merged commit 6cfb208 into master Jan 9, 2018

bboreham deleted the issues/3133-idempotent-bridge branch January 9, 2018 17:38

brb mentioned this pull request Jan 11, 2018

Move weave-npc checks to top of FORWARD chain #3210

Merged

brb mentioned this pull request Feb 1, 2018

Repair bridge and/or iptables rules without shutdown/restart #3106

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Do not skip bridge creation if bridge exists #3204

Do not skip bridge creation if bridge exists #3204

brb commented Dec 18, 2017 •

edited

Loading

deitch commented Dec 18, 2017

deitch commented Dec 18, 2017

brb commented Dec 18, 2017

deitch commented Dec 19, 2017

brb commented Dec 22, 2017 •

edited

Loading

brb commented Dec 22, 2017

deitch commented Dec 23, 2017

deitch commented Dec 23, 2017

brb commented Dec 28, 2017

bboreham commented Dec 28, 2017

deitch commented Jan 2, 2018

brb commented Jan 6, 2018

This comment was marked as abuse.

bboreham commented Jan 8, 2018

deitch commented Jan 9, 2018

bboreham commented Jan 9, 2018

deitch commented Jan 9, 2018

deitch commented Jan 22, 2018

brb commented Jan 23, 2018

deitch commented Jan 25, 2018

bboreham commented Jan 30, 2018

deitch commented Jan 30, 2018

bboreham commented Jan 30, 2018

deitch commented Jan 30, 2018

Do not skip bridge creation if bridge exists #3204

Do not skip bridge creation if bridge exists #3204

Conversation

brb commented Dec 18, 2017 • edited Loading

deitch commented Dec 18, 2017

deitch commented Dec 18, 2017

brb commented Dec 18, 2017

deitch commented Dec 19, 2017

brb commented Dec 22, 2017 • edited Loading

brb commented Dec 22, 2017

deitch commented Dec 23, 2017

deitch commented Dec 23, 2017

brb commented Dec 28, 2017

bboreham commented Dec 28, 2017

deitch commented Jan 2, 2018

brb commented Jan 6, 2018

This comment was marked as abuse.

bboreham commented Jan 8, 2018

deitch commented Jan 9, 2018

bboreham commented Jan 9, 2018

deitch commented Jan 9, 2018

deitch commented Jan 22, 2018

brb commented Jan 23, 2018

deitch commented Jan 25, 2018

bboreham commented Jan 30, 2018

deitch commented Jan 30, 2018

bboreham commented Jan 30, 2018

deitch commented Jan 30, 2018

brb commented Dec 18, 2017 •

edited

Loading

brb commented Dec 22, 2017 •

edited

Loading