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

gnrc/ipv6_auto_subnets: relax topology requirements #16750

Merged
merged 2 commits into from Sep 28, 2021

Conversation

benpicco
Copy link
Contributor

@benpicco benpicco commented Aug 16, 2021

Contribution description

#16536 provides a solution for networks that follow a 'skinny tree' topology, that is on each level only a single node can have children.
On such a simple topology we can automatically create subnets without coordination between routers given a sufficiently large prefix.

As it turns out we also have to deal with network topologies where this assumption does not hold and there are multiple routing nodes on a level.

auto_subnets

Now coordination between the routers is required: Each router must announce how many subnets it wants to create to determine the total number of subnets and thereby the required prefix length to make for unique subnets.

As I could not find any standard mechanism for this functionality, I turned to simply broadcasting a UDP packet with this information. (which makes this a RIOT specific functionality which only works in homogeneous networks. I therefore kept the protocol as simple as possible and only transfer a single byte of payload).

Testing procedure

Create the topology as usual with the setup_taps.sh script in examples/gnrc_networking-subnets.
This time however we will connect two routing nodes on the same network level:

# start the root node
make PORT="tap_a0 tap_b0" term 

# start the first routing node
make PORT="tap_b1 tap_c0" term

# to test prefix removal, we can wait here for a bit until the first routing node got it's prefix
# it should be replaced after starting the second routing node

# start the second routing node
make PORT="tap_b2 tap_d0" term

# start a child node on the next level
make PORT="tap_d1 tap_e0" term

All nodes should have a global address and be able to route to each other.

Issues/PRs references

extends #16536
depends on and includes #16729, #16734

@benpicco benpicco added Discussion: RFC The issue/PR is used as a discussion starting point about the item of the issue/PR State: waiting for other PR State: The PR requires another PR to be merged first labels Aug 16, 2021
@github-actions github-actions bot added Area: build system Area: Build system Area: doc Area: Documentation Area: examples Area: Example Applications Area: network Area: Networking Area: sys Area: System labels Aug 16, 2021
@benpicco benpicco removed the State: waiting for other PR State: The PR requires another PR to be merged first label Sep 2, 2021
@github-actions github-actions bot removed Area: examples Area: Example Applications Area: doc Area: Documentation labels Sep 2, 2021
@benpicco benpicco added Area: doc Area: Documentation and removed Area: build system Area: Build system Area: sys Area: System Area: doc Area: Documentation labels Sep 2, 2021
@github-actions github-actions bot added Area: build system Area: Build system Area: sys Area: System Area: doc Area: Documentation labels Sep 2, 2021
Comment on lines 4 to 6
participant "2e:a3:9e:a9:68:**23**" as A
participant "2e:a3:9e:a9:68:**f6**" as B
participant "2e:a3:9e:a9:68:**42**" as C
Copy link
Member

Choose a reason for hiding this comment

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

Does the least significant byte of the address being bold have any meaning? If so, this meaning should be apparent within the graphic somehow (a graphic should be able to stand on its own). Otherwise, it should be removed, as I think it confuses more than it helps understanding the algorithm.

Copy link
Contributor Author

@benpicco benpicco Sep 16, 2021

Choose a reason for hiding this comment

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

I thought it's a nice visual aid to see what is the difference between the l2 addresses.
I turned it down to italics

Copy link
Member

@miri64 miri64 Sep 16, 2021

Choose a reason for hiding this comment

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

The problem to me was more that they are highlighted in general. Now that I know what this is about, my perspective shifted, so I'm not sure if making them italic makes any difference. I think that L2 addresses are different can be assumed for a working network, so highlighting the difference seems somewhat redundant to me, however, still. Do they have to be literal addresses? Can't they just be Node A, B, C?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The idea was to visualize that the order in which the prefixes are chosen / where each router starts counting it's prefixes depends on the order of the l2 addresses.
I hoped changing only the last byte and highlighting that would make it easier to understand the concept.

Copy link
Member

Choose a reason for hiding this comment

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

The idea was to visualize that the order in which the prefixes are chosen / where each router starts counting it's prefixes depends on the order of the l2 addresses.
I hoped changing only the last byte and highlighting that would make it easier to understand the concept.

Mh, I think by abstracting this more to numeric or alphanumeric abstract L2 addresses would help to highlight this concept more. How about something like "L2 addr. 1", "L2 addr. 2", ...

Copy link
Member

Choose a reason for hiding this comment

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

If the order of their significance now would also be reflected in the order they are shown (so C in the second column, B in the third), I'd be happy :-).

Copy link
Member

Choose a reason for hiding this comment

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

(of course feel free to adjust the labels then as well).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If the order of their significance now would also be reflected in the order they are shown

I thought mixing them up made it clearer that it's not the order of appearance that matters but the order of addresses - but alright, I can shuffle them around.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

If the order of their significance now would also be reflected in the order they are shown

I thought mixing them up made it clearer that it's not the order of appearance that matters but the order of addresses - but alright, I can shuffle them around.

No, mixing things up to make them clearer, rarely works ;-)

@benpicco benpicco added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Sep 22, 2021
@benpicco
Copy link
Contributor Author

@miri64 do you think we can still get this into the release?
We would then avoid changing the semantics of the module name (the current gnrc_ipv6_auto_subnets module behavior becomes gnrc_ipv6_auto_subnets_simple and the general implementation takes that name) from one release to the next.

Copy link
Member

@miri64 miri64 left a comment

Choose a reason for hiding this comment

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

@miri64 do you think we can still get this into the release?

Ok, let's try this.

Some function names are IMHO confusing. Yes you documented the functions, but if you just scroll through the code, see a function _insert or _compare_addr it is not immediately clear what is happening.

Copy link
Member

@miri64 miri64 left a comment

Choose a reason for hiding this comment

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

Another round of more in-depth review.

@benpicco benpicco removed the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Sep 28, 2021
Copy link
Member

@miri64 miri64 left a comment

Choose a reason for hiding this comment

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

I'm fine with the code now and I trust your testing. Feel free to squash.

@github-actions github-actions bot added the Area: examples Area: Example Applications label Sep 28, 2021
@benpicco benpicco added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed Area: examples Area: Example Applications labels Sep 28, 2021
@benpicco
Copy link
Contributor Author

Thank you for the review!

@benpicco benpicco merged commit a39c0e1 into RIOT-OS:master Sep 28, 2021
@benpicco benpicco deleted the gnrc_ipv6_auto_subnets branch September 28, 2021 17:07
@benpicco benpicco added this to the Release 2021.10 milestone Oct 20, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: build system Area: Build system Area: doc Area: Documentation Area: network Area: Networking Area: sys Area: System CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Discussion: RFC The issue/PR is used as a discussion starting point about the item of the issue/PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants