8000 Disable multicast route in networks created via the plugin and add option to re-enable by bboreham · Pull Request #2327 · weaveworks/weave · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content
This repository was archived by the owner on Jun 20, 2024. It is now read-only.

Disable multicast route in networks created via the plugin and add option to re-enable #2327

Merged
merged 2 commits into from
Jun 6, 2016
Merged

Disable multicast route in networks created via the plugin and add option to re-enable #2327

merged 2 commits into from
Jun 6, 2016

Conversation

bboreham
Copy link
Contributor

This gives users a better way out of the problem in #1960

@bboreham bboreham added this to the 1.6.0 milestone May 31, 2016
@bboreham bboreham force-pushed the issues/1960-mcast-option branch from e424990 to c9dd833 Compare May 31, 2016 17:10
@rade
Copy link
Member
rade commented May 31, 2016

Docs?

Does docker allow you to specify option flags, i.e. no value, e.g. --opt works.weave.nomulticast? That would be marginally preferable in the UI, though I gather that the option actually acts as an override for whatever the overall plugin setting ("--no-multicast" or not) is, and for that the explicit true/false does work better.

@rade rade mentioned this pull request May 31, 2016
Closed
@bboreham bboreham force-pushed the issues/1960-mcast-option branch from c9dd833 to 5ba99ef Compare June 1, 2016 14:39
@bboreham
Copy link
Contributor Author
bboreham commented Jun 1, 2016

Yes, you can say docker network create --driver=weavemesh --opt works.weave.nomulticast weave2. What arrives at the plugin is a map from works.weave.nomulticast to blank.

@bboreham bboreham force-pushed the issues/1960-mcast-option branch from 5ba99ef to 8035620 Compare June 1, 2016 15:06
@bboreham
Copy link
Contributor Author
bboreham commented Jun 1, 2016

I amended the code to look for nomulticast, and added docs. Ping @abuehrle for docs review.

Note that you can say --opt works.weave.nomulticast=false if you want to invert the default. I didn't document this, as it seems a niche interest.

@rade
Copy link
Member
rade commented Jun 2, 2016

See my comment how I reckon the various options should interplay. In summary

  • get rid of the --no-multicast-route option on the plugin
  • created networks have multicast disabled by default
  • supplying the works.weave.multicast option on network creation enables multicast
  • create the weave network with multicast enabled

@bboreham
Copy link
Contributor Author
bboreham commented Jun 2, 2016

get rid of the --no-multicast-route option on the plugin

assume you mean "issue a deprecation warning and ignore"

@rade
Copy link
Member
rade commented Jun 2, 2016

assume you mean "issue a deprecation warning and ignore"

Meh. I don't think that is important. But if you must....

@bboreham bboreham force-pushed the issues/1960-mcast-option branch 5 times, most recently from 6bc689a to 6d60e85 Compare June 2, 2016 11:17
@bboreham
Invert multicast option in plugin
a1c3cba 
Now it's off by default, and there is an option to enable the
multicast route per-network.
We turn it on for the `weave` network created when we start the plugin.
@bboreham bboreham force-pushed the issues/1960-mcast-option branch from 6d60e85 to a1c3cba Compare June 2, 2016 11:53
@bboreham
Copy link
Contributor Author
bboreham commented Jun 2, 2016

Changed as suggested

@awh awh self-assigned this Jun 3, 2016
rade
rade reviewed Jun 3, 2016
View reviewed changes
prog/plugin/main.go
@@ -73,26 +73,29 @@ func main() {
}

Log.Println("Weave plugin", version, "Command line options:", os.Args[1:])
if noMulticastRoute {
Log.Warning("--no-multicast-route option has been removed; multicast is off by default")
}

This comment was marked as abuse.

Sign in to view

This comment was marked as abuse.

Sign in to view

This comment was marked as abuse.

Sign in to view

@awh
Copy link
Contributor
awh commented Jun 3, 2016

LGTM

@awh awh removed their assignment Jun 3, 2016
@bboreham bboreham force-pushed the issues/1960-mcast-option branch from c6e0af4 to ab74d7b Compare June 6, 2016 14:41
@bboreham
Copy link
Contributor Author
bboreham commented Jun 6, 2016

Added mention of how --net=weave has multicast turned on but docker network create --driver=weave will default to no multicast. Also removed obsolete flag from docs.

@bboreham bboreham removed their assignment Jun 6, 2016
@rade rade merged commit 7ffbeea into master Jun 6, 2016
@bboreham bboreham 6BA4 changed the title Add an option to disable multicast route per-network Disable multicast route in networks created via the plugin and add option to re-enable Jun 27, 2016
@bboreham bboreham deleted the issues/1960-mcast-option branch November 9, 2016 17:20
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
1.6.0
Development

Successfully merging this pull request may close these issues.
3 participants
@bboreham @rade @awh
0