8000 Change default CNI network name, bridge name, and subnet by stealthybox · Pull Request #460 · weaveworks/ignite · GitHub
[go: up one dir, main page]
Skip to content
This repository was archived by the owner on Dec 7, 2023. It is now read-only.

Change default CNI network name, bridge name, and subnet #460

Merged
merged 1 commit into from
Sep 26, 2019
Merged

Change default CNI network name, bridge name, and subnet #460

merged 1 commit into from
Sep 26, 2019

Conversation

stealthybox
Copy link
Contributor
@stealthybox stealthybox commented Sep 26, 2019 •
edited
Loading

Fixes #424
Fixes #455

The solution in this patch for #424 is to use a static subnet in the 10.0.0.0/8 range.
This avoids the complexity of collaborating with the docker defaulting logic which requires calculating and iterating a over a similar list of subnets and checking to see if they're in use by the kernel.
This:

  • keeps the code simple
  • has the future side-effect of making issues more searchable via google when users submit issue details with the default network setup

Rationale for the subnet choice is documented on the constant.

Changing these CNI details will ultimately necessitate that any current users of our (mostly broken) default CNI setup will likely want to migrate.
This is not forced on users though. The config is still only written when the CNI net conf dir is empty.

@stealthybox stealthybox requested a review from twelho as a code owner September 26, 2019 01:29
@stealthybox stealthybox added this to the v0.6.1 milestone Sep 26, 2019
@stealthybox stealthybox added the area/networking Issues related to networking label Sep 26, 2019
@stealthybox
Copy link
Contributor Author

cc @najeal -- pinging you as discussed
Please review at your leisure :)

chanwit
chanwit approved these changes Sep 26, 2019
View reviewed changes
Copy link
Member
@chanwit chanwit left a comment •
edited
Loading

Choose a reason for hiding this comment

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

both changes

  • ignite0
    and
  • 10.61.0.0/16
    lgtm

najeal
najeal approved these changes Sep 26, 2019
View reviewed changes
Copy link
Contributor
@najeal najeal left a comment

Choose a reason for hiding this comment

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

Looks good to me 👍

@chanwit
Copy link
Member
chanwit commented Sep 26, 2019

Thank you @najeal to help review this, and thank you @stealthybox for taking care of this.

@chanwit chanwit merged commit 292dd05 into weaveworks:master Sep 26, 2019
@chanwit chanwit removed the request for review from twelho September 26, 2019 16:36
@stealthybox stealthybox deleted the change-default-cni branch September 26, 2019 21:38
@stealthybox stealthybox mentioned this pull request Sep 26, 2019
Merged
@lucidprogrammer
Copy link

Thanks for this update, however, its possibly causing #462

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@chanwit chanwit chanwit approved these changes

@najeal najeal najeal approved these changes

Assignees

@chanwit chanwit

Labels
area/networking Issues related to networking
Projects
None yet
Milestone
v0.6.1
4 participants
@stealthybox @chanwit @lucidprogrammer @najeal
0