Feature #4465
closed
Feature #1624: Design and Implement Congestion Control
Management Support for Configuring Congestion Signaling
Added by Eric Newberry almost 7 years ago. Updated almost 7 years ago.
100%
Description
Add support to enable or disable congestion signaling (& detection) on a specific face to the NFD management system and the nfdc command line tool. Also add support to set the following options:
- The base congestion marking interval
- The default congestion threshold.
Amend nfdc face to show the current status of congestion signaling (enabled/disabled) and the values of these options.
Updated by Eric Newberry almost 7 years ago
- Status changed from New to In Progress
Updated by Eric Newberry almost 7 years ago
- Subject changed from Design and implement management support for allowing and disallowing congestion marking to Design and implement management support for enabling/disabling congestion marking
- Description updated (diff)
allowed/disallowed => enabled/disabled
Updated by Eric Newberry almost 7 years ago
- % Done changed from 0 to 10
For this issue, I suggest the following changes to FaceMgmt - feedback is requested:
- Add a static face attribute CongestionMarkingEnabled to indicate whether congestion marking based upon send queue length is enabled/disabled.
- The value of this can field can be yes(=1) to enable the feature or no(=0) to disable the feature.
- Add a static face attribute BaseCongestionMarkingInterval to control the base marking interval option.
- The value of this field is an unsigned integer representing nanoseconds.
- Add a static face attribute DefaultCongestionThreshold to control the default congestion threshold option.
- The value of this field is an unsigned integer representing bytes.
- Add a bit (bit 2) to the Flags and Mask fields to hold the value of the CongestionMarkingEnabled attribute.
- Modify the
faces/createcommand to add as optional input ControlParameters fields BaseCongestionMarkingInterval and DefaultCongestionThreshold (as well as adding CongestionMarkingEnabled as a Flag).- BaseCongestionMarkingInterval and DefaultCongestionThreshold can still be set if CongestionMarkEnabled=0 (incl. if the transport type does not support congestion detection).
- The value of these attributes, and CongestionMarkingEnabled (via Flags) will be provided in the responding ControlResponse block upon successful face creation.
- The ControlResponse Flags field will also contain the value of CongestionMarkingEnabled.
- Modify the
faces/updatecommand to add as optional input ControlParameters fields BaseCongestionMarkingInterval and DefaultCongestionThreshold (as well as adding CongestionMarkingEnabled as a Flag).- The value of these attributes, and CongestionMarkingEnabled (via Flags) will be provided in the responding ControlResponse block upon successful face update.
- Add BaseCongestionMarkingInterval and DefaultCongestionThreshold to the FaceStatus block.
- Assign TLV-TYPEs for the new static attributes BaseCongestionMarkingInterval and DefaultCongestionThreshold.
Updated by Davide Pesavento almost 7 years ago
- Subject changed from Design and implement management support for enabling/disabling congestion marking to Design and implement management support for configuring congestion marking
Updated by Anonymous almost 7 years ago
- Subject changed from Design and implement management support for configuring congestion marking to Management Support for Configuring Congestion Signaling
Updated by Anonymous almost 7 years ago
I like short titles and the word "signaling" :)
Updated by Anonymous almost 7 years ago
The design sounds good to me. Can someone else have a look?
Updated by Davide Pesavento almost 7 years ago
I agree with the protocol changes proposed in note-3. I don't think adding BaseCongestionMarkingInterval and DefaultCongestionThreshold to FaceEventNotification is necessary or useful, but I'm not against it either.
Updated by Davide Pesavento almost 7 years ago
In addition to note-3, I was considering whether adding an nfd.conf setting to globally enable or disable congestion marking would be useful. I think it would, because NFD's management API cannot manipulate Unix faces, and is not practical for on-demand faces in general. Flags specified on nfdc command line would still override the global setting.
Updated by Anonymous almost 7 years ago
Davide Pesavento wrote:
In addition to note-3, I was considering whether adding an
nfd.confsetting to globally enable or disable congestion marking would be useful. I think it would, because NFD's management API cannot manipulate Unix faces, and is not practical for on-demand faces in general. Flags specified onnfdccommand line would still override the global setting.
Yeah, that's a great idea.
Updated by Eric Newberry almost 7 years ago
Klaus Schneider wrote:
Davide Pesavento wrote:
In addition to note-3, I was considering whether adding an
nfd.confsetting to globally enable or disable congestion marking would be useful. I think it would, because NFD's management API cannot manipulate Unix faces, and is not practical for on-demand faces in general. Flags specified onnfdccommand line would still override the global setting.Yeah, that's a great idea.
I like this idea as well. I assume it would be in addition to the management interface above?
Also, I have a few minor corrections/changes to the design that I'll add in a minute.
Updated by Davide Pesavento almost 7 years ago
Eric Newberry wrote:
I like this idea as well. I assume it would be in addition to the management interface above?
yes
Updated by Eric Newberry almost 7 years ago
The design in note 3 has been updated. Here is a summary of the changes:
- Allow BaseCongestionMarkingInterval and DefaultCongestionThreshold to be set even if marking is disabled or disallowed on the transport type
- Only allow CongestionMarkingEnabled to be "yes" (value=1) when using a TCP, UDP, or Unix transport. (Unix is included to allow it to be displayed as enabled when this is done through
nfd.conf).
Updated by Davide Pesavento almost 7 years ago
By default, this feature will be disabled (value=0).
What does "by default" mean in this context? There needn't be a default for a face attribute.
A value of yes is only valid when the remote FaceUri scheme is "udp4", "udp6", "tcp4", "tcp6", or "unix". (Unix transports cannot be controlled by management, so they are excluded from this list).
The second sentence contradicts the first one. Also note that you're mixing protocol definition with implementation-specific behavior. The protocol doesn't need to say which attribute values are valid on which face types; the fact that Ethernet faces don't support marking is merely an implementation limitation (similarly, the protocol doesn't say that e.g. Unix faces cannot be permanent).
Updated by Eric Newberry almost 7 years ago
My design in note 3 has been updated to address Davide's comments in note 16.
Updated by Davide Pesavento almost 7 years ago
If unset, the corresponding link service options will use the default value set in the GenericLinkService::Options constructor.
Mentioning "link service" and the constructor is also an implementation detail. Either choose a fixed default value (like FacePersistency does), or don't say anything (implying that the default is implementation-defined).
Add BaseCongestionMarkingInterval and DefaultCongestionThreshold to the FaceStatus block.
Add BaseCongestionMarkingInterval and DefaultCongestionThreshold to the FaceEventNotification block.
Are these optional or required?
Updated by Eric Newberry almost 7 years ago
Davide Pesavento wrote:
If unset, the corresponding link service options will use the default value set in the GenericLinkService::Options constructor.
Mentioning "link service" and the constructor is also an implementation detail. Either choose a fixed default value (like FacePersistency does), or don't say anything (implying that the default is implementation-defined).
Ok, I removed this point.
Add BaseCongestionMarkingInterval and DefaultCongestionThreshold to the FaceStatus block.
Add BaseCongestionMarkingInterval and DefaultCongestionThreshold to the FaceEventNotification block.Are these optional or required?
Required.
Updated by Davide Pesavento almost 7 years ago
To avoid blocking/delaying testbed deployment, I suggest splitting this issue in (at least) two commits: the first commit introduces the configuration file option to globally enable/disable marking, while the second commit implements the rest (face management + nfdc). The second commit can be further split into as many commits as you see fit. Testbed deployment needs only the first commit. Eventually, there can be a third (or n-th) commit that flips the default allowCongestionMarking from false to true.
Updated by Eric Newberry almost 7 years ago
Davide Pesavento wrote:
To avoid blocking/delaying testbed deployment, I suggest splitting this issue in (at least) two commits: the first commit introduces the configuration file option to globally enable/disable marking, while the second commit implements the rest (face management + nfdc). The second commit can be further split into as many commits as you see fit. Testbed deployment needs only the first commit. Eventually, there can be a third (or n-th) commit that flips the default
allowCongestionMarkingfrom false to true.
I've pushed a commit implementing the config file option method.
Updated by Eric Newberry almost 7 years ago
The config file option commit has been merged. The face management + nfdc commit is in the works at the moment.
Updated by Eric Newberry almost 7 years ago
Per Davide's suggestion in note 10, I have removed BaseCongestionMarkingInterval and DefaultCongestionThreshold from the FaceEventNotification block.
Updated by Eric Newberry almost 7 years ago
I added the design in note 3 to FaceMgmt, including assigning TLV-TYPEs for BaseCongestionMarkingInterval and DefaultCongestionThreshold.
Updated by Davide Pesavento almost 7 years ago
Eric Newberry wrote:
I added the design in note 3 to FaceMgmt, including assigning TLV-TYPEs for BaseCongestionMarkingInterval and DefaultCongestionThreshold.
The description of BaseCongestionMarkingInterval doesn't explain what it means or how it's used.
Updated by Eric Newberry almost 7 years ago
Davide Pesavento wrote:
Eric Newberry wrote:
I added the design in note 3 to FaceMgmt, including assigning TLV-TYPEs for BaseCongestionMarkingInterval and DefaultCongestionThreshold.
The description of BaseCongestionMarkingInterval doesn't explain what it means or how it's used.
Fixed.
Updated by Junxiao Shi almost 7 years ago
In FaceMgmt rev78, it is unclear what’s the semantics of omitted BaseCongestionMarkingInterval and DefaultCongestionThreshold in requests and responses.
Updated by Eric Newberry almost 7 years ago
Junxiao Shi wrote:
In FaceMgmt rev78, it is unclear what’s the semantics of omitted BaseCongestionMarkingInterval and DefaultCongestionThreshold in requests and responses.
This has been addressed in r79.
Updated by Davide Pesavento almost 7 years ago
Eric Newberry wrote:
This has been addressed in r79.
r79 says:
the omitted static properties are left at the default values set by the link service.
"link service" is an implementation detail of NFD and should not be mentioned in the protocol spec. Just say that an implementation-defined default value is used.
If BaseCongestionMarkingInterval and DefaultCongestionThreshold are omitted in the response ControlParameters, this indicates that congestion marking is not supported
How do you intend to implement this?
Updated by Junxiao Shi almost 7 years ago
If BaseCongestionMarkingInterval or DefaultCongestionThreshold are omitted in the ControlParameters sent with the creation command, the omitted static properties are left at the default values set by the link service.
This rule causes these fields to take on a default value and becomes non-empty. In other words, there exists a ControlParameters with filled values that is equivalent to a ControlParameters with these fields omitted,
if a static property specified in the request is not acceptable, the command fails with StatusCode 406.
Congestion mark parameters are static properties.
A face not supporting congestion marks cannot accept any value of these static properties.
Therefore, it becomes impossible to create a face if it does not support congestion marks.
Do not explain or argue here. Instead, make the protocol non ambiguous. Suggestion: qualify the first quote above to indicate that a default value presents only if the face supports congestion marks.
Flags: contains effective values for LocalFieldsEnabled and LpReliabilityEnabled bits.
Why not include “CongestionMarkingEnabled” bit as well?
Updated by Eric Newberry almost 7 years ago
Junxiao Shi wrote:
Congestion mark parameters are static properties.
A face not supporting congestion marks cannot accept any value of these static properties.
Therefore, it becomes impossible to create a face if it does not support congestion marks.
Why not? Just because some static properties are set does not mean they have to be utilized.
Updated by Eric Newberry almost 7 years ago
- % Done changed from 10 to 70
Management protocol implementation has been merged. Up next is nfdc.
Updated by Junxiao Shi almost 7 years ago
Why not? Just because some static properties are set does not mean they have to be utilized.
If the properties are set, the response shall contain them regardless of whether they are utilized.
If the response does not carry them, the default on request shall be “empty for faces not supporting congestion marking, implementation-defined for faces supporting congestion marking”. Setting these values to non-empty on a face not supporting congestion marking shall cause 406.
Updated by Eric Newberry almost 7 years ago
Junxiao Shi wrote:
Why not? Just because some static properties are set does not mean they have to be utilized.
If the properties are set, the response shall contain them regardless of whether they are utilized.
Yes, this is what I'm saying.
If the response does not carry them, the default on request shall be “empty for faces not supporting congestion marking, implementation-defined for faces supporting congestion marking”. Setting these values to non-empty on a face not supporting congestion marking shall cause 406.
Why does this cause a 406 if they can still be set on the face? Above you said a 406 would only be caused if the parameters are not acceptable. These parameters are perfectly acceptable, although they would not be used.
Updated by Eric Newberry almost 7 years ago
The management protocol has been updated in r80.
Updated by Eric Newberry almost 7 years ago
- % Done changed from 70 to 90
nfdc changes are ready for review.
Updated by Junxiao Shi almost 7 years ago
If the properties are set, the response shall contain them regardless of whether they are utilized.
Yes, this is what I'm saying.
Then why are BaseCongestionMarkingInterval and DefaultCongestionThreshold declared as “optional” in faces/create command response?
Updated by Davide Pesavento almost 7 years ago
Junxiao Shi wrote:
If the properties are set, the response shall contain them regardless of whether they are utilized.
Yes, this is what I'm saying.
Then why are BaseCongestionMarkingInterval and DefaultCongestionThreshold declared as “optional” in faces/create command response?
Because an implementation of the management protocol is not required to support them, hence it should not be required to include them in the response.
Updated by Eric Newberry almost 7 years ago
- Status changed from In Progress to Closed
- % Done changed from 90 to 100
The nfdc changes allowing congestion signaling to be configured have been merged. Unless we want to enable congestion marking by default, I think this issue has been completed, so I'm going to close it.
Updated by Davide Pesavento almost 7 years ago
Eric Newberry wrote:
The nfdc changes allowing congestion signaling to be configured have been merged. Unless we want to enable congestion marking by default, I think this issue has been completed, so I'm going to close it.
Yes, we want it enabled by default. Plus the NFD devguide needs to be updated.
Updated by Eric Newberry almost 7 years ago
Davide Pesavento wrote:
Eric Newberry wrote:
The nfdc changes allowing congestion signaling to be configured have been merged. Unless we want to enable congestion marking by default, I think this issue has been completed, so I'm going to close it.
Yes, we want it enabled by default. Plus the NFD devguide needs to be updated.
Should it be enabled by default in the GenericLinkService::Options constructor or in the individual protocol factories? I would assume the latter since not all transport types support it.
Updated by Davide Pesavento almost 7 years ago
Eric Newberry wrote:
Davide Pesavento wrote:
Eric Newberry wrote:
The nfdc changes allowing congestion signaling to be configured have been merged. Unless we want to enable congestion marking by default, I think this issue has been completed, so I'm going to close it.
Yes, we want it enabled by default. Plus the NFD devguide needs to be updated.
Should it be enabled by default in the
GenericLinkService::Optionsconstructor or in the individual protocol factories? I would assume the latter since not all transport types support it.
I suppose in the factories and in the configuration file (which takes precedence anyway). At that point the default in GenericLinkService::Options won't matter much...
Updated by Eric Newberry almost 7 years ago
- % Done changed from 90 to 100
Commit enabling congestion marking by default on supported faces has been pushed to Gerrit.
Updated by Eric Newberry almost 7 years ago
Eric Newberry wrote:
Commit enabling congestion marking by default on supported faces has been pushed to Gerrit.
This change has been merged. I think this issue is completed now.
Updated by Anonymous almost 7 years ago
Eric Newberry wrote:
Eric Newberry wrote:
Commit enabling congestion marking by default on supported faces has been pushed to Gerrit.
This change has been merged. I think this issue is completed now.
Yeah looks completed to me. Good job!
Updated by Davide Pesavento almost 7 years ago
What about the documentation? The NFD devguide has been neglected way too much lately.
Updated by Eric Newberry almost 7 years ago
Davide Pesavento wrote:
What about the documentation? The NFD devguide has been neglected way too much lately.
I agree something should be written about this in the devguide, but I think that's a separate issue.
Updated by Davide Pesavento almost 7 years ago
Eric Newberry wrote:
but I think that's a separate issue.
We've usually handled documentation updates in the same issue as the implementation, but feel free to do whatever, I don't have any preference.
Updated by Eric Newberry almost 7 years ago
Davide Pesavento wrote:
Eric Newberry wrote:
but I think that's a separate issue.
We've usually handled documentation updates in the same issue as the implementation, but feel free to do whatever, I don't have any preference.
From what I understand, the devguide updates would be about congestion control in general, and not just the management updates. Therefore, I think it would be better suited for #1624. Just my thoughts.