Project

General

Profile

Actions

Feature #4318

closed

Content Store flush/erase command

Added by Junxiao Shi over 6 years ago. Updated over 5 years ago.

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

100%

Estimated time:
12.00 h

Description

In CsMgmt, design a protocol to erase a subset of Content Store entries.
Implement the protocol in NFD management, and provide nfdc subcommands.


Related issues 2 (0 open2 closed)

Related to NFD - Feature #4050: Content Store ManagerClosedJunxiao Shi

Actions
Related to ndn-cxx - Bug #4538: Congestion mark related elements missing in ControlParametersClosedEric Newberry

Actions
Actions #1

Updated by Davide Pesavento over 6 years ago

  • Start date deleted (10/02/2017)
Actions #2

Updated by Davide Pesavento over 6 years ago

Actions #3

Updated by Junxiao Shi about 6 years ago

  • Status changed from New to In Progress
  • Assignee set to Junxiao Shi
  • Target version set to v0.7
  • % Done changed from 0 to 10

CsMgmt rev9 and ControlCommand rev37 introduce cs/erase command.

Actions #4

Updated by Davide Pesavento about 6 years ago

Junxiao Shi wrote:

CsMgmt rev9 and ControlCommand rev37 introduce cs/erase command.

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 #5

Updated by Junxiao Shi about 6 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 #6

Updated by Junxiao Shi about 6 years ago

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

Actions #7

Updated by Davide Pesavento about 6 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 #8

Updated by Junxiao Shi about 6 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 #9

Updated by Davide Pesavento about 6 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 #10

Updated by Alex Afanasyev about 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 #11

Updated by Junxiao Shi about 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 #12

Updated by Davide Pesavento about 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 #13

Updated by Junxiao Shi about 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 #14

Updated by Davide Pesavento about 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 #15

Updated by Junxiao Shi about 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 #16

Updated by Alex Afanasyev about 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 #17

Updated by Junxiao Shi about 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 #18

Updated by Alex Afanasyev about 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 #19

Updated by Alex Afanasyev about 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 #20

Updated by Junxiao Shi about 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 #21

Updated by Davide Pesavento about 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 #22

Updated by Junxiao Shi about 6 years ago

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

Updated by Junxiao Shi about 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 #24

Updated by Davide Pesavento about 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 #25

Updated by Davide Pesavento about 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 #26

Updated by Junxiao Shi almost 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 #27

Updated by Junxiao Shi almost 6 years ago

  • % Done changed from 20 to 60
Actions #29

Updated by Junxiao Shi over 5 years ago

  • Status changed from In Progress to Code review
  • % Done changed from 60 to 100
Actions #30

Updated by Junxiao Shi over 5 years ago

  • Status changed from Code review to Closed
Actions

Also available in: Atom PDF