Project

General

Profile

Feature #3521

Netdev-bound faces

Added by Junxiao Shi almost 3 years ago. Updated about 2 months ago.

Status:
In Progress
Priority:
High
Assignee:
Category:
Faces
Target version:
Start date:
Due date:
% Done:

10%

Estimated time:
12.00 h

Description

Allow a UDP unicast face to associate with a netdev (network device, aka network interface) rather than a UdpChannel.
Automatically create and destroy these faces upon addition and removal of netdevs.

In NFD configuration file, add an option that specifies zero or more remote FaceUris to which netdev-bound faces will be created; these FaceUris must be in canonical form.
When the configuration file is loaded or reloaded, or when a netdev is added or removed, NFD creates and destroys netdev-bound faces as necessary to maintain one netdev-bound face per netdev per remote FaceUri.

When a netdev's IP address changes, the face should bind to the new IP address; in case the netdev does not have any available IP addresses, the face should be set as DOWN but not destroyed.


Related issues

Related to NFD - Feature #3352: Set transport state UP/DOWN based on the state of the underlying network interfaceIn Progress

Related to NFD - Feature #3831: Add RIB routes for automatically created permanent facesNew

Related to NFD - Task #4717: Clarify FacePersistency semanticsNew

Blocked by ndn-cxx - Feature #3522: FaceUri syntax for netdev-bound facesClosed

Blocked by NFD - Feature #3904: FaceManager refactoringClosed

Blocked by NFD - Feature #4021: FaceSystem: use NetworkMonitor::listNetworkInterfaces()Closed

History

#1 Updated by Davide Pesavento almost 3 years ago

  • Blocked by Feature #3353: NetworkMonitor: emit fine-grained signals when the state of a network interface changes added

#2 Updated by Davide Pesavento almost 3 years ago

  • Related to Feature #3352: Set transport state UP/DOWN based on the state of the underlying network interface added

#3 Updated by Junxiao Shi almost 3 years ago

  • Blocked by Feature #3522: FaceUri syntax for netdev-bound faces added

#4 Updated by Davide Pesavento over 2 years ago

  • Assignee deleted (Andrea Tosatto)
  • Start date deleted (03/08/2016)

#5 Updated by Jeff Burke over 2 years ago

  • Priority changed from Normal to High

This feature will be really valuable for demonstrating robust use of multiple interfaces. Can it be assigned?

#6 Updated by Junxiao Shi over 2 years ago

  • Assignee set to Weiwei Liu
  • Priority changed from High to Normal

Some implementation can be found in https://github.com/andreatosatto90/NFD/tree/interfaceFace, although I don't know the correctness or quality of that implementation.

#7 Updated by Jeff Burke over 2 years ago

Why did the priority for this issue get lowered? It remains important to the application team for trying to make field deployments of NDN more resilient to connectivity changes. Also, this appears to be targeted for v0.5 while its dependencies (#3353 and #3522) are targeted for v0.6. Please revise the target version for the dependencies to match.

#8 Updated by Davide Pesavento over 2 years ago

  • Target version changed from v0.5 to v0.6

#9 Updated by Jeff Burke over 2 years ago

I was suggesting this target v0.5, not the other way around.

#10 Updated by Junxiao Shi over 2 years ago

  • Description updated (diff)
  • Estimated time set to 18.00 h

#11 Updated by Junxiao Shi over 2 years ago

  • Priority changed from Normal to High
  • Estimated time changed from 18.00 h to 12.00 h

I explained to Weiwei that this implementation would involve changes in FaceManager and UnicastUdpTransport (or a new subclass of Transport).

Estimated time is 12 hours because this likely needs a lot of testing.

#12 Updated by Junxiao Shi about 2 years ago

  • Related to Feature #3831: Add RIB routes for automatically created permanent faces added

#13 Updated by Junxiao Shi about 2 years ago

When the configuration file is loaded or reloaded

As of NFD 0.5.0, NIC addition/removal triggers a partial configuration reload of face_system section. When we have NetworkMonitor (#3353), do we need to change anything in this part?

#14 Updated by Davide Pesavento about 2 years ago

Junxiao Shi wrote:

As of NFD 0.5.0, NIC addition/removal triggers a partial configuration reload of face_system section. When we have NetworkMonitor (#3353), do we need to change anything in this part?

I believe we should stop doing that. It's rather non-standard behavior and could be unexpected/confusing to some. The config file should be reloaded only upon an explicit user action (e.g. SIGHUP or SIGUSR).

The new NetworkMonitor should be wired up to NFD, so that addition and removal of network interfaces result in the creation and destruction of the corresponding multicast and NIC-associated faces, when needed and allowed by the config file.

#15 Updated by Weiwei Liu about 2 years ago

  • Status changed from New to In Progress

I added an option "nicfaces" into the NFD configuration file and as well as the corresponding code that parses it in face-manager.cpp. Please take a look if it makes sense to you. Thanks!

https://gerrit.named-data.net/#/c/3363/1

  ; The nicfaces section contains settings of NIC-associated permanent faces.
  @IF_HAVE_LIBPCAP@nicfaces
  @IF_HAVE_LIBPCAP@{
  @IF_HAVE_LIBPCAP@  ;Whitelist and blacklist can contain, in no particular order,
  @IF_HAVE_LIBPCAP@  ;interface names (e.g., ifname eth0),
  @IF_HAVE_LIBPCAP@  ;mac addresses (e.g., ether 85:3b:4d:d3:5f:c2),
  @IF_HAVE_LIBPCAP@  ;subnets (e.g., subnet 192.0.2.0/24, note that only IPv4 is supported here),
  @IF_HAVE_LIBPCAP@  ;or a wildcard (*) that matches all interfaces.
  @IF_HAVE_LIBPCAP@
  @IF_HAVE_LIBPCAP@  whitelist
  @IF_HAVE_LIBPCAP@  {
  @IF_HAVE_LIBPCAP@    *
  @IF_HAVE_LIBPCAP@  }
  @IF_HAVE_LIBPCAP@  blacklist
  @IF_HAVE_LIBPCAP@  {
  @IF_HAVE_LIBPCAP@  }
  @IF_HAVE_LIBPCAP@  ; remotes can contain the remote FaceUris to which the NIC-associated permanent
  @IF_HAVE_LIBPCAP@  ; faces will be created. These FaceUris must be in canonical form. Only UDP is
  @IF_HAVE_LIBPCAP@  ; supported for now.
  @IF_HAVE_LIBPCAP@  remotes
  @IF_HAVE_LIBPCAP@  {
  @IF_HAVE_LIBPCAP@  }
  @IF_HAVE_LIBPCAP@}

#16 Updated by Davide Pesavento about 2 years ago

In NFD configuration file, add an option that specifies zero or more remote FaceUris to which NIC-associated permanent faces will be created
[...]
NFD creates and destroys NIC-associated permanent faces as necessary to maintain one NIC-associated permanent face per NIC per remote FaceUri.

Does this mean that if I have two NICs and configure two remote URIs, I get four NIC-associated faces? What's the rationale? I doubt that's the expected behavior in the vast majority of the cases.

What we want to be able to do, at least for our V2I use cases, is: specify one remote URI (udp4: or udp6:) for each network interface available on the system, or a subset of them, and have NIC-associated face created on it.
A network interface would be identified by its name, or optionally by its hardware address if you want. We don't currently need an IP/subnet match like in the multicast faces whitelist, but I can see a scenario where one wants to use different remote URIs depending on the IP address assigned by the network, in other words, a different remote URI for each upstream provider/ISP (although identifying that via the assigned IP would not be 100% reliable).
We also don't need more than one remote URI per NIC at the moment, but I would include multiple remotes in the design, at least to make sure the config file syntax is sufficiently extensible to eventually support that. In any case each network interface should have its own independent set of remote URIs, i.e. no cartesian product.

#17 Updated by Junxiao Shi about 2 years ago

each network interface should have its own independent set of remote URIs

I agree. There should be multiple (whitelist,blacklist,remotes) tuples to support this use case. Cartesian product is used within each tuple.

The existing design (a single cartesian product) can also work, although it creates more faces than necessary. Those excessive faces simply don't get used by the strategy due to their bad performance. The overhead is the maintenance cost of those faces, such as keep-alives.

#18 Updated by Weiwei Liu about 2 years ago

I agree. There should be multiple (whitelist,blacklist,remotes) tuples to support this use case. Cartesian product is used within each tuple.

Just to make sure I understand what you are saying. In this case, the config file could look like this, right?

nicfaces
{
  whitelist
  {
    ifname eth0
  }
  blacklist
  {
  }
  remotes
  {
    udp4://192.168.1.2:6363 
  }

  whitelist
  {
    ether 85:3b:4d:d3:5f:c2
  }
  blacklist
  {
  }
  remotes
  {
    udp4://192.168.3.4:6363 
  }
}

Or alternatively, can we just use (interface, remotes) pairs like the following? Since multiple whitelists and blacklists seem a little bit confusing to me.

nicfaces
{
  ifname eth0
  remotes
  {
    udp4://192.168.1.2:6363 
  }

  ether 85:3b:4d:d3:5f:c2
  remotes
  {
    udp4://192.168.3.4:6363 
  }
}

#19 Updated by Weiwei Liu about 2 years ago

https://gerrit.named-data.net/#/c/3363/2
I added a field 'tuple' to explicitly group each (whitelist, blacklist, remotes) tuple. So the nicfaces section now has the following form.

nicfaces
{
  tuple
  {
    whitelist
    {
      ifname eth0
    }
    blacklist
    { 
    }
    remotes
    {
      udp4://192.168.1.2:6363 
    }
  }
}

#20 Updated by Junxiao Shi about 2 years ago

Reply to note-19:

tuple doesn't reflect semantics. remotes key is unnecessary.

I suggest the following:

nicfaces
{
  nicface
  {
    remote udp4://192.0.2.1:6363
    remote udp4://192.0.2.2:6363
    whitelist
    {
      ifname eth0
    }
    blacklist
    {
    }
  }

  nicface
  {
    remote udp4://192.0.2.3:6363 
    whitelist
    {
      ether 85:3b:4d:d3:5f:c2
    }
    blacklist
    {
    }
  }
}
  • nicfaces has zero or more nicface keys
  • nicface has one or more remote keys
  • nicface has zero or one whitelist key
  • nicface has zero or one blacklist key
  • If the same remote value appears under multiple nicface keys and a NIC matches multiple whitelist+blacklist combinations, only one face should be created between this NIC and this remote FaceUri.

#21 Updated by Davide Pesavento about 2 years ago

Is the nicfaces "wrapper element" really needed? can we just have a bunch of nicface specifications?

Also, I would be ok with the simplified syntax of the 2nd proposal in note-18, i.e. without using whitelist and blacklist keys. So for instance:

nicface
{
  ifname eth0
  remote udp4://192.168.1.2:6363
}
nicface
{
  ether 85:3b:4d:d3:5f:c2
  ether 85:3b:4d:a4:5b:f8
  remote udp6://[...]:6363
}
nicface
{
  ; applies to all interfaces (or all interfaces not matched by a previous 'nicface'?)
  remote udp4://192.168.100.1:6363
}

#22 Updated by Junxiao Shi about 2 years ago

Is the nicfaces "wrapper element" really needed? can we just have a bunch of nicface specifications?

Having a nicfaces wrapper keeps the section more organized.
But I have no objection eliminating it.

the simplified syntax without using whitelist and blacklist keys

No. You lose the capability of blacklist, and cannot easily reuse code for whitelist.

#23 Updated by Davide Pesavento about 2 years ago

Junxiao Shi wrote:

the simplified syntax without using whitelist and blacklist keys

No. You lose the capability of blacklist, and cannot easily reuse code for whitelist.

I'm aware of that. It's just an alternative proposal that is slightly less powerful, but also much more succinct and intuitive.

As for code reuse, it's a very minor concern in my opinion. I'm sure we can easily do a small refactoring in NetworkInterfacePredicate in order to maximize code reuse.

#24 Updated by Alex Afanasyev about 2 years ago

Let's go with the proposal in note 20. Agree with Davide that it is a bit more cumbersome syntax, but it should be ok.

#25 Updated by Weiwei Liu about 2 years ago

As suggested by Alex, I updated the code according to node 20.
https://gerrit.named-data.net/#/c/3363/3/

#26 Updated by Davide Pesavento about 2 years ago

I just realized there's a (IMHO serious) flaw with the syntax in note-20. The nicface stanza seems to imply from its name that it defines one NIC-associated face. However, it can in fact create any number of faces, zero, one, or more than one. There isn't a 1-to-1 relationship between a nicface stanza and a NIC-associated face. But the syntax seems to imply that there is one, which is confusing.

So I'm now against the syntax in note-20, and I'd like to propose the alternative in note-21 again.

#27 Updated by Junxiao Shi about 2 years ago

note-21 is no better than note-20 because each nicface key also corresponds to zero or more NIC-associated faces.

#28 Updated by Davide Pesavento about 2 years ago

err sorry, completely forgot an important piece in my previous comment... I meant to say: note-21 syntax with nicface renamed to nicfaces.

#29 Updated by Junxiao Shi about 2 years ago

I still disagree with note-21 and its amendment in note-28.
I acknowledge the issue pointed out in note-26, and my proposal is:

nicfaces
{
  rule
  {
    remote udp4://192.0.2.1:6363
    remote udp4://192.0.2.2:6363
    whitelist
    {
      ifname eth0
    }
    blacklist
    {
    }
  }

  rule
  {
    remote udp4://192.0.2.3:6363 
    whitelist
    {
      ether 85:3b:4d:d3:5f:c2
    }
    blacklist
    {
    }
  }
}

The whole section defines all NIC-associated permanent faces, while each rule causes the creation of zero or more faces.
I'd like to preserve whitelist+blacklist and be able to reuse existing whitelist+blacklist code with minimal changes.

#30 Updated by Weiwei Liu about 2 years ago

I changed the design to note-29 since I didn't see any objections. https://gerrit.named-data.net/#/c/3363/4

Besides, for the next step, we need to actually create NIC-associated permanent UDP unicast face. I plan to create a new transport class "NicAssociatedUnicastUdpTransport" as a subclass of DatagramTransport instead of reusing UnicastUdpTransport.. Any suggestions?

#31 Updated by Davide Pesavento about 2 years ago

Weiwei Liu wrote:

I plan to create a new transport class "NicAssociatedUnicastUdpTransport" as a subclass of DatagramTransport instead of reusing UnicastUdpTransport.. Any suggestions?

What are the advantages and disadvantages of doing so?

#32 Updated by Weiwei Liu about 2 years ago

What are the advantages and disadvantages of doing so?

What I can see for now is just if reusing UnicastUdpTransport we'll need to add a new constructor and the beforeChangePersistency() also needs to be changed to handle the NIC-associated case (Just to make sure, in this case we wouldn't allow the persistency to be changed to anything other than FACE_PERSISTENCY_PERMANENT, right?). I just think putting them into an independent class may make the code clearer and easier to understand.

#33 Updated by Junxiao Shi about 2 years ago

Before talking about what Transport type to use, we should first understand how to manage the lifetime of Face:

  • When a NIC is added or removed, or becomes matching / non-matching the whitelist+blacklist filter due to addition or removal of a particular IP address, which component is responsible for creating and destroying the Face objects?
  • When a NIC gains its first IP address or loses its last IP address but does not affect its matching status (such as a whitelist matching by MAC address), should we create/destroy the Face object, or should we simply set the transport up / down? Which component is responsible for do so?

I think the logical answer in current structure would be FaceManager, however I don't think it's the right answer: FaceManager is taking way too many responsibilities, and it's getting more and more complicated.

My proposal is to split FaceManager into three components:

  • FaceSystem owns all ProtocolFactory objects; it belongs in face/ rather than mgmt/. It directly processes the configuration section, but delegates nicfaces and multicast sections to NicManager.
  • NicManager, also a part of face/, listens to signals from NetworkMonitor and takes proper reactions. It controls the creation and destroying of NIC-associated permanent faces and multicast faces.
  • FaceManager is an interface to the management system. Its responsibility is to serve FaceMgmt commands and datasets. It calls into FaceSystem for those functions.

I know these sound like drastic changes, but I believe they lead to a cleaner architecture.

#34 Updated by Davide Pesavento about 2 years ago

Weiwei Liu wrote:

What I can see for now is just if reusing UnicastUdpTransport we'll need to add a new constructor and the beforeChangePersistency() also needs to be changed to handle the NIC-associated case (Just to make sure, in this case we wouldn't allow the persistency to be changed to anything other than FACE_PERSISTENCY_PERMANENT, right?). I just think putting them into an independent class may make the code clearer and easier to understand.

Sorry, I think I misunderstood note-30. I thought you were comparing subclassing DatagramTransport vs subclassing UnicastUdpTransport. Modifying UnicastUdpTransport itself to support NIC-associated faces is obviously out of the question. NicAssociatedUdpTransport is a separate transport type, hence it needs a separate class. Another question is whether you want to inherit from UnicastUdpTransport or DatagramTransport. I think you can start from the latter, and see how much duplication you end up having in the new class.

#35 Updated by Davide Pesavento about 2 years ago

Junxiao Shi wrote:

Before talking about what Transport type to use, we should first understand how to manage the lifetime of Face:
[...]
I think the logical answer in current structure would be FaceManager, however I don't think it's the right answer: FaceManager is taking way too many responsibilities, and it's getting more and more complicated.

While I agree with the above sentiment, and had the same thoughts when I was helping Andrea with his quick 'n' dirty implementation of this feature, I don't think doing this refactoring now is a good idea. This is an important feature and several applications would benefit from it. It should be implemented asap. Requiring such a large refactoring to be completed before this feature can be merged could delay the latter for weeks/months.

#36 Updated by Junxiao Shi about 2 years ago

#37 Updated by Junxiao Shi about 2 years ago

Requiring such a large refactoring to be completed before this feature can be merged could delay the latter for weeks/months.

I see no difficulty in this refactoring. It's just moving code around, and can be done in one week if review turnaround is fast enough.
Without the refactoring, it would be difficult doing note-14 changes.

In the meanwhile, Weiwei should finish up #3353 which blocks this issue, and also work on #3817 which is necessary in Jeff's scenario.

#38 Updated by Junxiao Shi almost 2 years ago

In https://gerrit.named-data.net/#/c/3551/6/tests/daemon/face/ethernet-fixture.hpp@38 Davide pointed out:

I'm insisting on not using "NIC". The "C" in NIC stands for Card or Controller... a NIC represents a piece of hardware. The "interfaces" we normally use do not necessarily correspond to a real hardware controller. Think about tun/tap, virtual ethernet pairs, or virtual bridges, etc... even the loopback interface is not a real hw device.
So NIC is incorrect, it's better to use "network interface" or one of its abbreviations.

Does the same argument apply to "NIC-associated permanent faces"? They can be associated with hardware interfaces, but can also be associated with virtual interfaces.
If so, is there a better name?

#39 Updated by Davide Pesavento almost 2 years ago

Junxiao Shi wrote:

Does the same argument apply to "NIC-associated permanent faces"? They can be associated with hardware interfaces, but can also be associated with virtual interfaces.

Yep.

If so, is there a better name?

I tried to come up with a better (and possibly shorter) name but I failed.

#40 Updated by Junxiao Shi almost 2 years ago

  • % Done changed from 0 to 10

I have provided an API design in https://gerrit.named-data.net/3363 patchset8. It depends on the completion of #3353. Weiwei should refine this design and implement TODO items after #3353 has been merged.

#41 Updated by Davide Pesavento almost 2 years ago

  • Assignee deleted (Weiwei Liu)

#42 Updated by Junxiao Shi almost 2 years ago

  • Blocked by deleted (Feature #3353: NetworkMonitor: emit fine-grained signals when the state of a network interface changes)

#43 Updated by Junxiao Shi almost 2 years ago

  • Blocked by Feature #4021: FaceSystem: use NetworkMonitor::listNetworkInterfaces() added

#44 Updated by Davide Pesavento over 1 year ago

  • Status changed from In Progress to Feedback

#45 Updated by Davide Pesavento over 1 year ago

  • Target version changed from v0.6 to v0.7

#46 Updated by Junxiao Shi 5 months ago

  • Assignee set to Junxiao Shi

I'll attempt to resurrect Change 3363.

#47 Updated by Davide Pesavento 5 months ago

Davide Pesavento wrote:

Junxiao Shi wrote:

Does the same argument apply to "NIC-associated permanent faces"? They can be associated with hardware interfaces, but can also be associated with virtual interfaces.

Yep.

If so, is there a better name?

I tried to come up with a better (and possibly shorter) name but I failed.

As a (better) alternative to "NIC", I suggest the terms "netdev" or "netdevice", which are used in the Linux kernel networking stack.

I also suggest "bound" rather than "associated", the former is shorter and already used in the Linux networking stack (e.g., SO_BINDTODEVICE socket option). However, I don't think we need to include "bound" (or "associated" or any other alternative) in the name of the feature or related classes that implement the feature; it can be used in documentation and code comments that talk about the feature, e.g., "...a face bound to a netdev/netdevice...".

#48 Updated by Davide Pesavento 5 months ago

  • Related to Task #4717: Clarify FacePersistency semantics added

#49 Updated by Junxiao Shi 4 months ago

  • Status changed from Feedback to In Progress

#50 Updated by Junxiao Shi 3 months ago

  • Subject changed from NIC-associated permanent faces to Netdev-bound faces
  • Description updated (diff)

After two years, config parsing code is finally done https://gerrit.named-data.net/3363.
Test coverage is incomplete for every possible config file error, but I think it's unimportant.

#51 Updated by Davide Pesavento about 2 months ago

  • Description updated (diff)

Also available in: Atom PDF