Project

General

Profile

Feature #3904

FaceManager refactoring

Added by Junxiao Shi about 3 years ago. Updated about 3 years ago.

Status:
Closed
Priority:
Normal
Assignee:
Category:
Management
Target version:
Start date:
Due date:
% Done:

100%

Estimated time:
15.00 h

Description

FaceManager is currently taking way too many responsibilities, and it's getting more and more complicated.
It shall be split into multiple components for a cleaner architecture.


Related issues

Blocks NFD - Feature #3521: Netdev-bound facesIn Progress

Actions
Blocks NFD - Bug #3970: Improve face_system error message when a ProtocolFactory is disabledNew

Actions

History

#1

Updated by Junxiao Shi about 3 years ago

  • Status changed from New to In Progress
  • Assignee set to Junxiao Shi
  • Estimated time set to 6.00 h

Davide has agreed with the necessity in #3521-35 but believes this is a large refactoring, but I believe this refactoring should be easy. I'll try this for a few hours and see what would be the difficulties.


The design 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.
#2

Updated by Junxiao Shi about 3 years ago

  • % Done changed from 0 to 30

https://gerrit.named-data.net/3513 splits configuration processing to FaceSystem class.
Currently FaceManager owns FaceSystem so that there is no API changes affecting NFD initialization code; this will be updated in another commit.

#3

Updated by Junxiao Shi about 3 years ago

#4

Updated by Davide Pesavento about 3 years ago

Junxiao Shi wrote:

https://gerrit.named-data.net/3513 splits configuration processing to FaceSystem class.

I don't like FaceSystem too much as a class name, as I commented on patch set 2. I don't have a better name unfortunately, maybe FaceSystemConfigurator?

#5

Updated by Junxiao Shi about 3 years ago

No. FaceSystemConfigurator is too long, and it's inappropriate for a "configurator" to own any ProtocolFactory objects. Instead, it's very logical to let FaceSystem own face system's high level ProtocolFactory objects.

#6

Updated by Junxiao Shi about 3 years ago

  • % Done changed from 30 to 50

https://gerrit.named-data.net/3517 splits config/NIC-triggered face creation/destroying to NicManager.
NetworkMonitor is now owned by NicManager, but configuration reload code is temporary kept in Nfd class and the upcall has an ugly API.

#7

Updated by Junxiao Shi about 3 years ago

There's a concern about NicManager::enableNetworkMonitor causing problem for simulator, and a request for disabling the effect of this function via #ifdef. But the exact purpose I introduced this separate function rather than initializing creating NetworkMonitor in NicManager is to support simulators: This function eventually will be exposed to the initialization procedure in Nfd class; when NFD is running in the simulator, the initialization code (such as ndnSIM's L3Protocol) should not invoke this function. I don't think a #ifdef is necessary.

#8

Updated by Davide Pesavento about 3 years ago

Junxiao Shi wrote:

https://gerrit.named-data.net/3517 splits config/NIC-triggered face creation/destroying to NicManager.

I disagree with this design. NicManager is still responsible for a bunch of random unrelated things: reacting to network AND config file changes, which are completely independent from one another. You simply moved this mess from FaceSystem to NicManager, but the new "design" is not any simpler. In fact, the initial creation of multicast faces is now split between the two classes, making the code even harder to read. The overall result is worse than the status quo.

#9

Updated by Junxiao Shi about 3 years ago

Reply to note-8:

The main responsibility of NicManager is to respond to NIC changes detected by NetworkMonitor. This responsibility is not "completely independent" from configuration file, because every time there is a NIC change, it is necessary to reevaluate a subset of the configuration. For example, if a new NIC is added, NicManager should check each whitelist+blacklist to see whether multicast faces and NIC-associated permanent faces should be created on the NIC. Therefore, NicManager needs to remember part of the configuration.
I decide to let FaceSystem parse the configuration rather than passing ConfigSection to NicManager because some sections such as face_system.udp contains both fields for unicast channels and fields for multicast faces. Also, since NIC changes are expected to occur more often than configuration reloads, the configuration is stored as structs rather than raw ConfigSection, so that they don't have to be re-parsed every time; this design is consistent with NLSR.

#10

Updated by Junxiao Shi about 3 years ago

I'm thinking about a different design:
Instead of having FaceSystem parse the configuration file, we delegate each config section to the ProtocolFactory. For example, when FaceSystem encounters face_system.udp section, it creates a UdpFactory instance, and let UdpFactory parse that section and create channels and multicast faces accordingly. NetworkMonitor is created by FaceSystem, and shared to ProtocolFactory instances via a shared pointer. Common procedures can be added to ProtocolFactory base class if needed.
For NIC-associated permanent faces, a separate component parses the nicfaces section, and gives relevant rule sections to UdpFactory and other ProtocolFactory instances (if we decide to support tcp4).

#11

Updated by Davide Pesavento about 3 years ago

note-10 idea sounds much better.

#12

Updated by Davide Pesavento about 3 years ago

Don't forget to update NFD's devguide.

#13

Updated by Junxiao Shi about 3 years ago

  • % Done changed from 50 to 40
  • Estimated time changed from 6.00 h to 15.00 h

https://gerrit.named-data.net/3535 implements note-10 design in TcpFactory.

#14

Updated by Junxiao Shi about 3 years ago

https://gerrit.named-data.net/3535 patchset8 ensures ProtocolFactory::processConfig is invoked even if the relevant subsection has been deleted.

#15

Updated by Junxiao Shi about 3 years ago

https://gerrit.named-data.net/3539 implements note-10 design in UnixStreamFactory.
patchset1 has a small behavior change: when Unix sockets are disabled in Waf script, the error message would be "Unrecognized option face_system.unix" instead of an explanation that Unix sockets were disabled during compilation.

#16

Updated by Davide Pesavento about 3 years ago

  • Blocks Feature #1712: FaceManager: create multicast face according to whitelist and blacklist added
#17

Updated by Junxiao Shi about 3 years ago

https://gerrit.named-data.net/3551 patchset2 is a preview revision of EthernetFactory implemention, whose structure will be applied to UdpFactory as well.
Notably, processConfig stores multicast configuration inside EthernetFactory, so that when NetworkMonitor reports a NIC addition, EthernetFactory can decide whether to create a multicast face on the new NIC without requiring a configuration reload.
createMulticastFace is made private, to ensure all multicast faces are created from the configuration and therefore all of them can be destroyed if the group address is changed in the configuration.

#18

Updated by Junxiao Shi about 3 years ago

patchset1 has a small behavior change: when Unix sockets are disabled in Waf script, the error message would be "Unrecognized option face_system.unix" instead of an explanation that Unix sockets were disabled during compilation.

I'm not happy with the above, but I can live with it.

It's possible to restore the old error message by registering a dummy factory on "unix".
I can consider this, but only after all factories are converted.

#19

Updated by Junxiao Shi about 3 years ago

  • % Done changed from 40 to 50

https://gerrit.named-data.net/3561 has the implementation for WebSocketFactory.
Notably, I'm eliminating WebSocketFactory::createChannel function, because there is no use case for more than one channel, and they may conflict if IPv4 and IPv6 channels are created on the same port.
Also, no channel would be created if face_system.websocket.listen is set to no; unlike TcpChannel which can be used to initiate outgoing connections, a non-listening WebSocketChannel is useless.

#20

Updated by Alex Afanasyev about 3 years ago

I'm not sure I agree with this change. Yes, we have a very restricted use of websocket channel right now, but I would exclude a possibility in the future implementation to connect to other NFD (read = in-browser NFD) over websocket channel.

What type of conflict for IPv4/6 websocket channels you're referring to?

#21

Updated by Junxiao Shi about 3 years ago

Reply to note-20:

Restricting to only one channel does not conflict with the possibility of initiating outgoing WebSocket connections. When that is needed, we can create a non-listening channel.

What type of conflict for IPv4/6 websocket channels you're referring to?

One may create an IPv4 channel at ws://192.0.2.1:9696 and an IPv6 channel at ws:/[::ffff:192.0.2.1]:9696. They are the same endpoint.

#22

Updated by Davide Pesavento about 3 years ago

I also disagree with the channel-related changes in note-19. The motivations given are bogus. I could say the same thing for TCP and UDP channels, at the moment they are always bound to a hardcoded 0.0.0.0/[::], and we don't have a way to change that or create other channels.

One may create an IPv4 channel at ws://192.0.2.1:9696 and an IPv6 channel at ws:/[::ffff:192.0.2.1]:9696. They are the same endpoint.

The same can happen for TCP and UDP, so this is not a valid reason to treat Websocket channels differently. In the above scenario, I would expect an error to be raised, and that is the proper response if one creates such a configuration. In other words, I don't see any problem here.

#23

Updated by Junxiao Shi about 3 years ago

  • % Done changed from 50 to 60

https://gerrit.named-data.net/3595 implements UdpFactory::processConfig.
One functionality change is: UDP multicast does not depend on IPv4 UDP channel, because they can operate independently.

#24

Updated by Junxiao Shi about 3 years ago

  • % Done changed from 60 to 70

https://gerrit.named-data.net/3612 introduces ProtocolFactory registry which has mostly same code as strategy registry.

you said the refactoring could be done in one week. It's been almost a month now...

The main reason is: NicManager design was rejected (it was done in less than one week), and moving configuration into ProtocolFactory subclasses became a series of larger changes.

#25

Updated by Junxiao Shi about 3 years ago

https://gerrit.named-data.net/3623 makes ProtocolFactory::processConfig pure virtual.

#26

Updated by Junxiao Shi about 3 years ago

  • % Done changed from 70 to 80

Currently FaceManager owns FaceSystem so that there is no API changes affecting NFD initialization code; this will be updated in another commit.

https://gerrit.named-data.net/3625 fulfills this change.

#27

Updated by Junxiao Shi about 3 years ago

  • Blocks deleted (Feature #1712: FaceManager: create multicast face according to whitelist and blacklist)
#28

Updated by Junxiao Shi about 3 years ago

  • Status changed from In Progress to Resolved
  • % Done changed from 80 to 100

NFD devguide nfd-docs:commit:f9e61e649f014defda8e0346b2feaf345214e7d3

#29

Updated by Davide Pesavento about 3 years ago

Junxiao Shi wrote:

NFD devguide nfd-docs:commit:f9e61e649f014defda8e0346b2feaf345214e7d3

I did a few minor adjustments, but generally LGTM. I also commented out the "NIC-associated permanent faces" subsection you added, I think it's way too early to publicly document that functionality, since it's largely unimplemented.

#30

Updated by Davide Pesavento about 3 years ago

This task is not done yet though.

  1. You promised Alex that you were going to add #ifdefs for each protocol factory type, including TCP and UDP, to reduce the amount of patches that ndnSIM devs need to apply
  2. See note-18
#31

Updated by Junxiao Shi about 3 years ago

  • Blocks Bug #3970: Improve face_system error message when a ProtocolFactory is disabled added
#32

Updated by Junxiao Shi about 3 years ago

  • Status changed from Resolved to Closed

add #ifdefs for each protocol factory type, including TCP and UDP, to reduce the amount of patches that ndnSIM devs need to apply

This is no longer needed. ndnSIM only needs to disable compilation of tcp-.cpp and udp-.cpp, just like it does for other ProtocolFactory types.

See note-18

This is split to #3970.

I also commented out the "NIC-associated permanent faces" subsection you added, I think it's way too early to publicly document that functionality, since it's largely unimplemented.

No. It's OK to document upcoming features, such as "V2V Link Service".

#33

Updated by Davide Pesavento about 3 years ago

Junxiao Shi wrote:

add #ifdefs for each protocol factory type, including TCP and UDP, to reduce the amount of patches that ndnSIM devs need to apply

This is no longer needed. ndnSIM only needs to disable compilation of tcp-.cpp and udp-.cpp, just like it does for other ProtocolFactory types.

It is my understanding that Alex wanted to do this via build-time configuration macros (e.g. HAVE_TCP, HAVE_UDP). I'll let him comment on whether the current way of excluding files is also acceptable for ndnSIM or not.

Also available in: Atom PDF