Feature #4318: Content Store flush/erase command - NFD - NDN project issue tracking system

In ControlParameters, can we give NCsEntries a more generic name, in case it's needed in the future for similar purposes but not related to the CS? Some suggestions are: NEntries, Count.

Other than that, LGTM.

Actions

Copy link

Updated by Junxiao Shi almost 7 years ago

In ControlParameters, can we give NCsEntries a more generic name, in case it's needed in the future for similar purposes but not related to the CS? Some suggestions are: NEntries, Count.

0x87 is associated with NCsEntries tag, so it's most convenient to keep the existing tag. Other purposes may reuse 0x87 regardless of what it's called.

Actions

Copy link

Updated by Junxiao Shi almost 7 years ago

https://gerrit.named-data.net/4521 declares cs/erase command.
CsMgmt rev14 requires NCsEntries in request to be positive.

Actions

Copy link

Updated by Davide Pesavento almost 7 years ago

Junxiao Shi wrote:

In ControlParameters, can we give NCsEntries a more generic name, in case it's needed in the future for similar purposes but not related to the CS? Some suggestions are: NEntries, Count.

0x87 is associated with NCsEntries tag, so it's most convenient to keep the existing tag. Other purposes may reuse 0x87 regardless of what it's called.

There is no "existing tag". You just added the assignment in r37 of ControlCommand.
There is no reason to tie this field to the CS. In the same spirit, the capacity field was named Capacity and not CsCapacity.

Actions

Copy link

Updated by Junxiao Shi almost 7 years ago

There is no "existing tag". You just added the assignment in r37 of ControlCommand.

The tag for 0x87 is NCsEntries and it existed in ForwarderStatus.

There is no reason to tie this field to the CS. In the same spirit, the capacity field was named Capacity and not CsCapacity.

Logically each command has its own parameters type. There should be no consideration for other commands whatsoever in adding a field for cs/erase.
Microsoft adCenter API declares a separate parameter class for each command, with automatic code generation out of WSDL. We should do the same for each command.

Actions

Copy link

Updated by Davide Pesavento almost 7 years ago

Junxiao Shi wrote:

The tag for 0x87 is NCsEntries and it existed in ForwarderStatus.

It's a different TLV element...

There is no reason to tie this field to the CS. In the same spirit, the capacity field was named Capacity and not CsCapacity.

Logically each command has its own parameters type. There should be no consideration for other commands whatsoever in adding a field for cs/erase.
Microsoft adCenter API declares a separate parameter class for each command, with automatic code generation out of WSDL. We should do the same for each command.

How is this related to my comment? Yes, the current ControlParameters structure is ugly, but that's a separate issue that should be discussed/solved separately. It doesn't answer my concern. In fact, if "logically each command has its own parameters type", then there's yet another reason to not include "Cs" in the param name.

Actions

Copy link

#10

Updated by Alex Afanasyev over 6 years ago

The spec needs to define how to remove "all" entries under the name. I don't think using max is a good option; I would use 0 that is currently prohibited.

Actions

Copy link

#11

Updated by Junxiao Shi over 6 years ago

The spec needs to define how to remove "all" entries under the name. I don't think using max is a good option; I would use 0 that is currently prohibited.

The caller has to repeat the operation until NCsEntries in the response becomes zero. The loop will be implemented in nfdc.
It's intentional to have an upper bound and disallow erasing too many entries at a time, because it would put too much stress on tables. This limitation can be lifted after #4528.

Actions

Copy link

#12

Updated by Davide Pesavento over 6 years ago

Junxiao Shi wrote:

This limitation can be lifted after #4528.

Not really. The mgmt thread still has to lock the table to perform the erasure.

Actions

Copy link

#13

Updated by Junxiao Shi over 6 years ago

This limitation can be lifted after #4528.

Not really. The mgmt thread still has to lock the table to perform the erasure.

Mgmt thread can erase CS entries in batches, and unlock the table in between.

Actions

Copy link

#14

Updated by Davide Pesavento over 6 years ago

Junxiao Shi wrote:

This limitation can be lifted after #4528.

Not really. The mgmt thread still has to lock the table to perform the erasure.

Mgmt thread can erase CS entries in batches, and unlock the table in between.

That doesn't require a separate thread... Anyway, we're going off-topic.

Regardless of the forwarder-side upper bound on the number of entries erased with a single request, I think what Alex is saying is that there should be a way to specify "erase all entries" at the protocol level (which becomes "erase as many as possible" if the forwarder has an upper limit). One could use a very large number, or query the CS capacity first and use that as limit. Both approaches don't look very elegant though...

Actions

Copy link

#15

Updated by Junxiao Shi over 6 years ago

Regardless of the forwarder-side upper bound on the number of entries erased with a single request, there should be a way to specify "erase all entries" at the protocol level (which becomes "erase as many as possible" if the forwarder has an upper limit).

CsMgmt rev15 specifies that omitted NCsEntries in a request indicates "no limit".
This revision also removes the requirement of changing StatusCode to 206 when exceeding the limit, because the inclusion of Capacity in a response already indicates this condition.

Actions

Copy link

#16

Updated by Alex Afanasyev over 6 years ago

Priority changed from Normal to Urgent

The merged ndn-cxx:commit:df5053824fc53eec6847067faa2da8b0ebb69917 breaks NFD. I have no clue how, but simply compiling ndn-cxx, NFD against it, and running NFD results in a segfault (assertion fault) with the last words:

...
1520295590.549215 TRACE: [ndn.security.v2.ValidationState] ~ValidationState
1520295590.549512 DEBUG: [ndn.security.v2.Validator] > Start validating data /localhost/nfd/faces/update/h%06l%01%01p%01%01/%00%00%01a%F8%AD%BA%0F/%16%AE%29%A5%C1%BBZ%1E/%16%1C%1B%01%01%1C%17%07%15%08%04test%08%03KEY%08%08%D0%80%A8%A7Hr0%C6/%17%FD%01%00x%89%24%1C%3AK4%816%86%5B%B3%9Bh%F8%99DHU%F3%19A%3Eh%F9%BFLTX%92%9B%06%BE%9E%60%81%7B%B6%5D%96%F9%27%22e%88%88%40%EAr%EF%A1%20%E5%AA7Z%FC%C6%C1%AF%DDX%E02%E3n%B2%EC%EB%B1%8B%A8%26%AC%00%BB%88%F3%20%FD%04%2C%8Dh%0C%3B%B2%97q%89HCx%94%19z%F0%E2%84%88%CF%C1%92%1C%2C%F4%B6%85%C8%10%DF8%5C%C9%14%FC%80%15%F9%A9%1E%C1%C1%3Ei%C7%CF%3F%23A%9B%3CE%0D%60%A1%A3%A4%3C%F7%B4%CF%A6%D32%B2%90%B2%C8%01%01%E2%E5%E76%05%E5%FD%B1%7D%0F%E6%1E%16%ECv%99%18c%D0J%F8%07%0D%A9%F8U%5E%5B%A1%95%28%B1%EB3%F2%EE%16%BC%D7%8F%E4%EC%C9uS%B6%83%B6%AEY%CF%2C%FF9%16%93%C9%5C%00%A8%16%00%8Fm%B1%A6%15%24z%2F%AE%CA%E6%92%28%E8%E7%17%83J%B6%15%D7%9E%9C%D7%AA%F1k%09%11%ED%86%97%2B%0B%EA%26%9E%F7%EF%C3.j0
1520295590.549592 TRACE: [ndn.security.v2.ValidationState] > Signature verification bypassed for data `/localhost/nfd/faces/update/h%06l%01%01p%01%01/%00%00%01a%F8%AD%BA%0F/%16%AE%29%A5%C1%BBZ%1E/%16%1C%1B%01%01%1C%17%07%15%08%04test%08%03KEY%08%08%D0%80%A8%A7Hr0%C6/%17%FD%01%00x%89%24%1C%3AK4%816%86%5B%B3%9Bh%F8%99DHU%F3%19A%3Eh%F9%BFLTX%92%9B%06%BE%9E%60%81%7B%B6%5D%96%F9%27%22e%88%88%40%EAr%EF%A1%20%E5%AA7Z%FC%C6%C1%AF%DDX%E02%E3n%B2%EC%EB%B1%8B%A8%26%AC%00%BB%88%F3%20%FD%04%2C%8Dh%0C%3B%B2%97q%89HCx%94%19z%F0%E2%84%88%CF%C1%92%1C%2C%F4%B6%85%C8%10%DF8%5C%C9%14%FC%80%15%F9%A9%1E%C1%C1%3Ei%C7%CF%3F%23A%9B%3CE%0D%60%A1%A3%A4%3C%F7%B4%CF%A6%D32%B2%90%B2%C8%01%01%E2%E5%E76%05%E5%FD%B1%7D%0F%E6%1E%16%ECv%99%18c%D0J%F8%07%0D%A9%F8U%5E%5B%A1%95%28%B1%EB3%F2%EE%16%BC%D7%8F%E4%EC%C9uS%B6%83%B6%AEY%CF%2C%FF9%16%93%C9%5C%00%A8%16%00%8Fm%B1%A6%15%24z%2F%AE%CA%E6%92%28%E8%E7%17%83J%B6%15%D7%9E%9C%D7%AA%F1k%09%11%ED%86%97%2B%0B%EA%26%9E%F7%EF%C3.j0`
1520295590.549854 DEBUG: [ndn.security.v2.ValidationState] > Internal implementation error (Validator/policy did not invoke success or failure callback)
Assertion failed: (!boost::logic::indeterminate(m_outcome)), fun1520295590.549944 TRACE: [ndn.security.v2.ValidationState] ~ValidationStatection ~ValidationState, file ../
src/security/v2/validation-state.cpp, line 44.
Abort trap: 6

Actions

Copy link

#17

Updated by Junxiao Shi over 6 years ago

Priority changed from Urgent to Normal

note-16 has nothing to do with this issue. Please open a separate bug with all informational requested in http://www.lists.cs.ucla.edu/pipermail/nfd-dev/2016-May/001748.html

Actions

Copy link

#18

Updated by Alex Afanasyev over 6 years ago

Junxiao, please be constructive and not obstructive. There is a problem that is seemingly caused by the commit related to this issue. I have provided all info to reproduce the problem and it was reliably reproduced by me, Davide, and Ashlesh. Help fix the problem, not instruct other on how to document the problem.

Actions

Copy link

#19

Updated by Alex Afanasyev over 6 years ago

Priority changed from Normal to High

Ok. After spending a few hours. The issue directly relates to this issue: you managed to use the same TLV for two different fields in the ControlParameters: 135 code is used twice causing all type of trouble. Please renumber and submit patch asap, so we back to working NFD in the master branch.

Actions

Copy link

#20

Updated by Junxiao Shi over 6 years ago

you managed to use the same TLV for two different fields in the ControlParameters: 135 code is used twice causing all type of trouble.

There is only one “135” in ControlCommand r39. I don’t see a second assignment.

Actions

Copy link

#21

Updated by Davide Pesavento over 6 years ago

Junxiao Shi wrote:

you managed to use the same TLV for two different fields in the ControlParameters: 135 code is used twice causing all type of trouble.

There is only one “135” in ControlCommand r39. I don’t see a second assignment.

https://github.com/named-data/ndn-cxx/blob/df5053824fc53eec6847067faa2da8b0ebb69917/src/encoding/tlv-nfd.hpp#L69

Actions

Copy link

#22

Updated by Junxiao Shi over 6 years ago

Related to Bug #4538: Congestion mark related elements missing in ControlParameters added

Actions

Copy link

#23

Updated by Junxiao Shi over 6 years ago

Priority changed from High to Normal

https://github.com/named-data/ndn-cxx/blob/df5053824fc53eec6847067faa2da8b0ebb69917/src/encoding/tlv-nfd.hpp#L69

This looks like congestion control group's fault because they forgot to update ControlCommand. Assigned to them in #4538.
If I had reviewed ndn-cxx:commit:07d05c97b435a8de5ff76986df6a0cb3a50a6484, I would have checked the implementation matches protocol spec, and would not have approved the code without having the protocol in place first.

Actions

Copy link

#24

Updated by Davide Pesavento over 6 years ago

Renumbering of BaseCongestionMarkingInterval is not going to happen. We have a full implementation based on it and we already shipped that code (and that TLV assignment) in v0.6.1. It is way simpler to just change the assignment for NCsEntries, since it was just merged and is still unimplemented in NFD.

And while at it, rename it to NEntries or Count like I said in note-4, since your previous argument no longer applies.

Actions

Copy link

#25

Updated by Davide Pesavento over 6 years ago

Davide Pesavento wrote:

And while at it, rename it to NEntries or Count like I said in note-4, since your previous argument no longer applies.

Done in https://gerrit.named-data.net/4607

Actions

Copy link

#26

Updated by Junxiao Shi over 6 years ago

% Done changed from 10 to 20

https://gerrit.named-data.net/4650 adds Cs::erase method. Like Cs::find, the API is asynchronous, to allow the possibility of offline (Solid State Drive) CS entries.

Actions

Copy link

#27