Project

General

Profile

Task #4787

Consolidate src/tlv/**lsa** into src/**lsa**

Added by Ashlesh Gawande over 1 year ago. Updated about 1 month ago.

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

100%

Estimated time:

Description

Seems like duplicated functionality. Also then we can use wireEncode for LSAs instead of creating a string manually to represent the LSA.


Related issues

Is duplicate of NLSR - Feature #4267: Remove explicit TLV classesNew

Actions
#1

Updated by Ashlesh Gawande over 1 year ago

  • Assignee set to Saurab Dulal
#2

Updated by Ashlesh Gawande 4 months ago

  • Status changed from New to Code review
  • Assignee changed from Saurab Dulal to Ashlesh Gawande
  • % Done changed from 0 to 100

https://gerrit.named-data.net/c/NLSR/+/5935

Major changes:
tlv/lsa classes are removed. wireEncode/wireDecode functions from it are moved to lsa/lsa where they are modified as followed:

  • Lsa (Used to be LsaInfo)
    • Use ndn::tlv::Name instead of ndn::tlv::nlsr::OriginRouter, OriginRouter is removed from tlvs
    • Use expiration timepoint as string instead of expiration period as was the case in LsaInfo
      • Clients are expected to subtracted from current time if they want to show time period
      • Nlsrc is modified to do so
  • Adjacent: Include status as AdjacencyStatus as well as number of interest timed out as AdjacencyInterestTimedOutNo

If these changes are acceptable then I can update the wiki pages. This will be a breaking change.

Serialize/Deserialize functions are removed from Lsa classes and replaced with the newly moved wireEncode/wireDecode.
Lsdb and Dataset publisher are now using these to send LSAs on the wire.

All the other changes are to either facilitate the above or cleaning up the code as seen during the move.
The change deletes ~3000 lines of code including tests (~1000 added, ~4000 deleted).
This improves clean compilation by 2 seconds without tests and 6 seconds with tests on my machine.

#3

Updated by Ashlesh Gawande 2 months ago

  • Is duplicate of Feature #4267: Remove explicit TLV classes added
#4

Updated by Ashlesh Gawande 2 months ago

Adjacent: Include status as AdjacencyStatus as well as number of interest timed out as AdjacencyInterestTimedOutNo

This is redundant info that NLSR receiving the AdjLsa does not use. So leaning towards not sending it.
I put it there initially to be complete and so that printing them using ostream from AdjLsa in nlsrc prints non-default values. Both arguments seem weak - so not sure if there is any point in including them. Currently NLSR does not send this info and uses the default value when decoding AdjLsa from the wire.

Clients are expected to subtracted from current time if they want to show time period

If subtraction is negative clients need to put some default value.
An option to solve this is to include also Expiration Period in addition to the timestamp in the Lsa. (Some complaints on the mailing list: https://www.lists.cs.ucla.edu/pipermail/ndn-interest/2019-June/002483.html). Also the existing TLV encoding did include Expiration period and not Expiration Point - so this would maintain that part.

Also added some unit tests for testing encoding (in place of existing tlv tests which are removed in this change).

Ref:

#5

Updated by Ashlesh Gawande 2 months ago

Correct mailing list link outlining the problem with using ExpirationTime set at the sender:
https://www.lists.cs.ucla.edu/pipermail/ndn-interest/2019-August/002569.html

#6

Updated by Ashlesh Gawande 2 months ago

https://redmine.named-data.net/projects/nlsr/wiki/Proposed_Common_TLV_for_NLSR-to-NLSR_and_NLSR-to-NLSRC_communication
is updated to remove ExpirationPeriod from LSA:

The reason to not use ExpirationPeriod even though it would solve this particular clock sync problem in NLSR, other things like certificate validity still depend on loose clock sync b/w nodes. So clock synchronization problem between nodes is independent of NLSR.

Current change https://gerrit.named-data.net/c/NLSR/+/5935 (patchset 10,11) follows the updated proposed specification above.

#7

Updated by Saurab Dulal about 2 months ago

Ashlesh Gawande wrote:

https://redmine.named-data.net/projects/nlsr/wiki/Proposed_Common_TLV_for_NLSR-to-NLSR_and_NLSR-to-NLSRC_communication
is updated to remove ExpirationPeriod from LSA:

The reason to not use ExpirationPeriod even though it would solve this particular clock sync problem in NLSR, other things like certificate validity still depend on loose clock sync b/w nodes. So clock synchronization problem between nodes is independent of NLSR.

Personally, I have read about this so many times but still don't have a strong opinion on what can be the right choice. But, the way you have explained and implemented might be the right way to go for now.

Current change https://gerrit.named-data.net/c/NLSR/+/5935 (patchset 10,11) follows the updated proposed specification above.

Thanks for the detailed explanation of the changes.

#8

Updated by Ashlesh Gawande about 1 month ago

  • Status changed from Code review to Closed

Also available in: Atom PDF