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

Warn if a docker network may cause issues on macOS #1115

Merged
merged 1 commit into from
Apr 17, 2020

Conversation

sykesm
Copy link
Contributor

@sykesm sykesm commented Apr 17, 2020

The default docker local network address pool includes addresses that are commonly used on home networks. This configuration can prevent containers from accessing the network host on the LAN addresses discovered by the peer. This often surfaces in integration tests as chaincode registration failures with a "no route to host" message.

This change attempts to discover and highlight the broken configuration so that it can be addressed. (pun, heh)

The network pools can be configured by adding some JSON to the Docker daemon configuration to prevent the overlaps. The comment near the warning message describes the syntax.

@sykesm sykesm force-pushed the dastardly-docker-defaults branch from 2e5f7ba to a43bf71 Compare April 17, 2020 12:55
The default docker local network address pool includes addresses that
are commonly used on home networks. This configuration can prevent
containers from accessing the network host on the LAN addresses
discovered by the peer. This often surfaces in integration tests as
chaincode registration failures with a "no route to host" message.

This change attempts to discover and highlight the broken configuration
so that it can be addressed. (pun, heh)

The network pools can be configured by adding some JSON to the Docker
daemon configuration to prevent the overlaps. The comment near the
warning message describes the syntax.

Change-Id: I11b5ad72df497053dc2a07f68b6370b06679d995
Signed-off-by: Matthew Sykes <sykesmat@us.ibm.com>
@sykesm sykesm force-pushed the dastardly-docker-defaults branch from a43bf71 to 85d931a Compare April 17, 2020 13:23
@sykesm
Copy link
Contributor Author

sykesm commented Apr 17, 2020

Hit flake FAB-17762 multiple times.

@cendhu
Copy link
Contributor

cendhu commented Apr 17, 2020

#1092 should help for now.

@sykesm
Copy link
Contributor Author

sykesm commented Apr 17, 2020

#1092 should help for now.

I'm not sure I see how. The change in that PR modifies timing but does not appear to address the root cause. I'll go comment over there.

@sykesm sykesm marked this pull request as ready for review April 17, 2020 15:33
@sykesm sykesm requested a review from a team as a code owner April 17, 2020 15:33
Comment on lines +730 to +731
// {"base":"172.80.0.0/16","size":24},
// {"base":"172.81.0.0/16","size":24}
Copy link
Contributor

Choose a reason for hiding this comment

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

Just a nit, but wouldn't it be better to pick from the officially reserved ranges just in case someone looks at the comment?

Suggested change
// {"base":"172.80.0.0/16","size":24},
// {"base":"172.81.0.0/16","size":24}
// {"base":"172.16.0.0/16","size":16},
// {"base":"172.17.0.0/16","size":16}

but I guess those are in the default. You can also override the bridge network with the "bip" config option but again I guess this is just a comment

Copy link
Contributor Author

@sykesm sykesm Apr 17, 2020

Choose a reason for hiding this comment

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

Just a nit, but wouldn't it be better to pick from the officially reserved ranges just in case someone looks at the comment?

They're from the reserved range of 172.16.0.0/12. So, I'm not sure why it's wrong? The example is showing how to get 512 different /24 networks for local use that do no overlap with common prefixes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are correct and my math is not. I'll address this with the spelling fix.

//
// https://github.com/moby/libnetwork/blob/1a17fb36132631a95fe6bb055b91e24a516ad81d/ipamutils/utils.go#L18-L20
//
// Docker can be configured to use different addresses by addding an
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// Docker can be configured to use different addresses by addding an
// Docker can be configured to use different addresses by adding an

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll push a spelling fix on top.

@caod123 caod123 self-requested a review April 17, 2020 19:52
@caod123 caod123 merged commit 1559374 into hyperledger:master Apr 17, 2020
@sykesm sykesm deleted the dastardly-docker-defaults branch February 20, 2021 02:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants