Project

General

Profile

Feature #3818

Readvertise end-host routes into NLSR

Added by Junxiao Shi over 4 years ago. Updated almost 4 years ago.

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

100%

Estimated time:
18.00 h

Description

Implement the readvertise component which monitors locally registered routes and readvertises them into another NFD-RIB or routing protocol.
Implement a readvertise policy to readvertise routes registered by an end host through AutoPrefixPropagation into NLSR.


Files

readvertise-test.tar.gz (5.14 KB) readvertise-test.tar.gz Nicholas Gordon, 02/23/2017 02:37 PM

Related issues

Blocked by NFD - Feature #3784: Design readvertiseClosedJunxiao Shi

Actions
Blocks NFD - Task #3819: Use readvertise for auto prefix propagationClosedYanbiao Li

Actions
Blocked by ndn-cxx - Feature #3932: Add operator== for SigningInfoClosedNicholas Gordon01/25/2017

Actions
#1

Updated by Junxiao Shi over 4 years ago

#2

Updated by Junxiao Shi over 4 years ago

  • Subject changed from Route readvertise to Readvertise end-host routes into NLSR
  • Description updated (diff)
  • Assignee set to Nicholas Gordon
  • Estimated time set to 15.00 h

I suggest this implementation to be performed in stages as multiple commits:

  1. RIB route additional/removal signals, including RibRouteRef struct (1.5 hours)
  2. ReadvertiseDestination and NfdRibReadvertiseDestination, including ReadvertisedRoute struct (4.5 hours)
  3. ReadvertisePolicy and ClientToNlsrReadvertisePolicy and Readvertise (9 hours)

Each change listed above is individually testable. First and second changes can be worked in parallel.

#3

Updated by Junxiao Shi over 4 years ago

  • Estimated time changed from 15.00 h to 18.00 h

NFD developer guide should be updated as part of this issue, thus adding 3 hours.

#4

Updated by Junxiao Shi over 4 years ago

  • Blocks Task #3819: Use readvertise for auto prefix propagation added
#5

Updated by Nicholas Gordon over 4 years ago

I have finished implementing the added Signals to rib.cpp, and added the struct definition to rib.hpp. I am unsure what you mean by "separate commits." Do you intend for me to push this patch to gerrit, and then to move on to step 2?

#6

Updated by Junxiao Shi over 4 years ago

Do you intend for me to push this patch to gerrit, and then to move on to step 2?

Yes, you should upload RIB signals commit to Gerrit. Subsequent steps should be performed in separate commits.

#7

Updated by Nicholas Gordon over 4 years ago

I have been struggling with code review and unit tests for a while now (mostly as a result of general unfamiliarity with the NFD codebase and its design). However today I have had some success that prompts a question:
To make the unit test succeed it was required that I fully specify all of the parameters in the ControlParameters used in the advertise function. I don't understand why I've had to do this, though, because the RibMgmt documentation says that these fields should have defaults applied to them if they are absent. Further, I wasn't able to find a line outside of the unit tests where the functions "applyDefaultsToRequest" is used, which seems to me like they aren't used anywhere at all.

Can someone explain to me what's going on?

#8

Updated by Junxiao Shi over 4 years ago

I wasn't able to find a line outside of the unit tests where the functions "applyDefaultsToRequest" is used

If you grep the entire codebase, you should be able to find the only invocation.

#9

Updated by Nicholas Gordon over 4 years ago

I've managed to muddy the waters for myself again. I'm currently implementing the methods for Readvertise, but I've noticed that ReadvertisedRoute was designed with only a getter for a const version of the vector. The header design is unclear whether on afterAddRoute() a new, distinct ReadvertisedRoute should be created and added to the Readvertise instance, or if the existing one by prefix should be updated with more RibRouteRefs.

It is unclear whether RibRouteRefs for a common name should be kept together in one ReadvertisedRoute instance. If this is the case, then the ReadvertisedRoute needs to be extended to allow for adding a RibRouteRef to the member vector. If that is not the case, then why was it designed with a vector of RibRouteRefs in the first place and not just a singular instance? To be clear, in the case that the are meant to be aggregated, then on .advertise() and .withdraw(), each RibRouteRef in the vector must be used, correct?

Some clarification would be appreciated.

#10

Updated by Nicholas Gordon over 4 years ago

Since I have received no comments on note-9, I have assumed that the ReadvertisedRoute's list of ribRouteRefs should be mutable in some way, and have added mutator methods to it.

I don't understand exactly what is meant by "destination available." Is this some functionality available elsewhere, or is it implemented by each derived ReadvertiseDestination class? I cannot proceed until I understand this, so some help would be much appreciated.

#11

Updated by Junxiao Shi over 4 years ago

ReadvertisedRoute was designed with only a getter for a const version of the vector<RibRouteRef>

https://gist.github.com/yoursunny/bcee67893134d7d49c0868f5a8b9ed41/2e3a6e24042be5f02984e90815fe34567a9f8ace#file-readvertise-hpp-L39-L42
ReadvertisedRoute::getRibRoutes() returns a mutable reference of the vector, so that you can modify the returned reference directly.

#12

Updated by Nicholas Gordon over 4 years ago

  • % Done changed from 0 to 90

I am probably 90% done with pre-review implementation. However, I'm running into a problem with testing. I have written some fakes to examine Readvertise's methods since it has no public API, including Destination and Policy fakes. However, I can't make unique_ptrs out of these things because their parent class is noncopyable. It appears that the implementation of make_unique calls the copy constructor instead of the move constructor. This results in errors since the parent class is noncopyable...

I don't know enough about the implementation details of our make_unique, but I can say that it would be very cumbersome to test this without fakes/mocks. Is there any way to work around this? I have uploaded my readvertise.t.cpp to gist if it helps. (https://gist.github.com/gorgonical/4b3be61b1faac093693a484213457b93)

Note that I intend to revise and refactor the tests, as it is clear there is a lot of overlap.

#13

Updated by Nicholas Gordon about 4 years ago

I've been trying to write the unit tests for the NLSR component that listens for Readvertise commands, however I am having some difficulty using the DummyClientFace class to mock receiving interests through the dispatcher. I have posted my code snippets here:

https://gist.github.com/gorgonical/2ab5eff58c81a60e45dbaee99e087298

I understand that this is rather vague, but I don't really even know what I'm doing wrong, much less how to resolve it. At this point, none of my dispatcher callbacks are being called, as verified by GDB, but the ControlCommand handlers appear to be registered successfully in the dispatcher. This suggests to me that I am misunderstanding something about how to simulate Interests on this Face or about how the dispatcher works. I've tried emulating the ndn-cxx and nfd tests that involve the dispatcher, but that hasn't worked either. Any tips or discussion about how to correctly mock up what I'm doing would be very helpful and much appreciated.

#14

Updated by Nicholas Gordon about 4 years ago

  • Status changed from New to In Progress
#15

Updated by Nicholas Gordon about 4 years ago

#16

Updated by Nicholas Gordon about 4 years ago

I need a little help understanding how to effectively use this boost::multi_index_container. First, I can't use std::set because it only allows searching based on element identity. This doesn't mimic the previous data structure very well. This multi_index_container is working well, except all of the elements in the container are const. This means that the object has to be copied (or moved) from the container, modified, then placed back into the container.

I can do that, but I have to implement a lot of move mechanics and heavily refactor the code. Is that really worth the cost saving of storing two Names? Names are essentially fancy strings, and I wonder about the tradeoff in simplicity for the small improvement in memory efficiency.

If this refactor is insisted upon, I would appreciate some help about how to use this container effectively.

I have uploaded some code snippets to this gist: https://gist.github.com/gorgonical/3308d405ccbbc3279c7576e7c7678d7d

Any comments would be appreciated.

#17

Updated by Junxiao Shi about 4 years ago

This multi_index_container is working well, except all of the elements in the container are const. This means that the object has to be copied (or moved) from the container, modified, then placed back into the container.

Have you looked at the modify method on an index type?

#18

Updated by Nicholas Gordon about 4 years ago

modify will work for this situation, however, I have a problem in that the const semantics of the multi_index_container complicated things significantly. I'm running into issues using the container effectively, and in the interest of time, would it be an acceptable alternative to use the string representation the ReadvertisedRoute's prefix as the key in a map? I think that I could work out the issues given enough time, but I don't see it as being worth it; I'm unfamiliar with the construct and the const semantics ask for a large refactor of the patch. However, I see that Name's string representations (should) be unique, so that should be a cheap key.

#19

Updated by Junxiao Shi about 4 years ago

would it be an acceptable alternative to use the string representation the ReadvertisedRoute's prefix as the key in a map?

No, this is unacceptable.

#20

Updated by Nicholas Gordon about 4 years ago

The current patch set fails on the Ubuntu 14.04 slave because the version of the Boost library installed there (1.54) does not have the emplace function.
I see two options:

  1. Refactor the patch set to use insert, which I don't think would be too much hassle.
  2. Update the libraries on that host.

The only caveat is that insert is slightly less efficient, because the move constructor would be used to move the objects.

#21

Updated by Junxiao Shi about 4 years ago

The current patch set fails on the Ubuntu 14.04 slave because the version of the Boost library installed there (1.54) does not have the emplace function.

  1. Update the libraries on that host.

You can't. See platform policy.

The only caveat is that insert is slightly less efficient, because the move constructor would be used to move the objects.

To avoid the performance penalty:

#if BOOST_VERSION >= 105500
  m_index.emplace(..);
#else
  m_index.insert(..);
#endif
#22

Updated by Nicholas Gordon about 4 years ago

Regarding 02-16-2017's NFD call, I claimed that the Readvertise patch would be 100% feature-complete when this current patch on gerrit cleared, and Junxiao agreed with me. However, I do not think this is true. Nowhere in the patches is a Readvertise instance created that would actually connect to the RIB signals added earlier. This is not a large patch, but it does need to come along.

I propose adding the Readvertise instance to the RibManager. It could be added to the RIB itself, but it would require refactoring the RIB some; NfdRibReadvertiseDestination requires a controller instance, which the RIB does not have, but the RibManager does. Additionally, the AutoPrefixPropagator instance is already located in the RibManager. Further, the design of Readvertise requires that the instance not be created if readvertising is not desired. This is easiest to accomplish when the RibManager processes the config file, if the RibManager had for example a unique_ptr to its Readvertise instance.

#23

Updated by Junxiao Shi about 4 years ago

Readvertise should be owned by nfd::rib::Service.

I propose adding the Readvertise instance to the RibManager.

No. RibManager should only be responsible for processing incoming commands.

the AutoPrefixPropagator instance is already located in the RibManager.

That was a mistake. It should be moved into nfd::rib::Service.

Further, the design of Readvertise requires that the instance not be created if readvertising is not desired. This is easiest to accomplish when the RibManager processes the config file, if the RibManager had for example a unique_ptr to its Readvertise instance.

rib config section should be processed in nfd::rib::Service.

You may add two more commits after the commit adding Readvertise itself:

  • rib: process rib config section in rib::Service (this commit also moves AutoPrefixPropagator)
  • rib: enable readvertise into NLSR

Finally, the config key for readvertising into NLSR should be rib.readvertise_nlsr. It can be a simple yes/no or a section like auto_prefix_propagate.

#24

Updated by Nicholas Gordon about 4 years ago

I have developed an integration test for mini-ndn using the two patches still on gerrit, as well as a third patch to enable Readvertise in NFD itself. That patchfile is included in the .tar.gz

It is a 5-node, linear topology that does verify that a prefix that is AutoPrefixPropagated to a hub router will be Readvertised into NLSR, and thus show up on remote routers through NLSR.

#25

Updated by Junxiao Shi about 4 years ago

  • Assignee changed from Nicholas Gordon to Junxiao Shi
  • % Done changed from 90 to 80

As I promised, after Nick is gone at end of Feb 2017, I'll complete this feature.

https://gerrit.named-data.net/3747 simplifies ReadvertisedRoute as a POD type, so that it can be kept noncopyable and used in a STL container.
https://gerrit.named-data.net/3573 adds ReadvertisePolicy and implements ClientToNlsrReadvertisePolicy.
https://gerrit.named-data.net/3748 introduces Readvertise class.

#26

Updated by Junxiao Shi about 4 years ago

  • Status changed from In Progress to Code review
  • % Done changed from 80 to 100

After a careful examination on current code, I'm unable to fulfill note-23 design. Thus, I just updated https://gerrit.named-data.net/3734 to use the recommended config option name.

#27

Updated by Junxiao Shi about 4 years ago

  • Status changed from Code review to In Progress

Code is merged. Need devguide update.

#28

Updated by Junxiao Shi about 4 years ago

  • Assignee changed from Junxiao Shi to Nicholas Gordon

I'd leave NFD devguide work to Nick.

#29

Updated by Nicholas Gordon about 4 years ago

I've pushed a first draft of the Readvertise documentation to the Dev guide repo. Let me know what you think of the additions. I've placed the section just above the AutoPrefixPropagator section.

#30

Updated by Nicholas Gordon almost 4 years ago

If no one has any comments, I can close this issue then?

#31

Updated by Junxiao Shi almost 4 years ago

  • Status changed from In Progress to Closed

Also available in: Atom PDF