Fix issue DAN-169 #11
Conversation
| pattern "0[Xx][a-fA-F0-9]+"; | ||
| } | ||
| must "(../enable)" { | ||
| error-message "link-params enable must be set." |
There was a problem hiding this comment.
All of these are missing semicolons.
There was a problem hiding this comment.
The missing semicolons have been added here.
| augment /if:interfaces/interfaces-dataplane:dataplane { | ||
| uses link-params; | ||
| } | ||
| augment /if:interfaces/interfaces-dataplane:dataplane/interfaces-dataplane:vif/interfaces-dataplane { |
There was a problem hiding this comment.
The path is now correct after adding the :ip at the end of the path
akashniral
force-pushed
the
niral_zebra_cli
branch
5 times, most recently
from
ff42fe5
to
a9ac900
Compare
| @@ -0,0 +1,227 @@ | |||
| module vyatta-protocols-frr-zebra-v1 { | |||
There was a problem hiding this comment.
Generally the names used in the YANG should not be tied to the implementation. Call this something other than zebra, maybe something like link-params.
There was a problem hiding this comment.
I have changed the file name to vyatta-protocols-frr-link-params-v1.yang
| namespace "urn:vyatta.com:mgmt:vyatta-protocols-frr-zebra:1"; | ||
| prefix vyatta-protocols-frr-zebra-v1; | ||
|
|
||
| import vyatta-protocols-v1 { |
There was a problem hiding this comment.
This doesn't seem to be used.
There was a problem hiding this comment.
I have removed the unused imports i.e vyatta-protocols-v1
| "Added link-params option."; | ||
| } | ||
|
|
||
| typedef ieee-float { |
There was a problem hiding this comment.
Both this and the float type below seem needlessly complicated. What's the intention here? Why was this method used rather than using a decimal64? Why are prefixing with 0x in some cases and not in others? The data model should abstract complexities in the underlying implementation, not expose them
There was a problem hiding this comment.
The prefix 0x was used so that we even can take hex decimal format as inputs. In FRR the input format takes both hex and floating-point and is of type IEEE-float. Should I stick to decimal64 instead of the above float types?
There was a problem hiding this comment.
Eg:
1.
2.
In Frr we were using both hex format and decimal format for taking input values like here in available bandwidth.
Should I just stick to floating-point instead of taking hex format for Danos CLI? What are your thoughts on this?
There was a problem hiding this comment.
The only difference between your ieee-float and float types is the 0x in front, both of them seem to accept hex digits.
What is the difference between those types supposed to be? Configuring a bandwidth in hexadecimal also seems very strange, do you know of anyone that actually does that?
Thinking about it more, I'm not even convinced that a decimal type should be required here. If there's a suitable unit that could be used in all cases, you could just have a uint with that unit. If not, possibly something like mutually exclusive containers named after the unit containing a leaf with the value would be a good way of modelling this. @wivory, @jsouthworth any thoughts?
There was a problem hiding this comment.
People do not generally use hexadecimal for setting up the values. Earlier I tried to keep the existing functionality as per FRR. However, I feel a simple uint32 would do and fit in all cases but I am open to hearing if you have any other suggestions @wivory, @jsouthworth.
| @@ -0,0 +1,227 @@ | |||
| module vyatta-protocols-frr-zebra-v1 { | |||
There was a problem hiding this comment.
You have a mixture of tabs and spaces in there. Please stick to tabs, and keep lines to 80 characters wide when the tabs are represented as 4 spaces wide.
There was a problem hiding this comment.
The file has been converted to tabs
| grouping link-params { | ||
| container link-params { | ||
| configd:help "Configure interface link parameters"; | ||
| leaf admin-grp { |
There was a problem hiding this comment.
Go with admin-group, shortening to grp is unclear and unnecessary.
| configd:help "Unidirectional Link Delay Variation"; | ||
| description "Unidirectional Link Delay Variation"; | ||
| } | ||
| leaf max-bw { |
| configd:help "Maximum bandwidth that can be used"; | ||
| description "Maximum bandwidth that can be used"; | ||
| } | ||
| leaf max-rsv-bw { |
There was a problem hiding this comment.
renamed to max-reserved-bandwidth
| description "Configure remote ASBR information (Neighbor IP address and AS number)"; | ||
|
|
||
| } | ||
| leaf res-bw { |
There was a problem hiding this comment.
renamed to residual-bandwidth
| configd:help "Unidirectional Residual Bandwidth"; | ||
| description "Unidirectional Residual Bandwidth"; | ||
| } | ||
| leaf unrsv-bw { |
There was a problem hiding this comment.
renamed to unreserved-bandwidth
| configd:help "Unreserved bandwidth at each priority level"; | ||
| description "Unreserved bandwidth at each priority level"; | ||
| } | ||
| leaf use-bw { |
There was a problem hiding this comment.
renamed to utilised-bandwidth
1. Added link parameters to Danos yang model. Signed-off-by: akashniral <[email protected]>
akashniral
force-pushed
the
niral_zebra_cli
branch
from
b5e9d65
to
8c241f2
Compare
1. remove enable from danos cli 2. change the type from ieee-float to uint32 Signed-off-by: akashniral <[email protected]>
There was a problem hiding this comment.
Looking much better. Once these are fixed, unless anything else comes up that needs fixing it would be good to see some testing showing that this works, as there's been a few iterations presented for review that wouldn't have worked.
Also there's now a conflict in the control file, which will need resolving.
| type string { | ||
| pattern "0[Xx][a-fA-F0-9]+"; | ||
| } | ||
| must "(../enable)" { |
There was a problem hiding this comment.
Given that enable is gone, these must statements don't seem right (on all of the leaf nodes below as well).
There was a problem hiding this comment.
I've removed the must statement as enable was gone
| description "Administrative group membership"; | ||
| } | ||
| leaf available-bandwidth { | ||
| type uint32; |
There was a problem hiding this comment.
Much cleaner. Please add units statements to all of these so it's clear what units are being used. Maybe also mention the units in the configd:help as units aren't exposed via the CLI help.
There was a problem hiding this comment.
added units statements in each of the leaf nodes and configd:help
There was a problem hiding this comment.
So you're never going to exceed ~4GB? Would a uint64 be safer here? Same applies to all bandwidth fields
There was a problem hiding this comment.
uint64 would be able to allocate more bandwidth than uint32.. It has been modified
There was a problem hiding this comment.
Please push changes - presumably you will be modifying ALL bandwidth nodes to uint64?
There was a problem hiding this comment.
I've pushed my changes. In all the bandwidth I've modified to uint64
1. remove must statement 2. add units statement in each of the leaf nodes and in confid:help Signed-off-by: akashniral <[email protected]>
The conflict is now resolved. The following commands are now being shown in the Danos CLI for link-params with units Below is the simple configuration for link-params |
| } | ||
| leaf enable { | ||
| type empty; | ||
| must "(../enable)" { |
There was a problem hiding this comment.
leaf must be configured if it is configured - well yes, that's a racing certainty, but doesn't really add a lot to the model. I'm not clear why you need the enable leaf at all to be honest - why would you need to enable params? If any are set, then they are set; if not, they're not set. Is this simply so your code can check the 'enable' leaf to determine whether to look for other params? Or am I missing something here?
There was a problem hiding this comment.
In the updated code the must statement has been removed.
|
|
||
| revision 2021-03-02 { | ||
| description | ||
| "Added link-params option."; |
There was a problem hiding this comment.
'Initial revision' would seem more appropriate.
There was a problem hiding this comment.
The description has been modified to "Initial revision"
| type string { | ||
| pattern "0[Xx][a-fA-F0-9]+"; | ||
| } | ||
| must "(../enable)" { |
There was a problem hiding this comment.
Does this not require an 'enable' leaf in this container, which is missing? Did you mean '../../enable' or would you be better removing all these musts, and having it so any link-param configuration here implicitly enables link-params? Or make it a presence container so configuration of the container with no child nodes means link-params are enabled?
There was a problem hiding this comment.
The must statement has been removed in the updated code.
| } | ||
| leaf metric { | ||
| type uint32 { | ||
| range "1..4294967295"; |
There was a problem hiding this comment.
I think this is equivalent '1..max' which might read better?
There was a problem hiding this comment.
max would be better to read. It has been updated.
| must "(../enable)" { | ||
| error-message "link-params enable must be set."; | ||
| } | ||
| configd:help "Unidirectional Available Bandwidth"; |
There was a problem hiding this comment.
You might want to give the user a clue about units? bits? Gigabytes?
There was a problem hiding this comment.
The units have already been added i.e (Bytes/second).
| configd:help "Maximum bandwidth that can be used (Bytes/second)"; | ||
| description "Maximum bandwidth that can be used"; | ||
| } | ||
| leaf max-reserved-bandwidth { |
There was a problem hiding this comment.
Can this exceed max-bandwidth? If not then perhaps a must statement enforcing this would be wise.
There was a problem hiding this comment.
How do these 2 relate to available-bandwidth, residual, unreserved and utilised?
There was a problem hiding this comment.
- I've enforced must statement in each of the bandwidths. Before configuring other bandwidths the max-bandwidth has to be set in order to compare the values i.e (max-bandwidth >= ).
- I have described the bandwidth-related fields following draft-ietf-isis-te-metrics-extension-03.
There was a problem hiding this comment.
What determines the max bandwidth that can be used? Is this an inherent property of the link, or, like ADSL, depends on how well the link is working at a given time?
There was a problem hiding this comment.
I am not sure about it. In frr the max-bandwidth is set upon setting link-param however in Danos CLI user can provide max-bandwidth according to the need.
There was a problem hiding this comment.
How can you code a feature when you don't know what it means???
| configd:help "Maximum bandwidth that may be reserved (Bytes/second)"; | ||
| description "Maximum bandwidth that may be reserved"; | ||
| } | ||
| leaf metric { |
There was a problem hiding this comment.
Could you ever have other metrics? Maybe mpls-te-metric would be a better name?
There was a problem hiding this comment.
It seems better.. It has been changed
| range 0..100; | ||
| } | ||
| units "Percentage"; | ||
| configd:help "Configure remote ASBR information (Neighbor IP address and AS number) in percentage"; |
There was a problem hiding this comment.
Cut and paste from previous element? Just adding 'in percentage' does not make it make sense.
There was a problem hiding this comment.
The configd:help and description has been changed to Unidirectional Link Packet Loss (Percentage).
| type uint32; | ||
| units "Bytes/second"; | ||
| configd:help "Unidirectional Residual Bandwidth (Bytes/second)"; | ||
| description "Unidirectional Residual Bandwidth"; |
There was a problem hiding this comment.
What is residual bandwidth? Description can (and often should) be multi-line to give a fuller description than configd:help.
There was a problem hiding this comment.
Fuller description has been provided for residual bandwidth following draft-ietf-isis-te-metrics-extension-03.
| configd:help "Unreserved bandwidth at each priority level"; | ||
| description "Unreserved bandwidth at each priority level"; | ||
| } | ||
| leaf utilised-bandwidth { |
There was a problem hiding this comment.
Is this config or state? Please provide a better description.
There was a problem hiding this comment.
Fuller description has been provided for utilised bandwidth following draft-ietf-isis-te-metrics-extension-03.
There was a problem hiding this comment.
This still sounds more like state than config - 'as measured in the router' is state not config AFAICT.
There was a problem hiding this comment.
This similar command is there in vtysh shell where I can use this to adjust utilized-bandwidth in Bytes/second and was req in Danos CLI and in the draft-ietf-isis-te-metrics-extension-03 the above description is given of the same therefore I've provided that description.
modified the uint32 to uint64 for allocating more bandwidth greater than 4GB Signed-off-by: akashniral <[email protected]>
…yatta-protocols-frr into niral_zebra_cli
1. add description in each of the bandwidth releated fields. 2. enforce must statement so other bandwidth related field do not exceed maximum bandwidth 3. change unreserved bandwidth field to set the amount of bandwidth for each of the priority levels. Signed-off-by: akashniral <[email protected]>
| configd:help "Unidirectional Residual Bandwidth (Bytes/second)"; | ||
| description "Unidirectional Residual Bandwidth. For a link or forwarding adjacency, residual | ||
| bandwidth is defined to be Maximum Bandwidth [RFC3630] minus the bandwidth currently allocated | ||
| to RSVP-TE LSPs. For a bundled link, residual bandwidth is defined to be the sum of the |
There was a problem hiding this comment.
Where is the bandwidth allocated to RSVP-TE LSPs defined? What if there is a mismatch?
There was a problem hiding this comment.
I have found this description in draft-ietf-isis-te-metrics-extension-03. Do I have to define bandwidth allocated to RSVP-TE LSP too in this DAN-169 fix?
There was a problem hiding this comment.
You aren't convincing me that you fully understand what all these fields mean, nor how they relate to each other. I am not convinced that this data model is complete / consistent, and until I am persuaded that it is, I cannot approve.
| error-message "Residual Bandwidth cannot be greater than Maximum Bandwidth"; | ||
| } | ||
| } | ||
| container unreserved-bandwidth { |
There was a problem hiding this comment.
If max-bandwidth is X, it seems you can have X unreserved bandwidth at each of the 8 priority levels, making 8X?
There was a problem hiding this comment.
In each of the priority levels, the bandwidth can be set equal to max-bandwidth. The same way has been implemented in FRR.
There was a problem hiding this comment.
That's as maybe. I'd like you to explain to me why this is correct, rather than just saying it's done this way somewhere else.
There was a problem hiding this comment.
The [RFC3630] has been followed here wherein section 2.5.8 it stated about Unreserved Bandwidth as follows:
The Unreserved Bandwidth sub-TLV specifies the amount of bandwidth not yet reserved at each of the eight priority levels in IEEE floating-point format. The values correspond to the bandwidth that can be reserved with a setup priority of 0 through 7, arranged in increasing order with priority 0 occurring at the start of the sub-TLV, and priority 7 at the end of the sub-TLV. The initial values (before any bandwidth is reserved) are all set to the Maximum Reservable Bandwidth. Each value will be less than or equal to the Maximum Reservable Bandwidth. The units are bytes per second.
The Unreserved Bandwidth sub-TLV is TLV type 8, and is 32 octets in length.
There was a problem hiding this comment.
I'm still not convinced this is config as opposed to state information.
There was a problem hiding this comment.
or at least information that should be dynamically derived not set in stone in the config
| container unreserved-bandwidth { | ||
| configd:help "Unreserved bandwidth at each priority level"; | ||
| description "Unreserved bandwidth at each priority level. Unreserved Bandwidth [RFC3630], is | ||
| subtracted from the Maximum Reservable Bandwidth (the bandwidth that can theoretically |
There was a problem hiding this comment.
I've aligned the description.
| units "Bytes/second"; | ||
| configd:help "Bandwidth (Bytes/second)"; | ||
| description "Bandwidth (Bytes/second) for each priority levels"; | ||
| must "../../../max-bandwidth" { |
There was a problem hiding this comment.
Still looks to be 2 spaces too far to the left ...
There was a problem hiding this comment.
The changes have been reflected in the last commit
align description field Signed-off-by: akashniral <[email protected]>
fix alignment issues Signed-off-by: akashniral <[email protected]>
There was a problem hiding this comment.
I don't feel that the submitter understands the interaction between all the different bandwidth fields. Simply cutting and pasting descriptions from another document is not enough. I cannot approve until the submitter has satisfactorily explained to me how they all fit together.
Signed-off-by: akashniral [email protected]